This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] convert MIN_EXPR operands to the same type (PR 87059)


On Mon, Aug 27, 2018 at 5:51 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 08/27/2018 09:28 AM, Richard Biener wrote:
> > On Mon, Aug 27, 2018 at 4:41 PM Martin Sebor <msebor@gmail.com> wrote:
> >>
> >> On 08/27/2018 02:32 AM, Richard Biener wrote:
> >>> On Sat, Aug 25, 2018 at 9:14 PM Jeff Law <law@redhat.com> wrote:
> >>>>
> >>>> On 08/24/2018 01:06 PM, Martin Sebor wrote:
> >>>>> PR 87059 points out an ICE in the recently enhanced VRP code
> >>>>> that was traced back to a MIN_EXPR built out of operands of
> >>>>> types with different sign by expand_builtin_strncmp().
> >>>>>
> >>>>> The attached patch adjusts the function to make sure both
> >>>>> operands have the same type, and to make these mismatches
> >>>>> easier to detect, also adds an assertion to fold_binary_loc()
> >>>>> for these expressions.
> >>>>>
> >>>>> Bootstrapped on x86_64-linux.
> >>>>>
> >>>>> Martin
> >>>>>
> >>>>> PS Aldy, I have not tested this on powerpc64le.
> >>>>>
> >>>>> gcc-87059.diff
> >>>>>
> >>>>>
> >>>>> PR tree-optimization/87059 - internal compiler error: in set_value_range
> >>>>>
> >>>>> gcc/ChangeLog:
> >>>>>
> >>>>>       PR tree-optimization/87059
> >>>>>       * builtins.c (expand_builtin_strncmp): Convert MIN_EXPR operand
> >>>>>       to the same type as the other.
> >>>>>       * fold-const.c (fold_binary_loc): Assert expectation.
> >>>> I bootstrapped (but did not regression test) this on ppc64le and also
> >>>> built the linux kernel (which is where my tester tripped over this problem).
> >>>>
> >>>> Approved and installed on the trunk.
> >>>
> >>> Please remove the assertion in fold_binary_loc again, we do not do this kind
> >>> of assertions there.
> >>
> >> What kind of assertions are appropriate here and what's a better
> >> place to make sure the MIN/MAX expression operands are valid, i.e.,
> >> have the same type or sign or whatever the downstream assumptions
> >> are?
> >
> > Generally we have such checks in place for IL verification.  There's also
> > a separate fold-checking mode (but checking unrelated stuff only at the
> > moment).
> >
> > fold is heavily used so slowing it down isn't wanted.  You'll also run into
> > existing issues too easily if you really pin down types to requirements
> > (matching TYPE_MAIN_VARIANT).
> >
> > So just checking MIN/MAX for matching mode and sign just invites
> > for more random checks here.
>
> So where should we check these things (or what's a good way to
> make this less error-prone -- would documenting what kind of
> assertions are appropriate be a good way to go?  Or add an API
> that did the validation for all EXPRs that require operands
> of the same type?)

If you need to catch bogus entries to fold_* then of course the place
to instrument is fold_*.

> The ICE in the original bug report was hard to reproduce (it
> didn't trigger in an x86_64 bootstrap and apparently even needed
> a patched powerpc64le cross compiler to bring about).  I added
> the assertion to catch these kinds of mistakes more readily.
>
> FWIW, I had used TYPE_MAIN_VARIANT initially but as you said,
> it triggered ICEs in the test suite so I relaxed the assertion
> it to the current state.

But then it's of course less useful.  You could go for the
GIMPLE requirements given when GENERIC ones are
fulfilled GIMPLE ones are as well.  That would be
types_compatible_p () which of course is even more
expensive.  So if we decide to go the way of instrumenting
fold_* entries then I'd go for sth like

  if (flag_checking)
    switch (code)
    {
      case ...:
         verify;
      default:
        gcc_unreachable ();
    }

but note that FE specific trees may appear here as well.  If we
decide we want to verify GIMPLE requirements only then
please refactor tree-cfg.c:verify_gimple_assign_binary and
friends to split out the type verification part so we could share
that from other places and not duplicate things.

Be prepared to fix a _lot_ of code (as I did when I originally
added the verify_gimple_* stuff).  Mostly stuff will be harmless
but calling out "exceptions" in the verification code itself is bad.

Richard.

> Martin
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]