V5 [PATCH] C/C++: Add -Waddress-of-packed-member

On Thu, Dec 13, 2018 at 12:50 PM Jason Merrill <jason@redhat.com> wrote:
>
> On 9/25/18 11:46 AM, H.J. Lu 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.
>
> You can take the address of an element of complex:
>
>    __complex int i;
>    int *p = &__real(i);
>
> You may get VIEW_CONVERT_EXPR with location wrappers.

Fixed.  I replaced gcc_unreachable with return NULL_TREE;

> >>> +  /* 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?
>
> In that testcase, 'i' is the top COMPONENT_EXPR.  What I was talking
> about would be more like
>
>   struct A { int i; };
>   struct B { struct A a; };
>   struct C { struct B b __attribute__ ((packed)); };
>
>   extern struct C *p;
>
>   int* g8 (void) { return &p->b.a.i; }
>

Fixed with a recursive call:

  tree context = check_alignment_of_packed_member (type, field);
  if (context)
    return context;

  /* Check alignment of the object.  */
  while (TREE_CODE (object) == COMPONENT_REF)
    {
      field = TREE_OPERAND (object, 1);
      context = check_alignment_of_packed_member (type, field);
      if (context)
        return context;
      object = TREE_OPERAND (object, 0);
    }

  return NULL_TREE;

Here is the updated patch.

Tested on i686 and x86-64.  OK for trunk?

Thanks.

Message ID	CAMe9rOq67grqBtdDL7YQdAwGp+9KAqiuQbs_gyiXdemNoX19Ng@mail.gmail.com
State	New
Headers	show Return-Path: <gcc-patches-return-492428-incoming=patchwork.ozlabs.org@gcc.gnu.org> DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:references:in-reply-to:from:date:message-id :subject:to:cc:content-type; q=dns; s=default; b=gbnCyRlMGiGiOSA ndl4mULmAa1otqEoHgMJhQ6zIpVO1MkRH6oXf0poANDf1Q8de5nsRqvEcpvXLsKx QZxPHqqJTLCPgythTEbSaLXoboboQXiRbj0yG14xGzFNC+9SMO1RBDiw589cwLId WrUsZGU4tuQaVYCliauAjinIywrU= Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk Sender: gcc-patches-owner@gcc.gnu.org MIME-Version: 1.0 References: <CAMe9rOohUOR+eta2gvwNxbjMR1y=P=vA3+Sniaj_tS03qamj6g@mail.gmail.com> <d3a6bf60-4c66-54d7-b159-a6f6b5ecfdbe@redhat.com> In-Reply-To: <d3a6bf60-4c66-54d7-b159-a6f6b5ecfdbe@redhat.com> From: "H.J. Lu" <hjl.tools@gmail.com> Date: Thu, 13 Dec 2018 15:56:08 -0800 Message-ID: <CAMe9rOq67grqBtdDL7YQdAwGp+9KAqiuQbs_gyiXdemNoX19Ng@mail.gmail.com> Subject: V5 [PATCH] C/C++: Add -Waddress-of-packed-member To: Jason Merrill <jason@redhat.com> Cc: "Joseph S. Myers" <joseph@codesourcery.com>, Martin Sebor <msebor@gmail.com>, GCC Patches <gcc-patches@gcc.gnu.org> Content-Type: multipart/mixed; boundary="000000000000278d7e057cf011a2"
Series	V5 [PATCH] C/C++: Add -Waddress-of-packed-member \| expand V5 [PATCH] C/C++: Add -Waddress-of-packed-member

V5 [PATCH] C/C++: Add -Waddress-of-packed-member

Commit Message

Comments

Patch