On 1/20/19 4:40 PM, H.J. Lu wrote:
> On Sun, Jan 20, 2019 at 5:29 AM Bernd Edlinger
> <bernd.edlin...@hotmail.de> wrote:
>>
>> Hi,
>>
>>
>> I tried to build linux yesterday, and became aware that there are a few
>> false-positive warnings with -Waddress-of-packed-member:
>>
>> struct t {
>> char a;
>> int b;
>> int *c;
>> int d[10];
>> } __attribute__((packed));
>>
>> struct t t0;
>> struct t *t1;
>> struct t **t2;
>>
>> t2 = &t1;
>> i1 = t0.c;
>>
>> I fixed them quickly with the attached patch, and added a new test case,
>> which also revealed a few missing warnings:
>>
>> struct t t100[10][10];
>> struct t (*baz())[10];
>>
>> t2 = (struct t**) t100;
>> t2 = (struct t**) baz();
>>
>>
>> Well I remembered that Joseph wanted me to use min_align_of_type instead
>> of TYPE_ALIGN in the -Wcast-align warning, so I changed
>> -Waddress-of-packed-member
>> to do that as well.
>>
>> Since I was not sure if the test coverage is good enough, I added a few more
>> test cases, which just make sure the existing warning works properly.
>
> You should also fix
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88928
>
Yes, thanks. I added a check for NULL from TYPE_STUB_DECL here:
+ tree decl = TYPE_STUB_DECL (rhstype);
+ if (decl)
+ inform (DECL_SOURCE_LOCATION (decl), "defined here");
and the test case from the PR.
>> I am however not sure of a warning that is as noisy as this one, should be
>> default-enabled, because it might interfere with configure tests. That might
>
> These codes can lead unaligned access which may be very hard to track
> down or fix since the codes in question may be in a library. I think it is a
> very useful warning. Besides, these noisy warnings also reveal implementation
> issues in GCC, which, otherwise, may not be known for a long time.
>
No doubt that is a useful warning there is no question about that.
Are you concerned about production code that gets built w/o at least -Wall ?
I am however not sure, why it is limited to packed structures.
>> be better to enable this warning, in -Wall or -Wextra, and, well maybe
>> enable -Wcast-align=strict also at the same warning level, since it is
>> currently
>> not enabled at all, unless explicitly requested, what do you think?
>>
I just see, your warning as being similar in spirit as -Wcast-align, since
if type casts are involved, you don't need packed structures to get unaligned
pointers. So what I wonder now is, if it is a bit inconsistent to have
-Wcast-align=strict not being enabled at least with -Wextra, while
-Waddress-of-packed-member being default enabled. I am just unsure if one day
-Wcast-align should be enabled by -Wall/-Wextra or per default as well, or being
superseded by -Waddress-of-packed-member. I mean, except for the limitation
on requiring the packed keyword on structures, -Waddress-of-packed-member is now
a superset of -Wcast-align=strict, you know.
Anyway, new patch version with fix for PR 88928 attached.
Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?
Thanks
Bernd.
2019-01-20 Bernd Edlinger <bernd.edlin...@hotmail.de>
PR c/88928
* c-warn.c (check_alignment_of_packed_member): Add a boolean parameter
for rvalue context. Handle rvalues correctly. Use min_align_of_type
instead of TYPE_ALIGN.
(check_address_or_pointer_of_packed_member): Handle rvalues coorrectly.
Use min_align_of_type instead of TYPE_ALIGN_UNIT. Check for NULL
pointer from TYPE_STUB_DECL.
testsuite:
2019-01-20 Bernd Edlinger <bernd.edlin...@hotmail.de>
PR c/88928
* c-c++-common/Waddress-of-packed-member-1.c: New test case.
* gcc.dg/pr88928.c: New test case.
Index: gcc/c-family/c-warn.c
===================================================================
--- gcc/c-family/c-warn.c (revision 268102)
+++ gcc/c-family/c-warn.c (working copy)
@@ -35,6 +35,7 @@ along with GCC; see the file COPYING3. If not see
#include "gimplify.h"
#include "c-family/c-indentation.h"
#include "calls.h"
+#include "stor-layout.h"
/* Print a warning if a constant expression had overflow in folding.
Invoke this function on every expression that the language
@@ -2688,25 +2689,26 @@ warn_for_multistatement_macros (location_t body_lo
}
/* Return struct or union type if the alignment of data memeber, FIELD,
- is less than the alignment of TYPE. Otherwise, return NULL_TREE. */
+ is less than the alignment of TYPE. Otherwise, return NULL_TREE.
+ If RVALUE is true, only arrays evaluate to pointers. */
static tree
-check_alignment_of_packed_member (tree type, tree field)
+check_alignment_of_packed_member (tree type, tree field, bool rvalue)
{
/* Check alignment of the data member. */
if (TREE_CODE (field) == FIELD_DECL
- && (DECL_PACKED (field)
- || TYPE_PACKED (TREE_TYPE (field))))
+ && (DECL_PACKED (field) || TYPE_PACKED (TREE_TYPE (field)))
+ && (!rvalue || TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE))
{
/* Check the expected alignment against the field alignment. */
- unsigned int type_align = TYPE_ALIGN (type);
+ unsigned int type_align = min_align_of_type (type);
tree context = DECL_CONTEXT (field);
- unsigned int record_align = TYPE_ALIGN (context);
- if ((record_align % type_align) != 0)
+ unsigned int record_align = min_align_of_type (context);
+ if (record_align < type_align)
return context;
tree field_off = byte_position (field);
if (!multiple_of_p (TREE_TYPE (field_off), field_off,
- size_int (type_align / BITS_PER_UNIT)))
+ size_int (type_align)))
return context;
}
@@ -2722,19 +2724,27 @@ static tree
static tree
check_address_or_pointer_of_packed_member (tree type, tree rhs)
{
+ bool rvalue = true;
+
if (INDIRECT_REF_P (rhs))
rhs = TREE_OPERAND (rhs, 0);
if (TREE_CODE (rhs) == ADDR_EXPR)
- rhs = TREE_OPERAND (rhs, 0);
+ {
+ rhs = TREE_OPERAND (rhs, 0);
+ rvalue = false;
+ }
- if (POINTER_TYPE_P (type))
- type = TREE_TYPE (type);
+ if (!POINTER_TYPE_P (type))
+ return NULL_TREE;
+ type = TREE_TYPE (type);
+
if (TREE_CODE (rhs) == PARM_DECL
|| VAR_P (rhs)
|| TREE_CODE (rhs) == CALL_EXPR)
{
+ tree rhstype = TREE_TYPE (rhs);
if (TREE_CODE (rhs) == CALL_EXPR)
{
rhs = CALL_EXPR_FN (rhs); /* Pointer expression. */
@@ -2742,24 +2752,30 @@ check_address_or_pointer_of_packed_member (tree ty
return NULL_TREE;
rhs = TREE_TYPE (rhs); /* Pointer type. */
rhs = TREE_TYPE (rhs); /* Function type. */
+ rhstype = TREE_TYPE (rhs);
+ if (!POINTER_TYPE_P (rhstype))
+ return NULL_TREE;
+ rvalue = true;
}
- tree rhstype = TREE_TYPE (rhs);
- if ((POINTER_TYPE_P (rhstype)
- || TREE_CODE (rhstype) == ARRAY_TYPE)
- && TYPE_PACKED (TREE_TYPE (rhstype)))
+ if (rvalue && POINTER_TYPE_P (rhstype))
+ rhstype = TREE_TYPE (rhstype);
+ while (TREE_CODE (rhstype) == ARRAY_TYPE)
+ rhstype = TREE_TYPE (rhstype);
+ if (TYPE_PACKED (rhstype))
{
- unsigned int type_align = TYPE_ALIGN_UNIT (type);
- unsigned int rhs_align = TYPE_ALIGN_UNIT (TREE_TYPE (rhstype));
- if ((rhs_align % type_align) != 0)
+ unsigned int type_align = min_align_of_type (type);
+ unsigned int rhs_align = min_align_of_type (rhstype);
+ if (rhs_align < type_align)
{
location_t location = EXPR_LOC_OR_LOC (rhs, input_location);
warning_at (location, OPT_Waddress_of_packed_member,
"converting a packed %qT pointer (alignment %d) "
- "to %qT (alignment %d) may result in an "
+ "to a %qT pointer (alignment %d) may result in an "
"unaligned pointer value",
rhstype, rhs_align, type, type_align);
- tree decl = TYPE_STUB_DECL (TREE_TYPE (rhstype));
- inform (DECL_SOURCE_LOCATION (decl), "defined here");
+ tree decl = TYPE_STUB_DECL (rhstype);
+ if (decl)
+ inform (DECL_SOURCE_LOCATION (decl), "defined here");
decl = TYPE_STUB_DECL (type);
if (decl)
inform (DECL_SOURCE_LOCATION (decl), "defined here");
@@ -2776,7 +2792,7 @@ check_address_or_pointer_of_packed_member (tree ty
if (TREE_CODE (rhs) == COMPONENT_REF)
{
tree field = TREE_OPERAND (rhs, 1);
- context = check_alignment_of_packed_member (type, field);
+ context = check_alignment_of_packed_member (type, field, rvalue);
if (context)
break;
}
Index: gcc/testsuite/c-c++-common/Waddress-of-packed-member-1.c
===================================================================
--- gcc/testsuite/c-c++-common/Waddress-of-packed-member-1.c (revision 0)
+++ gcc/testsuite/c-c++-common/Waddress-of-packed-member-1.c (working copy)
@@ -0,0 +1,55 @@
+/* { dg-do compile } */
+/* { dg-options "-Waddress-of-packed-member" } */
+
+struct t {
+ char a;
+ int b;
+ int *c;
+ int d[10];
+} __attribute__((packed));
+
+struct t t0;
+struct t t10[10];
+struct t t100[10][10];
+struct t *t1;
+struct t **t2;
+struct t *bar();
+struct t (*baz())[10];
+struct t (*bazz())[10][10];
+int *i1;
+__UINTPTR_TYPE__ u1;
+__UINTPTR_TYPE__ baa();
+
+void foo (void)
+{
+ t1 = &t0; /* { dg-bogus "may result in an unaligned pointer value" } */
+ t1 = t10; /* { dg-bogus "may result in an unaligned pointer value" } */
+ t2 = &t1; /* { dg-bogus "may result in an unaligned pointer value" } */
+ t2 = t2; /* { dg-bogus "may result in an unaligned pointer value" } */
+ t2 = (struct t**)t2; /* { dg-bogus "may result in an unaligned pointer value" } */
+ t1 = (struct t*)t2; /* { dg-bogus "may result in an unaligned pointer value" } */
+ t1 = bar(); /* { dg-bogus "may result in an unaligned pointer value" } */
+ t1 = (struct t*) baz(); /* { dg-bogus "may result in an unaligned pointer value" } */
+ t1 = (struct t*) bazz(); /* { dg-bogus "may result in an unaligned pointer value" } */
+ t1 = *baz(); /* { dg-bogus "may result in an unaligned pointer value" } */
+ t1 = **bazz(); /* { dg-bogus "may result in an unaligned pointer value" } */
+ t1 = (struct t*) baa(); /* { dg-bogus "may result in an unaligned pointer value" } */
+ t2 = (struct t**) baa(); /* { dg-bogus "may result in an unaligned pointer value" } */
+ i1 = t0.c; /* { dg-bogus "may result in an unaligned pointer value" } */
+ i1 = t1->c; /* { dg-bogus "may result in an unaligned pointer value" } */
+ i1 = t10[0].c; /* { dg-bogus "may result in an unaligned pointer value" } */
+ u1 = (__UINTPTR_TYPE__) &t0; /* { dg-bogus "may result in an unaligned pointer value" } */
+ u1 = (__UINTPTR_TYPE__) t1; /* { dg-bogus "may result in an unaligned pointer value" } */
+ t2 = (struct t**) t10; /* { dg-warning "may result in an unaligned pointer value" } */
+ t2 = (struct t**) t100; /* { dg-warning "may result in an unaligned pointer value" } */
+ t2 = (struct t**) t1; /* { dg-warning "may result in an unaligned pointer value" } */
+ t2 = (struct t**) bar(); /* { dg-warning "may result in an unaligned pointer value" } */
+ t2 = (struct t**) baz(); /* { dg-warning "may result in an unaligned pointer value" } */
+ t2 = (struct t**) bazz(); /* { dg-warning "may result in an unaligned pointer value" } */
+ i1 = &t0.b; /* { dg-warning "may result in an unaligned pointer value" } */
+ i1 = &t1->b; /* { dg-warning "may result in an unaligned pointer value" } */
+ i1 = &t10[0].b; /* { dg-warning "may result in an unaligned pointer value" } */
+ i1 = t0.d; /* { dg-warning "may result in an unaligned pointer value" } */
+ i1 = t1->d; /* { dg-warning "may result in an unaligned pointer value" } */
+ i1 = t10[0].d; /* { dg-warning "may result in an unaligned pointer value" } */
+}
Index: gcc/testsuite/gcc.dg/pr88928.c
===================================================================
--- gcc/testsuite/gcc.dg/pr88928.c (revision 0)
+++ gcc/testsuite/gcc.dg/pr88928.c (working copy)
@@ -0,0 +1,6 @@
+/* { dg-do compile } */
+/* { dg-options "-Wno-pedantic -Waddress-of-packed-member" } */
+struct a { } __attribute__((__packed__));
+void c (struct a **);
+void d (const struct a *b) { c ((struct a **) b); }
+/* { dg-warning "may result in an unaligned pointer value" "" { target *-*-* } .-1 } */
