[PATCH] C-family: Only check the non-pointer data member

[PATCH] C-family: Only check the non-pointer data member

[H.J. Lu]

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.

gcc/testsuite/

	PR c++/88664
	* gcc.dg/pr88664-1.c: New test.
	* g++.dg/pr88664-2.C: Likewise.
---
 gcc/c-family/c-warn.c            |  8 +++++---
 gcc/testsuite/g++.dg/pr88664-2.C | 21 +++++++++++++++++++++
 gcc/testsuite/gcc.dg/pr88664-1.c | 19 +++++++++++++++++++
 3 files changed, 45 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr88664-2.C
 create mode 100644 gcc/testsuite/gcc.dg/pr88664-1.c

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;
+}
-- 
2.20.1

Re: [PATCH] C-family: Only check the non-pointer data member

[Jakub Jelinek]

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

Re: [PATCH] C-family: Only check the non-pointer data member

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 885 bytes --]

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.

-- 
H.J.

[-- Attachment #2: 0001-Don-t-warn-address-of-packed-member-if-address-isn-t.patch --]
[-- Type: text/x-patch, Size: 3919 bytes --]

From 2ffb941641a5eb9ce02136b3afbff35a433b3edf 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] Don't warn address of packed member if address isn't taken

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 (warn_for_address_or_pointer_of_packed_member): Move
	NOP_EXPR check to ...
	(check_and_warn_address_of_packed_member): Here.  Don't warn if
	address isn't taken.

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/c-family/c-warn.c                   | 11 ++++++++---
 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 ++++++++++++++++++++++
 4 files changed, 69 insertions(+), 3 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

diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c
index 79b2d8ad449..61e5b76bba4 100644
--- a/gcc/c-family/c-warn.c
+++ b/gcc/c-family/c-warn.c
@@ -2755,6 +2755,14 @@ check_and_warn_address_of_packed_member (tree type, tree rhs)
       while (TREE_CODE (rhs) == COMPOUND_EXPR)
 	rhs = TREE_OPERAND (rhs, 1);
 
+      if (TREE_CODE (rhs) == NOP_EXPR)
+	{
+	  STRIP_NOPS (rhs);
+	  /* For NOP_EXPR, address is taken only with ADDR_EXPR.   */
+	  if (TREE_CODE (rhs) != ADDR_EXPR)
+	    return;
+	}
+
       tree context = check_address_of_packed_member (type, rhs);
       if (context)
 	{
@@ -2844,9 +2852,6 @@ warn_for_address_or_pointer_of_packed_member (bool convert_p, tree type,
       /* 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);
     }
 }
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 } */
+}
-- 
2.20.1

Re: [PATCH] C-family: Only check the non-pointer data member

[Jakub Jelinek]

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

[PATCH] c-family: Update unaligned adress of packed member check

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1944 bytes --]

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.

-- 
H.J.

[-- Attachment #2: 0001-c-family-Update-unaligned-adress-of-packed-member-ch.patch --]
[-- Type: application/x-patch, Size: 10558 bytes --]

Re: [PATCH] c-family: Update unaligned adress of packed member check

[H.J. Lu]

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

-- 
H.J.

Re: [PATCH] c-family: Update unaligned adress of packed member check

[Jakub Jelinek]

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

Re: [PATCH] c-family: Update unaligned adress of packed member check

[H.J. Lu]

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.

-- 
H.J.

Re: [PATCH] c-family: Update unaligned adress of packed member check

[Jakub Jelinek]

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

Re: [PATCH] c-family: Update unaligned adress of packed member check

[Jakub Jelinek]

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

V2 [PATCH] c-family: Update unaligned adress of packed member check

[H.J. Lu]

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

Re: V2 [PATCH] c-family: Update unaligned adress of packed member check

[H.J. Lu]

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

Re: [PATCH] c-family: Update unaligned adress of packed member check

[H.J. Lu]

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


-- 
H.J.

Re: V2 [PATCH] c-family: Update unaligned adress of packed member check

[Jakub Jelinek]

On Wed, Jan 16, 2019 at 08:57:25PM -0800, H.J. Lu wrote:
> Check unaligned pointer conversion and strip NOPS.

> -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);

