On Tue, May 29, 2018 at 5:15 AM, H.J. Lu <hjl.to...@gmail.com> wrote:
> 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
>
PING.
--
H.J.
