From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 94839 invoked by alias); 17 Jan 2019 12:56:40 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 94225 invoked by uid 89); 17 Jan 2019 12:56:40 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-ot1-f68.google.com Received: from mail-ot1-f68.google.com (HELO mail-ot1-f68.google.com) (209.85.210.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 17 Jan 2019 12:56:34 +0000 Received: by mail-ot1-f68.google.com with SMTP id 32so10949540ota.12 for ; Thu, 17 Jan 2019 04:56:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Y4u5M71F6IPhphZV4lTWsn7hy311yYiHQnaBX0QwYIo=; b=eP8GjGKAUrQ2x8NMaJPpODHTsfe6PpqyNWm7pUsXyC+0JE+YezJPlXpcJMpedBNQhB Jks+idopgSLKUTt4M4djaT1JMTAMmplqNABnM519vXYVhVVYNUGktC/VuGU/72rQ08Ko +nBDfLk/N0hIBbwTcUz2/H9Tpg6vPnu+Sc+er8igd6BNIwD/ZHakrw1TCmPmTOea1MVp D59D8Rta6hCBikxODxuxuHSgLFD61SayfB3QbN8bUUQSaCjl/VI99K+XMSHSp7OzHaiN n4YOUELsYx6h4Pt51GVmP1/Xm0LV/XxzQNRDeF32qywxOA1lTQhE0sYLgA7OUVitHh6g RIFg== MIME-Version: 1.0 References: <20190113124838.29506-1-hjl.tools@gmail.com> <20190113132020.GY30353@tucnak> <20190114142214.GE30353@tucnak> <20190116113006.GQ30353@tucnak> <20190116122826.GS30353@tucnak> <20190117045725.GA1119@gmail.com> In-Reply-To: <20190117045725.GA1119@gmail.com> From: "H.J. Lu" Date: Thu, 17 Jan 2019 12:56:00 -0000 Message-ID: Subject: Re: V2 [PATCH] c-family: Update unaligned adress of packed member check To: Jakub Jelinek Cc: GCC Patches , Jason Merrill Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-01/txt/msg00999.txt.bz2 On Wed, Jan 16, 2019 at 8:57 PM H.J. Lu wrote: > > 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. > There is no regression. > 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 > -- H.J.