PING^2: V4 [PATCH] C/C++: Add -Waddress-of-packed-member

["H.J. Lu" <hjl.tools@gmail.com>]

On Sun, Nov 4, 2018 at 7:16 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Sep 25, 2018 at 8:46 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill <jason@redhat.com> wrote:
> > > On 07/23/2018 05:24 PM, H.J. Lu wrote:
> > >>
> > >> On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers <joseph@codesourcery.com>
> > >> wrote:
> > >>>
> > >>> On Mon, 18 Jun 2018, Jason Merrill wrote:
> > >>>
> > >>>> On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers <joseph@codesourcery.com>
> > >>>> wrote:
> > >>>>>
> > >>>>> On Mon, 18 Jun 2018, Jason Merrill wrote:
> > >>>>>
> > >>>>>>> +  if (TREE_CODE (rhs) == COND_EXPR)
> > >>>>>>> +    {
> > >>>>>>> +      /* Check the THEN path first.  */
> > >>>>>>> +      tree op1 = TREE_OPERAND (rhs, 1);
> > >>>>>>> +      context = check_address_of_packed_member (type, op1);
> > >>>>>>
> > >>>>>>
> > >>>>>> This should handle the GNU extension of re-using operand 0 if operand
> > >>>>>> 1 is omitted.
> > >>>>>
> > >>>>>
> > >>>>> Doesn't that just use a SAVE_EXPR?
> > >>>>
> > >>>>
> > >>>> Hmm, I suppose it does, but many places in the compiler seem to expect
> > >>>> that it produces a COND_EXPR with TREE_OPERAND 1 as NULL_TREE.
> > >>>
> > >>>
> > >>> Maybe that's used somewhere inside the C++ front end.  For C a SAVE_EXPR
> > >>> is produced directly.
> > >>
> > >>
> > >> Here is the updated patch.  Changes from the last one:
> > >>
> > >> 1. Handle COMPOUND_EXPR.
> > >> 2. Fixed typos in comments.
> > >> 3. Combined warn_for_pointer_of_packed_member and
> > >> warn_for_address_of_packed_member into
> > >> warn_for_address_or_pointer_of_packed_member.
> > >
> > >
> > >> c.i:4:33: warning: converting a packed ‘struct C *’ pointer increases the
> > >> alignment of ‘long int *’ pointer from 1 to 8 [-Waddress-of-packed-member]
> > >
> > >
> > > I think this would read better as
> > >
> > > c.i:4:33: warning: converting a packed ‘struct C *’ pointer (alignment 1) to
> > > ‘long int *’ (alignment 8) may result in an unaligned pointer value
> > > [-Waddress-of-packed-member]
> >
> > Fixed.
> >
> > >> +      while (TREE_CODE (base) == ARRAY_REF)
> > >> +       base = TREE_OPERAND (base, 0);
> > >> +      if (TREE_CODE (base) != COMPONENT_REF)
> > >> +       return NULL_TREE;
> > >
> > >
> > > Are you deliberately not handling the other handled_component_p cases? If
> > > so, there should be a comment.
> >
> > I changed it to
> >
> >      while (handled_component_p (base))
> >         {
> >           enum tree_code code = TREE_CODE (base);
> >           if (code == COMPONENT_REF)
> >             break;
> >           switch (code)
> >             {
> >             case ARRAY_REF:
> >               base = TREE_OPERAND (base, 0);
> >               break;
> >             default:
> >               /* FIXME: Can it ever happen?  */
> >               gcc_unreachable ();
> >               break;
> >             }
> >         }
> >
> > Is there a testcase to trigger this ICE? I couldn't find one.
> >
> > >> +  /* Check alignment of the object.  */
> > >> +  if (TREE_CODE (object) == COMPONENT_REF)
> > >> +    {
> > >> +      field = TREE_OPERAND (object, 1);
> > >> +      if (TREE_CODE (field) == FIELD_DECL && DECL_PACKED (field))
> > >> +       {
> > >> +         type_align = TYPE_ALIGN (type);
> > >> +         context = DECL_CONTEXT (field);
> > >> +         record_align = TYPE_ALIGN (context);
> > >> +         if ((record_align % type_align) != 0)
> > >> +           return context;
> > >> +       }
> > >> +    }
> > >
> > >
> > > Why doesn't this recurse?  What if you have a packed field three
> > > COMPONENT_REFs down?
> >
> > My patch works on
> > [hjl@gnu-cfl-1 pr51628-4]$ cat x.i
> > struct A { int i; } __attribute__ ((packed));
> > struct B { struct A a; };
> > struct C { struct B b; };
> >
> > extern struct C *p;
> >
> > int* g8 (void) { return &p->b.a.i; }
> > [hjl@gnu-cfl-1 pr51628-4]$ make x.s
> > /export/build/gnu/tools-build/gcc-debug/build-x86_64-linux/gcc/xgcc
> > -B/export/build/gnu/tools-build/gcc-debug/build-x86_64-linux/gcc/ -O2
> > -S x.i
> > x.i: In function ‘g8’:
> > x.i:7:25: warning: taking address of packed member of ‘struct A’ may
> > result in an unaligned pointer value [-Waddress-of-packed-member]
> > 7 | int* g8 (void) { return &p->b.a.i; }
> >   |                         ^~~~~~~~~
> > [hjl@gnu-cfl-1 pr51628-4]$
> >
> > If it isn't what you had in mind, can you give me a testcase?
> >
> > >> +  if (TREE_CODE (rhs) == COND_EXPR)
> > >> +    {
> > >> +      /* Check the THEN path first.  */
> > >> +      tree op1 = TREE_OPERAND (rhs, 1);
> > >> +      context = check_address_of_packed_member (type, op1);
> > >> +      if (context)
> > >> +       rhs = op1;
> > >> +      else
> > >> +       {
> > >> +         /* Check the ELSE path.  */
> > >> +         rhs = TREE_OPERAND (rhs, 2);
> > >> +         context = check_address_of_packed_member (type, rhs);
> > >> +       }
> > >> +    }
> > >
> > >
> > > Likewise, what if you have more levels of COND_EXPR?  Or COMPOUND_EXPR
> > > within COND_EXPR?
> >
> > Fixed, now I got
> >
> > [hjl@gnu-cfl-1 pr51628-5]$ cat z.i
> > struct A {
> >   int i;
> > } __attribute__ ((packed));
> >
> > int*
> > foo3 (struct A *p1, int *q1, int *q2, struct A *p2)
> > {
> >   return (q1
> >           ? &p1->i
> >           : (q2 ? &p2->i : q2));
> > }
> > [hjl@gnu-cfl-1 pr51628-5]$ make z.s
> > /export/build/gnu/tools-build/gcc-debug/build-x86_64-linux/gcc/xgcc
> > -B/export/build/gnu/tools-build/gcc-debug/build-x86_64-linux/gcc/ -O2
> > -S z.i
> > z.i: In function ‘foo3’:
> > z.i:9:13: warning: taking address of packed member of ‘struct A’ may
> > result in an unaligned pointer value [-Waddress-of-packed-member]
> > 9 |           ? &p1->i
> >   |             ^~~~~~
> > z.i:10:19: warning: taking address of packed member of ‘struct A’ may
> > result in an unaligned pointer value [-Waddress-of-packed-member]
> > 10 |           : (q2 ? &p2->i : q2));
> >    |                   ^~~~~~
> > [hjl@gnu-cfl-1 pr51628-5]$
> >
> > >> @@ -7470,6 +7470,9 @@ convert_for_arg_passing (tree type, tree val,
> > >> tsubst_flags_t complain)
> > >> +  warn_for_address_or_pointer_of_packed_member (true, type, val);
> > >
> > >
> > >> @@ -8914,6 +8914,8 @@ convert_for_assignment (tree type, tree rhs,
> > >> +  warn_for_address_or_pointer_of_packed_member (true, type, rhs);
> > >
> > >
> > > Why would address_p be true in these calls?  It seems that you are warning
> > > at the point of assignment but looking for the warning about taking the
> > > address rather than the one about assignment.
> >
> > It happens only with C for incompatible pointer conversion:
> >
> > [hjl@gnu-cfl-1 pr51628-2]$ cat c.i
> > struct B { int i; };
> > struct C { struct B b; } __attribute__ ((packed));
> >
> > long* g8 (struct C *p) { return p; }
> > [hjl@gnu-cfl-1 pr51628-2]$ make c.s
> > /export/build/gnu/tools-build/gcc-debug/build-x86_64-linux/gcc/xgcc
> > -B/export/build/gnu/tools-build/gcc-debug/build-x86_64-linux/gcc/ -O2
> > -S c.i
> > c.i: In function ‘g8’:
> > c.i:4:33: warning: returning ‘struct C *’ from a function with
> > incompatible return type ‘long int *’ [-Wincompatible-pointer-types]
> > 4 | long* g8 (struct C *p) { return p; }
> >   |                                 ^
> > c.i:4:18: warning: converting a packed ‘struct C *’ pointer (alignment
> > 1) to ‘long int *’ (alignment 8) may may result in an unaligned
> > pointer value [-Waddress-of-packed-member]
> > 4 | long* g8 (struct C *p) { return p; }
> >   |                  ^
> > c.i:2:8: note: defined here
> > 2 | struct C { struct B b; } __attribute__ ((packed));
> >   |        ^
> > [hjl@gnu-cfl-1 pr51628-2]$
> >
> > address_p is false in this case and rhs is PARM_DECL, VAR_DECL or
> > NOP_EXPR.  This comes from convert_for_assignment in c/c-typeck.c.
> >
> > For other compatible pointer assignment, address_p is true and rhs is
> > ADDR_EXPR, PARM_DECL, VAR_DECL or NOP_EXPR.   Check
> > for  ADDR_EXPR won't work.
> >
> > address_p isn't an appropriate parameter name.  I changed it to convert_p
> > to indicate that it is an incompatible pointer type conversion.
> >
> > > If you want to warn about taking the address, shouldn't that happen under
> > > cp_build_addr_expr?  Alternately, drop the address_p parameter and choose
> > > your path inside warn_for_*_packed_member based on whether rhs is an
> > > ADDR_EXPR there rather than in the caller.
> > >
> >
> > Here is the updated patch.  OK for trunk?
> >
> > Thanks.
>
> PING:
>
> https://gcc.gnu.org/ml/gcc-patches/2018-09/msg01452.html
>
>

PING.

-- 
H.J.