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...
...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.
(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.
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;
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 ask because the phrasing of the warning "may result" suggests
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 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.
@@ -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
