Re: V10 [PATCH] C/C++: Add -Waddress-of-packed-member
On Thu, Dec 20, 2018 at 1:28 PM Jason Merrill wrote:
>
> On 12/20/18 2:52 PM, H.J. Lu wrote:
> > On Thu, Dec 20, 2018 at 11:28 AM Jason Merrill wrote:
> >>
> >> On 12/19/18 12:35 PM, H.J. Lu wrote:
> >>> + while (handled_component_p (rhs))
> >>> +{
> >>> + if (TREE_CODE (rhs) == COMPONENT_REF)
> >>> + break;
> >>> + rhs = TREE_OPERAND (rhs, 0);
> >>> +}
> >>> +
> >>> + if (TREE_CODE (rhs) != COMPONENT_REF)
> >>> +return NULL_TREE;
> >>> +
> >>> + object = TREE_OPERAND (rhs, 0);
> >>> + field = TREE_OPERAND (rhs, 1);
> >>> +
> >>> + tree context = check_alignment_of_packed_member (type, field);
> >>> + if (context)
> >>> +return context;
> >>
> >> All the above looks unnecessary; it will be handled by the loop below.
> >>
> >>> + /* Check alignment of the object. */
> >>> + while (handled_component_p (object))
> >>> +{
> >>> + if (TREE_CODE (object) == COMPONENT_REF)
> >>> + {
> >>> + do
> >>> + {
> >>> + field = TREE_OPERAND (object, 1);
> >>> + context = check_alignment_of_packed_member (type, field);
> >>> + if (context)
> >>> + return context;
> >>> + object = TREE_OPERAND (object, 0);
> >>> + }
> >>> + while (TREE_CODE (object) == COMPONENT_REF);
> >>
> >> This inner loop also seems unnecessary.
> >>
> >>> + }
> >>> + else
> >>> + object = TREE_OPERAND (object, 0);
> >>> +}
> >>
> >> Jason
> >
> > I changed it to
> >
> > static tree
> > check_address_of_packed_member (tree type, tree rhs)
> > {
> >if (INDIRECT_REF_P (rhs))
> > rhs = TREE_OPERAND (rhs, 0);
> >
> >if (TREE_CODE (rhs) == ADDR_EXPR)
> > rhs = TREE_OPERAND (rhs, 0);
> >
> >tree context = NULL_TREE;
> >
> >/* Check alignment of the object. */
> >while (handled_component_p (rhs))
> > {
> >if (TREE_CODE (rhs) == COMPONENT_REF)
> > {
> >tree field = TREE_OPERAND (rhs, 1);
> >context = check_alignment_of_packed_member (type, field);
> >if (context)
> > break;
> > }
> >rhs = TREE_OPERAND (rhs, 0);
> > }
> >
> >return context;
> > }
> >
> > Here is the updated patch. OK for trunk?
>
> OK.
>
Checked in. Thanks for everyone, especially Jason.
--
H.J.
Re: V10 [PATCH] C/C++: Add -Waddress-of-packed-member
On 12/20/18 2:52 PM, H.J. Lu wrote:
On Thu, Dec 20, 2018 at 11:28 AM Jason Merrill wrote:
On 12/19/18 12:35 PM, H.J. Lu wrote:
+ while (handled_component_p (rhs))
+{
+ if (TREE_CODE (rhs) == COMPONENT_REF)
+ break;
+ rhs = TREE_OPERAND (rhs, 0);
+}
+
+ if (TREE_CODE (rhs) != COMPONENT_REF)
+return NULL_TREE;
+
+ object = TREE_OPERAND (rhs, 0);
+ field = TREE_OPERAND (rhs, 1);
+
+ tree context = check_alignment_of_packed_member (type, field);
+ if (context)
+return context;
All the above looks unnecessary; it will be handled by the loop below.
+ /* Check alignment of the object. */
+ while (handled_component_p (object))
+{
+ if (TREE_CODE (object) == COMPONENT_REF)
+ {
+ do
+ {
+ field = TREE_OPERAND (object, 1);
+ context = check_alignment_of_packed_member (type, field);
+ if (context)
+ return context;
+ object = TREE_OPERAND (object, 0);
+ }
+ while (TREE_CODE (object) == COMPONENT_REF);
This inner loop also seems unnecessary.
+ }
+ else
+ object = TREE_OPERAND (object, 0);
+}
Jason
I changed it to
static tree
check_address_of_packed_member (tree type, tree rhs)
{
if (INDIRECT_REF_P (rhs))
rhs = TREE_OPERAND (rhs, 0);
if (TREE_CODE (rhs) == ADDR_EXPR)
rhs = TREE_OPERAND (rhs, 0);
tree context = NULL_TREE;
/* Check alignment of the object. */
while (handled_component_p (rhs))
{
if (TREE_CODE (rhs) == COMPONENT_REF)
{
tree field = TREE_OPERAND (rhs, 1);
context = check_alignment_of_packed_member (type, field);
if (context)
break;
}
rhs = TREE_OPERAND (rhs, 0);
}
return context;
}
Here is the updated patch. OK for trunk?
OK.
Jason
V10 [PATCH] C/C++: Add -Waddress-of-packed-member
On Thu, Dec 20, 2018 at 11:28 AM Jason Merrill wrote:
>
> On 12/19/18 12:35 PM, H.J. Lu wrote:
> > + while (handled_component_p (rhs))
> > +{
> > + if (TREE_CODE (rhs) == COMPONENT_REF)
> > + break;
> > + rhs = TREE_OPERAND (rhs, 0);
> > +}
> > +
> > + if (TREE_CODE (rhs) != COMPONENT_REF)
> > +return NULL_TREE;
> > +
> > + object = TREE_OPERAND (rhs, 0);
> > + field = TREE_OPERAND (rhs, 1);
> > +
> > + tree context = check_alignment_of_packed_member (type, field);
> > + if (context)
> > +return context;
>
> All the above looks unnecessary; it will be handled by the loop below.
>
> > + /* Check alignment of the object. */
> > + while (handled_component_p (object))
> > +{
> > + if (TREE_CODE (object) == COMPONENT_REF)
> > + {
> > + do
> > + {
> > + field = TREE_OPERAND (object, 1);
> > + context = check_alignment_of_packed_member (type, field);
> > + if (context)
> > + return context;
> > + object = TREE_OPERAND (object, 0);
> > + }
> > + while (TREE_CODE (object) == COMPONENT_REF);
>
> This inner loop also seems unnecessary.
>
> > + }
> > + else
> > + object = TREE_OPERAND (object, 0);
> > +}
>
> Jason
I changed it to
static tree
check_address_of_packed_member (tree type, tree rhs)
{
if (INDIRECT_REF_P (rhs))
rhs = TREE_OPERAND (rhs, 0);
if (TREE_CODE (rhs) == ADDR_EXPR)
rhs = TREE_OPERAND (rhs, 0);
tree context = NULL_TREE;
/* Check alignment of the object. */
while (handled_component_p (rhs))
{
if (TREE_CODE (rhs) == COMPONENT_REF)
{
tree field = TREE_OPERAND (rhs, 1);
context = check_alignment_of_packed_member (type, field);
if (context)
break;
}
rhs = TREE_OPERAND (rhs, 0);
}
return context;
}
Here is the updated patch. OK for trunk?
Thanks.
--
H.J.
From f2982a694a2fab4dd1ce2a6819006b451fa0c9a7 Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Fri, 12 Jan 2018 21:12:05 -0800
Subject: [PATCH] C/C++: Add -Waddress-of-packed-member
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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:
$ 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]
8 | 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:18: warning: converting a packed ‘struct C *’ pointer (alignment 1) to ‘long int *’ (alignment 8) may may result in an unaligned pointer value [-Waddress-of-packed-member]
4 | long* g8 (struct C *p) { return p; }
| ^
c.i:2:8: note: defined here
2 | struct C { struct B b; } __attribute__ ((packed));
|^
$
This warning is enabled by default. 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.
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_or_pointer_of_packed_member): New.
* c-warn.c (check_alignment_of_packed_member): New function.
(check_address_of_packed_member): Likewise.
(check_and_warn_address_of_packed_member): Likewise.
(warn_for_address_or_pointer_of_packed_member): Likewise.
* c.opt: Add -Wno-address-of-packed-member.
gcc/c/
PR c/51628
* c-typeck.c (convert_for_assignment): Call
warn_for_address_or_pointer_of_packed_member.
gcc/cp/
PR c/51628
* call.c (convert_for_arg_passing): Call
wa
Re: V9 [PATCH] C/C++: Add -Waddress-of-packed-member
On 12/19/18 12:35 PM, H.J. Lu wrote:
+ while (handled_component_p (rhs))
+{
+ if (TREE_CODE (rhs) == COMPONENT_REF)
+ break;
+ rhs = TREE_OPERAND (rhs, 0);
+}
+
+ if (TREE_CODE (rhs) != COMPONENT_REF)
+return NULL_TREE;
+
+ object = TREE_OPERAND (rhs, 0);
+ field = TREE_OPERAND (rhs, 1);
+
+ tree context = check_alignment_of_packed_member (type, field);
+ if (context)
+return context;
All the above looks unnecessary; it will be handled by the loop below.
+ /* Check alignment of the object. */
+ while (handled_component_p (object))
+{
+ if (TREE_CODE (object) == COMPONENT_REF)
+ {
+ do
+ {
+ field = TREE_OPERAND (object, 1);
+ context = check_alignment_of_packed_member (type, field);
+ if (context)
+ return context;
+ object = TREE_OPERAND (object, 0);
+ }
+ while (TREE_CODE (object) == COMPONENT_REF);
This inner loop also seems unnecessary.
+ }
+ else
+ object = TREE_OPERAND (object, 0);
+}
Jason
V9 [PATCH] C/C++: Add -Waddress-of-packed-member
bject))
> {
> if (TREE_CODE (object) == COMPONENT_REF)
> {
> do
> {
> field = TREE_OPERAND (object, 1);
> context = check_alignment_of_packed_member (type, field);
> if (context)
> return context;
> object = TREE_OPERAND (object, 0);
> }
> while (TREE_CODE (object) == COMPONENT_REF);
> }
> else
> object = TREE_OPERAND (object, 0);
> }
I got
[hjl@gnu-cfl-1 pr51628-6]$ cat a.i
struct A
{
int i;
};
struct B
{
char c;
__attribute ((packed)) struct A ar[4];
};
struct B b;
int *p = &b.ar[1].i;
[hjl@gnu-cfl-1 pr51628-6]$ make a.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 a.i
a.i:14:10: warning: taking address of packed member of ‘struct B’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
14 | int *p = &b.ar[1].i;
| ^~
[hjl@gnu-cfl-1 pr51628-6]$
> > > + if (TREE_CODE (rhs) != COND_EXPR)
> > > +{
> > > + while (TREE_CODE (rhs) == COMPOUND_EXPR)
> > > + rhs = TREE_OPERAND (rhs, 1);
> >
> > What if you have a COND_EXPR inside a COMPOUND_EXPR?
> >
>
> It works for me:
>
> [hjl@gnu-cfl-1 pr51628-5]$ cat c.i
> struct A {
> int i;
> } __attribute__ ((packed));
>
> int*
> foo3 (struct A *p1, int **q1, int *q2, int *q3, struct A *p2)
> {
> return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q1 = &p1->i, *q2 = 2, &p2->i): q2);
> }
> [hjl@gnu-cfl-1 pr51628-5]$
> /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 c.i
> c.i: In function \u2018foo3\u2019:
> c.i:8:20: warning: assignment to \u2018int *\u2019 from
> \u2018int\u2019 makes pointer from integer without a cast
> [-Wint-conversion]
> 8 | return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q1 = &p1->i, *q2 =
> 2, &p2->i): q2);
> |^
> c.i:8:48: warning: taking address of packed member of \u2018struct
> A\u2019 may result in an unaligned pointer value
> [-Waddress-of-packed-member]
> 8 | return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q1 = &p1->i, *q2 =
> 2, &p2->i): q2);
> |^~
> c.i:8:25: warning: taking address of packed member of \u2018struct
> A\u2019 may result in an unaligned pointer value
> [-Waddress-of-packed-member]
> 8 | return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q1 = &p1->i, *q2 =
> 2, &p2->i): q2);
> | ^~
> c.i:8:65: warning: taking address of packed member of \u2018struct
> A\u2019 may result in an unaligned pointer value
> [-Waddress-of-packed-member]
> 8 | return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q1 = &p1->i, *q2 =
> 2, &p2->i): q2);
> | ^~
> [hjl@gnu-cfl-1 pr51628-5]$
>
> If it isn't what you meant, can you give me a testcase?
>
Here is the updated patch. OK for trunk?
Thanks.
--
H.J.
From 4a5418d5e93369cb5648ba6c0f33203bb6fa8a3f Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Fri, 12 Jan 2018 21:12:05 -0800
Subject: [PATCH] C/C++: Add -Waddress-of-packed-member
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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:
$ 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]
8 | 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:18: warning: converting a packed ‘struct C *’ pointer (alignment 1) to ‘long int *’ (alignment 8) may may result in an unaligned pointer value [-Waddress-of-packed-member]
4 | long* g8 (struct C *p) { return p; }
| ^
c.i:2:8: note: defined here
2 | struct C { struct B b; } __attribute__ ((packed));
|^
$
This warning is enabled by default. Since read_encoded_value_with_base
in unwind-pe.h has
union unaligned
{
void *ptr;
unsigned u2 _
Re: V8 [PATCH] C/C++: Add -Waddress-of-packed-member
On Tue, Dec 18, 2018 at 7:19 PM Sandra Loosemore
wrote:
>
> On 12/18/18 2:12 PM, H.J. Lu wrote:
> >
> > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > index ac2ee59d92c..47f2fc3f518 100644
> > --- a/gcc/doc/invoke.texi
> > +++ b/gcc/doc/invoke.texi
> > @@ -358,6 +358,7 @@ Objective-C and Objective-C++ Dialects}.
> > -Wuseless-cast -Wvariadic-macros -Wvector-operation-performance @gol
> > -Wvla -Wvla-larger-than=@var{byte-size} -Wvolatile-register-var @gol
> > -Wwrite-strings @gol
> > +-Waddress-of-packed-member @gol
> > -Wzero-as-null-pointer-constant -Whsa}
> >
> > @item C and Objective-C-only Warning Options
>
> Minor documentation nit: it looks like some effort has been made to
> alphabetize that list. Can you please put -Waddress-of-packed member in
> the right place, and also fix the misplaced -Whsa at the end?
>
I am applying
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 47f2fc3f518..14365fba501 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -281,7 +281,8 @@ Objective-C and Objective-C++ Dialects}.
@xref{Warning Options,,Options to Request or Suppress Warnings}.
@gccoptlist{-fsyntax-only -fmax-errors=@var{n} -Wpedantic @gol
-pedantic-errors @gol
--w -Wextra -Wall -Waddress -Waggregate-return -Waligned-new @gol
+-w -Wextra -Wall -Waddress -Waddress-of-packed-member @gol
+-Waggregate-return -Waligned-new @gol
-Walloc-zero -Walloc-size-larger-than=@var{byte-size} @gol
-Walloca -Walloca-larger-than=@var{byte-size} @gol
-Wno-aggressive-loop-optimizations -Warray-bounds -Warray-bounds=@var{n} @gol
@@ -310,7 +311,7 @@ Objective-C and Objective-C++ Dialects}.
-Wformat-y2k -Wframe-address @gol
-Wframe-larger-than=@var{byte-size} -Wno-free-nonheap-object @gol
-Wjump-misses-init @gol
--Wif-not-aligned @gol
+-Whsa -Wif-not-aligned @gol
-Wignored-qualifiers -Wignored-attributes -Wincompatible-pointer-types @gol
-Wimplicit -Wimplicit-fallthrough -Wimplicit-fallthrough=@var{n} @gol
-Wimplicit-function-declaration -Wimplicit-int @gol
@@ -358,8 +359,7 @@ Objective-C and Objective-C++ Dialects}.
-Wuseless-cast -Wvariadic-macros -Wvector-operation-performance @gol
-Wvla -Wvla-larger-than=@var{byte-size} -Wvolatile-register-var @gol
-Wwrite-strings @gol
--Waddress-of-packed-member @gol
--Wzero-as-null-pointer-constant -Whsa}
+-Wzero-as-null-pointer-constant}
@item C and Objective-C-only Warning Options
@gccoptlist{-Wbad-function-cast -Wmissing-declarations @gol
Thanks.
--
H.J.
Re: V8 [PATCH] C/C++: Add -Waddress-of-packed-member
On Tue, Dec 18, 2018 at 2:14 PM Jason Merrill wrote:
>
> On 12/18/18 4:12 PM, H.J. Lu wrote:
> > On Tue, Dec 18, 2018 at 12:36 PM Jason Merrill wrote:
> >>
> >> On 12/18/18 9:10 AM, H.J. Lu wrote:
> >>> + switch (TREE_CODE (rhs))
> >>> +{
> >>> +case ADDR_EXPR:
> >>> + base = TREE_OPERAND (rhs, 0);
> >>> + while (handled_component_p (base))
> >>> + {
> >>> + if (TREE_CODE (base) == COMPONENT_REF)
> >>> + break;
> >>> + base = TREE_OPERAND (base, 0);
> >>> + }
> >>> + if (TREE_CODE (base) != COMPONENT_REF)
> >>> + return NULL_TREE;
> >>> + object = TREE_OPERAND (base, 0);
> >>> + field = TREE_OPERAND (base, 1);
> >>> + break;
> >>> +case COMPONENT_REF:
> >>> + object = TREE_OPERAND (rhs, 0);
> >>> + field = TREE_OPERAND (rhs, 1);
> >>> + break;
> >>> +default:
> >>> + return NULL_TREE;
> >>> +}
> >>> +
> >>> + tree context = check_alignment_of_packed_member (type, field);
> >>> + if (context)
> >>> +return context;
> >>> +
> >>> + /* Check alignment of the object. */
> >>> + while (TREE_CODE (object) == COMPONENT_REF)
> >>> +{
> >>> + field = TREE_OPERAND (object, 1);
> >>> + context = check_alignment_of_packed_member (type, field);
> >>> + if (context)
> >>> + return context;
> >>> + object = TREE_OPERAND (object, 0);
> >>> +}
> >>> +
> >>
> >> You can see interleaved COMPONENT_REF and ARRAY_REF that this still
> >> doesn't look like it will handle, something like
> >>
> >> struct A
> >> {
> >> int i;
> >> };
> >>
> >> struct B
> >> {
> >> char c;
> >> __attribute ((packed)) A ar[4];
> >> };
> >>
> >> B b;
> >>
> >> int *p = &b.ar[1].i;
> >>
> >> Rather than have a loop in the ADDR_EXPR case of the switch, you can
> >> handle everything in the lower loop. And not have a switch at all, just
> >> strip any ADDR_EXPR before the loop.
> >
> > I changed it to
> >
> > if (TREE_CODE (rhs) == ADDR_EXPR)
> > rhs = TREE_OPERAND (rhs, 0);
> >while (handled_component_p (rhs))
> > {
> >if (TREE_CODE (rhs) == COMPONENT_REF)
> > break;
> >rhs = TREE_OPERAND (rhs, 0);
> > }
> >
> >if (TREE_CODE (rhs) != COMPONENT_REF)
> > return NULL_TREE;
> >
> >object = TREE_OPERAND (rhs, 0);
> >field = TREE_OPERAND (rhs, 1);
>
> That still doesn't warn about my testcase above.
>
> > [hjl@gnu-cfl-1 pr51628-6]$ cat a.i
> > struct A
> > {
> > int i;
> > } __attribute ((packed));
> >
> > struct B
> > {
> > char c;
> > struct A ar[4];
> > };
> >
> > struct B b;
> >
> > int *p = &b.ar[1].i;
>
> This testcase is importantly different because 'i' is packed, whereas in
> my testcase only the ar member of B is packed.
>
> My suggestion was that this loop:
>
> > + /* Check alignment of the object. */
> > + while (TREE_CODE (object) == COMPONENT_REF)
> > +{
> > + field = TREE_OPERAND (object, 1);
> > + context = check_alignment_of_packed_member (type, field);
> > + if (context)
> > + return context;
> > + object = TREE_OPERAND (object, 0);
> > +}
>
> could loop over all handled_component_p, but only call
> check_alignment_of_packed_member for COMPONENT_REF.
Thanks for the hint. I changed it to
/* Check alignment of the object. */
while (handled_component_p (object))
{
if (TREE_CODE (object) == COMPONENT_REF)
{
do
{
field = TREE_OPERAND (object, 1);
context = check_alignment_of_packed_member (type, field);
if (context)
return context;
object = TREE_OPERAND (object, 0);
}
while (TREE_CODE (object) == COMPONENT_REF);
}
else
object = TREE_OPERAND (object, 0);
}
> > + if (TREE_CODE (rhs) != COND_EXPR)
> > +{
> > + while (TREE_CODE (rhs) == COMPOUND_EXPR)
> > + rhs = TREE_OPERAND (rhs, 1);
>
> What if you have a COND_EXPR inside a COMPOUND_EXPR?
>
It works for me:
[hjl@gnu-cfl-1 pr51628-5]$ cat c.i
struct A {
int i;
} __attribute__ ((packed));
int*
foo3 (struct A *p1, int **q1, int *q2, int *q3, struct A *p2)
{
return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q1 = &p1->i, *q2 = 2, &p2->i): q2);
}
[hjl@gnu-cfl-1 pr51628-5]$
/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 c.i
c.i: In function \u2018foo3\u2019:
c.i:8:20: warning: assignment to \u2018int *\u2019 from
\u2018int\u2019 makes pointer from integer without a cast
[-Wint-conversion]
8 | return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q1 = &p1->i, *q2 =
2, &p2->i): q2);
|^
c.i:8:48: warning: taking address of packed member of \u2018struct
A\u2019 may result in an unaligned pointer value
[-Waddress-of-packed-member]
8 | return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q1 = &p1->i, *q2 =
2, &p2->i): q2);
|
Re: V8 [PATCH] C/C++: Add -Waddress-of-packed-member
On 12/18/18 2:12 PM, H.J. Lu wrote:
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index ac2ee59d92c..47f2fc3f518 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -358,6 +358,7 @@ Objective-C and Objective-C++ Dialects}.
-Wuseless-cast -Wvariadic-macros -Wvector-operation-performance @gol
-Wvla -Wvla-larger-than=@var{byte-size} -Wvolatile-register-var @gol
-Wwrite-strings @gol
+-Waddress-of-packed-member @gol
-Wzero-as-null-pointer-constant -Whsa}
@item C and Objective-C-only Warning Options
Minor documentation nit: it looks like some effort has been made to
alphabetize that list. Can you please put -Waddress-of-packed member in
the right place, and also fix the misplaced -Whsa at the end?
-Sandra
Re: V8 [PATCH] C/C++: Add -Waddress-of-packed-member
On 12/18/18 4:12 PM, H.J. Lu wrote:
On Tue, Dec 18, 2018 at 12:36 PM Jason Merrill wrote:
On 12/18/18 9:10 AM, H.J. Lu wrote:
+ switch (TREE_CODE (rhs))
+{
+case ADDR_EXPR:
+ base = TREE_OPERAND (rhs, 0);
+ while (handled_component_p (base))
+ {
+ if (TREE_CODE (base) == COMPONENT_REF)
+ break;
+ base = TREE_OPERAND (base, 0);
+ }
+ if (TREE_CODE (base) != COMPONENT_REF)
+ return NULL_TREE;
+ object = TREE_OPERAND (base, 0);
+ field = TREE_OPERAND (base, 1);
+ break;
+case COMPONENT_REF:
+ object = TREE_OPERAND (rhs, 0);
+ field = TREE_OPERAND (rhs, 1);
+ break;
+default:
+ return NULL_TREE;
+}
+
+ tree context = check_alignment_of_packed_member (type, field);
+ if (context)
+return context;
+
+ /* Check alignment of the object. */
+ while (TREE_CODE (object) == COMPONENT_REF)
+{
+ field = TREE_OPERAND (object, 1);
+ context = check_alignment_of_packed_member (type, field);
+ if (context)
+ return context;
+ object = TREE_OPERAND (object, 0);
+}
+
You can see interleaved COMPONENT_REF and ARRAY_REF that this still
doesn't look like it will handle, something like
struct A
{
int i;
};
struct B
{
char c;
__attribute ((packed)) A ar[4];
};
B b;
int *p = &b.ar[1].i;
Rather than have a loop in the ADDR_EXPR case of the switch, you can
handle everything in the lower loop. And not have a switch at all, just
strip any ADDR_EXPR before the loop.
I changed it to
if (TREE_CODE (rhs) == ADDR_EXPR)
rhs = TREE_OPERAND (rhs, 0);
while (handled_component_p (rhs))
{
if (TREE_CODE (rhs) == COMPONENT_REF)
break;
rhs = TREE_OPERAND (rhs, 0);
}
if (TREE_CODE (rhs) != COMPONENT_REF)
return NULL_TREE;
object = TREE_OPERAND (rhs, 0);
field = TREE_OPERAND (rhs, 1);
That still doesn't warn about my testcase above.
[hjl@gnu-cfl-1 pr51628-6]$ cat a.i
struct A
{
int i;
} __attribute ((packed));
struct B
{
char c;
struct A ar[4];
};
struct B b;
int *p = &b.ar[1].i;
This testcase is importantly different because 'i' is packed, whereas in
my testcase only the ar member of B is packed.
My suggestion was that this loop:
+ /* Check alignment of the object. */
+ while (TREE_CODE (object) == COMPONENT_REF)
+{
+ field = TREE_OPERAND (object, 1);
+ context = check_alignment_of_packed_member (type, field);
+ if (context)
+ return context;
+ object = TREE_OPERAND (object, 0);
+}
could loop over all handled_component_p, but only call
check_alignment_of_packed_member for COMPONENT_REF.
+ if (TREE_CODE (rhs) != COND_EXPR)
+{
+ while (TREE_CODE (rhs) == COMPOUND_EXPR)
+ rhs = TREE_OPERAND (rhs, 1);
What if you have a COND_EXPR inside a COMPOUND_EXPR?
Jason
V8 [PATCH] C/C++: Add -Waddress-of-packed-member
On Tue, Dec 18, 2018 at 12:36 PM Jason Merrill wrote:
>
> On 12/18/18 9:10 AM, H.J. Lu wrote:
> > + switch (TREE_CODE (rhs))
> > +{
> > +case ADDR_EXPR:
> > + base = TREE_OPERAND (rhs, 0);
> > + while (handled_component_p (base))
> > + {
> > + if (TREE_CODE (base) == COMPONENT_REF)
> > + break;
> > + base = TREE_OPERAND (base, 0);
> > + }
> > + if (TREE_CODE (base) != COMPONENT_REF)
> > + return NULL_TREE;
> > + object = TREE_OPERAND (base, 0);
> > + field = TREE_OPERAND (base, 1);
> > + break;
> > +case COMPONENT_REF:
> > + object = TREE_OPERAND (rhs, 0);
> > + field = TREE_OPERAND (rhs, 1);
> > + break;
> > +default:
> > + return NULL_TREE;
> > +}
> > +
> > + tree context = check_alignment_of_packed_member (type, field);
> > + if (context)
> > +return context;
> > +
> > + /* Check alignment of the object. */
> > + while (TREE_CODE (object) == COMPONENT_REF)
> > +{
> > + field = TREE_OPERAND (object, 1);
> > + context = check_alignment_of_packed_member (type, field);
> > + if (context)
> > + return context;
> > + object = TREE_OPERAND (object, 0);
> > +}
> > +
>
> You can see interleaved COMPONENT_REF and ARRAY_REF that this still
> doesn't look like it will handle, something like
>
> struct A
> {
>int i;
> };
>
> struct B
> {
>char c;
>__attribute ((packed)) A ar[4];
> };
>
> B b;
>
> int *p = &b.ar[1].i;
>
> Rather than have a loop in the ADDR_EXPR case of the switch, you can
> handle everything in the lower loop. And not have a switch at all, just
> strip any ADDR_EXPR before the loop.
I changed it to
if (TREE_CODE (rhs) == ADDR_EXPR)
rhs = TREE_OPERAND (rhs, 0);
while (handled_component_p (rhs))
{
if (TREE_CODE (rhs) == COMPONENT_REF)
break;
rhs = TREE_OPERAND (rhs, 0);
}
if (TREE_CODE (rhs) != COMPONENT_REF)
return NULL_TREE;
object = TREE_OPERAND (rhs, 0);
field = TREE_OPERAND (rhs, 1);
[hjl@gnu-cfl-1 pr51628-6]$ cat a.i
struct A
{
int i;
} __attribute ((packed));
struct B
{
char c;
struct A ar[4];
};
struct B b;
int *p = &b.ar[1].i;
[hjl@gnu-cfl-1 pr51628-6]$ make a.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 a.i
a.i:14:10: warning: taking address of packed member of ‘struct A’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
14 | int *p = &b.ar[1].i;
| ^~
[hjl@gnu-cfl-1 pr51628-6]$
> > +check_and_warn_address_of_packed_member (tree type, tree rhs)
> > +{
> > + if (TREE_CODE (rhs) != COND_EXPR)
> > +{
> > + tree context = check_address_of_packed_member (type, rhs);
> > + if (context)
> > + {
> > + location_t loc = EXPR_LOC_OR_LOC (rhs, input_location);
> > + warning_at (loc, OPT_Waddress_of_packed_member,
> > + "taking address of packed member of %qT may result "
> > + "in an unaligned pointer value",
> > + context);
> > + }
> > + return;
> > +}
> > +
> > + /* Check the THEN path. */
> > + check_and_warn_address_of_packed_member (type, TREE_OPERAND (rhs, 1));
> > +
> > + /* Check the ELSE path. */
> > + check_and_warn_address_of_packed_member (type, TREE_OPERAND (rhs, 2));
> > +}
>
> You probably also want to handle COMPOUND_EXPR.
>
Done.
[hjl@gnu-cfl-1 pr51628-5]$ cat c.i
struct A {
int i;
} __attribute__ ((packed));
int*
foo3 (struct A *p1, int *q1, int *q2, struct A *p2)
{
return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q2 = 2, &p2->i): q2);
}
[hjl@gnu-cfl-1 pr51628-5]$
/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 c.i
c.i: In function ‘foo3’:
c.i:8:25: warning: taking address of packed member of ‘struct A’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
8 | return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q2 = 2, &p2->i): q2);
| ^~
c.i:8:51: warning: taking address of packed member of ‘struct A’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
8 | return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q2 = 2, &p2->i): q2);
| ^~
[hjl@gnu-cfl-1 pr51628-5]$
Here is the updated patch. OK for trunk?
Thanks.
--
Re: V7 [PATCH] C/C++: Add -Waddress-of-packed-member
On 12/18/18 9:10 AM, H.J. Lu wrote:
+ switch (TREE_CODE (rhs))
+{
+case ADDR_EXPR:
+ base = TREE_OPERAND (rhs, 0);
+ while (handled_component_p (base))
+ {
+ if (TREE_CODE (base) == COMPONENT_REF)
+ break;
+ base = TREE_OPERAND (base, 0);
+ }
+ if (TREE_CODE (base) != COMPONENT_REF)
+ return NULL_TREE;
+ object = TREE_OPERAND (base, 0);
+ field = TREE_OPERAND (base, 1);
+ break;
+case COMPONENT_REF:
+ object = TREE_OPERAND (rhs, 0);
+ field = TREE_OPERAND (rhs, 1);
+ break;
+default:
+ return NULL_TREE;
+}
+
+ tree context = check_alignment_of_packed_member (type, field);
+ if (context)
+return context;
+
+ /* Check alignment of the object. */
+ while (TREE_CODE (object) == COMPONENT_REF)
+{
+ field = TREE_OPERAND (object, 1);
+ context = check_alignment_of_packed_member (type, field);
+ if (context)
+ return context;
+ object = TREE_OPERAND (object, 0);
+}
+
You can see interleaved COMPONENT_REF and ARRAY_REF that this still
doesn't look like it will handle, something like
struct A
{
int i;
};
struct B
{
char c;
__attribute ((packed)) A ar[4];
};
B b;
int *p = &b.ar[1].i;
Rather than have a loop in the ADDR_EXPR case of the switch, you can
handle everything in the lower loop. And not have a switch at all, just
strip any ADDR_EXPR before the loop.
+check_and_warn_address_of_packed_member (tree type, tree rhs)
+{
+ if (TREE_CODE (rhs) != COND_EXPR)
+{
+ tree context = check_address_of_packed_member (type, rhs);
+ if (context)
+ {
+ location_t loc = EXPR_LOC_OR_LOC (rhs, input_location);
+ warning_at (loc, OPT_Waddress_of_packed_member,
+ "taking address of packed member of %qT may result "
+ "in an unaligned pointer value",
+ context);
+ }
+ return;
+}
+
+ /* Check the THEN path. */
+ check_and_warn_address_of_packed_member (type, TREE_OPERAND (rhs, 1));
+
+ /* Check the ELSE path. */
+ check_and_warn_address_of_packed_member (type, TREE_OPERAND (rhs, 2));
+}
You probably also want to handle COMPOUND_EXPR.
Jason
V7 [PATCH] C/C++: Add -Waddress-of-packed-member
On Mon, Dec 17, 2018 at 08:53:32AM -0500, Jason Merrill wrote:
> On 12/17/18 7:42 AM, H.J. Lu wrote:
> > On Mon, Dec 17, 2018 at 1:39 AM Richard Biener
> > wrote:
> > >
> > > On Fri, Dec 14, 2018 at 11:48 PM H.J. Lu wrote:
> > > >
> > > > On Fri, Dec 14, 2018 at 2:10 PM Jason Merrill wrote:
> > > > >
> > > > > On 12/13/18 6:56 PM, H.J. Lu wrote:
> > > > > > On Thu, Dec 13, 2018 at 12:50 PM Jason Merrill
> > > > > > wrote:
> > > > > > >
> > > > > > > On 9/25/18 11:46 AM, H.J. Lu wrote:
> > > > > > > > On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill
> > > > > > > > wrote:
> > > > > > > > > On 07/23/2018 05:24 PM, H.J. Lu wrote:
> > > > > > > > > >
> > > > > > > > > > On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers
> > > > > > > > > >
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Mon, 18 Jun 2018, Jason Merrill wrote:
> > > > > > > > > > >
> > > > > > > > > > > > On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers
> > > > > > > > > > > >
> > > > > > > > > > > > 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);
> > > > > > >
> > > > >
Re: V6 [PATCH] C/C++: Add -Waddress-of-packed-member
On 12/17/18 7:42 AM, H.J. Lu wrote:
On Mon, Dec 17, 2018 at 1:39 AM Richard Biener
wrote:
On Fri, Dec 14, 2018 at 11:48 PM H.J. Lu wrote:
On Fri, Dec 14, 2018 at 2:10 PM Jason Merrill wrote:
On 12/13/18 6:56 PM, H.J. Lu wrote:
On Thu, Dec 13, 2018 at 12:50 PM Jason Merrill wrote:
On 9/25/18 11:46 AM, H.J. Lu wrote:
On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill wrote:
On 07/23/2018 05:24 PM, H.J. Lu wrote:
On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers
wrote:
On Mon, 18 Jun 2018, Jason Merrill wrote:
On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers
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)
Do you have a testcase?
Is there a reason you only want to handle some component references and
not others? If not, checking handled_component_p is simpler and more
future proof than enumerating specific codes.
Jason
Re: V6 [PATCH] C/C++: Add -Waddress-of-packed-member
On Mon, Dec 17, 2018 at 1:43 PM H.J. Lu wrote:
>
> On Mon, Dec 17, 2018 at 1:39 AM Richard Biener
> wrote:
> >
> > On Fri, Dec 14, 2018 at 11:48 PM H.J. Lu wrote:
> > >
> > > On Fri, Dec 14, 2018 at 2:10 PM Jason Merrill wrote:
> > > >
> > > > On 12/13/18 6:56 PM, H.J. Lu wrote:
> > > > > On Thu, Dec 13, 2018 at 12:50 PM Jason Merrill
> > > > > wrote:
> > > > >>
> > > > >> On 9/25/18 11:46 AM, H.J. Lu wrote:
> > > > >>> On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill
> > > > >>> wrote:
> > > > On 07/23/2018 05:24 PM, H.J. Lu wrote:
> > > > >
> > > > > On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers
> > > > >
> > > > > wrote:
> > > > >>
> > > > >> On Mon, 18 Jun 2018, Jason Merrill wrote:
> > > > >>
> > > > >>> On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers
> > > > >>>
> > > > >>> 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 VIE
Re: V6 [PATCH] C/C++: Add -Waddress-of-packed-member
On Mon, Dec 17, 2018 at 1:39 AM Richard Biener
wrote:
>
> On Fri, Dec 14, 2018 at 11:48 PM H.J. Lu wrote:
> >
> > On Fri, Dec 14, 2018 at 2:10 PM Jason Merrill wrote:
> > >
> > > On 12/13/18 6:56 PM, H.J. Lu wrote:
> > > > On Thu, Dec 13, 2018 at 12:50 PM Jason Merrill wrote:
> > > >>
> > > >> On 9/25/18 11:46 AM, H.J. Lu wrote:
> > > >>> On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill
> > > >>> wrote:
> > > On 07/23/2018 05:24 PM, H.J. Lu wrote:
> > > >
> > > > On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers
> > > >
> > > > wrote:
> > > >>
> > > >> On Mon, 18 Jun 2018, Jason Merrill wrote:
> > > >>
> > > >>> On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers
> > > >>>
> > > >>> 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)
>
Do you have a testcase?
--
H.J.
Re: V6 [PATCH] C/C++: Add -Waddress-of-packed-member
On Fri, Dec 14, 2018 at 11:48 PM H.J. Lu wrote:
>
> On Fri, Dec 14, 2018 at 2:10 PM Jason Merrill wrote:
> >
> > On 12/13/18 6:56 PM, H.J. Lu wrote:
> > > On Thu, Dec 13, 2018 at 12:50 PM Jason Merrill wrote:
> > >>
> > >> On 9/25/18 11:46 AM, H.J. Lu wrote:
> > >>> On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill wrote:
> > On 07/23/2018 05:24 PM, H.J. Lu wrote:
> > >
> > > On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers
> > >
> > > wrote:
> > >>
> > >> On Mon, 18 Jun 2018, Jason Merrill wrote:
> > >>
> > >>> On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers
> > >>>
> > >>> 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/exp
V6 [PATCH] C/C++: Add -Waddress-of-packed-member
n 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:
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.
From cd49ab146d2ce883b88707d3d2538e9976c6c5cc Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Fri, 12 Jan 2018 21:12:05 -0800
Subject: [PATCH] C/C++: Add -Waddress-of-packed-member
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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:
$ 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]
8 | 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:18: warning: converting a packed ‘struct C *’ pointer (alignment 1) to ‘long int *’ (alignment 8) may may result in an unaligned pointer value [-Waddress-of-packed-member]
4 | long* g8 (struct C *p) { return p; }
| ^
c.i:2:8: note: defined here
2 | struct C { struct B b; } __attribute__ ((packed));
|^
$
This warning is enabled by default. 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.
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_or_pointer_of_packed_member): New.
* c-warn.c (check_alignment_of_packed_member): New function.
(check_address_of_packed_member): Likewise.
(check_and_warn_address_of_packed_member): Likewise.
(warn_for_address_or_pointer_of_packed_member): Likewise.
* c.opt: Add -Wno-address-of-packed-member.
gcc/c/
PR c/51628
* c-typeck.c (convert_for_assignment): Call
warn_for_address_or_pointer_of_packed_member.
gcc/cp/
PR c/51628
* call.c (convert_for_arg_passing): Call
warn_for_address_or_pointer_of_packed_member.
* typeck.c (convert_for_assignment): Likewise.
gcc/testsuite/
PR c/51628
* c-c++-common/pr51628-1.c: New test.
* c-c++-common/pr51628-2.c: Likewise.
* c-c++-common/pr51628-3.c:
Re: V5 [PATCH] C/C++: Add -Waddress-of-packed-member
On 12/13/18 6:56 PM, H.J. Lu wrote:
On Thu, Dec 13, 2018 at 12:50 PM Jason Merrill wrote:
On 9/25/18 11:46 AM, H.J. Lu wrote:
On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill wrote:
On 07/23/2018 05:24 PM, H.J. Lu wrote:
On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers
wrote:
On Mon, 18 Jun 2018, Jason Merrill wrote:
On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers
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?
Jason
V5 [PATCH] C/C++: Add -Waddress-of-packed-member
MPONENT_REFs down?
> >
> > My patch works on
> > [hjl@gnu-cfl-1 pr51628-4]$ cat x.i
> > struct A { int i; } __attribute__ ((packed));
> > struct B { struct A a; };
> > struct C { struct B b; };
> >
> > extern struct C *p;
> >
> > int* g8 (void) { return &p->b.a.i; }
> > [hjl@gnu-cfl-1 pr51628-4]$ make x.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 x.i
> > x.i: In function ‘g8’:
> > x.i:7:25: warning: taking address of packed member of ‘struct A’ may
> > result in an unaligned pointer value [-Waddress-of-packed-member]
> > 7 | int* g8 (void) { return &p->b.a.i; }
> >| ^
> > [hjl@gnu-cfl-1 pr51628-4]$
> >
> > If it isn't what you had in mind, can you give me a testcase?
>
> In that testcase, 'i' is the top COMPONENT_EXPR. What I was talking
> about would be more like
>
> struct A { int i; };
> struct B { struct A a; };
> struct C { struct B b __attribute__ ((packed)); };
>
> extern struct C *p;
>
> int* g8 (void) { return &p->b.a.i; }
>
Fixed with a recursive call:
tree context = check_alignment_of_packed_member (type, field);
if (context)
return context;
/* Check alignment of the object. */
while (TREE_CODE (object) == COMPONENT_REF)
{
field = TREE_OPERAND (object, 1);
context = check_alignment_of_packed_member (type, field);
if (context)
return context;
object = TREE_OPERAND (object, 0);
}
return NULL_TREE;
Here is the updated patch.
Tested on i686 and x86-64. OK for trunk?
Thanks.
--
H.J.
From a2d11ab284fa9d04b38c05c9df6a615f951b304d Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Fri, 12 Jan 2018 21:12:05 -0800
Subject: [PATCH] C/C++: Add -Waddress-of-packed-member
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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:
$ 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]
8 | 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:18: warning: converting a packed ‘struct C *’ pointer (alignment 1) to ‘long int *’ (alignment 8) may may result in an unaligned pointer value [-Waddress-of-packed-member]
4 | long* g8 (struct C *p) { return p; }
| ^
c.i:2:8: note: defined here
2 | struct C { struct B b; } __attribute__ ((packed));
|^
$
This warning is enabled by default. 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.
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_or_pointer_of_packed_member): New.
* c-warn.c (check_alignment_of_packed_member): New function.
(check_address_of_packed_member): Likewise.
(check_and_warn_address_of_packed_member): Likewise.
(warn_for_address_or_pointer_of_packed_member): Likewise.
* c.opt: Add -Wno-address-of-packed-member.
gcc/c/
PR c/51628
* c-typeck.c (convert_for_assignment): Call
warn_for_address_or_pointer_of_packed_member.
gcc/cp/
PR c/51628
* call.c (convert_for_arg_passing): Call
warn_for_address_or_pointer_of_packed_member.
* typeck.c (convert_for_assignment): Likewise.
gcc/testsuite/
PR c/51628
* c-c++-common/pr51628-1.c: New test.
* c-c++-common/pr51628-2.c: Likewise.
* c-c++-common/pr51628-3.c: Likewise.
* c-c++-common/pr51628-4.c: Likewise.
* c-c++-common/pr51628-5.c: Likewise.
* c-c++-common/pr51628-6.c: Li
Re: V4 [PATCH] C/C++: Add -Waddress-of-packed-member
On 9/25/18 11:46 AM, H.J. Lu wrote:
On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill wrote:
On 07/23/2018 05:24 PM, H.J. Lu wrote:
On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers
wrote:
On Mon, 18 Jun 2018, Jason Merrill wrote:
On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers
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.
+ /* Check alignment of the object. */
+ if (TREE_CODE (object) == COMPONENT_REF)
+{
+ field = TREE_OPERAND (object, 1);
+ if (TREE_CODE (field) == FIELD_DECL && DECL_PACKED (field))
+ {
+ type_align = TYPE_ALIGN (type);
+ context = DECL_CONTEXT (field);
+ record_align = TYPE_ALIGN (context);
+ if ((record_align % type_align) != 0)
+ return context;
+ }
+}
Why doesn't this recurse? What if you have a packed field three
COMPONENT_REFs down?
My patch works on
[hjl@gnu-cfl-1 pr51628-4]$ cat x.i
struct A { int i; } __attribute__ ((packed));
struct B { struct A a; };
struct C { struct B b; };
extern struct C *p;
int* g8 (void) { return &p->b.a.i; }
[hjl@gnu-cfl-1 pr51628-4]$ make x.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 x.i
x.i: In function ‘g8’:
x.i:7:25: warning: taking address of packed member of ‘struct A’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
7 | int* g8 (void) { return &p->b.a.i; }
| ^
[hjl@gnu-cfl-1 pr51628-4]$
If it isn't what you had in mind, can you give me a testcase?
In that testcase, 'i' is the top COMPONENT_EXPR. What I was talking
about would be more like
struct A { int i; };
struct B { struct A a; };
struct C { struct B b __attribute__ ((packed)); };
extern struct C *p;
int* g8 (void) { return &p->b.a.i; }
Jason
PING^2: V4 [PATCH] C/C++: Add -Waddress-of-packed-member
On Sun, Nov 4, 2018 at 7:16 AM H.J. Lu wrote:
>
> On Tue, Sep 25, 2018 at 8:46 AM H.J. Lu wrote:
> >
> > On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill wrote:
> > > On 07/23/2018 05:24 PM, H.J. Lu wrote:
> > >>
> > >> On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers
> > >> wrote:
> > >>>
> > >>> On Mon, 18 Jun 2018, Jason Merrill wrote:
> > >>>
> > On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers
> >
> > 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.
> >
> > >> + /* Check alignment of the object. */
> > >> + if (TREE_CODE (object) == COMPONENT_REF)
> > >> +{
> > >> + field = TREE_OPERAND (object, 1);
> > >> + if (TREE_CODE (field) == FIELD_DECL && DECL_PACKED (field))
> > >> + {
> > >> + type_align = TYPE_ALIGN (type);
> > >> + context = DECL_CONTEXT (field);
> > >> + record_align = TYPE_ALIGN (context);
> > >> + if ((record_align % type_align) != 0)
> > >> + return context;
> > >> + }
> > >> +}
> > >
> > >
> > > Why doesn't this recurse? What if you have a packed field three
> > > COMPONENT_REFs down?
> >
> > My patch works on
> > [hjl@gnu-cfl-1 pr51628-4]$ cat x.i
> > struct A { int i; } __attribute__ ((packed));
> > struct B { struct A a; };
> > struct C { struct B b; };
> >
> > extern struct C *p;
> >
> > int* g8 (void) { return &p->b.a.i; }
> > [hjl@gnu-cfl-1 pr51628-4]$ make x.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 x.i
> > x.i: In function ‘g8’:
> > x.i:7:25: warning: taking address of packed member of ‘struct A’ may
> > result in an unaligned pointer value [-Waddress-of-packed-member]
> > 7 | int* g8 (void) { return &p->b.a.i; }
> > | ^
> > [hjl@gnu-cfl-1 pr51628-4]$
> >
> > If it isn't what you had in mind, can you give me a testcase?
> >
> > >> + 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);
> > >> + if (context)
> > >> + rhs = op1;
> > >> + else
> > >> + {
> > >> + /* Check the ELSE path. */
> > >> + rhs = TREE_OPERAND (rhs, 2);
> > >> + context = check_address_of_packed_member (type, rhs);
> > >> + }
> > >> +}
> > >
> > >
> > > Likewise, what if you have more levels of COND_EXPR? Or COMPOUND_EXPR
> > > within COND_EXPR?
> >
> > Fixed, now
PING: V4 [PATCH] C/C++: Add -Waddress-of-packed-member
On Tue, Sep 25, 2018 at 8:46 AM H.J. Lu wrote:
>
> On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill wrote:
> > On 07/23/2018 05:24 PM, H.J. Lu wrote:
> >>
> >> On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers
> >> wrote:
> >>>
> >>> On Mon, 18 Jun 2018, Jason Merrill wrote:
> >>>
> On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers
> 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.
>
> >> + /* Check alignment of the object. */
> >> + if (TREE_CODE (object) == COMPONENT_REF)
> >> +{
> >> + field = TREE_OPERAND (object, 1);
> >> + if (TREE_CODE (field) == FIELD_DECL && DECL_PACKED (field))
> >> + {
> >> + type_align = TYPE_ALIGN (type);
> >> + context = DECL_CONTEXT (field);
> >> + record_align = TYPE_ALIGN (context);
> >> + if ((record_align % type_align) != 0)
> >> + return context;
> >> + }
> >> +}
> >
> >
> > Why doesn't this recurse? What if you have a packed field three
> > COMPONENT_REFs down?
>
> My patch works on
> [hjl@gnu-cfl-1 pr51628-4]$ cat x.i
> struct A { int i; } __attribute__ ((packed));
> struct B { struct A a; };
> struct C { struct B b; };
>
> extern struct C *p;
>
> int* g8 (void) { return &p->b.a.i; }
> [hjl@gnu-cfl-1 pr51628-4]$ make x.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 x.i
> x.i: In function ‘g8’:
> x.i:7:25: warning: taking address of packed member of ‘struct A’ may
> result in an unaligned pointer value [-Waddress-of-packed-member]
> 7 | int* g8 (void) { return &p->b.a.i; }
> | ^
> [hjl@gnu-cfl-1 pr51628-4]$
>
> If it isn't what you had in mind, can you give me a testcase?
>
> >> + 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);
> >> + if (context)
> >> + rhs = op1;
> >> + else
> >> + {
> >> + /* Check the ELSE path. */
> >> + rhs = TREE_OPERAND (rhs, 2);
> >> + context = check_address_of_packed_member (type, rhs);
> >> + }
> >> +}
> >
> >
> > Likewise, what if you have more levels of COND_EXPR? Or COMPOUND_EXPR
> > within COND_EXPR?
>
> Fixed, now I got
>
> [hjl@gnu-cfl-1 pr51628-5]$ cat z.i
> struct A {
> int i;
> } __attribute__ ((packed));
>
> int*
> foo3 (struct A *p1, int *q1, int *q2, struct A *p2)
> {
> return (q1
> ? &p1->i
> : (q2 ? &p2->i : q2));
> }
> [hjl@gnu-cfl-1 pr51628-5]$ make z.s
> /export/build/gnu/tools-build/gcc-debug/build-x86_64-linux/gcc/xgcc
> -B/export/build/gnu
V4 [PATCH] C/C++: Add -Waddress-of-packed-member
you have more levels of COND_EXPR? Or COMPOUND_EXPR
> within COND_EXPR?
Fixed, now I got
[hjl@gnu-cfl-1 pr51628-5]$ cat z.i
struct A {
int i;
} __attribute__ ((packed));
int*
foo3 (struct A *p1, int *q1, int *q2, struct A *p2)
{
return (q1
? &p1->i
: (q2 ? &p2->i : q2));
}
[hjl@gnu-cfl-1 pr51628-5]$ make z.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 z.i
z.i: In function ‘foo3’:
z.i:9:13: warning: taking address of packed member of ‘struct A’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
9 | ? &p1->i
| ^~
z.i:10:19: warning: taking address of packed member of ‘struct A’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
10 | : (q2 ? &p2->i : q2));
| ^~
[hjl@gnu-cfl-1 pr51628-5]$
>> @@ -7470,6 +7470,9 @@ convert_for_arg_passing (tree type, tree val,
>> tsubst_flags_t complain)
>> + warn_for_address_or_pointer_of_packed_member (true, type, val);
>
>
>> @@ -8914,6 +8914,8 @@ convert_for_assignment (tree type, tree rhs,
>> + warn_for_address_or_pointer_of_packed_member (true, type, rhs);
>
>
> Why would address_p be true in these calls? It seems that you are warning
> at the point of assignment but looking for the warning about taking the
> address rather than the one about assignment.
It happens only with C for incompatible pointer conversion:
[hjl@gnu-cfl-1 pr51628-2]$ cat c.i
struct B { int i; };
struct C { struct B b; } __attribute__ ((packed));
long* g8 (struct C *p) { return p; }
[hjl@gnu-cfl-1 pr51628-2]$ make c.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 c.i
c.i: In function ‘g8’:
c.i:4:33: warning: returning ‘struct C *’ from a function with
incompatible return type ‘long int *’ [-Wincompatible-pointer-types]
4 | long* g8 (struct C *p) { return p; }
| ^
c.i:4:18: warning: converting a packed ‘struct C *’ pointer (alignment
1) to ‘long int *’ (alignment 8) may may result in an unaligned
pointer value [-Waddress-of-packed-member]
4 | long* g8 (struct C *p) { return p; }
| ^
c.i:2:8: note: defined here
2 | struct C { struct B b; } __attribute__ ((packed));
|^
[hjl@gnu-cfl-1 pr51628-2]$
address_p is false in this case and rhs is PARM_DECL, VAR_DECL or
NOP_EXPR. This comes from convert_for_assignment in c/c-typeck.c.
For other compatible pointer assignment, address_p is true and rhs is
ADDR_EXPR, PARM_DECL, VAR_DECL or NOP_EXPR. Check
for ADDR_EXPR won't work.
address_p isn't an appropriate parameter name. I changed it to convert_p
to indicate that it is an incompatible pointer type conversion.
> If you want to warn about taking the address, shouldn't that happen under
> cp_build_addr_expr? Alternately, drop the address_p parameter and choose
> your path inside warn_for_*_packed_member based on whether rhs is an
> ADDR_EXPR there rather than in the caller.
>
Here is the updated patch. OK for trunk?
Thanks.
--
H.J.
From b5cecbaabfe0fde7aeb3d3c24d8959853985951c Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Fri, 12 Jan 2018 21:12:05 -0800
Subject: [PATCH] C/C++: Add -Waddress-of-packed-member
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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:
$ 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]
8 | 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:18: warning: converting a packed ‘struct C *’ pointer (alignment 1) to ‘long int *’ (alignment 8) may may result in an unaligned pointer value [-Waddress-of-packed-member]
4 | long* g8 (struct C *p) { return p; }
| ^
c.i:2:8: note: defined here
2 | struct C { struct B b; } __attribute__ ((packed));
|^
$
This warning is enabled by default. 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)))
Re: V3 [PATCH] C/C++: Add -Waddress-of-packed-member
On 07/23/2018 05:24 PM, H.J. Lu wrote:
On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers wrote:
On Mon, 18 Jun 2018, Jason Merrill wrote:
On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers 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]
+ 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.
+ /* Check alignment of the object. */
+ if (TREE_CODE (object) == COMPONENT_REF)
+{
+ field = TREE_OPERAND (object, 1);
+ if (TREE_CODE (field) == FIELD_DECL && DECL_PACKED (field))
+ {
+ type_align = TYPE_ALIGN (type);
+ context = DECL_CONTEXT (field);
+ record_align = TYPE_ALIGN (context);
+ if ((record_align % type_align) != 0)
+ return context;
+ }
+}
Why doesn't this recurse? What if you have a packed field three
COMPONENT_REFs down?
+ 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);
+ if (context)
+ rhs = op1;
+ else
+ {
+ /* Check the ELSE path. */
+ rhs = TREE_OPERAND (rhs, 2);
+ context = check_address_of_packed_member (type, rhs);
+ }
+}
Likewise, what if you have more levels of COND_EXPR? Or COMPOUND_EXPR
within COND_EXPR?
@@ -7470,6 +7470,9 @@ convert_for_arg_passing (tree type, tree val,
tsubst_flags_t complain)
+ warn_for_address_or_pointer_of_packed_member (true, type, val);
@@ -8914,6 +8914,8 @@ convert_for_assignment (tree type, tree rhs,
+ warn_for_address_or_pointer_of_packed_member (true, type, rhs);
Why would address_p be true in these calls? It seems that you are
warning at the point of assignment but looking for the warning about
taking the address rather than the one about assignment.
If you want to warn about taking the address, shouldn't that happen
under cp_build_addr_expr? Alternately, drop the address_p parameter and
choose your path inside warn_for_*_packed_member based on whether rhs is
an ADDR_EXPR there rather than in the caller.
Jason
PING^1: V3 [PATCH] C/C++: Add -Waddress-of-packed-member
On Mon, Jul 23, 2018 at 2:24 PM, H.J. Lu wrote:
> On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers
> wrote:
>> On Mon, 18 Jun 2018, Jason Merrill wrote:
>>
>>> On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers
>>> 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.
>
> Tested on Linux/x86-64 and Linux/i686. OK for trunk.
>
PING^1.
--
H.J.
V3 [PATCH] C/C++: Add -Waddress-of-packed-member
On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers wrote:
> On Mon, 18 Jun 2018, Jason Merrill wrote:
>
>> On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers
>> 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.
Tested on Linux/x86-64 and Linux/i686. OK for trunk.
Thanks.
--
H.J.
From 2ddae2d57d2875e80c9186b281edfabfddb42e86 Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Fri, 12 Jan 2018 21:12:05 -0800
Subject: [PATCH] C/C++: Add -Waddress-of-packed-member
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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:
$ 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: returning ‘struct C *’ from a function with incompatible return type ‘long int *’ [-Wincompatible-pointer-types]
long* g8 (struct C *p) { return p; }
^
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]
c.i:2:8: note: defined here
struct C { struct B b; } __attribute__ ((packed));
^
$
This warning is enabled by default. 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.
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_or_pointer_of_packed_member): New.
* c-warn.c (check_address_of_packed_member): New function.
(warn_for_address_or_pointer_of_packed_member): Likewise.
* c.opt: Add -Wno-address-of-packed-member.
gcc/c/
PR c/51628
* c-typeck.c (convert_for_assignment): Call
warn_for_address_or_pointer_of_packed_member.
gcc/cp/
PR c/51628
* call.c (convert_for_arg_passing): Call
warn_for_address_or_pointer_of_packed_member.
* typeck.c (convert_for_assignment): Likewise.
gcc/testsuite/
PR c/51628
* c-c++-common/pr51628-1.c: New test.
* c-c++-common/pr51628-2.c: Likewise.
* c-c++-common/pr51628-3.c: Likewise.
* c-c++-common/pr51628-4.c: Likewise.
* c-c++-common/pr51628-5.c: Likewise.
* c-c++-common/pr51628-6.c: Likewise.
* c-c++-common/pr51628-7.c: Likewise.
* c-c++-common/pr51628-8.c: Likewise.
* c-c++-common/pr51628-9.c: Likewise.
* c-c++-common/pr51628-10.c: Likewise.
* c-c++-common/pr51628-11.c: Likewise.
* c-c++-common/pr51628-12.c: Likewise.
* c-c++-common/pr51628-13.c: Likewise.
* c-c++-common/pr51628-14.c: Likewise.
* c-c++-common/pr51628-15.c: Likewise.
* c-c++-common/pr51628-26.c: Likewise.
* gcc.dg/pr51628-17.c: Likewise.
* gcc.dg/pr51628-18.c:
Re: PING^1: [PATCH] C/C++: Add -Waddress-of-packed-member
On Mon, May 14, 2018 at 8:00 PM, Martin Sebor wrote:
> On 05/14/2018 01:10 PM, H.J. Lu wrote:
>>
>> On Mon, May 14, 2018 at 10:40 AM, H.J. Lu 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]$
--
H.J.
Re: PING^1: [PATCH] C/C++: Add -Waddress-of-packed-member
On 05/14/2018 01:10 PM, H.J. Lu wrote:
On Mon, May 14, 2018 at 10:40 AM, H.J. Lu 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.
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.
Martin
Re: PING^1: [PATCH] C/C++: Add -Waddress-of-packed-member
On Mon, May 14, 2018 at 10:40 AM, H.J. Lu 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));
^
[hjl@gnu-skx-1 pr51628]$
--
H.J.
Re: PING^1: [PATCH] C/C++: Add -Waddress-of-packed-member
On Mon, May 14, 2018 at 9:18 AM, Martin Sebor wrote:
> On 05/14/2018 07:44 AM, H.J. Lu wrote:
>>
>> On Wed, Apr 25, 2018 at 7:54 PM, H.J. Lu 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 sym
Re: PING^1: [PATCH] C/C++: Add -Waddress-of-packed-member
On 05/14/2018 07:44 AM, H.J. Lu wrote:
On Wed, Apr 25, 2018 at 7:54 PM, H.J. Lu 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
PING^1: [PATCH] C/C++: Add -Waddress-of-packed-member
On Wed, Apr 25, 2018 at 7:54 PM, H.J. Lu 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:
>
> $ 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]
> long* g8 (struct C *p) { return p; }
> ^
> $
>
> This warning is enabled by default. 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.
>
> gcc/cp/
>
> PR c/51628
> * call.c (convert_for_arg_passing): Call
> warn_for_address_of_packed_member.
> * typeck.c (convert_for_assignment): Likewise.
>
> gcc/testsuite/
>
> PR c/51628
> * c-c++-common/pr51628-1.c: New test.
> * c-c++-common/pr51628-2.c: Likewise.
> * c-c++-common/pr51628-3.c: Likewise.
> * c-c++-common/pr51628-4.c: Likewise.
> * c-c++-common/pr51628-5.c: Likewise.
> * c-c++-common/pr51628-6.c: Likewise.
> * c-c++-common/pr51628-7.c: Likewise.
> * c-c++-common/pr51628-8.c: Likewise.
> * c-c++-common/pr51628-9.c: Likewise.
> * c-c++-common/pr51628-10.c: Likewise.
> * c-c++-common/pr51628-11.c: Likewise.
> * c-c++-common/pr51628-12.c: Likewise.
> * c-c++-common/pr51628-13.c: Likewise.
> * c-c++-common/pr51628-14.c: Likewise.
> * c-c++-common/pr51628-15.c: Likewise.
> * gcc.dg/pr51628-16.c: Likewise.
> * gcc.dg/pr51628-17.c: Likewise.
> * gcc.dg/pr51628-18.c: Likewise.
> * gcc.dg/pr51628-19.c: Likewise.
> * gcc.dg/pr51628-20.c: Likewise.
> * gcc.dg/pr51628-21.c: Likewise.
> * gcc.dg/pr51628-22.c: Likewise.
> * gcc.dg/pr51628-23.c: Likewise.
> * gcc.dg/pr51628-24.c: Likewise.
> * c-c++-common/asan/misalign-1.c: Add
> -Wno-address-of-packed-member.
> * c-c++-common/asan/misalign-2.c: Likewise.
> * c-c++-common/ubsan/align-2.c: Likewise.
> * c-c++-common/ubsan/align-4.c: Likewise.
> * c-c++-common/ubsan/align-6.c: Likewise.
> * c-c++-common/ubsan/align-7.c: Likewise.
> * c-c++-common/ubsan/align-8.c: Likewise.
> * c-c++-common/ubsan/align-10.c: Likewise.
> * g++.dg/ubsan/align-2.C: Likewise.
> * gcc.target/i386/avx512bw-vmovdqu16-2.c: Likewise.
> * gcc.target/i386/avx512f-vmovdqu32-2.c: Likewise.
> * gcc.target/i386/avx512f-vmovdqu64-2.c: Likewise.
> * gcc.target/i386/avx512vl-vmovdqu16-2.c: Likewise.
> * gcc.target/i386/avx512vl-vmovdqu32-2.c: Likewise.
> * gcc.target/i386/avx512vl-vmovdqu64-2.c: Likewise.
>
> libgcc/
>
> * unwind-pe.h (read_encoded_value_with_base): Add GCC pragma
> to ignore -Waddress-of-packed-member.
Jason, Joseph, is this
https://gcc.gnu.org/ml/gcc-patches/2018-04/msg01155.html
OK for tru
[PATCH] C/C++: Add -Waddress-of-packed-member
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:
$ 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]
long* g8 (struct C *p) { return p; }
^
$
This warning is enabled by default. 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.
gcc/cp/
PR c/51628
* call.c (convert_for_arg_passing): Call
warn_for_address_of_packed_member.
* typeck.c (convert_for_assignment): Likewise.
gcc/testsuite/
PR c/51628
* c-c++-common/pr51628-1.c: New test.
* c-c++-common/pr51628-2.c: Likewise.
* c-c++-common/pr51628-3.c: Likewise.
* c-c++-common/pr51628-4.c: Likewise.
* c-c++-common/pr51628-5.c: Likewise.
* c-c++-common/pr51628-6.c: Likewise.
* c-c++-common/pr51628-7.c: Likewise.
* c-c++-common/pr51628-8.c: Likewise.
* c-c++-common/pr51628-9.c: Likewise.
* c-c++-common/pr51628-10.c: Likewise.
* c-c++-common/pr51628-11.c: Likewise.
* c-c++-common/pr51628-12.c: Likewise.
* c-c++-common/pr51628-13.c: Likewise.
* c-c++-common/pr51628-14.c: Likewise.
* c-c++-common/pr51628-15.c: Likewise.
* gcc.dg/pr51628-16.c: Likewise.
* gcc.dg/pr51628-17.c: Likewise.
* gcc.dg/pr51628-18.c: Likewise.
* gcc.dg/pr51628-19.c: Likewise.
* gcc.dg/pr51628-20.c: Likewise.
* gcc.dg/pr51628-21.c: Likewise.
* gcc.dg/pr51628-22.c: Likewise.
* gcc.dg/pr51628-23.c: Likewise.
* gcc.dg/pr51628-24.c: Likewise.
* c-c++-common/asan/misalign-1.c: Add
-Wno-address-of-packed-member.
* c-c++-common/asan/misalign-2.c: Likewise.
* c-c++-common/ubsan/align-2.c: Likewise.
* c-c++-common/ubsan/align-4.c: Likewise.
* c-c++-common/ubsan/align-6.c: Likewise.
* c-c++-common/ubsan/align-7.c: Likewise.
* c-c++-common/ubsan/align-8.c: Likewise.
* c-c++-common/ubsan/align-10.c: Likewise.
* g++.dg/ubsan/align-2.C: Likewise.
* gcc.target/i386/avx512bw-vmovdqu16-2.c: Likewise.
* gcc.target/i386/avx512f-vmovdqu32-2.c: Likewise.
* gcc.target/i386/avx512f-vmovdqu64-2.c: Likewise.
* gcc.target/i386/avx512vl-vmovdqu16-2.c: Likewise.
* gcc.target/i386/avx512vl-vmovdqu32-2.c: Likewise.
* gcc.target/i386/avx512vl-vmovdqu64-2.c: Likewise.
libgcc/
* unwind-pe.h (read_encoded_value_with_base): Add GCC pragma
to ignore -Waddress-of-packed-member.
---
gcc/c-family/c-common.h| 1 +
gcc/c-family/c-warn.c | 117 +
gcc/c-family/c.opt | 4 +
gcc/c/c-typeck.c | 60 ++-
gcc/cp/call.c | 3 +
gcc/cp/typeck.c| 2
Re: [PATCH] C/C++: Add -Waddress-of-packed-member
On Sat, Jan 20, 2018 at 9:50 AM, H.J. Lu wrote:
> On Sat, Jan 20, 2018 at 8:31 AM, H.J. Lu wrote:
>> On Fri, Jan 19, 2018 at 7:57 PM, Martin Sebor wrote:
>>> On 01/19/2018 10:14 AM, Martin Sebor wrote:
>>>>
>>
>>> After reading the Clang code review for the warning
>>> (https://reviews.llvm.org/D20561) and experimenting with a few
>>> more test cases I noticed some additional false negatives that
>>> I think would be worthwhile diagnosing:
>>>
>>> struct A {
>>> int i;
>>> } __attribute__ ((packed));
>>>
>>> int f (struct A *p)
>>> {
>>> return *&p->i;
>>> }
>>
>> I now got
>>
>> [hjl@gnu-tools-1 pr51628]$ cat a1.i
>> struct A {
>> int i;
>> } __attribute__ ((packed));
>>
>> int f (struct A *p)
>> {
>> return *&p->i;
>> }
>> [hjl@gnu-tools-1 pr51628]$ make a1.s
>> /export/build/gnu/gcc/build-x86_64-linux/gcc/xgcc
>> -B/export/build/gnu/gcc/build-x86_64-linux/gcc/ -O2 -S a1.i
>> a1.i: In function ‘f’:
>> a1.i:7:10: warning: taking address of packed member of ‘struct A’ may
>> result in an unaligned pointer value [-Waddress-of-packed-member]
>>return *&p->i;
>> ^~
>> [hjl@gnu-tools-1 pr51628]$
>
>
> It is wrong to warn "*&p->i;" since GCC always converts to "p->i;". Unless
> we want to warn
>
> [hjl@gnu-tools-1 pr51628]$ cat g5.i
> struct A {
> int i;
> } __attribute__ ((packed));
>
> int f (struct A *p)
> {
> return p->i;
> }
>
> we shouldn't warn:
>
> [hjl@gnu-tools-1 pr51628]$ cat a1.i
> struct A {
> int i;
> } __attribute__ ((packed));
>
> int f (struct A *p)
> {
> return *&p->i;
> }
> [hjl@gnu-tools-1 pr51628]$
>
> There is no unaligned load here. I am testing a new patch.
>
Here is the updated patch. Tested on x86-64 and i686.
--
H.J.
From e4fb83119762ac8aaa0cd1095e273f5c70e9d4a3 Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Fri, 12 Jan 2018 21:12:05 -0800
Subject: [PATCH] C/C++: Add -Waddress-of-packed-member
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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:
$ 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]
long* g8 (struct C *p) { return p; }
^
$
This warning is enabled by default. 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.
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.
gcc/cp/
PR c/51628
* call.c (convert_for_arg_passing): Call
warn_for_address_of_packed_member.
* typeck.c (convert_for_assignment): Likewise.
gcc/testsuite/
PR c/51628
* c-c++-common/pr51628-1.c: New test.
* c-c++-common/pr51628-2.c: Lik
Re: [PATCH] C/C++: Add -Waddress-of-packed-member
On Sat, Jan 20, 2018 at 8:31 AM, H.J. Lu wrote:
> On Fri, Jan 19, 2018 at 7:57 PM, Martin Sebor wrote:
>> On 01/19/2018 10:14 AM, Martin Sebor wrote:
>>>
>
>> After reading the Clang code review for the warning
>> (https://reviews.llvm.org/D20561) and experimenting with a few
>> more test cases I noticed some additional false negatives that
>> I think would be worthwhile diagnosing:
>>
>> struct A {
>> int i;
>> } __attribute__ ((packed));
>>
>> int f (struct A *p)
>> {
>> return *&p->i;
>> }
>
> I now got
>
> [hjl@gnu-tools-1 pr51628]$ cat a1.i
> struct A {
> int i;
> } __attribute__ ((packed));
>
> int f (struct A *p)
> {
> return *&p->i;
> }
> [hjl@gnu-tools-1 pr51628]$ make a1.s
> /export/build/gnu/gcc/build-x86_64-linux/gcc/xgcc
> -B/export/build/gnu/gcc/build-x86_64-linux/gcc/ -O2 -S a1.i
> a1.i: In function ‘f’:
> a1.i:7:10: warning: taking address of packed member of ‘struct A’ may
> result in an unaligned pointer value [-Waddress-of-packed-member]
>return *&p->i;
> ^~
> [hjl@gnu-tools-1 pr51628]$
It is wrong to warn "*&p->i;" since GCC always converts to "p->i;". Unless
we want to warn
[hjl@gnu-tools-1 pr51628]$ cat g5.i
struct A {
int i;
} __attribute__ ((packed));
int f (struct A *p)
{
return p->i;
}
we shouldn't warn:
[hjl@gnu-tools-1 pr51628]$ cat a1.i
struct A {
int i;
} __attribute__ ((packed));
int f (struct A *p)
{
return *&p->i;
}
[hjl@gnu-tools-1 pr51628]$
There is no unaligned load here. I am testing a new patch.
--
H.J.
Re: [PATCH] C/C++: Add -Waddress-of-packed-member
uct C *p) { return &p->b; } // Clang only
^
m1.c:15:32: warning: taking address of packed member of ‘struct C’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
m1.c: In function ‘h4’:
m1.c:17:32: warning: taking address of packed member of ‘struct C’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
int* h4 (struct C *p) { return &p->b.i; } // Clang only
^~~
[hjl@gnu-tools-1 pr51628]$
> After reading the Clang code review for the warning
> (https://reviews.llvm.org/D20561) and experimenting with a few
> more test cases I noticed some additional false negatives that
> I think would be worthwhile diagnosing:
>
> struct A {
> int i;
> } __attribute__ ((packed));
>
> int f (struct A *p)
> {
> return *&p->i;
> }
I now got
[hjl@gnu-tools-1 pr51628]$ cat a1.i
struct A {
int i;
} __attribute__ ((packed));
int f (struct A *p)
{
return *&p->i;
}
[hjl@gnu-tools-1 pr51628]$ make a1.s
/export/build/gnu/gcc/build-x86_64-linux/gcc/xgcc
-B/export/build/gnu/gcc/build-x86_64-linux/gcc/ -O2 -S a1.i
a1.i: In function ‘f’:
a1.i:7:10: warning: taking address of packed member of ‘struct A’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
return *&p->i;
^~
[hjl@gnu-tools-1 pr51628]$
> An example similar to one of those discussed in the review is
> one involving a conditional expression (Clang diagnoses this):
>
> int* f (struct A *p, int *q)
> {
> return q ? q : &p->i; // missing warning
> }
I now got
[hjl@gnu-tools-1 pr51628]$ cat a2.i
struct A {
int i;
} __attribute__ ((packed));
int* f (struct A *p, int *q)
{
return q ? q : &p->i;
}
[hjl@gnu-tools-1 pr51628]$ make a2.s
/export/build/gnu/gcc/build-x86_64-linux/gcc/xgcc
-B/export/build/gnu/gcc/build-x86_64-linux/gcc/ -O2 -S a2.i
a2.i: In function ‘f’:
a2.i:7:18: warning: taking address of packed member of ‘struct A’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
return q ? q : &p->i;
^
[hjl@gnu-tools-1 pr51628]$
> Clang doesn't diagnose the conversion of a packed member array
> to a pointer to its element. It seems to me that it should be
> diagnosed:
>
> struct B {
> int a[1];
> } __attribute__ ((packed));
>
> int* f (struct B *p)
> {
> return p->a; // missing warning
> }
[hjl@gnu-tools-1 pr51628]$ cat a3.i
struct B {
int a[1];
} __attribute__ ((packed));
int* f (struct B *p)
{
return p->a; // missing warning
}
[hjl@gnu-tools-1 pr51628]$ make a3.s
/export/build/gnu/gcc/build-x86_64-linux/gcc/xgcc
-B/export/build/gnu/gcc/build-x86_64-linux/gcc/ -O2 -S a3.i
a3.i: In function ‘f’:
a3.i:7:12: warning: taking address of packed member of ‘struct B’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
return p->a; // missing warning
^
[hjl@gnu-tools-1 pr51628]$
> Similarly, in C++, binding to more strictly aligned references
> should probably be diagnosed (Clang misses it):
>
> int* g (struct A &r)
> {
> int &ir = r.i; // missing warning here
> return &ir; // regardless of how ir is used
> }
This won't compile:
[hjl@gnu-tools-1 pr51628]$ cat r1.ii
struct A {
int i;
} __attribute__ ((packed));
int* g (struct A &r)
{
int &ir = r.i; // missing warning here
return &ir; // regardless of how ir is used
}
[hjl@gnu-tools-1 pr51628]$ make r1.s
/export/build/gnu/gcc/build-x86_64-linux/gcc/xgcc
-B/export/build/gnu/gcc/build-x86_64-linux/gcc/ -S r1.ii
r1.ii: In function ‘int* g(A&)’:
r1.ii:7:15: error: cannot bind packed field ‘r.A::i’ to ‘int&’
int &ir = r.i; // missing warning here
~~^
make: *** [Makefile:14: r1.s] Error 1
[hjl@gnu-tools-1 pr51628]$
Here is the updated patch. You can also get it from
https://github.com/hjl-tools/gcc/tree/hjl/pr51628/master
Thanks.
--
H.J.
From a9cbca91d6c53a46e9787fc5c888fccc78e3 Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Fri, 12 Jan 2018 21:12:05 -0800
Subject: [PATCH] C/C++: Add -Waddress-of-packed-member
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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 warn it:
$ 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;
^
$
This warning is enabled by default.
Since read_encoded_value_wit
Re: [PATCH] C/C++: Add -Waddress-of-packed-member
On 01/19/2018 10:14 AM, Martin Sebor wrote:
On 01/14/2018 07:29 AM, H.J. Lu 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 warn it:
$ 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: initialization of 'int *' from address of packed
member of 'struct pair_t' may result in an unaligned pointer value
[-Waddress-of-packed-member]
int *addr = &p.i;
^
$
This warning is enabled by default.
I like this enhancement. It would be useful for data types,
packed or not, such as casting int* to long*.
I noticed some differences from Clang for the test case below.
It seems that GCC should warn on all the cases Clang does.
Also, since converting the address of a struct to that of its
first member is common (especially in C and when the member
itself is a struct) I wonder if the warning should trigger
for those conversions as well.
struct A {
int i;
} __attribute__ ((packed));
long* f8 (struct A *p) { return &p->i; } // Clang only
int* f4 (struct A *p) { return &p->i; }// Clang, GCC
short* f2 (struct A *p) { return &p->i; } // Clang only
char* f1 (struct A *p) { return &p->i; }
void* f0 (struct A *p) { return &p->i; }
struct B { int i; };
struct C { struct B b; } __attribute__ ((packed));
long* g8 (struct C *p) { return p; }// should warn?
int* g4 (struct C *p) { return &p->b; } // Clang only
int* h4 (struct C *p) { return &p->b.i; } // Clang only
After reading the Clang code review for the warning
(https://reviews.llvm.org/D20561) and experimenting with a few
more test cases I noticed some additional false negatives that
I think would be worthwhile diagnosing:
struct A {
int i;
} __attribute__ ((packed));
int f (struct A *p)
{
return *&p->i;
}
An example similar to one of those discussed in the review is
one involving a conditional expression (Clang diagnoses this):
int* f (struct A *p, int *q)
{
return q ? q : &p->i; // missing warning
}
Clang doesn't diagnose the conversion of a packed member array
to a pointer to its element. It seems to me that it should be
diagnosed:
struct B {
int a[1];
} __attribute__ ((packed));
int* f (struct B *p)
{
return p->a; // missing warning
}
Similarly, in C++, binding to more strictly aligned references
should probably be diagnosed (Clang misses it):
int* g (struct A &r)
{
int &ir = r.i; // missing warning here
return &ir; // regardless of how ir is used
}
Martin
Re: [PATCH] C/C++: Add -Waddress-of-packed-member
On 01/14/2018 07:29 AM, H.J. Lu 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 warn it:
$ 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: initialization of 'int *' from address of packed member of
'struct pair_t' may result in an unaligned pointer value
[-Waddress-of-packed-member]
int *addr = &p.i;
^
$
This warning is enabled by default.
I like this enhancement. It would be useful for data types,
packed or not, such as casting int* to long*.
I noticed some differences from Clang for the test case below.
It seems that GCC should warn on all the cases Clang does.
Also, since converting the address of a struct to that of its
first member is common (especially in C and when the member
itself is a struct) I wonder if the warning should trigger
for those conversions as well.
struct A {
int i;
} __attribute__ ((packed));
long* f8 (struct A *p) { return &p->i; } // Clang only
int* f4 (struct A *p) { return &p->i; }// Clang, GCC
short* f2 (struct A *p) { return &p->i; } // Clang only
char* f1 (struct A *p) { return &p->i; }
void* f0 (struct A *p) { return &p->i; }
struct B { int i; };
struct C { struct B b; } __attribute__ ((packed));
long* g8 (struct C *p) { return p; }// should warn?
int* g4 (struct C *p) { return &p->b; } // Clang only
int* h4 (struct C *p) { return &p->b.i; } // Clang only
Martin
Re: [PATCH] C/C++: Add -Waddress-of-packed-member
On Mon, Jan 15, 2018 at 7:02 AM, H.J. Lu wrote:
> On Mon, Jan 15, 2018 at 1:42 AM, Jakub Jelinek wrote:
>> On Sun, Jan 14, 2018 at 06:29:54AM -0800, H.J. Lu wrote:
>>> + if (TREE_CODE (field) == FIELD_DECL && DECL_PACKED (field))
>>> + {
>>> + tree field_type = TREE_TYPE (field);
>>> + unsigned int type_align = TYPE_ALIGN (field_type);
>>> + tree context = DECL_CONTEXT (field);
>>> + unsigned int record_align = TYPE_ALIGN (context);
>>> + if ((record_align % type_align) != 0)
>>> + return context;
>>> + type_align /= BITS_PER_UNIT;
>>> + unsigned HOST_WIDE_INT field_off
>>> + = (tree_to_uhwi (DECL_FIELD_OFFSET (field))
>>> + + (tree_to_uhwi (DECL_FIELD_BIT_OFFSET (field))
>>> +/ BITS_PER_UNIT));
>>
>> This has the same bug I've just created PR83844 for, you can't assume
>> DECL_FIELD_OFFSET is INTEGER_CST that fits into UHWI, and also we have
>> byte_position wrapper that should be used to compute the offset from
>> DECL_FIELD_*OFFSET.
>
> Here is the updated patch to use byte_position wrapper. OK for trunk?
>
Here is the updated patch not to warn:
struct pair_t {
char c;
__int128_t i;
} __attribute__((packed));
typedef struct unaligned_int128_t_ {
__int128_t value;
} __attribute__((packed)) unaligned_int128_t;
struct pair_t p = {0, 1};
unaligned_int128_t *addr = (unaligned_int128_t *)(&p.i);
int main() {
addr->value = ~(__int128_t)0;
return (p.i != 1) ? 0 : 1;
}
by properly checking the expected alignment against the field alignment.
--
H.J.
From 54b68f11c18971d1371d5bb5bde7b0c1d3e6ee7b Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Fri, 12 Jan 2018 21:12:05 -0800
Subject: [PATCH] C/C++: Add -Waddress-of-packed-member
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 warn it:
$ 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: initialization of 'int *' from address of packed member of 'struct pair_t' may result in an unaligned pointer value [-Waddress-of-packed-member]
int *addr = &p.i;
^
$
This warning is enabled by default.
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 (warn_for_address_of_packed_member): New function.
* c.opt: Add -Wno-address-of-packed-member.
gcc/c/
PR c/51628
* c-typeck.c (convert_for_assignment): Call
warn_for_address_of_packed_member. Issue an warning if address
of packed member is taken.
gcc/cp/
PR c/51628
* call.c (convert_for_arg_passing): Call
warn_for_address_of_packed_member. Issue an warning if address
of packed member is taken.
* typeck.c (convert_for_assignment): Likewise.
gcc/testsuite/
PR c/51628
* c-c++-common/pr51628-1.c: New tests.
* c-c++-common/pr51628-10.c: Likewise.
* c-c++-common/pr51628-2.c: Likewise.
* c-c++-common/pr51628-3.c: Likewise.
* c-c++-common/pr51628-4.c: Likewise.
* c-c++-common/pr51628-5.c: Likewise.
* c-c++-common/pr51628-6.c: Likewise.
* c-c++-common/pr51628-7.c: Likewise.
* c-c++-common/pr51628-8.c: Likewise.
* c-c++-common/pr51628-9.c: Likewise.
* gcc.dg/pr51628-10.c: Likewise.
* gcc.dg/pr51628-11.c: Likewise.
* c-c++-common/ubsan/align-10.c: Add -Wno-address-of-packed-member.
* c-c++-common/ubsan/align-2.c: Likewise.
* c-c++-common/ubsan/align-4.c: Likewise.
* c-c++-common/ubsan/align-6.c: Likewise.
* c-c++-common/ubsan/align-7.c: Likewise.
* c-c++-common/ubsan/align-8.c: Likewise.
* g++.dg/ubsan/align-2.C: Likewise.
* gcc.target/i386/avx512bw-vmovdqu16-2.c: Likewise.
* gcc.target/i386/avx512f-vmovdqu32-2.c: Likewise.
* gcc.target/i386/avx512f-vmovdqu64-2.c: Likewise.
* gcc.target/i386/avx512vl-vmovdqu16-2.c: Likewise.
* gcc.target/i386/avx512vl-vmovdqu32-2.c: Likewise.
* gcc.target/i386/avx512vl-vmovdqu64-2.c: Likewise.
---
gcc/c-family/c-common.h| 1 +
gcc/c-family/c-warn.c | 56 ++
gcc/c-family/c.opt | 4 ++
gcc/c/c-typeck.c | 40 +++-
gcc/cp/call.c | 8
gcc/cp/typeck.c| 41
gcc/doc/invoke.texi| 11 -
gcc/testsuite/c-c++-common/pr51628-1.c | 29 +++
gcc/testsuite/c-c++-common/pr51628-10.c| 24 ++
gcc/testsuite/c-c++-com
Re: [PATCH] C/C++: Add -Waddress-of-packed-member
On Mon, Jan 15, 2018 at 1:42 AM, Jakub Jelinek wrote:
> On Sun, Jan 14, 2018 at 06:29:54AM -0800, H.J. Lu wrote:
>> + if (TREE_CODE (field) == FIELD_DECL && DECL_PACKED (field))
>> + {
>> + tree field_type = TREE_TYPE (field);
>> + unsigned int type_align = TYPE_ALIGN (field_type);
>> + tree context = DECL_CONTEXT (field);
>> + unsigned int record_align = TYPE_ALIGN (context);
>> + if ((record_align % type_align) != 0)
>> + return context;
>> + type_align /= BITS_PER_UNIT;
>> + unsigned HOST_WIDE_INT field_off
>> + = (tree_to_uhwi (DECL_FIELD_OFFSET (field))
>> + + (tree_to_uhwi (DECL_FIELD_BIT_OFFSET (field))
>> +/ BITS_PER_UNIT));
>
> This has the same bug I've just created PR83844 for, you can't assume
> DECL_FIELD_OFFSET is INTEGER_CST that fits into UHWI, and also we have
> byte_position wrapper that should be used to compute the offset from
> DECL_FIELD_*OFFSET.
Here is the updated patch to use byte_position wrapper. OK for trunk?
--
H.J.
From 2a26ed809f7af5f52a24367bfa0b29898ac7fa87 Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Fri, 12 Jan 2018 21:12:05 -0800
Subject: [PATCH] C/C++: Add -Waddress-of-packed-member
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 warn it:
$ 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: initialization of 'int *' from address of packed member of 'struct pair_t' may result in an unaligned pointer value [-Waddress-of-packed-member]
int *addr = &p.i;
^
$
This warning is enabled by default.
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 (warn_for_address_of_packed_member): New function.
* c.opt: Add -Wno-address-of-packed-member.
gcc/c/
PR c/51628
* c-typeck.c (convert_for_assignment): Call
warn_for_address_of_packed_member. Issue an warning if address
of packed member is taken.
gcc/cp/
PR c/51628
* call.c (convert_for_arg_passing): Call
warn_for_address_of_packed_member. Issue an warning if address
of packed member is taken.
* typeck.c (convert_for_assignment): Likewise.
gcc/testsuite/
PR c/51628
* c-c++-common/pr51628-1.c: New tests.
* c-c++-common/pr51628-2.c: Likewise.
* c-c++-common/pr51628-3.c: Likewise.
* c-c++-common/pr51628-4.c: Likewise.
* c-c++-common/pr51628-5.c: Likewise.
* c-c++-common/pr51628-6.c: Likewise.
* c-c++-common/pr51628-7.c: Likewise.
* c-c++-common/pr51628-8.c: Likewise.
* c-c++-common/pr51628-9.c: Likewise.
* gcc.dg/pr51628-10.c: Likewise.
* gcc.dg/pr51628-11.c: Likewise.
* c-c++-common/ubsan/align-10.c: Add -Wno-address-of-packed-member.
* c-c++-common/ubsan/align-2.c: Likewise.
* c-c++-common/ubsan/align-4.c: Likewise.
* c-c++-common/ubsan/align-6.c: Likewise.
* c-c++-common/ubsan/align-7.c: Likewise.
* c-c++-common/ubsan/align-8.c: Likewise.
* g++.dg/ubsan/align-2.C: Likewise.
---
gcc/c-family/c-common.h | 1 +
gcc/c-family/c-warn.c | 56 +
gcc/c-family/c.opt | 4 +++
gcc/c/c-typeck.c| 40 -
gcc/cp/call.c | 8 +
gcc/cp/typeck.c | 41 +
gcc/doc/invoke.texi | 11 --
gcc/testsuite/c-c++-common/pr51628-1.c | 29 +++
gcc/testsuite/c-c++-common/pr51628-2.c | 29 +++
gcc/testsuite/c-c++-common/pr51628-3.c | 35 ++
gcc/testsuite/c-c++-common/pr51628-4.c | 35 ++
gcc/testsuite/c-c++-common/pr51628-5.c | 35 ++
gcc/testsuite/c-c++-common/pr51628-6.c | 35 ++
gcc/testsuite/c-c++-common/pr51628-7.c | 29 +++
gcc/testsuite/c-c++-common/pr51628-8.c | 36 +++
gcc/testsuite/c-c++-common/pr51628-9.c | 36 +++
gcc/testsuite/c-c++-common/ubsan/align-10.c | 2 +-
gcc/testsuite/c-c++-common/ubsan/align-2.c | 2 +-
gcc/testsuite/c-c++-common/ubsan/align-4.c | 2 +-
gcc/testsuite/c-c++-common/ubsan/align-6.c | 2 +-
gcc/testsuite/c-c++-common/ubsan/align-7.c | 2 +-
gcc/testsuite/c-c++-common/ubsan/align-8.c | 2 +-
gcc/testsuite/g++.dg/ubsan/align-2.C| 2 +-
gcc/testsuite/gcc.dg/pr51628-10.c | 23
gcc/testsuite/gcc.dg/pr51628-11.c | 26 ++
25 files changed, 513 insertions(+), 10 deletions(-)
Re: [PATCH] C/C++: Add -Waddress-of-packed-member
On Sun, Jan 14, 2018 at 06:29:54AM -0800, H.J. Lu wrote:
> + if (TREE_CODE (field) == FIELD_DECL && DECL_PACKED (field))
> + {
> + tree field_type = TREE_TYPE (field);
> + unsigned int type_align = TYPE_ALIGN (field_type);
> + tree context = DECL_CONTEXT (field);
> + unsigned int record_align = TYPE_ALIGN (context);
> + if ((record_align % type_align) != 0)
> + return context;
> + type_align /= BITS_PER_UNIT;
> + unsigned HOST_WIDE_INT field_off
> + = (tree_to_uhwi (DECL_FIELD_OFFSET (field))
> + + (tree_to_uhwi (DECL_FIELD_BIT_OFFSET (field))
> +/ BITS_PER_UNIT));
This has the same bug I've just created PR83844 for, you can't assume
DECL_FIELD_OFFSET is INTEGER_CST that fits into UHWI, and also we have
byte_position wrapper that should be used to compute the offset from
DECL_FIELD_*OFFSET.
Jakub
[PATCH] C/C++: Add -Waddress-of-packed-member
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 warn it:
$ 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: initialization of 'int *' from address of packed member of
'struct pair_t' may result in an unaligned pointer value
[-Waddress-of-packed-member]
int *addr = &p.i;
^
$
This warning is enabled by default.
Tested on i686 and x86-64. 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 (warn_for_address_of_packed_member): New function.
* c.opt: Add -Wno-address-of-packed-member.
gcc/c/
PR c/51628
* c-typeck.c (convert_for_assignment): Call
warn_for_address_of_packed_member. Issue an warning if address
of packed member is taken.
gcc/cp/
PR c/51628
* call.c (convert_for_arg_passing): Call
warn_for_address_of_packed_member. Issue an warning if address
of packed member is taken.
* typeck.c (convert_for_assignment): Likewise.
gcc/testsuite/
PR c/51628
* c-c++-common/pr51628-1.c: New tests.
* c-c++-common/pr51628-2.c: Likewise.
* c-c++-common/pr51628-3.c: Likewise.
* c-c++-common/pr51628-4.c: Likewise.
* c-c++-common/pr51628-5.c: Likewise.
* c-c++-common/pr51628-6.c: Likewise.
* c-c++-common/ubsan/align-2.c: Add -Wno-address-of-packed-member.
* c-c++-common/ubsan/align-4.c: Likewise.
* c-c++-common/ubsan/align-6.c: Likewise.
* c-c++-common/ubsan/align-7.c: Likewise.
* c-c++-common/ubsan/align-8.c: Likewise.
* g++.dg/ubsan/align-2.C: Likewise.
---
gcc/c-family/c-common.h| 1 +
gcc/c-family/c-warn.c | 38 +++
gcc/c-family/c.opt | 4 +++
gcc/c/c-typeck.c | 40 -
gcc/cp/call.c | 8 ++
gcc/cp/typeck.c| 41 ++
gcc/doc/invoke.texi| 11 ++--
gcc/testsuite/c-c++-common/pr51628-1.c | 29 +
gcc/testsuite/c-c++-common/pr51628-2.c | 29 +
gcc/testsuite/c-c++-common/pr51628-3.c | 35 +
gcc/testsuite/c-c++-common/pr51628-4.c | 35 +
gcc/testsuite/c-c++-common/pr51628-5.c | 35 +
gcc/testsuite/c-c++-common/pr51628-6.c | 35 +
gcc/testsuite/c-c++-common/ubsan/align-2.c | 2 +-
gcc/testsuite/c-c++-common/ubsan/align-4.c | 2 +-
gcc/testsuite/c-c++-common/ubsan/align-6.c | 2 +-
gcc/testsuite/c-c++-common/ubsan/align-7.c | 2 +-
gcc/testsuite/c-c++-common/ubsan/align-8.c | 2 +-
gcc/testsuite/g++.dg/ubsan/align-2.C | 2 +-
19 files changed, 344 insertions(+), 9 deletions(-)
create mode 100644 gcc/testsuite/c-c++-common/pr51628-1.c
create mode 100644 gcc/testsuite/c-c++-common/pr51628-2.c
create mode 100644 gcc/testsuite/c-c++-common/pr51628-3.c
create mode 100644 gcc/testsuite/c-c++-common/pr51628-4.c
create mode 100644 gcc/testsuite/c-c++-common/pr51628-5.c
create mode 100644 gcc/testsuite/c-c++-common/pr51628-6.c
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index d090881e95d..ef31e4d0aa2 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1384,6 +1384,7 @@ extern void c_do_switch_warnings (splay_tree, location_t,
tree, tree, bool,
bool);
extern void warn_for_omitted_condop (location_t, tree);
extern void warn_for_restrict (unsigned, tree *, unsigned);
+extern tree warn_for_address_of_packed_member (tree type, tree rhs);
/* Places where an lvalue, or modifiable lvalue, may be required.
Used to select diagnostic messages in lvalue_error and
diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c
index 7d87c455ec0..ac0a2ffcb42 100644
--- a/gcc/c-family/c-warn.c
+++ b/gcc/c-family/c-warn.c
@@ -2576,3 +2576,41 @@ warn_for_multistatement_macros (location_t body_loc,
location_t next_loc,
inform (guard_loc, "some parts of macro expansion are not guarded by "
"this %qs clause", guard_tinfo_to_string (keyword));
}
+
+/* Return struct or union type if the right hand type, RHS, is the
+ address of packed member of struct or union when assigning to TYPE.
+ Otherwise, return NULL_TREE. */
+
+tree
+warn_for_address_of_packed_member (tree type, tree rhs)
+{
+ if (!warn_address_of_packed_member)
+return NULL_TREE;
+
+ if (TREE_CODE (rhs) == ADDR_EXPR && POINTER_TYPE_P (type))
+{
+ tree base = TREE_OPER
