On Tue, Nov 14, 2017 at 6:45 PM, Martin Sebor <mse...@gmail.com> wrote:
> 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.
>> 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
>>>
>>>
>>>
>
