On Wed, Jan 16, 2019 at 01:28:26PM +0100, Jakub Jelinek wrote: > 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. >
I am testing this patch. H.J. --- Check unaligned pointer conversion and strip NOPS. gcc/c-family/ PR c/51628 PR c/88664 * c-common.h (warn_for_address_or_pointer_of_packed_member): Remove the boolean argument. * c-warn.c (check_address_of_packed_member): Renamed to ... (check_address_or_pointer_of_packed_member): This. Also warn pointer conversion. (check_and_warn_address_of_packed_member): Renamed to ... (check_and_warn_address_or_pointer_of_packed_member): This. Also warn pointer conversion. (warn_for_address_or_pointer_of_packed_member): Remove the boolean argument. Don't check pointer conversion here. gcc/c PR c/51628 PR c/88664 * c-typeck.c (convert_for_assignment): Upate the warn_for_address_or_pointer_of_packed_member call. gcc/cp PR c/51628 PR c/88664 * call.c (convert_for_arg_passing): Upate the warn_for_address_or_pointer_of_packed_member call. * typeck.c (convert_for_assignment): Likewise. gcc/testsuite/ PR c/51628 PR c/88664 * c-c++-common/pr51628-33.c: New test. * c-c++-common/pr51628-35.c: New test. * c-c++-common/pr88664-1.c: Likewise. * c-c++-common/pr88664-2.c: Likewise. * gcc.dg/pr51628-34.c: Likewise. --- gcc/c-family/c-common.h | 2 +- gcc/c-family/c-warn.c | 154 +++++++++++++----------- gcc/c/c-typeck.c | 6 +- gcc/cp/call.c | 2 +- gcc/cp/typeck.c | 2 +- gcc/testsuite/c-c++-common/pr51628-33.c | 19 +++ gcc/testsuite/c-c++-common/pr51628-35.c | 15 +++ gcc/testsuite/c-c++-common/pr88664-1.c | 20 +++ gcc/testsuite/c-c++-common/pr88664-2.c | 22 ++++ gcc/testsuite/gcc.dg/pr51628-34.c | 25 ++++ 10 files changed, 190 insertions(+), 77 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/pr51628-33.c create mode 100644 gcc/testsuite/c-c++-common/pr51628-35.c create mode 100644 gcc/testsuite/c-c++-common/pr88664-1.c create mode 100644 gcc/testsuite/c-c++-common/pr88664-2.c create mode 100644 gcc/testsuite/gcc.dg/pr51628-34.c diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index db16ae94b64..460954fefd8 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -1284,7 +1284,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 bool warn_for_restrict (unsigned, tree *, unsigned); -extern void warn_for_address_or_pointer_of_packed_member (bool, tree, tree); +extern void warn_for_address_or_pointer_of_packed_member (tree, tree); /* 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 79b2d8ad449..5df17ba2e1b 100644 --- a/gcc/c-family/c-warn.c +++ b/gcc/c-family/c-warn.c @@ -2713,12 +2713,14 @@ check_alignment_of_packed_member (tree type, tree field) return NULL_TREE; } -/* Return struct or union type if the right hand value, RHS, takes the - unaligned address of packed member of struct or union when assigning - to TYPE. Otherwise, return NULL_TREE. */ +/* Return struct or union type if the right hand value, RHS: + 1. Is a pointer value which isn't aligned to a pointer type TYPE. + 2. Is an address which takes the unaligned address of packed member + of struct or union when assigning to TYPE. + Otherwise, return NULL_TREE. */ static tree -check_address_of_packed_member (tree type, tree rhs) +check_address_or_pointer_of_packed_member (tree type, tree rhs) { if (INDIRECT_REF_P (rhs)) rhs = TREE_OPERAND (rhs, 0); @@ -2726,6 +2728,36 @@ check_address_of_packed_member (tree type, tree rhs) if (TREE_CODE (rhs) == ADDR_EXPR) rhs = TREE_OPERAND (rhs, 0); + if (!TYPE_P (type) || POINTER_TYPE_P (type)) + type = TREE_TYPE (type); + + if (TREE_CODE (rhs) == PARM_DECL || TREE_CODE (rhs) == VAR_DECL) + { + tree rhstype = TREE_TYPE (rhs); + if ((POINTER_TYPE_P (rhstype) + || TREE_CODE (rhstype) == ARRAY_TYPE) + && TYPE_PACKED (TREE_TYPE (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) + { + 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 " + "unaligned pointer value", + rhstype, rhs_align, type, type_align); + tree decl = TYPE_STUB_DECL (TREE_TYPE (rhstype)); + inform (DECL_SOURCE_LOCATION (decl), "defined here"); + decl = TYPE_STUB_DECL (type); + if (decl) + inform (DECL_SOURCE_LOCATION (decl), "defined here"); + } + } + return NULL_TREE; + } + tree context = NULL_TREE; /* Check alignment of the object. */ @@ -2744,18 +2776,53 @@ check_address_of_packed_member (tree type, tree rhs) return context; } -/* Check and warn if the right hand value, RHS, takes the unaligned - address of packed member of struct or union when assigning to TYPE. */ +/* Check and warn if the right hand value, RHS: + 1. Is a pointer value which isn't aligned to a pointer type TYPE. + 2. Is an address which takes the unaligned address of packed member + of struct or union when assigning to TYPE. + */ static void -check_and_warn_address_of_packed_member (tree type, tree rhs) +check_and_warn_address_or_pointer_of_packed_member (tree type, tree rhs) { + bool nop_p; + + if (TREE_CODE (rhs) == NOP_EXPR) + { + STRIP_NOPS (rhs); + nop_p = true; + } + else + nop_p = false; + if (TREE_CODE (rhs) != COND_EXPR) { while (TREE_CODE (rhs) == COMPOUND_EXPR) rhs = TREE_OPERAND (rhs, 1); - tree context = check_address_of_packed_member (type, rhs); + if (TREE_CODE (rhs) == NOP_EXPR) + { + STRIP_NOPS (rhs); + nop_p = true; + } + + if (nop_p) + { + switch (TREE_CODE (rhs)) + { + case ADDR_EXPR: + /* Address is taken. */ + case PARM_DECL: + case VAR_DECL: + /* Pointer conversion. */ + break; + default: + return; + } + } + + tree context + = check_address_or_pointer_of_packed_member (type, rhs); if (context) { location_t loc = EXPR_LOC_OR_LOC (rhs, input_location); @@ -2768,22 +2835,22 @@ check_and_warn_address_of_packed_member (tree type, tree rhs) } /* Check the THEN path. */ - check_and_warn_address_of_packed_member (type, TREE_OPERAND (rhs, 1)); + check_and_warn_address_or_pointer_of_packed_member (type, + TREE_OPERAND (rhs, 1)); /* Check the ELSE path. */ - check_and_warn_address_of_packed_member (type, TREE_OPERAND (rhs, 2)); + check_and_warn_address_or_pointer_of_packed_member (type, + TREE_OPERAND (rhs, 2)); } /* Warn if the right hand value, RHS: - 1. For CONVERT_P == true, is a pointer value which isn't aligned to a - pointer type TYPE. - 2. For CONVERT_P == false, is an address which takes the unaligned - address of packed member of struct or union when assigning to TYPE. + 1. Is a pointer value which isn't aligned to a pointer type TYPE. + 2. Is an address which takes the unaligned address of packed member + of struct or union when assigning to TYPE. */ void -warn_for_address_or_pointer_of_packed_member (bool convert_p, tree type, - tree rhs) +warn_for_address_or_pointer_of_packed_member (tree type, tree rhs) { if (!warn_address_of_packed_member) return; @@ -2795,58 +2862,5 @@ warn_for_address_or_pointer_of_packed_member (bool convert_p, tree type, while (TREE_CODE (rhs) == COMPOUND_EXPR) rhs = TREE_OPERAND (rhs, 1); - if (convert_p) - { - bool rhspointer_p; - tree rhstype; - - /* Check the original type of RHS. */ - switch (TREE_CODE (rhs)) - { - case PARM_DECL: - case VAR_DECL: - rhstype = TREE_TYPE (rhs); - rhspointer_p = POINTER_TYPE_P (rhstype); - break; - case NOP_EXPR: - rhs = TREE_OPERAND (rhs, 0); - if (TREE_CODE (rhs) == ADDR_EXPR) - rhs = TREE_OPERAND (rhs, 0); - rhstype = TREE_TYPE (rhs); - rhspointer_p = TREE_CODE (rhstype) == ARRAY_TYPE; - break; - default: - return; - } - - if (rhspointer_p && TYPE_PACKED (TREE_TYPE (rhstype))) - { - unsigned int type_align = TYPE_ALIGN_UNIT (TREE_TYPE (type)); - unsigned int rhs_align = TYPE_ALIGN_UNIT (TREE_TYPE (rhstype)); - if ((rhs_align % type_align) != 0) - { - 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 " - "unaligned pointer value", - rhstype, rhs_align, type, type_align); - tree decl = TYPE_STUB_DECL (TREE_TYPE (rhstype)); - inform (DECL_SOURCE_LOCATION (decl), "defined here"); - decl = TYPE_STUB_DECL (TREE_TYPE (type)); - if (decl) - inform (DECL_SOURCE_LOCATION (decl), "defined here"); - } - } - } - else - { - /* Get the type of the pointer pointing to. */ - type = TREE_TYPE (type); - - if (TREE_CODE (rhs) == NOP_EXPR) - rhs = TREE_OPERAND (rhs, 0); - - check_and_warn_address_of_packed_member (type, rhs); - } + check_and_warn_address_or_pointer_of_packed_member (type, rhs); } diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index 63d177f7a6f..05e171e4bda 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -6725,8 +6725,7 @@ convert_for_assignment (location_t location, location_t expr_loc, tree type, if (TYPE_MAIN_VARIANT (type) == TYPE_MAIN_VARIANT (rhstype)) { - warn_for_address_or_pointer_of_packed_member (false, type, - orig_rhs); + warn_for_address_or_pointer_of_packed_member (type, orig_rhs); return rhs; } @@ -7285,8 +7284,7 @@ convert_for_assignment (location_t location, location_t expr_loc, tree type, /* If RHS isn't an address, check pointer or array of packed struct or union. */ - warn_for_address_or_pointer_of_packed_member - (TREE_CODE (orig_rhs) != ADDR_EXPR, type, orig_rhs); + warn_for_address_or_pointer_of_packed_member (type, orig_rhs); return convert (type, rhs); } diff --git a/gcc/cp/call.c b/gcc/cp/call.c index 8bc8566e8d6..3b937d059d0 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -7631,7 +7631,7 @@ convert_for_arg_passing (tree type, tree val, tsubst_flags_t complain) } if (complain & tf_warning) - warn_for_address_or_pointer_of_packed_member (false, type, val); + warn_for_address_or_pointer_of_packed_member (type, val); return val; } diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c index 43d2899a3c4..9f5b2ec77e9 100644 --- a/gcc/cp/typeck.c +++ b/gcc/cp/typeck.c @@ -9074,7 +9074,7 @@ convert_for_assignment (tree type, tree rhs, } if (complain & tf_warning) - warn_for_address_or_pointer_of_packed_member (false, type, rhs); + warn_for_address_or_pointer_of_packed_member (type, rhs); return perform_implicit_conversion_flags (strip_top_quals (type), rhs, complain, flags); diff --git a/gcc/testsuite/c-c++-common/pr51628-33.c b/gcc/testsuite/c-c++-common/pr51628-33.c new file mode 100644 index 00000000000..0092f32202f --- /dev/null +++ b/gcc/testsuite/c-c++-common/pr51628-33.c @@ -0,0 +1,19 @@ +/* PR c/51628. */ +/* { dg-do compile } */ +/* { dg-options "-O" } */ + +struct pair_t +{ + char x; + int i[4]; +} __attribute__ ((packed, aligned (4))); + +extern struct pair_t p; +extern void bar (int *); + +void +foo (struct pair_t *p) +{ + bar (p ? p->i : (int *) 0); +/* { dg-warning "may result in an unaligned pointer value" "" { target *-*-* } .-1 } */ +} diff --git a/gcc/testsuite/c-c++-common/pr51628-35.c b/gcc/testsuite/c-c++-common/pr51628-35.c new file mode 100644 index 00000000000..597f1b7d15f --- /dev/null +++ b/gcc/testsuite/c-c++-common/pr51628-35.c @@ -0,0 +1,15 @@ +/* PR c/51628. */ +/* { dg-do compile } */ +/* { dg-options "-O" } */ + +struct B { int i; }; +struct C { struct B b; } __attribute__ ((packed)); + +extern struct C *p; + +long * +foo (void) +{ + return (long *)p; +/* { dg-warning "may result in an unaligned pointer value" "" { target *-*-* } .-1 } */ +} diff --git a/gcc/testsuite/c-c++-common/pr88664-1.c b/gcc/testsuite/c-c++-common/pr88664-1.c new file mode 100644 index 00000000000..5e680b9ae90 --- /dev/null +++ b/gcc/testsuite/c-c++-common/pr88664-1.c @@ -0,0 +1,20 @@ +/* PR c/88664. */ +/* { dg-do compile } */ +/* { dg-options "-O" } */ + +struct data +{ + void *ptr; +} __attribute__((packed)); + +int * +fun1 (struct data *p) +{ + return (int *) p->ptr; +} + +int * +fun2 (struct data *p, int *x) +{ + return x ? (*x = 1, (int *) p->ptr) : (int *) 0; +} diff --git a/gcc/testsuite/c-c++-common/pr88664-2.c b/gcc/testsuite/c-c++-common/pr88664-2.c new file mode 100644 index 00000000000..d2d880a66d7 --- /dev/null +++ b/gcc/testsuite/c-c++-common/pr88664-2.c @@ -0,0 +1,22 @@ +/* PR c/88664. */ +/* { dg-do compile } */ +/* { dg-options "-O" } */ + +struct data +{ + void *ptr; +} __attribute__((packed)); + +void ** +fun1 (struct data *p) +{ + return &p->ptr; +/* { dg-warning "may result in an unaligned pointer value" "" { target *-*-* } .-1 } */ +} + +int * +fun2 (struct data *p, int *x) +{ + return p ? (*x = 1, (int *) &p->ptr) : (int *) 0; +/* { dg-warning "may result in an unaligned pointer value" "" { target *-*-* } .-1 } */ +} diff --git a/gcc/testsuite/gcc.dg/pr51628-34.c b/gcc/testsuite/gcc.dg/pr51628-34.c new file mode 100644 index 00000000000..51d4b26a114 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr51628-34.c @@ -0,0 +1,25 @@ +/* PR c/51628. */ +/* { dg-do compile } */ +/* { dg-options "-O -Wno-incompatible-pointer-types" } */ + +struct __attribute__((packed)) S { char p; int a, b, c; }; + +short * +baz (int x, struct S *p) +{ + return (x + ? &p->a +/* { dg-warning "may result in an unaligned pointer value" "" { target *-*-* } .-1 } */ + : &p->b); +/* { dg-warning "may result in an unaligned pointer value" "" { target *-*-* } .-1 } */ +} + +short * +qux (int x, struct S *p) +{ + return (short *) (x + ? &p->a +/* { dg-warning "may result in an unaligned pointer value" "" { target *-*-* } .-1 } */ + : &p->b); +/* { dg-warning "may result in an unaligned pointer value" "" { target *-*-* } .-1 } */ +} -- 2.20.1