On Mon, May 14, 2018 at 9:18 AM, Martin Sebor <mse...@gmail.com> wrote:
> On 05/14/2018 07:44 AM, H.J. Lu wrote:
>>
>> On Wed, Apr 25, 2018 at 7:54 PM, H.J. Lu <hongjiu...@intel.com> wrote:
>>>
>>> When address of packed member of struct or union is taken, it may result
>>> in an unaligned pointer value. This patch adds
>>> -Waddress-of-packed-member
>>> to check alignment at pointer assignment and warn unaligned address as
>>> well as unaligned pointer:
>
>
> This isn't a complete review, just some high level observations
> and suggestions for things I noticed.
>
>>> $ cat x.i
>>> struct pair_t
>>> {
>>> char c;
>>> int i;
>>> } __attribute__ ((packed));
>>>
>>> extern struct pair_t p;
>>> int *addr = &p.i;
>>> $ gcc -O2 -S x.i
>>> x.i:8:13: warning: taking address of packed member of 'struct pair_t'
>>> may result in an unaligned pointer value [-Waddress-of-packed-member]
>>> int *addr = &p.i;
>>> ^
>>> $ 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.
> Here's a question: Should the following trigger the warning (it
> doesn't, either with your patch or with Clang, even though the
> pointer is misaligned).
>
> struct __attribute__ ((packed)) A { int i; } a;
> struct B: A { int j; } b;
>
> int B::*pbi = &A::i;
Can you show the misaligned pointer in the generated code?
>>> long* g8 (struct C *p) { return p; }
>>> ^
>>> $
>>>
>>> This warning is enabled by default.
>
>
> Can you please explain the reasoning behind this decision?
>
> Is it because the warning is meant to have no false positives
> (and the GCC diagnostic guidelines suggest that such warnings
> be on by default), or because Clang enables it by default?
I don't have strong opinion here.
> I ask because the phrasing of the warning "may result" suggests
"may result" is used since 4-byte alignment is also 1-byte alignment.
> it's not free of false positives. I'm probably missing something,
> and so...
>
> ...I would suggest to expand the documentation a bit and add
> an example showing when the warning triggers, when it doesn't.
> The added text says that taking the address of a packed member
> "usually results in an unaligned pointer value." When does it
> not result in one? Would a warning in such cases be a false
> positive?
Since any alignment is 1-byte alignment, a 1-byte aligned address
may be 8-byte aligned.
>
>>> Since read_encoded_value_with_base
>>> in unwind-pe.h has
>>>
>>> union unaligned
>>> {
>>> void *ptr;
>>> unsigned u2 __attribute__ ((mode (HI)));
>>> unsigned u4 __attribute__ ((mode (SI)));
>>> unsigned u8 __attribute__ ((mode (DI)));
>>> signed s2 __attribute__ ((mode (HI)));
>>> signed s4 __attribute__ ((mode (SI)));
>>> signed s8 __attribute__ ((mode (DI)));
>>> } __attribute__((__packed__));
>>> _Unwind_Internal_Ptr result;
>>>
>>> and GCC warns:
>>>
>>> gcc/libgcc/unwind-pe.h:210:37: warning: taking address of packed member
>>> of 'union unaligned' may result in an unaligned pointer value
>>> [-Waddress-of-packed-member]
>>> result = (_Unwind_Internal_Ptr) u->ptr;
>>> ^
>>> we need to add GCC pragma to ignore -Waddress-of-packed-member.
>>>
>>> OK for trunk?
>>>
>>> H.J.
>>> ----
>>> gcc/c/
>>>
>>> PR c/51628
>>> * doc/invoke.texi: Document -Wno-address-of-packed-member.
>>>
>>> gcc/c-family/
>>>
>>> PR c/51628
>>> * c-common.h (warn_for_address_of_packed_member): New.
>>> * c-warn.c (check_address_of_packed_member): New function.
>>> (warn_for_address_of_packed_member): Likewise.
>>> * c.opt: Add -Wno-address-of-packed-member.
>>>
>>> gcc/c/
>>>
>>> PR c/51628
>>> * c-typeck.c (warn_for_pointer_of_packed_member): New function.
>>> (convert_for_assignment): Call warn_for_address_of_packed_member
>>> and warn_for_pointer_of_packed_member.
>
>
> The comment in the hunk below refers to symbols that don't correspond
> to any of the arguments (ERRTYPE, PARNUM, and NAME).
>
> +/* Warn if the right hand poiner value RHS isn't aligned to a
> + pointer type TYPE. ERRTYPE says whether it is argument passing,
> + assignment, initialization or return. PARMNUM is the number of
> + the argument, for printing in error messages. NAME is the name
> + of the function. */
> +
> +static void
> +warn_for_pointer_of_packed_member (location_t location, tree type,
> + tree rhs)
> +{
>
> Similarly, in the hunk below, LHS doesn't refer to any variable
> either used or declared in the context.
I will fix them.
> @@ -6986,6 +7037,13 @@ convert_for_assignment (location_t location,
> location_t expr_loc, tree type,
> }
> }
>
> + /* If LHS is't an address, check pointer or array of packed
> + struct or union. */
> + if (TREE_CODE (orig_rhs) != ADDR_EXPR)
> + warn_for_pointer_of_packed_member (location, type, orig_rhs);
> + else
> + warn_for_address_of_packed_member (type, orig_rhs);
> +
>
> Martin
>
Thanks.
--
H.J.
