On Fri, Dec 14, 2018 at 11:48 PM H.J. Lu <hjl.to...@gmail.com> wrote: > > On Fri, Dec 14, 2018 at 2:10 PM Jason Merrill <ja...@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 <ja...@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 <ja...@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 > > >>>>> <jos...@codesourcery.com> > > >>>>> wrote: > > >>>>>> > > >>>>>> On Mon, 18 Jun 2018, Jason Merrill wrote: > > >>>>>> > > >>>>>>> On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers > > >>>>>>> <jos...@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:
don't we have handled_component_p () for this? (you're still missing BIT_FIELD_REF which might be used for vector element accesses) > > 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.