On Fri, May 18, 2018 at 4:36 AM, H.J. Lu <hjl.to...@gmail.com> wrote:
> On Thu, May 17, 2018 at 10:32:56AM -0700, H.J. Lu wrote:
>> On Mon, May 14, 2018 at 8:00 PM, Martin Sebor <mse...@gmail.com> wrote:
>> > On 05/14/2018 01:10 PM, H.J. Lu wrote:
>> >>
>> >> On Mon, May 14, 2018 at 10:40 AM, H.J. Lu <hjl.to...@gmail.com> wrote:
>> >>
>> >>>>>> $ cat c.i
>> >>>>>> struct B { int i; };
>> >>>>>> struct C { struct B b; } __attribute__ ((packed));
>> >>>>>>
>> >>>>>> long* g8 (struct C *p) { return p; }
>> >>>>>> $ gcc -O2 -S c.i -Wno-incompatible-pointer-types
>> >>>>>> c.i: In function ‘g8’:
>> >>>>>> c.i:4:33: warning: taking value of packed 'struct C *' may result in
>> >>>>>> an
>> >>>>>> unaligned pointer value [-Waddress-of-packed-member]
>> >>>>
>> >>>>
>> >>>> ^^^^^
>> >>>> That should read "taking address" (not value) but...
>> >>>
>> >>>
>> >>> The value of 'struct C *' is an address. There is no address taken here.
>> >>>
>> >>>> ...to help explain the problem I would suggest to mention the expected
>> >>>> and actual alignment in the warning message. E.g.,
>> >>>>
>> >>>> storing the address of a packed 'struct C' in 'struct C *' increases
>> >>>> the
>> >>>> alignment of the pointer from 1 to 4.
>> >>>
>> >>>
>> >>> I will take a look.
>> >>>
>> >>>> (IIUC, the source type and destination type need not be the same so
>> >>>> including both should be helpful in those cases.)
>> >>>>
>> >>>> Adding a note pointing to the declaration of either the struct or
>> >>>> the member would help users find it if it's a header far removed
>> >>>> from the point of use.
>> >>>
>> >>>
>> >>> I will see what I can do.
>> >>
>> >>
>> >> How about this
>> >>
>> >> [hjl@gnu-skx-1 pr51628]$ cat n9.i
>> >> struct B { int i; };
>> >> struct C { struct B b; } __attribute__ ((packed));
>> >>
>> >> long* g8 (struct C *p) { return p; }
>> >> [hjl@gnu-skx-1 pr51628]$
>> >> /export/build/gnu/gcc-test/build-x86_64-linux/gcc/xgcc
>> >> -B/export/build/gnu/gcc-test/build-x86_64-linux/gcc/ -O2 -S n9.i
>> >> n9.i: In function ‘g8’:
>> >> n9.i:4:33: warning: returning ‘struct C *’ from a function with
>> >> incompatible return type ‘long int *’ [-Wincompatible-pointer-types]
>> >> long* g8 (struct C *p) { return p; }
>> >> ^
>> >> n9.i:4:33: warning: taking value of packed ‘struct C *’ increases the
>> >> alignment of the pointer from 1 to 8 [-Waddress-of-packed-member]
>> >> n9.i:2:8: note: defined here
>> >> struct C { struct B b; } __attribute__ ((packed));
>> >
>> >
>> > Mentioning the alignments looks good.
>> >
>> > I still find the "taking value" phrasing odd. I think we would
>> > describe what's going on as "converting a pointer to a packed C
>> > to a pointer to C (with an alignment of 8)" so I'd suggest to
>> > use the term converting instead.
>>
>> How about this?
>>
>> [hjl@gnu-skx-1 pr51628]$ cat n12.i
>> struct B { int i; };
>> struct C { struct B b; } __attribute__ ((packed));
>>
>> struct B* g8 (struct C *p) { return p; }
>> [hjl@gnu-skx-1 pr51628]$ make n12.s
>> /export/build/gnu/gcc-test/build-x86_64-linux/gcc/xgcc
>> -B/export/build/gnu/gcc-test/build-x86_64-linux/gcc/ -O2 -S n12.i
>> n12.i: In function ‘g8’:
>> n12.i:4:37: warning: returning ‘struct C *’ from a function with
>> incompatible return type ‘struct B *’ [-Wincompatible-pointer-types]
>> struct B* g8 (struct C *p) { return p; }
>> ^
>> n12.i:4:37: warning: converting a pointer to packed ‘struct C *’
>> increases the alignment of the pointer to ‘struct B *’ from 1 to 4
>> [-Waddress-of-packed-member]
>> n12.i:2:8: note: defined here
>> struct C { struct B b; } __attribute__ ((packed));
>> ^
>> n12.i:1:8: note: defined here
>> struct B { int i; };
>> ^
>> [hjl@gnu-skx-1 pr51628]$
>>
>> > I also think mentioning both the source and the destination types
>> > is useful irrespective of -Wincompatible-pointer-types because
>> > the latter is often suppressed using a cast, as in:
>> >
>> > struct __attribute__ ((packed)) A { int i; };
>> > struct B {
>> > struct A a;
>> > } b;
>> >
>> > long *p = (long*)&b.a.i; // -Waddress-of-packed-member
>> > int *q = (int*)&b.a; // missing warning
>> >
>> > If the types above were obfuscated by macros, typedefs, or in
>> > C++ template parameters, it could be difficult to figure out
>> > what the type of the member is because neither it nor the name
>> > of the member appears in the message.
>>
>> How about this
>>
>> [hjl@gnu-skx-1 pr51628]$ cat n13.i
>> struct __attribute__ ((packed)) A { int i; };
>> struct B {
>> struct A a;
>> } b;
>>
>> long *p = (long*)&b.a.i;
>> int *q = (int*)&b.a;
>> [hjl@gnu-skx-1 pr51628]$ make n13.s
>> /export/build/gnu/gcc-test/build-x86_64-linux/gcc/xgcc
>> -B/export/build/gnu/gcc-test/build-x86_64-linux/gcc/ -O2 -S n13.i
>> n13.i:6:18: warning: taking address of packed member of ‘struct A’ may
>> result in an unaligned pointer value [-Waddress-of-packed-member]
>> long *p = (long*)&b.a.i;
>> ^~~~~~
>> n13.i:7:16: warning: taking address of packed member of ‘struct B’ may
>> result in an unaligned pointer value [-Waddress-of-packed-member]
>> int *q = (int*)&b.a;
>> ^~~~
>> [hjl@gnu-skx-1 pr51628]$
>>
>>
>
> Here is the updated patch. OK for trunk?
>
>
PING:
https://gcc.gnu.org/ml/gcc-patches/2018-05/msg00890.html
--
H.J.
