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.
