On Mon, Jan 14, 2019 at 6:22 AM Jakub Jelinek <ja...@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.
--
H.J.
From 7b7281896a731371ecd8c293582c0c4dcff0c92f Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sat, 12 Jan 2019 21:03:50 -0800
Subject: [PATCH] c-family: Update unaligned adress of packed member check
Properly check unaligned pointer conversion as well as properly strip
NOPS and don't warn address of packed member if address isn't taken
with NOPS.
gcc/c-family/
PR c/51628
PR c/88664
* c-warn.c (check_address_of_packed_member): Renamed to ...
(check_address_of_packed_member): This. Add a boolean argument
to also warn pointer conversion.
(check_and_warn_address_of_packed_member): Renamed to ...
(check_and_warn_address_or_pointer_of_packed_member): This.
Add a boolean argument to also warn pointer conversion.
(warn_for_address_or_pointer_of_packed_member): Don't check
pointer conversion here.
gcc/testsuite/
PR c/51628
PR c/88664
* c-c++-common/pr51628-33.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-warn.c | 140 ++++++++++++++----------
gcc/testsuite/c-c++-common/pr51628-33.c | 19 ++++
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 +++++
5 files changed, 166 insertions(+), 60 deletions(-)
create mode 100644 gcc/testsuite/c-c++-common/pr51628-33.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-warn.c b/gcc/c-family/c-warn.c
index 79b2d8ad449..d3660ffd2c1 100644
--- a/gcc/c-family/c-warn.c
+++ b/gcc/c-family/c-warn.c
@@ -2713,12 +2713,16 @@ 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. 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.
+ Otherwise, return NULL_TREE. */
static tree
-check_address_of_packed_member (tree type, tree rhs)
+check_address_or_pointer_of_packed_member (bool convert_p, tree type,
+ tree rhs)
{
if (INDIRECT_REF_P (rhs))
rhs = TREE_OPERAND (rhs, 0);
@@ -2726,6 +2730,35 @@ check_address_of_packed_member (tree type, tree rhs)
if (TREE_CODE (rhs) == ADDR_EXPR)
rhs = TREE_OPERAND (rhs, 0);
+ if (convert_p
+ && (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 (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");
+ }
+ }
+ return NULL_TREE;
+ }
+
tree context = NULL_TREE;
/* Check alignment of the object. */
@@ -2744,18 +2777,49 @@ 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. 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.
+ */
static void
-check_and_warn_address_of_packed_member (tree type, tree rhs)
+check_and_warn_address_or_pointer_of_packed_member (bool convert_p,
+ 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)
+ {
+ /* For NOP_EXPR, address is taken only with ADDR_EXPR. */
+ if (TREE_CODE (rhs) != ADDR_EXPR)
+ return;
+ }
+
+ tree context
+ = check_address_or_pointer_of_packed_member (convert_p, type,
+ rhs);
if (context)
{
location_t loc = EXPR_LOC_OR_LOC (rhs, input_location);
@@ -2768,10 +2832,12 @@ 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 (convert_p, 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 (convert_p, type,
+ TREE_OPERAND (rhs, 2));
}
/* Warn if the right hand value, RHS:
@@ -2795,58 +2861,12 @@ 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
+ if (!convert_p)
{
/* 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 (convert_p, type,
+ rhs);
}
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/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