Bad formatting.  Plus, when would you pass around a non-type here?
And, isn't type always a POINTER_TYPE_P here anyway?  If not, whether
you use TREE_TYPE on it or not shouldn't depend on whether it is a pointer,
but on some other conditions, because a field can have pointer type too,
so if you come in through sometimes type being the address of the var and
sometimes the type of its value, the bug is in allowing that.
> +
> +  if (TREE_CODE (rhs) == PARM_DECL || TREE_CODE (rhs) == VAR_DECL)

VAR_P (rhs) instead of TREE_CODE (rhs) == VAR_DECL.  What about RESULT_DECL?

>  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)

This should be probably if (CONVERT_EXPR_P (rhs)) or maybe just do
  tree orig_rhs = rhs;
  STRIP_NOPS (rhs);
  nop_p = orig_rhs != rhs;
?

I must say I don't fully understand the nop_p stuff, why you handle
it differently if there were any nops vs. if there were none.
And, e.g. if you have NOP_EXPR around COND_EXPR, that outer nop_p isn't
taken into account.  So, again, what exactly do you want to achieve,
why do you care if there are any conversions in between or not.
Isn't all that matters what the innermost ADDR_EXPR is and what the
outermost type is?

>    if (TREE_CODE (rhs) != COND_EXPR)

I think it would be more readable to do:
  if (TREE_CODE (rhs) == COND_EXPR)
    {
      recurse;
      recurse;
      return;
    }
and handle the remaining code (longer) normally indented below that.

Another thing is, the NOP_EXPRs/CONVERT_EXPRs, COMPOUND_EXPRs and
COND_EXPRs can be arbitrarily nested, while you handle only a subset
of those cases.  You could e.g. move the
  while (TREE_CODE (rhs) == COMPOUND_EXPR)
   rhs = TREE_OPERAND (rhs, 1);

before the if (TREE_CODE (rhs) == COND_EXPR) check and stick another
STRIP_NOPS in between.

> @@ -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);

and then it would be pointless to do this here.

	Jakub

V2 [PATCH] c-family: Update unaligned adress of packed member check

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 3101 bytes --]

On Thu, Jan 17, 2019 at 7:36 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Wed, Jan 16, 2019 at 08:57:25PM -0800, H.J. Lu wrote:
> > Check unaligned pointer conversion and strip NOPS.
>
> > -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);
>
> Bad formatting.  Plus, when would you pass around a non-type here?
> And, isn't type always a POINTER_TYPE_P here anyway?  If not, whether
> you use TREE_TYPE on it or not shouldn't depend on whether it is a pointer,
> but on some other conditions, because a field can have pointer type too,
> so if you come in through sometimes type being the address of the var and
> sometimes the type of its value, the bug is in allowing that.

Fixed.

> > +
> > +  if (TREE_CODE (rhs) == PARM_DECL || TREE_CODE (rhs) == VAR_DECL)
>
> VAR_P (rhs) instead of TREE_CODE (rhs) == VAR_DECL.  What about RESULT_DECL?

Fixed.  I couldn't get RESULT_DECL.  I checked CALL_EXPR instead
with a testcase.

> >  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)
>
> This should be probably if (CONVERT_EXPR_P (rhs)) or maybe just do
>   tree orig_rhs = rhs;
>   STRIP_NOPS (rhs);
>   nop_p = orig_rhs != rhs;
> ?

Fixed.

> I must say I don't fully understand the nop_p stuff, why you handle
> it differently if there were any nops vs. if there were none.
> And, e.g. if you have NOP_EXPR around COND_EXPR, that outer nop_p isn't
> taken into account.  So, again, what exactly do you want to achieve,
> why do you care if there are any conversions in between or not.
> Isn't all that matters what the innermost ADDR_EXPR is and what the
> outermost type is?
>
> >    if (TREE_CODE (rhs) != COND_EXPR)
>
> I think it would be more readable to do:
>   if (TREE_CODE (rhs) == COND_EXPR)
>     {
>       recurse;
>       recurse;
>       return;
>     }
> and handle the remaining code (longer) normally indented below that.

Fixed.

> Another thing is, the NOP_EXPRs/CONVERT_EXPRs, COMPOUND_EXPRs and
> COND_EXPRs can be arbitrarily nested, while you handle only a subset
> of those cases.  You could e.g. move the
>   while (TREE_CODE (rhs) == COMPOUND_EXPR)
>    rhs = TREE_OPERAND (rhs, 1);
>
> before the if (TREE_CODE (rhs) == COND_EXPR) check and stick another
> STRIP_NOPS in between.

Fixed.

> > @@ -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);
>
> and then it would be pointless to do this here.

