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

[Martin Sebor <msebor@gmail.com>]

On 3/3/21 10:33 PM, Jason Merrill wrote:
> On 3/3/21 6:20 PM, Martin Sebor wrote:
...
>> I see what you mean, thanks, but I can't think of a test case to
>> trigger a false negative due to the smaller offset.  Any suggestions?
> 
> My only ideas involve undefined behavior, casting the address to a 
> pointer to an unrelated too-large class.  I don't know if that would 
> show up as a MEM_REF of this pattern.

Okay, let me see if I can come up with something based on that.

> 
>> With more testing I also realized that focusing solely on an underlying
>> DECL like in the above doesn't prevent the warning when an object is
>> created in dynamically allocated memory or in a backing buffer.  So
>> the attached revision both adjusts the offset computation upward and
>> handles all kinds of backing store and moves the logic into a helper
>> function for better readability.  I've also added more tests.
>>
>> Retested on x86_64-linux.
>>
>> Thanks again for your help!
>>
>> Martin
>>
>> PS The TYPE_BINFO test isn't as quite as restrictive as I had hoped.
>> It means we consider all derived class, not just those with virtual
>> bases.
> 
> It's even less restrictive than that: all C++ classes have TYPE_BINFO.
> 
>> I tried also requiring BINFO_VIRTUAL_P() to be true but that
>> doesn't work.
> 
> Right, BINFO_VIRTUAL_P is true for the binfo representing the virtual 
> base in the inheritance hierarchy, not on the binfo for the derived class.
> 
>> Using BINFO_VTABLE() does work but it still isn't the same.
> 
> Indeed, virtual functions and virtual bases both cause a class to have a 
> vtable.  You could also check BINFO_N_BASE_BINFOS.
> 
> Or not bother trying to restrict this to classes with virtual bases 
> (i.e. leave the patch as it is), since the result is just as correct for 
> other classes.
> 
> Jason
> 

I think I'll just go with this last version if you're okay with it
as is.  Okay to commit?

Martin

PS Let me propose a separate patch to add some text to the BINFO
comments in tree.h to clarify the macros per the above.