On 10/29/2017 10:01 AM, Martin Sebor wrote:
> In my work on -Wrestrict, to issue meaningful warnings, I found
> it important to detect both out of bounds array indices as well
> as offsets in calls to restrict-qualified functions like strcpy.
> GCC already detects some of these cases but my tests for
> the enhanced warning exposed a few gaps.
>
> The attached patch enhances -Warray-bounds to detect more instances
> out-of-bounds indices and offsets to member arrays and non-array
> members. For example, it detects the out-of-bounds offset in the
> call to strcpy below.
>
> The patch is meant to be applied on top posted here but not yet
> committed:
> https://gcc.gnu.org/ml/gcc-patches/2017-10/msg01304.html
>
> Richard, since this also touches tree-vrp.c I look for your comments.
>
> Jeff, this is the enhancement you were interested in when we spoke
> last week.
>
> Thanks
> Martin
>
> $ cat a.c && gcc -O2 -S -Wall a.c
> struct A { char a[4]; void (*pf)(void); };
>
> void f (struct A *p)
> {
> p->a[5] = 'x'; // existing -Warray-bounds
>
> strcpy (p->a + 6, "y"); // enhanced -Warray-bounds
> }
>
> a.c: In function ‘f’:
> a.c:7:3: warning: offset 6 is out of bounds of ‘char[4]’ [-Warray-bounds]
> strcpy (p->a + 6, "y");
> ^~~~~~~~~~~~~~~~~~~~~~
> a.c:1:17: note: member declared here
> struct A { char a[4]; void (*pf)(void); };
> ^
> a.c:5:7: warning: array subscript 5 is above array bounds of
> ‘char[4]’ [-Warray-bounds]
> p->a[5] = 'x';
> ~~~~^~~
>
> gcc-82455.diff
>
>
> PR tree-optimization/82455 - missing -Warray-bounds on strcpy offset in an
> out-of-bounds range
>
> gcc/ChangeLog:
>
> PR tree-optimization/82455
> * tree-ssa-forwprop.c (maybe_warn_offset_out_of_bounds): New function.
> (forward_propagate_addr_expr_1): Call it.
> * tree-vrp.c (check_array_ref): Return bool. Handle flexible array
> members, string literals, and inner indices.
> (search_for_addr_array): Return bool. Add an argument. Add detail
> to diagnostics.
> (check_array_bounds): New function.
> * tree-vrp.h (check_array_bounds): Declare it.
> * wide-int.h (sign_mask): Assert a precondition.So one of the things I
> see here is exposing the array bounds checking
bits from VRP to other passes. I'm not inherently against that -- I'd
like to be able to do that in other passes for various reasons.
It also seems like you're improving that code.
So I'd probably start with pulling the improvements into their own
patch. That shouldn't be terribly controversial.
I'd have to look more closely, but let's make sure we have a query API
that does not warn, but merely returns a boolean. I think we'll have a
desire for that kind of query.
I wonder if all this would be better implemented in the context of
_b_o_s. That may also make it easy to use a domwalk to gather the
ranges given potentially better ranges than we're getting out of VRP.
I would also request that we dig deep into the routines we use here.
I'm currently trying to pull apart the various dependencies within VRP
and wouldn't want to make that worse. Even if it's just a proof of
concept, pull the routines into a different file to see what the "touch"
points are. I'm rather dismayed at how intertwined various bits within
VRP are.
> +/* Check to see if the expression (char*)PTR + OFF is valid (i.e., it
> + doesn't overflow and the result is within the bounds of the object
> + pointed to by PTR. OFF is an offset in bytes. If it isn't valid,
> + issue a -Warray-bounds warning. */
> +
> +static bool
> +maybe_warn_offset_out_of_bounds (location_t loc, tree ptr, tree off)
> +{
> + offset_int cstoff = 0;
> +
> + bool ignore_off_by_1 = false;
> +
> + if (TREE_CODE (ptr) == SSA_NAME)
> + {
> + gimple *stmt = SSA_NAME_DEF_STMT (ptr);
> + if (is_gimple_assign (stmt))
> + {
> + tree_code code = gimple_assign_rhs_code (stmt);
> + if (code == ADDR_EXPR)
> + {
> + ptr = gimple_assign_rhs1 (stmt);
> + ignore_off_by_1 = true;
> + }
> + else if (code == POINTER_PLUS_EXPR)
> + {
> + ptr = gimple_assign_rhs1 (stmt);
> + tree offset = gimple_assign_rhs2 (stmt);
> + if (maybe_warn_offset_out_of_bounds (loc, ptr, offset))
> + return true;
> +
> + /* To do: handle ranges. */
> + if (TREE_CODE (offset) != INTEGER_CST)
> + return false;
Even if we don't handle full ranges, handling ranges that have collapsed
to singletons ought to be trivial. Similarly handling a range that
lives entirely out of bounds shouldn't be too hard either.
> +
> + if (check_array_bounds (ptr, ignore_off_by_1))
> + return true;
> +
> + bool addr_expr = false;
> +
> + if (TREE_CODE (ptr) == ADDR_EXPR)
> + {
> + ptr = TREE_OPERAND (ptr, 0);
> + addr_expr = true;
> + }
> +
> + /* The type of the array to whichj the offset applies and the type
nit. s/whichj/which/
I'm getting really tired.. More later...
Jeff