Fixed.

Here is the updated patch I am testing.

-- 
H.J.

[-- Attachment #2: 0001-c-family-Update-unaligned-adress-of-packed-member-ch.patch --]
[-- Type: text/x-patch, Size: 15373 bytes --]

From 1db08f118643bf4c28bd819014350dbca8590f8d 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

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                   | 175 +++++++++++++-----------
 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 |  23 ++++
 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, 208 insertions(+), 88 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 9fe90f32b16..69cb76cf49d 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1293,7 +1293,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..635dff70931 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,44 @@ check_address_of_packed_member (tree type, tree rhs)
   if (TREE_CODE (rhs) == ADDR_EXPR)
     rhs = TREE_OPERAND (rhs, 0);
 
+  if (POINTER_TYPE_P (type))
+    type = TREE_TYPE (type);
+
+  if (TREE_CODE (rhs) == PARM_DECL
+      || VAR_P (rhs)
+      || TREE_CODE (rhs) == CALL_EXPR)
+    {
+      if (TREE_CODE (rhs) == CALL_EXPR)
+	{
+	  rhs = CALL_EXPR_FN (rhs);	/* Pointer expression.  */
+	  rhs = TREE_TYPE (rhs);	/* Pointer type.  */
+	  rhs = TREE_TYPE (rhs);	/* Function type.  */
+	}
+      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 +2784,56 @@ 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)
 {
-  if (TREE_CODE (rhs) != COND_EXPR)
+  bool nop_p;
+
+  while (TREE_CODE (rhs) == COMPOUND_EXPR)
+    rhs = TREE_OPERAND (rhs, 1);
+
+  tree orig_rhs = rhs;
+  STRIP_NOPS (rhs);
+  nop_p = orig_rhs != rhs;
+
+  if (TREE_CODE (rhs) == COND_EXPR)
     {
-      while (TREE_CODE (rhs) == COMPOUND_EXPR)
-	rhs = TREE_OPERAND (rhs, 1);
+      /* Check the THEN path.  */
+      check_and_warn_address_or_pointer_of_packed_member
+	(type, TREE_OPERAND (rhs, 1));
 
-      tree context = check_address_of_packed_member (type, rhs);
+      /* Check the ELSE path.  */
+      check_and_warn_address_or_pointer_of_packed_member
+	(type, TREE_OPERAND (rhs, 2));
+    }
+  else
+    {
+      if (nop_p)
+	{
+	  switch (TREE_CODE (rhs))
+	    {
+	    case ADDR_EXPR:
+	      /* Address is taken.   */
+	    case PARM_DECL:
+	    case VAR_DECL:
+	      /* Pointer conversion.  */
+	      break;
+	    case CALL_EXPR:
+	      /* Function call. */
+	      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);
@@ -2764,26 +2842,17 @@ check_and_warn_address_of_packed_member (tree type, tree rhs)
 		      "in an unaligned pointer value",
 		      context);
 	}
-      return;
     }
-
-  /* Check the THEN path.  */
-  check_and_warn_address_of_packed_member (type, TREE_OPERAND (rhs, 1));
-
-  /* Check the ELSE path.  */
-  check_and_warn_address_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;
@@ -2792,61 +2861,5 @@ warn_for_address_or_pointer_of_packed_member (bool convert_p, tree type,
   if (!POINTER_TYPE_P (type))
     return;
 
-  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 4f04b610004..f17474a1dae 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 fc61991de35..81b3b51105e 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -9075,7 +9075,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..20877792fd8
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/pr51628-35.c
@@ -0,0 +1,23 @@
+/* PR c/51628.  */
+/* { dg-do compile } */
+/* { dg-options "-O" } */
+
+struct B { int i; };
+struct C { struct B b; } __attribute__ ((packed));
+
+extern struct C *p;
+extern struct C *bar (void);
+
+long *
+foo1 (void)
+{
+  return (long *) p;
+/* { dg-warning "may result in an unaligned pointer value" "" { target *-*-* } .-1 } */
+}
+
+long *
+foo2 (void)
+{
+  return (long *) bar ();
+/* { 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

V3 [PATCH] c-family: Update unaligned adress of packed member check

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 3394 bytes --]

On Thu, Jan 17, 2019 at 12:27 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Thu, Jan 17, 2019 at 7:36 AM Jakub Jelinek <jakub@redhat.com> wrote:
> >
> > On Wed, Jan 16, 2019 at 08:57:25PM -0800, H.J. Lu wrote:
> > > Check unaligned pointer conversion and strip NOPS.
> >
> > > -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);
> >
> > Bad formatting.  Plus, when would you pass around a non-type here?
> > And, isn't type always a POINTER_TYPE_P here anyway?  If not, whether
> > you use TREE_TYPE on it or not shouldn't depend on whether it is a pointer,
> > but on some other conditions, because a field can have pointer type too,
> > so if you come in through sometimes type being the address of the var and
> > sometimes the type of its value, the bug is in allowing that.
>
> Fixed.
>
> > > +
> > > +  if (TREE_CODE (rhs) == PARM_DECL || TREE_CODE (rhs) == VAR_DECL)
> >
> > VAR_P (rhs) instead of TREE_CODE (rhs) == VAR_DECL.  What about RESULT_DECL?
>
> Fixed.  I couldn't get RESULT_DECL.  I checked CALL_EXPR instead
> with a testcase.
>
> > >  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)
> >
> > This should be probably if (CONVERT_EXPR_P (rhs)) or maybe just do
> >   tree orig_rhs = rhs;
> >   STRIP_NOPS (rhs);
> >   nop_p = orig_rhs != rhs;
> > ?
>
> Fixed.
>
> > I must say I don't fully understand the nop_p stuff, why you handle
> > it differently if there were any nops vs. if there were none.
> > And, e.g. if you have NOP_EXPR around COND_EXPR, that outer nop_p isn't
> > taken into account.  So, again, what exactly do you want to achieve,
> > why do you care if there are any conversions in between or not.
> > Isn't all that matters what the innermost ADDR_EXPR is and what the
> > outermost type is?
> >
> > >    if (TREE_CODE (rhs) != COND_EXPR)
> >
> > I think it would be more readable to do:
> >   if (TREE_CODE (rhs) == COND_EXPR)
> >     {
> >       recurse;
> >       recurse;
> >       return;
> >     }
> > and handle the remaining code (longer) normally indented below that.
>
> Fixed.
>
> > Another thing is, the NOP_EXPRs/CONVERT_EXPRs, COMPOUND_EXPRs and
> > COND_EXPRs can be arbitrarily nested, while you handle only a subset
> > of those cases.  You could e.g. move the
> >   while (TREE_CODE (rhs) == COMPOUND_EXPR)
> >    rhs = TREE_OPERAND (rhs, 1);
> >
> > before the if (TREE_CODE (rhs) == COND_EXPR) check and stick another
> > STRIP_NOPS in between.
>
> Fixed.
>
> > > @@ -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);
> >
> > and then it would be pointless to do this here.
>
> Fixed.
>
> Here is the updated patch I am testing.
>

Here is the patch I tested without regressions.

-- 
H.J.

[-- Attachment #2: 0001-c-family-Update-unaligned-adress-of-packed-member-ch.patch --]
[-- Type: text/x-patch, Size: 15422 bytes --]

From bed69225767e064db7df58c7cb93b6ab14eee27d 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

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                   | 177 +++++++++++++-----------
 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 |  23 +++
 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, 210 insertions(+), 88 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 9fe90f32b16..69cb76cf49d 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1293,7 +1293,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..7821cc894a7 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,46 @@ check_address_of_packed_member (tree type, tree rhs)
   if (TREE_CODE (rhs) == ADDR_EXPR)
     rhs = TREE_OPERAND (rhs, 0);
 
+  if (POINTER_TYPE_P (type))
+    type = TREE_TYPE (type);
+
+  if (TREE_CODE (rhs) == PARM_DECL
+      || VAR_P (rhs)
+      || TREE_CODE (rhs) == CALL_EXPR)
+    {
+      if (TREE_CODE (rhs) == CALL_EXPR)
+	{
+	  rhs = CALL_EXPR_FN (rhs);	/* Pointer expression.  */
+	  if (rhs == NULL_TREE)
+	    return NULL_TREE;
+	  rhs = TREE_TYPE (rhs);	/* Pointer type.  */
+	  rhs = TREE_TYPE (rhs);	/* Function type.  */
+	}
+      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 +2786,56 @@ 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)
 {
-  if (TREE_CODE (rhs) != COND_EXPR)
+  bool nop_p;
+
+  while (TREE_CODE (rhs) == COMPOUND_EXPR)
+    rhs = TREE_OPERAND (rhs, 1);
+
+  tree orig_rhs = rhs;
+  STRIP_NOPS (rhs);
+  nop_p = orig_rhs != rhs;
+
+  if (TREE_CODE (rhs) == COND_EXPR)
     {
-      while (TREE_CODE (rhs) == COMPOUND_EXPR)
-	rhs = TREE_OPERAND (rhs, 1);
+      /* Check the THEN path.  */
+      check_and_warn_address_or_pointer_of_packed_member
+	(type, TREE_OPERAND (rhs, 1));
 
-      tree context = check_address_of_packed_member (type, rhs);
+      /* Check the ELSE path.  */
+      check_and_warn_address_or_pointer_of_packed_member
+	(type, TREE_OPERAND (rhs, 2));
+    }
+  else
+    {
+      if (nop_p)
+	{
+	  switch (TREE_CODE (rhs))
+	    {
+	    case ADDR_EXPR:
+	      /* Address is taken.   */
+	    case PARM_DECL:
+	    case VAR_DECL:
+	      /* Pointer conversion.  */
+	      break;
+	    case CALL_EXPR:
+	      /* Function call. */
+	      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);
@@ -2764,26 +2844,17 @@ check_and_warn_address_of_packed_member (tree type, tree rhs)
 		      "in an unaligned pointer value",
 		      context);
 	}
-      return;
     }
-
-  /* Check the THEN path.  */
-  check_and_warn_address_of_packed_member (type, TREE_OPERAND (rhs, 1));
-
-  /* Check the ELSE path.  */
-  check_and_warn_address_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;
@@ -2792,61 +2863,5 @@ warn_for_address_or_pointer_of_packed_member (bool convert_p, tree type,
   if (!POINTER_TYPE_P (type))
     return;
 
-  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 4f04b610004..f17474a1dae 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 fc61991de35..81b3b51105e 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -9075,7 +9075,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..20877792fd8
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/pr51628-35.c
@@ -0,0 +1,23 @@
+/* PR c/51628.  */
+/* { dg-do compile } */
+/* { dg-options "-O" } */
+
+struct B { int i; };
+struct C { struct B b; } __attribute__ ((packed));
+
+extern struct C *p;
+extern struct C *bar (void);
+
+long *
+foo1 (void)
+{
+  return (long *) p;
+/* { dg-warning "may result in an unaligned pointer value" "" { target *-*-* } .-1 } */
+}
+
+long *
+foo2 (void)
+{
+  return (long *) bar ();
+/* { 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

Re: V3 [PATCH] c-family: Update unaligned adress of packed member check

[Jakub Jelinek]

On Thu, Jan 17, 2019 at 04:00:47PM -0800, H.J. Lu wrote:
> 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.

Ok, thanks.

	Jakub

Re: V3 [PATCH] c-family: Update unaligned adress of packed member check

[Maxim Kuvyrkov]

Hi H.J.,

I've bisected compiler crash on building linux kernel for ARM down to this commit.  Search for
==
fs/ntfs/super.c:597:3: internal compiler error: Segmentation fault
==
in https://ci.linaro.org/view/tcwg_kernel-gnu/job/tcwg_kernel-build-gnu-master-arm-mainline-defconfig/285/artifact/artifacts/5-count_linux_objs/console.log/*view*/ .

This should be trivial to reproduce, but let me know if you need assistance.

Regards,

--
Maxim Kuvyrkov
www.linaro.org



> On Jan 18, 2019, at 2:10 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Thu, Jan 17, 2019 at 04:00:47PM -0800, H.J. Lu wrote:
>> 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.
> 
> Ok, thanks.
> 
> 	Jakub

Re: V3 [PATCH] c-family: Update unaligned adress of packed member check

[H.J. Lu]

On Mon, Jan 21, 2019 at 4:57 AM Maxim Kuvyrkov
<maxim.kuvyrkov@linaro.org> wrote:
>
> Hi H.J.,
>
> I've bisected compiler crash on building linux kernel for ARM down to this commit.  Search for
> ==
> fs/ntfs/super.c:597:3: internal compiler error: Segmentation fault
> ==
> in https://ci.linaro.org/view/tcwg_kernel-gnu/job/tcwg_kernel-build-gnu-master-arm-mainline-defconfig/285/artifact/artifacts/5-count_linux_objs/console.log/*view*/ .
>
> This should be trivial to reproduce, but let me know if you need assistance.

That is

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88928

and a patch is posted.

-- 
H.J.