Re: [PATCH] have __builtin_object_size handle POINTER_PLUS with non-const offset (pr 77608)

[Martin Sebor <msebor at gmail dot com>]

Thanks for the review and comments!

> @@ -158,14 +170,149 @@ compute_object_offset (const_tree expr, const_tree var)
>    return size_binop (code, base, off);
>  }
>
> +static bool
> +operand_unsigned_p (tree op)
> +{
> +  if (TREE_CODE (op) == SSA_NAME)
>
> new functions need a comment.  But maybe you want to use tree_expr_nonnegative_p
> to also allow signed but known positive ones?

Let me add a comment.

operand_unsigned_p returns true if the type of the original offset
before conversion to sizetype is unsigned.  tree_expr_nonnegative_p
returns true if the argument's type is unsigned, which is always
the case here.

> +/* Fill the 2-element OFFRANGE array with the range of values OFF
> +   is known to be in.  Postcodition: OFFRANGE[0] <= OFFRANGE[1].  */
> +
> +static bool
> +get_offset_range (tree off, HOST_WIDE_INT offrange[2])
> +{
> +  STRIP_NOPS (off);
>
> why strip nops (even sign changes!) here?

That might be a leftover from an earlier/failed attempt to simplify
things that I forgot to remove.  Let me do that in a followup patch.
Unless I misunderstand your comment there are no sign changes (AFAIK)
because the offset is always represented as sizetype.

> Why below convert things
> via to_uhwi when offrange is of type signed HOST_WIDE_INT[2]?

The offset is always represented as sizetype but the code treats
it as signed because in reality it can be negative.  That said,
I don't find dealing with ranges very intuitive so I could very
well be missing something and there may be a better way to code
this.  I welcome suggestions.

> +         gimple *def = SSA_NAME_DEF_STMT (off);
> +         if (is_gimple_assign (def))
> +           {
> +             tree_code code = gimple_assign_rhs_code (def);
> +             if (code == PLUS_EXPR)
> +               {
> +                 /* Handle offset in the form VAR + CST where VAR's type
> +                    is unsigned so the offset must be the greater of
> +                    OFFRANGE[0] and CST.  This assumes the PLUS_EXPR
> +                    is in a canonical form with CST second.  */
> +                 tree rhs2 = gimple_assign_rhs2 (def);
>
> err, what?  What about overflow?  Aren't you just trying to decompose
> 'off' into a variable and a constant part here and somehow extracting a
> range for the variable part?  So why not just do that?

Sorry, what about overflow?

The purpose of this code is to handle cases of the form

   & PTR [range (MIN, MAX)] + CST

where CST is unsigned implying that the lower bound of the offset
is the greater of CST and MIN.  For instance, in the following it
determines that bos(p, 0) is 4 (and if the 3 were greater than 7
and overflowed the addition the result would be zero).  I'm not
sure I understand what you suggest I do differently to make this
work.

   char d[7];

   #define bos(p, t) __builtin_object_size (p, t)

   long f (unsigned i)
   {
     if (2 < i) i = 2;

     char *p = &d[i] + 3;

     return bos (p, 0);
   }
> +      else if (range_type == VR_ANTI_RANGE)
> +       {
> +         offrange[0] = max.to_uhwi () + 1;
> +         offrange[1] = min.to_uhwi () - 1;
> +         return true;
> +       }
>
> first of all, how do you know it fits uhwi?  Second, from ~[5, 9] you get
> [10, 4] !?  That looks bogus (and contrary to the function comment
> postcondition)

I admit I have some trouble working with anti-ranges.  It's also
difficult to fully exercise them in this pass because it only runs
after EVRP but not after VRP1 (except with -g), so only limited
range information is available. (I'm hoping to eventually change
it but moving the passes broke a test in a way that seemed too
complex to fix for this project).

The code above is based on the observation that an anti-range is
often used to represent the full subrange of a narrower signed type
like signed char (as ~[128, -129]).  I haven't been able to create
an anti-range like ~[5, 9]. When/how would a range like that come
about (so I can test it and implement the above correctly)?

> +      else if (range_type == VR_VARYING)
> +       {
> +         gimple *def = SSA_NAME_DEF_STMT (off);
> +         if (is_gimple_assign (def))
> +           {
> +             tree_code code = gimple_assign_rhs_code (def);
> +             if (code == NOP_EXPR)
> +               {
>
> please trust range-info instead of doing your own little VRP here.
> VR_VARYING -> return false.

I would prefer to rely on the range information and not have to work
around it like I do here but, unfortunately, it doesn't always appear
to be available.  For example, in the following test case:

   char d[130];

   #define bos(p, t) __builtin_object_size (p, t)

   void f (void*);

   void g (signed char i)
   {
      char *p = &d [i] + 128;
     f(__builtin___memset_chk (p, '*', 2, bos (p, 0)));   // okay

     f(__builtin___memset_chk (p, '*', 3, bos (p, 0)));   // overflow
   }

the range type is VR_VARYING but the code makes it possible to diagnose
the overflow in the second call to memset.

Maybe the poor range info i a consequence of the pass only benefiting
from EVRP and not VRP?

> stopping review here noting that other parts of the compiler
> split constant parts from variable parts and it looks like this is
> what you want to do here?  That is, enhance
>
> static vec<unsigned HOST_WIDE_INT> object_sizes[4];
>
> and cache a SSA-NAME, constant offset pair in addition?  Or just a range
> (range of that SSA name + offset), if that's good enough -- wait, that's
> what you get from get_range_info!

I agree that storing the offsets could be an enhancement to consider.
 The patch mentions it in the FIXME part of the following comment:

/* Bitmaps of variables whose object sizes have been computed without
   regard to the (non-constant) offset into them.  A bit in each bitmap
   is valid only if the corresponding bit in the COMPUTED bitmap is
   non-zero (otherwise it's zero).
   FIXME: Change this to a vector of offset ranges to make it possible
   to handle cases like &A[I] + J where both I and J are non-const and
   bounded by some range.  */
static bitmap nonconst_offsets[4];

It's just something I haven't had time to work on yet and with the
close of stage 1 approaching I wanted to put out this version for
review.  Do you view this enhancement as prerequisite for approving
the patch or is it something that you'd be fine with adding later?

> So I'm not sure where the whole complication in the patch comes from...
>
> Possibly from the fact that VRP on pointers is limited and thus &a[i] + 5
> doesn't get you a range for i + 5?

get_range_info doesn't accept pointers at all (perhaps that's what
you meant by "VRP on pointers is limited").  In Gimple, &a[i] + 5
is represented as just that (i.e., _1 = &a[i_3]; p_6 = _1 + 5;) and
so to get the right answer for bos(&a[i] + 5) the patch jumps though
some hoops.  But again, I could very well be missing something that's
obvious to you so if you think that's the case I'd be happy to simplify
the code if you point me in the right direction.

Martin