Re: [PATCH] handle pathological anti-ranges in gimple_fold_builtin_memory_op (PR 81908)

[Martin Sebor <msebor at gmail dot com>]

On 08/24/2017 03:52 PM, Jeff Law wrote:
> On 08/24/2017 09:03 AM, Martin Sebor wrote:
> > On 08/24/2017 08:03 AM, Richard Biener wrote:
> > > On Wed, Aug 23, 2017 at 9:42 PM, Martin Sebor <msebor@gmail.com> wrote:
> > > > Bug 81908 is about a -Wstringop-overflow warning for a Fortran
> > > > test triggered by a recent VRP improvement.  A simple test case
> > > > that approximates the warning is:
> > > >
> > > >   void f (char *d, const char *s, size_t n)
> > > >   {
> > > >     if (n > 0 && n <= SIZE_MAX / 2)
> > > >       n = 0;
> > > >
> > > >     memcpy (d, s, n);   // n in ~[1, SIZE_MAX / 2]
> > > >   }
> > > >
> > > > Since the only valid value of n is zero the call to memcpy can
> > > > be considered a no-op (a value of n > SIZE_MAX is in excess of
> > > > the maximum size of the largest object and would surely make
> > > > the call crash).
> > > >
> > > > The important difference between the test case and Bug 81908
> > > > is that in the latter, the code is emitted by GCC itself from
> > > > what appears to be correct source (looks like it's the result
> > > > of the loop distribution pass).  I believe the warning for
> > > > the test case above and for other human-written code like it
> > > > is helpful, but warning for code emitted by GCC, even if it's
> > > > dead or unreachable, is obviously not (at least not to users).
> > > >
> > > > The attached patch enhances the gimple_fold_builtin_memory_op
> > > > function to eliminate this patholohgical case by making use
> > > > of range information to fold into no-ops calls to memcpy whose
> > > > size argument is in a range where the only valid value is zero.
> > > > This gets rid of the warning and improves the emitted code.
> > > >
> > > > Tested on x86_64-linux.
> > >
> > > @@ -646,6 +677,12 @@ gimple_fold_builtin_memory_op
> > > (gimple_stmt_iterator *gsi,
> > >    tree destvar, srcvar;
> > >    location_t loc = gimple_location (stmt);
> > >
> > > +  if (tree valid_len = only_valid_value (len))
> > > +    {
> > > +      /* LEN is in range whose only valid value is zero.  */
> > > +      len = valid_len;
> > > +    }
> > > +
> > >    /* If the LEN parameter is zero, return DEST.  */
> > >    if (integer_zerop (len))
> > >
> > > just enhance this check to
> > >
> > >   if (integer_zerop (len)
> > >       || size_must_be_zero_p (len))
> > >
> > > ?  'only_valid_value' looks too generic for this.
> >
> > Sure.
> >
> > FWIW, the reason I did it this was because my original patch
> > returned error_mark_node for entirely invalid ranges and had
> > the caller replace the call with a trap.  I decided not to
> > include that part in this fix to keep it contained.
> Seems reasonable.  Though I would suggest going forward with trap
> replacement for clearly invalid ranges as a follow-up.  Once you do trap
> replacement, the input operands all become dead as does all the code
> after the trap and the outgoing edges in the CFG.
>
> That often exposes a fair amount of cleanup.  Furthermore on targets
> that have conditional traps, the controlling condition often turns into
> a conditional trap.  In all, you get a lot of nice cascading effects
> when you turn something that is clearly bogus into a trap.
>
> gimple-ssa-isolate-erroneous-paths probably has the infrastructure you
> need, including a code you can crib to detect PHI arguments which would
> cause bogus behavior and allow you to isolate that specific path.

Thanks for the pointer.  I'll look into it.

> > > +  if (!wi::fits_uhwi_p (min) || !wi::fits_uhwi_p (max))
> > > +    return NULL_TREE;
> > > +
> > >
> > > why?
> >
> > Only because I never remember what APIs are safe to use with
> > what input.
> >
> > > +  if (wi::eq_p (min, wone)
> > > +      && wi::geu_p (max + 1, ssize_max))
> > >
> > >    if (wi::eq_p (min, 1)
> > >       && wi::gtu_p (max, wi::max_value (prec, SIGNED)))
> > >
> > > your ssize_max isn't signed size max, and max + 1 might overflow to zero.
> >
> > You're right that the addition to max would be better done
> > as subtraction from the result of (1 << N).  Thank you.
> >
> > If (max + 1) overflowed then (max == TYPE_MAX) would have
> > to hold which I thought could never be true for an anti
> > range. (The patch includes tests for this case.)  Was I
> > wrong?
> Couldn't we have an anti range like ~[TYPE_MAX,TYPE_MAX]?  Or am I
> misunderstanding something.

I've never seen such an anti-range.  In my tests, a range one
of whose bounds is equal to one of the type's limits) ends up
canonicalized into an ordinary range.  If I'm reading the code
right it's done in set_and_canonicalize_value_range.

> > Attached is an updated version with the suggested changes
> > plus an additional test to verify the absence of warnings.
> The patch is OK.

Thanks.  Patch committed in r251347.

> I'll note this is another use of anti-ranges.  I'd really like to see
> Aldy's work on the new range API move forward and get rid of anti-ranges.

You and me both :)

Martin