On Fri, Dec 14, 2018 at 2:10 PM Jason Merrill <jason@redhat.com> wrote:
>
> On 12/13/18 6:56 PM, H.J. Lu wrote:
> > 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;
>
> Then we're back to my earlier question: are you deliberately not
> handling the other cases?  Why not look through them as well?  What if
> e.g. the operand of __real is a packed field?
>

Here is the updated patch with

diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c
index 615134cfdac..f105742598e 100644
--- a/gcc/c-family/c-warn.c
+++ b/gcc/c-family/c-warn.c
@@ -2669,6 +2669,9 @@ check_address_of_packed_member (tree type, tree rhs)
    switch (code)
      {
      case ARRAY_REF:
+     case REALPART_EXPR:
+     case IMAGPART_EXPR:
+     case VIEW_CONVERT_EXPR:
        base = TREE_OPERAND (base, 0);
        break;
      default:

Now I got

[hjl@gnu-cfl-1 pr51628-6]$ cat foo.i
struct A { __complex int i; };
struct B { struct A a; };
struct C { struct B b __attribute__ ((packed)); };

extern struct C *p;

int*
foo1 (void)
{
  return &__real(p->b.a.i);
}
int*
foo2 (void)
{
  return &__imag(p->b.a.i);
}
[hjl@gnu-cfl-1 pr51628-6]$ make foo.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 foo.i
foo.i: In function ‘foo1’:
foo.i:10:10: warning: taking address of packed member of ‘struct C’
may result in an unaligned pointer value [-Waddress-of-packed-member]
   10 |   return &__real(p->b.a.i);
      |          ^~~~~~~~~~~~~~~~~
foo.i: In function ‘foo2’:
foo.i:15:10: warning: taking address of packed member of ‘struct C’
may result in an unaligned pointer value [-Waddress-of-packed-member]
   15 |   return &__imag(p->b.a.i);
      |          ^~~~~~~~~~~~~~~~~
[hjl@gnu-cfl-1 pr51628-6]$

OK for trunk?

-- 
H.J.