| Message ID | AM0PR07MB3874860FEF4EF1C52FB1E6F7E49E0@AM0PR07MB3874.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix issues with -Waddress-of-packed-member | expand |
On Sun, Jan 20, 2019 at 5:29 AM Bernd Edlinger <bernd.edlinger@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 with something like diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c index 7821cc894a7..4daadc0f6c7 100644 --- a/gcc/c-family/c-warn.c +++ b/gcc/c-family/c-warn.c @@ -2744,9 +2744,14 @@ check_address_or_pointer_of_packed_member (tree type, tre e rhs) rhs = TREE_TYPE (rhs); /* Function type. */ } tree rhstype = TREE_TYPE (rhs); + tree basetype = type; + while (POINTER_TYPE_P (basetype)) + basetype = TREE_TYPE (basetype); if ((POINTER_TYPE_P (rhstype) || TREE_CODE (rhstype) == ARRAY_TYPE) - && TYPE_PACKED (TREE_TYPE (rhstype))) + && TYPE_PACKED (TREE_TYPE (rhstype)) + && (!RECORD_OR_UNION_TYPE_P (basetype) + || TYPE_FIELDS (basetype) != NULL_TREE)) { unsigned int type_align = TYPE_ALIGN_UNIT (type); unsigned int rhs_align = TYPE_ALIGN_UNIT (TREE_TYPE (rhstype)); @@ -2759,7 +2764,8 @@ check_address_or_pointer_of_packed_member (tree type, tree rhs) "unaligned pointer value", rhstype, rhs_align, type, type_align); tree decl = TYPE_STUB_DECL (TREE_TYPE (rhstype)); - inform (DECL_SOURCE_LOCATION (decl), "defined here"); + if (decl) + inform (DECL_SOURCE_LOCATION (decl), "defined here"); decl = TYPE_STUB_DECL (type); if (decl) inform (DECL_SOURCE_LOCATION (decl), "defined here"); > 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. > 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? > > > Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > Is it OK for trunk? > >
On 1/20/19 4:40 PM, H.J. Lu wrote: > On Sun, Jan 20, 2019 at 5:29 AM Bernd Edlinger > <bernd.edlinger@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.edlinger@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.edlinger@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 } */
On Sun, Jan 20, 2019 at 9:10 AM Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > > On 1/20/19 4:40 PM, H.J. Lu wrote: > > On Sun, Jan 20, 2019 at 5:29 AM Bernd Edlinger > > <bernd.edlinger@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. Unlike -Wcast-align=strict in typedef char __attribute__ ((__aligned__(__BIGGEST_ALIGNMENT__))) c; typedef struct __attribute__ ((__aligned__(__BIGGEST_ALIGNMENT__))) { char x; } d; char *x; c *y; d *z; struct s { long long x; } *p; struct t { double x; } *q; which has a workaround by increasing source alignment, there is no workaround for -Waddress-of-packed-member. > > Anyway, new patch version with fix for PR 88928 attached. Thanks. > > Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > Is it OK for trunk? >
On 1/20/19 6:29 AM, Bernd Edlinger 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. > > 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 > 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? Across Fedora there's only around a dozen packages that tripped over this. That's a small enough set that I'm not terribly concerned about the noise factor. jeff
On 1/20/19 10:10 AM, Bernd Edlinger wrote: > On 1/20/19 4:40 PM, H.J. Lu wrote: >> On Sun, Jan 20, 2019 at 5:29 AM Bernd Edlinger >> <bernd.edlinger@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. > > > patch-fix-packed-warning-v2.diff > > 2019-01-20 Bernd Edlinger <bernd.edlinger@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.edlinger@hotmail.de> > > PR c/88928 > * c-c++-common/Waddress-of-packed-member-1.c: New test case. > * gcc.dg/pr88928.c: New test case. OK. Thanks for digging into this. jeff
2019-01-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * 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. testsuite: 2019-01-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * c-c++-common/Waddress-of-packed-member-1.c: New test case. Index: gcc/c-family/c-warn.c =================================================================== --- gcc/c-family/c-warn.c (revision 268084) +++ 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,23 +2752,28 @@ 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)); + tree decl = TYPE_STUB_DECL (rhstype); inform (DECL_SOURCE_LOCATION (decl), "defined here"); decl = TYPE_STUB_DECL (type); if (decl) @@ -2776,7 +2791,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" } */ +}
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. 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 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? Bootstrapped and reg-tested on x86_64-pc-linux-gnu. Is it OK for trunk? Thanks Bernd.