On Wed, Jan 16, 2019 at 8:57 PM H.J. Lu <hjl.to...@gmail.com> 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.
