Re: PING 2 [PATCH] correct handling of variable offset minus constant in -Warray-bounds (PR 100137)

[Martin Sebor <msebor@gmail.com>]

On 7/7/21 7:48 PM, Marek Polacek wrote:
> On Wed, Jul 07, 2021 at 02:38:11PM -0600, Martin Sebor via Gcc-patches wrote:
>> On 7/7/21 1:38 AM, Richard Biener wrote:
>>> On Tue, Jul 6, 2021 at 5:47 PM Martin Sebor via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>
>>>> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573349.html
>>>
>>> +  if (TREE_CODE (axstype) != UNION_TYPE)
>>>
>>> what about QUAL_UNION_TYPE?  (why constrain union type accesses
>>> here - note you don't seem to constrain accesses of union members here)
>>
>> I didn't know a QUAL_UNION_TYPE was a thing.  Removing the test
>> doesn't seem to cause any regressions so let me do that in a followup.
>>
>>>
>>> +    if (tree access_size = TYPE_SIZE_UNIT (axstype))
>>>
>>> +  /* The byte size of the array has already been determined above
>>> +     based on a pointer ARG.  Set ELTSIZE to the size of the type
>>> +     it points to and REFTYPE to the array with the size, rounded
>>> +     down as necessary.  */
>>> +  if (POINTER_TYPE_P (reftype))
>>> +    reftype = TREE_TYPE (reftype);
>>> +  if (TREE_CODE (reftype) == ARRAY_TYPE)
>>> +    reftype = TREE_TYPE (reftype);
>>> +  if (tree refsize = TYPE_SIZE_UNIT (reftype))
>>> +    if (TREE_CODE (refsize) == INTEGER_CST)
>>> +      eltsize = wi::to_offset (refsize);
>>>
>>> probably pre-existing but the pointer indirection is definitely confusing
>>> me again and again given the variable is named 'reftype' - obviously
>>> an access to a pointer does not have any element size.  Possibly the
>>> paths arriving here ensure somehow that the only case is when
>>> reftype is not the access type but a pointer to the accessed memory.
>>> "jump-threading" the source might help me avoiding to trip over this
>>> again and again ...
>>
>> I agree (it is confusing).  There's more to simplify here.  It's on
>> my to do list so let me see about this piece of code then.
>>
>>>
>>> The patch removes a lot of odd code, I like that.  You know this code best
>>> and it's hard to spot errors.
>>>
>>> So OK, you'll deal with the fallout.
>>
>> I certainly will.  Pushed in r12-2132.
> 
> I think this patch breaks bootstrap on x86_64:
> 
> In member function ‘availability varpool_node::get_availability(symtab_node*)’,
>      inlined from ‘availability symtab_node::get_availability(symtab_node*)’ at /opt/notnfs/polacek/gcc/gcc/cgraph.h:3360:63,
>      inlined from ‘availability symtab_node::get_availability(symtab_node*)’ at /opt/notnfs/polacek/gcc/gcc/cgraph.h:3355:1,
>      inlined from ‘symtab_node* symtab_node::ultimate_alias_target(availability*, symtab_node*)’ at /opt/notnfs/polacek/gcc/gcc/cgraph.h:3199:35,
>      inlined from ‘symtab_node* symtab_node::ultimate_alias_target(availability*, symtab_node*)’ at /opt/notnfs/polacek/gcc/gcc/cgraph.h:3193:1,
>      inlined from ‘varpool_node* varpool_node::ultimate_alias_target(availability*, symtab_node*)’ at /opt/notnfs/polacek/gcc/gcc/cgraph.h:3234:5,
>      inlined from ‘availability varpool_node::_ZN12varpool_node16get_availabilityEP11symtab_node.part.0(symtab_node*)’ at /opt/notnfs/polacek/gcc/gcc/varpool.c:501:29:
> /opt/notnfs/polacek/gcc/gcc/varpool.c:490:19: error: array subscript ‘varpool_node[0]’ is partly outside array bounds of ‘varpool_node [0]’ [-Werror=array-bounds]
>    490 |   if (!definition && !in_other_partition)
>        |       ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
> In file included from /opt/notnfs/polacek/gcc/gcc/varpool.c:29:
> /opt/notnfs/polacek/gcc/gcc/cgraph.h: In member function ‘availability varpool_node::_ZN12varpool_node16get_availabilityEP11symtab_node.part.0(symtab_node*)’:
> /opt/notnfs/polacek/gcc/gcc/cgraph.h:1969:39: note: object ‘varpool_node::<anonymous>’ of size 120
>   1969 | struct GTY((tag ("SYMTAB_VARIABLE"))) varpool_node : public symtab_node
>        |                                       ^~~~~~~~~~~~
> cc1plus: all warnings being treated as errors

I bootstrapped & regtested it on top of r12-2131 just before pushing
it but let me try with the top of trunk (r12-2135 as of now).

[a bit later]

The bootstrap succeeded with the same configuration settings:

   --enable-languages=ada,c,c++,d,fortran,jit,lto,objc,obj-c++ 
--enable-checking=yes --enable-host-shared --enable-valgrind-annotations

But with --enable-checking=release I was able to reproduce the error
above.  Since there is a simple way to bootstrap I'm not going to
revert the patch tonight.  I'll look into the problem tomorrow and
see if it can be easily fixed.  If not, I'll revert it then.

Martin