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

[Richard Biener]

On Thu, Jul 8, 2021 at 5:12 AM Martin Sebor <msebor@gmail.com> wrote:
>
> 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.

plain ./configure triggers the failure already, I guess your
--enable-host-shared
hides it.

Richard.

>
> Martin