| Message ID | 20190113124838.29506-1-hjl.tools@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | C-family: Only check the non-pointer data member | expand |
On Sun, Jan 13, 2019 at 04:48:38AM -0800, H.J. Lu wrote: > When checking alignment of packed member, we should only check the > non-pointer data member. > > gcc/c-family/ > > PR c++/88664 > * c-family/c-warn.c (check_alignment_of_packed_member): Only > check the non-pointer data member. Doesn't that mean we'd stop warning about: struct S { short s; void *p; } __attribute__ ((__packed__)); void ** foo (struct S *x) { return static_cast <void **> (&x->p); } where we do want to warn? What always matters is whether we take address of a packed structure field/non-static data member or whether we just read that field. The former should be warned about, the latter not. Jakub
On Sun, Jan 13, 2019 at 5:20 AM Jakub Jelinek <jakub@redhat.com> wrote: > > On Sun, Jan 13, 2019 at 04:48:38AM -0800, H.J. Lu wrote: > > When checking alignment of packed member, we should only check the > > non-pointer data member. > > > > gcc/c-family/ > > > > PR c++/88664 > > * c-family/c-warn.c (check_alignment_of_packed_member): Only > > check the non-pointer data member. > > Doesn't that mean we'd stop warning about: > struct S { short s; void *p; } __attribute__ ((__packed__)); > > void ** > foo (struct S *x) > { > return static_cast <void **> (&x->p); > } > where we do want to warn? > What always matters is whether we take address of a packed structure > field/non-static data member or whether we just read that field. > The former should be warned about, the latter not. > How about this patch? It checks if address is taken with NOP.
On Sun, Jan 13, 2019 at 06:54:05AM -0800, H.J. Lu wrote: > > What always matters is whether we take address of a packed structure > > field/non-static data member or whether we just read that field. > > The former should be warned about, the latter not. > > > > How about this patch? It checks if address is taken with NOP. I'd like to first understand the convert_p argument to warn_for_address_or_pointer_of_packed_member. To me it seems you want to emit two different warnings, perhaps one surpressed if the other one is emitted, but you actually from the start decide which of the two you are going to check for. That is just weird. Consider -O2 -Waddress-of-packed-member -Wno-incompatible-pointer-types: struct __attribute__((packed)) S { char p; int a, b, c; }; int * foo (int x, struct S *p) { return x ? &p->a : &p->b; } int * bar (int x, struct S *p) { return (int *) (x ? &p->a : &p->b); } short * baz (int x, struct S *p) { return x ? &p->a : &p->b; } short * qux (int x, struct S *p) { return (short *) (x ? &p->a : &p->b); } This warns in foo, bar and qux, but doesn't warn in baz, because we've decided upfront that that case is convert_p = true. I would have expected that the convert_p argument isn't passed at all, the function always does the diagnostics about taking address that is done with !convert_p right now, and either do the pointer -> pointer conversion warning somewhere else (wherever we detect a pointer to pointer conversion, even in the middle of expression?), or do it wherever you do currently, but again always if the orig_rhs and type pointer types are different. Jakub
On Mon, Jan 14, 2019 at 6:22 AM Jakub Jelinek <jakub@redhat.com> wrote: > > On Sun, Jan 13, 2019 at 06:54:05AM -0800, H.J. Lu wrote: > > > What always matters is whether we take address of a packed structure > > > field/non-static data member or whether we just read that field. > > > The former should be warned about, the latter not. > > > > > > > How about this patch? It checks if address is taken with NOP. > > I'd like to first understand the convert_p argument to > warn_for_address_or_pointer_of_packed_member. > > To me it seems you want to emit two different warnings, perhaps one > surpressed if the other one is emitted, but you actually from the start > decide which of the two you are going to check for. That is just weird. convert_p is only for C. > Consider -O2 -Waddress-of-packed-member -Wno-incompatible-pointer-types: > > struct __attribute__((packed)) S { char p; int a, b, c; }; > > int * > foo (int x, struct S *p) > { > return x ? &p->a : &p->b; > } > > int * > bar (int x, struct S *p) > { > return (int *) (x ? &p->a : &p->b); > } > > short * > baz (int x, struct S *p) > { > return x ? &p->a : &p->b; > } > > short * > qux (int x, struct S *p) > { > return (short *) (x ? &p->a : &p->b); > } > > This warns in foo, bar and qux, but doesn't warn in baz, because we've > decided upfront that that case is convert_p = true. > > I would have expected that the convert_p argument isn't passed at all, > the function always does the diagnostics about taking address that is > done with !convert_p right now, and either do the pointer -> pointer > conversion warning somewhere else (wherever we detect a pointer to pointer > conversion, even in the middle of expression?), or do it wherever you do > currently, but again always if the orig_rhs and type pointer types are > different. > When convert_p is true, we need to treat pointer conversion as a special case. I am testing this updated patch.
On Mon, Jan 14, 2019 at 10:00 AM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Mon, Jan 14, 2019 at 6:22 AM Jakub Jelinek <jakub@redhat.com> wrote: > > > > On Sun, Jan 13, 2019 at 06:54:05AM -0800, H.J. Lu wrote: > > > > What always matters is whether we take address of a packed structure > > > > field/non-static data member or whether we just read that field. > > > > The former should be warned about, the latter not. > > > > > > > > > > How about this patch? It checks if address is taken with NOP. > > > > I'd like to first understand the convert_p argument to > > warn_for_address_or_pointer_of_packed_member. > > > > To me it seems you want to emit two different warnings, perhaps one > > surpressed if the other one is emitted, but you actually from the start > > decide which of the two you are going to check for. That is just weird. > > convert_p is only for C. > > > Consider -O2 -Waddress-of-packed-member -Wno-incompatible-pointer-types: > > > > struct __attribute__((packed)) S { char p; int a, b, c; }; > > > > int * > > foo (int x, struct S *p) > > { > > return x ? &p->a : &p->b; > > } > > > > int * > > bar (int x, struct S *p) > > { > > return (int *) (x ? &p->a : &p->b); > > } > > > > short * > > baz (int x, struct S *p) > > { > > return x ? &p->a : &p->b; > > } > > > > short * > > qux (int x, struct S *p) > > { > > return (short *) (x ? &p->a : &p->b); > > } > > > > This warns in foo, bar and qux, but doesn't warn in baz, because we've > > decided upfront that that case is convert_p = true. > > > > I would have expected that the convert_p argument isn't passed at all, > > the function always does the diagnostics about taking address that is > > done with !convert_p right now, and either do the pointer -> pointer > > conversion warning somewhere else (wherever we detect a pointer to pointer > > conversion, even in the middle of expression?), or do it wherever you do > > currently, but again always if the orig_rhs and type pointer types are > > different. > > > > When convert_p is true, we need to treat pointer conversion > as a special case. I am testing this updated patch. > There are no regressions with this patch: https://gcc.gnu.org/ml/gcc-patches/2019-01/msg00792.html
On Mon, Jan 14, 2019 at 10:00:11AM -0800, H.J. Lu wrote: > On Mon, Jan 14, 2019 at 6:22 AM Jakub Jelinek <jakub@redhat.com> wrote: > > > > On Sun, Jan 13, 2019 at 06:54:05AM -0800, H.J. Lu wrote: > > > > What always matters is whether we take address of a packed structure > > > > field/non-static data member or whether we just read that field. > > > > The former should be warned about, the latter not. > > > > > > > > > > How about this patch? It checks if address is taken with NOP. > > > > I'd like to first understand the convert_p argument to > > warn_for_address_or_pointer_of_packed_member. > > > > To me it seems you want to emit two different warnings, perhaps one > > surpressed if the other one is emitted, but you actually from the start > > decide which of the two you are going to check for. That is just weird. > > convert_p is only for C. Why? What is so special about C and (implicit?) casts where the rhs isn't ADDR_EXPR? Aren't all casts (explicit or implicit) from one pointer type to another pointer and satisfy the rules something that should be warned about if the conditions are met? Jakub
On Wed, Jan 16, 2019 at 3:30 AM Jakub Jelinek <jakub@redhat.com> wrote: > > On Mon, Jan 14, 2019 at 10:00:11AM -0800, H.J. Lu wrote: > > On Mon, Jan 14, 2019 at 6:22 AM Jakub Jelinek <jakub@redhat.com> wrote: > > > > > > On Sun, Jan 13, 2019 at 06:54:05AM -0800, H.J. Lu wrote: > > > > > What always matters is whether we take address of a packed structure > > > > > field/non-static data member or whether we just read that field. > > > > > The former should be warned about, the latter not. > > > > > > > > > > > > > How about this patch? It checks if address is taken with NOP. > > > > > > I'd like to first understand the convert_p argument to > > > warn_for_address_or_pointer_of_packed_member. > > > > > > To me it seems you want to emit two different warnings, perhaps one > > > surpressed if the other one is emitted, but you actually from the start > > > decide which of the two you are going to check for. That is just weird. > > > > convert_p is only for C. > > Why? What is so special about C and (implicit?) casts where the rhs isn't > ADDR_EXPR? Aren't all casts (explicit or implicit) from one pointer type > to another pointer and satisfy the rules something that should be warned -Wincompatible-pointer-types is C only. In C++, incompatible pointer types aren't allowed at all. > about if the conditions are met? > convert_p is set to TRUE to handle this case for incompatible pointer types: [hjl@gnu-cfl-1 pr51628-3]$ cat x.i struct B { int i; }; struct C { struct B b; } __attribute__ ((packed)); extern struct C *p; long* g8 (void) { return p; } [hjl@gnu-cfl-1 pr51628-3]$ 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 \u2018g8\u2019: x.i:6:26: warning: returning \u2018struct C *\u2019 from a function with incompatible return type \u2018long int *\u2019 [-Wincompatible-pointer-types] 6 | long* g8 (void) { return p; } | ^ x.i:6:1: warning: converting a packed \u2018struct C *\u2019 pointer (alignment 1) to \u2018long int *\u2019 (alignment 8) may result in an unaligned pointer value [-Waddress-of-packed-member] 6 | long* g8 (void) { return p; } | ^~~~ x.i:2:8: note: defined here 2 | struct C { struct B b; } __attribute__ ((packed)); | ^ [hjl@gnu-cfl-1 pr51628-3]$ I am using the same function to warn both addresses and pointers. I need a way to tell that we are converting pointers, not assigning address to pointer. This info is readily available in the front-end.
On Wed, Jan 16, 2019 at 04:11:44AM -0800, H.J. Lu wrote: > > Why? What is so special about C and (implicit?) casts where the rhs isn't > > ADDR_EXPR? Aren't all casts (explicit or implicit) from one pointer type > > to another pointer and satisfy the rules something that should be warned > > -Wincompatible-pointer-types is C only. In C++, incompatible pointer types > aren't allowed at all. How so? You can certainly: struct B { int i; }; struct C { struct B b; } __attribute__ ((packed)); extern struct C *p; long* g8 (void) { return (long *)p; } and similarly for C. So, why is explicit cast something that shouldn't be warned about in this case and implicit cast should get a warning, especially when it already does get one (and one even enabled by default, -Wincompatible-pointer-types)? Either such casts are problematic and then it should treat explicit and implicit casts the same, or they aren't, and then -Wincompatible-pointer-types is all you need. Jakub
On Mon, Jan 14, 2019 at 03:23:07PM -0800, H.J. Lu wrote: > There are no regressions with this patch: > > https://gcc.gnu.org/ml/gcc-patches/2019-01/msg00792.html As the patch seems to be a step forward and fixes an important regression, the patch is ok for trunk, but I'd like to keep discussions on the convert_p stuff afterwards (see other mail). Jakub
On Wed, Jan 16, 2019 at 3:09 PM Jakub Jelinek <jakub@redhat.com> wrote: > > On Mon, Jan 14, 2019 at 03:23:07PM -0800, H.J. Lu wrote: > > There are no regressions with this patch: > > > > https://gcc.gnu.org/ml/gcc-patches/2019-01/msg00792.html > > As the patch seems to be a step forward and fixes an important regression, > the patch is ok for trunk, but I'd like to keep discussions on the convert_p > stuff afterwards (see other mail). > I posted an updated patch: https://gcc.gnu.org/ml/gcc-patches/2019-01/msg00982.html
diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c index 070934ab2b6..97b92d3493a 100644 --- a/gcc/c-family/c-warn.c +++ b/gcc/c-family/c-warn.c @@ -2687,14 +2687,16 @@ warn_for_multistatement_macros (location_t body_loc, location_t next_loc, "this %qs clause", guard_tinfo_to_string (keyword)); } -/* Return struct or union type if the alignment of data memeber, FIELD, - is less than the alignment of TYPE. Otherwise, return NULL_TREE. */ +/* Return struct or union type if the alignment of non-pointer data + memeber, FIELD, is less than the alignment of TYPE. Otherwise, + return NULL_TREE. */ static tree check_alignment_of_packed_member (tree type, tree field) { - /* Check alignment of the data member. */ + /* Check alignment of the non-pointer data member. */ if (TREE_CODE (field) == FIELD_DECL + && !POINTER_TYPE_P (TREE_TYPE (field)) && (DECL_PACKED (field) || TYPE_PACKED (TREE_TYPE (field)))) { diff --git a/gcc/testsuite/g++.dg/pr88664-2.C b/gcc/testsuite/g++.dg/pr88664-2.C new file mode 100644 index 00000000000..712600cdeb7 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr88664-2.C @@ -0,0 +1,21 @@ +/* PR c++/88664. */ +/* { dg-do compile } */ +/* { dg-options "-O" } */ + +struct epoll_event +{ + short events; + void *ptr; +} __attribute__ ((__packed__)); + +int * +foo1 (epoll_event *event) +{ + return static_cast <int *> (event->ptr); +} + +int * +foo2 (epoll_event *event) +{ + return event ? static_cast <int *> (event->ptr) : (int *) 0; +} diff --git a/gcc/testsuite/gcc.dg/pr88664-1.c b/gcc/testsuite/gcc.dg/pr88664-1.c new file mode 100644 index 00000000000..410167b542c --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr88664-1.c @@ -0,0 +1,19 @@ +/* PR c++/88664. */ +/* { dg-do compile } */ +/* { dg-options "-O" } */ + +struct data { + void *ptr; +} __attribute__((packed)) var; + +int * +fun1 (void) +{ + return (int *)var.ptr; +} + +int * +fun2 (int i) +{ + return i ? (int *)var.ptr : (int *) 0; +}