PING [PATCH] avoid -Warray-bounds checks for vtable assignments (PR 98266)

[Jason Merrill]

On 2/23/21 11:02 AM, Martin Sebor wrote:
> [CC Jason for any further comments/clarification]
> 
> On 2/9/21 10:49 AM, Martin Sebor wrote:
>> On 2/8/21 4:11 PM, Jeff Law wrote:
>>>
>>>
>>> On 2/8/21 3:44 PM, Martin Sebor wrote:
>>>> On 2/8/21 3:26 PM, Jeff Law wrote:
>>>>>
>>>>>
>>>>> On 2/8/21 2:56 PM, Martin Sebor wrote:
>>>>>> On 2/8/21 12:59 PM, Jeff Law wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 1/19/21 5:56 PM, Martin Sebor via Gcc-patches wrote:
>>>>>>>> Similar to the problem reported for -Wstringop-overflow in pr98266
>>>>>>>> and already fixed, -Warray-bounds is also susceptible to false
>>>>>>>> positives in assignments and copies involving virtual inheritance.
>>>>>>>> Because the two warnings don't share code yet (hopefully in GCC 12)
>>>>>>>> the attached patch adds its own workaround for this problem to
>>>>>>>> gimple-array-bounds.cc, this one slightly more crude because of
>>>>>>>> the limited insight the array bounds checking has into the checked
>>>>>>>> expressions.
>>>>>>>>
>>>>>>>> Tested on x86_64-linux.
>>>>>>>>
>>>>>>>> Martin
>>>>>>>>
>>>>>>>> gcc-98266.diff
>>>>>>>>
>>>>>>>> PR middle-end/98266 - bogus array subscript is partly outside array
>>>>>>>> bounds on virtual inheritance
>>>>>>>>
>>>>>>>> gcc/ChangeLog:
>>>>>>>>
>>>>>>>>       PR middle-end/98266
>>>>>>>>       * gimple-array-bounds.cc
>>>>>>>> (array_bounds_checker::check_array_bounds):
>>>>>>>>       Avoid checking references involving artificial members.
>>>>>>>>
>>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>>
>>>>>>>>       PR middle-end/98266
>>>>>>>>       * g++.dg/warn/Warray-bounds-15.C: New test.
>>>>>>> It seems to me that we've got the full statement at some point  and
>>>>>>> thus
>>>>>>> the full expression so at some point couldn't we detect when
>>>>>>> TYPE_SIZE_UNIT!= DECL_SIZE_UNIT?  Or should we be using 
>>>>>>> TYPE_SIZE_UNIT
>>>>>>> rather than DECL_SIZE_UNIT in gimple-array-bounds.cc
>>>>>>>
>>>>>>> Am I missing something?
>>>>>>
>>>>>> The expression we're looking at when the false positive is issued
>>>>>> is the MEM_REF in the LHS of:
>>>>>>
>>>>>> MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void
>>>>>> *)&_ZTC1E24_1D + 24B];
>>>>>>
>>>>>> TREE_TYPE(LHS) is D, DECL_SIZE_UNIT (D.2652) is 24, and
>>>>>> TYPE_SIZE_UNIT(D) is also 24, so there's no discrepancy between
>>>>>> DECL_SIZE and TYPE_SIZE.
>>>>> So that seems like it's a different issue then, unrelated to 97595.
>>>>> Right?
>>>>
>>>> I think the underlying problem is the same.  We're getting a size
>>>> that doesn't correspond to what's actually being accessed, and it
>>>> happens because of the virtual inheritance.  In pr97595 Jason
>>>> suggested to use the decl/type size inequality to identify this
>>>> case but I think we could have just as well used DECL_ARTIFICIAL
>>>> instead.  At least the test cases from pr97595 both pass with
>>>> this change.
>>> But in the 98266 case the type and decl sizes are the same.  So to be
>>> true that would mean that the underlying type we're using to access
>>> memory differs from its actual type.  Is that the case in the IL?  And
>>> does this have wider implications for diagnostics or optimizations that
>>> rely on accurate type sizing?
>>>
>>> I'm just trying to make sure I understand, not accepting or rejecting
>>> the patch yet.
>>
>> The part of the IL with the MEM_REF is this:
>>
>> void g ()
>> {
>>    void * D.2789;
>>    struct E D.2652;
>>
>>    <bb 2> [local count: 1073741824]:
>>    E::E (&D.2652, "");
>>    f (&D.2652);
>>
>>    <bb 3> [local count: 1073741824]:
>>    MEM[(struct D *)&D.2652 + 24B]._vptr.D = &MEM <int (*) ()> [(void 
>> *)&_ZTC1E24_1D + 24B];
>>    ...
>>
>> The access here is to the _vptr.D pointer member of D.2652 which is
>> just past the end of the parent object (as reflected by its SIZE):
>> it sets sets up the virtual table pointer.
>>
>> The access in pr97595 is to the member subobject, which, as Jason
>> explained (and I accordingly documented under DECL_SIZE in tree.h),
>> is also laid out separately from the parent object.
>>
>> These cases aren't exactly the same (which is also why the test
>> I added for -Warray-bounds in pr97595 didn't expose this bug) but
>> they are closely related.  The one here can be distinguished by
>> DECL_ARTIFICAL.  The other by the DECL_SIZE != TYPE_SIZE member
>> inequality.
>>
>> Might this impact other warnings?  I'd say so if they don't take
>> these things into account.  I just learned about this in pr97595
>> which was a -Wstringop-overflow false positive but I also saw
>> a similar instance of -Warray-bounds with my patch to improve
>> caching and enhance array bounds checking.  I dealt with that
>> instance of the warning in that patch but proactively added
>> a test case to the fix for pr97595.  But the test case is focused
>> on the subobject access and not on one to the virtual table so
>> (as I said above) it didn't expose this bug.
>>
>> Might this also impact optimizations?  I can imagine someone
>> unaware of this "gotcha" making the same "naive" assumption
>> I did, but I'd also expect such an invalid assumption to be
>> found either in code review or quickly cause problems in
>> testing.
> 
> Jeff, does this answer your question?

I don't see how the issue here depends on the artificiality of the vptr; 
I'd expect to see the same problem with a data member.  The problem is 
that a D base subobject is smaller than a complete D object, and in this 
case the base subobject is allocated such that if it were a full D 
object, it would overlap the end of E.  And we're checking the MEM_REF 
as though accessing a full D object, so we get a warning.

The general issue about the confusion between complete and as-base types 
is PR 22488; I have various thoughts about a proper fix, but there have 
always been higher-priority things to work on.

One possible approach to this PR is that we don't need to check an 
intermediate _REF; in

MEM[(struct D *)&D.2651 + 24B]._vptr.D

we aren't loading a whole D, we're only looking at the _vptr, which is 
within the bounds of E.

Jason