On 11/14/2017 05:28 AM, Richard Biener wrote:
On Mon, Nov 13, 2017 at 6:37 PM, Martin Sebor <mse...@gmail.com> wrote:
Richard, this thread may have been conflated with the one Re:
[PATCH] enhance -Warray-bounds to detect out-of-bounds offsets
(PR 82455) They are about different things.
I'm still looking for approval of:
https://gcc.gnu.org/ml/gcc-patches/2017-10/msg01208.html
Sorry, I pointed to an outdated version. This is the latest
version:
https://gcc.gnu.org/ml/gcc-patches/2017-10/msg01304.html
My bad...
+ tree maxbound
+ = build_int_cst (sizetype, ~(1LLU << (TYPE_PRECISION (sizetype) - 1)));
this looks possibly bogus. Can you instead use
up_bound_p1
= wide_int_to_tree (sizetype, wi::div_trunc (wi::max_value
(TYPE_PRECISION (sizetype), SIGNED), wi::to_wide (eltsize)));
please? Note you are _not_ computing the proper upper bound here because that
is what you compute plus low_bound.
+ up_bound_p1 = int_const_binop (TRUNC_DIV_EXPR, maxbound, eltsize);
+
+ tree arg = TREE_OPERAND (ref, 0);
+ tree_code code = TREE_CODE (arg);
+ if (code == COMPONENT_REF)
+ {
+ HOST_WIDE_INT off;
+ if (tree base = get_addr_base_and_unit_offset (ref, &off))
+ {
+ tree size = TYPE_SIZE_UNIT (TREE_TYPE (base));
+ if (TREE_CODE (size) == INTEGER_CST)
+ up_bound_p1 = int_const_binop (MINUS_EXPR, up_bound_p1, size);
I think I asked this multiple times now but given 'ref' is the
variable array-ref
a.b.c[i] when you call get_addr_base_and_unit_offset (ref, &off) you always
get a NULL_TREE return value.
So I asked you to pass it 'arg' instead ... which gets you the offset of
a.b.c, which looks like what you intended to get anyway.
I also wonder what you compute here - you are looking at the size of 'base'
but that is the size of 'a'. You don't even use the computed offset! Which
means you could have used get_base_address instead!? Also the type
of 'base' may be completely off given MEM[&blk + 8].b.c[i] would return blk
as base which might be an array of chars and not in any way related to
the type of the innermost structure we access with COMPONENT_REFs.
Why are you only looking at COMPONENT_REF args anyways? You
don't want to handle a.b[3][i]?
That is, I'd have expected you do
if (get_addr_base_and_unit_offset (ref, &off))
up_bound_p1 = wide_int_to_tree (sizetype, wi::sub (wi::to_wide
(up_bound_p1), off));
Richard.
Thanks
Martin
The difficulty with a testcase like
struct { struct A { int b[1]; } a[5]; } x;
x.a[i].b[j]
is that b is not considered an array at struct end since one of my
recent changes to array_at_struct_end (basically it disallows having
a flex array as the last member of an array).
It would still stand for non-array components with variable offset
but you can't create C testcases for that.
So yes, for the specific case within the array_at_struct_end_p condition
get_addr_base_and_unit_offset is enough. IIRC the conditon was
a bit more than just get_addr_base_and_unit_offset. up_bound !=
INTEGER_CST for example. So make the above
void foo (int n, int i)
{
struct { struct A { int b[n]; } a[5]; } x;
return x.a[i].b[PTRDIFF_MAX/2];
}
with appropriately adjusted constant. Does that give you the testcase
you want?
Thank you for the test case. It is diagnosed the same way
irrespective of which of the two functions is used so it serves
to confirm my understanding that the only difference between
the two functions is bits vs bytes.
Unless you have another test case that does demonstrate that
get_ref_base_and_extent is necessary/helpful, is the last patch
okay to commit?
(Again, to be clear, I'm happy to change or enhance the patch if
I can verify that the change handles cases that the current patch
misses.)
As of "it works, catches corner-cases, ..." - yes, it does, but it
adds code that needs to be maintained, may contain bugs, is
executed even for valid code.
Understood. I don't claim the enhancement is free of any cost
whatsoever. But it is teeny by most standards and it doesn't
detect just excessively large indices but also negative indices
into last member arrays (bug 68325) and out-of-bounds indices
(bug 82583). The "excessively large" part does come largely
for free with the other checks.
Martin
