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
+ 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
>
>
