[PATCH] Refine -Waddress-of-packed-member once more

[PATCH] Refine -Waddress-of-packed-member once more

[Bernd Edlinger]

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

Hi,

unfortunately there are some more issues with -Waddress-of-packed-member, that show that my previous
patch was not yet complete.

That is a bogus warning, in C and C++ (see addition in test case 1), and a couple of missing warnings
in C++ (test case 2).

The latter warning regressions were accidentally introduced by my previous patch, sorry.

As a side note, the generic tree for an expression like struct.array is folded differently by
C and C++ FE.  While C folds that to &struct.array, C++ folds it to struct.array, without the
ADDR_EXPR.  That might help to explain why this needs to be done in this slightly confusing way.


Fixed the warning regression, and added some more test cases.


Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-fix-packed-warning-v3.diff --]
[-- Type: text/x-patch; name="patch-fix-packed-warning-v3.diff", Size: 4238 bytes --]

2019-01-22  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR c/88928
	* c-warn.c (check_address_or_pointer_of_packed_member): Handle the case
	when rhs is of array type correctly.  Fix handling of nested structures.

testsuite:
2019-01-22  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR c/88928
	* c-c++-common/Waddress-of-packed-member-1.c: Extended test case.
	* c-c++-common/Waddress-of-packed-member-2.c: New test case.

Index: gcc/c-family/c-warn.c
===================================================================
--- gcc/c-family/c-warn.c	(revision 268119)
+++ gcc/c-family/c-warn.c	(working copy)
@@ -2796,6 +2796,10 @@ check_address_or_pointer_of_packed_membe
 	  if (context)
 	    break;
 	}
+      if (TREE_CODE (TREE_TYPE (rhs)) == ARRAY_TYPE)
+	rvalue = false;
+      if (rvalue)
+	return NULL_TREE;
       rhs = TREE_OPERAND (rhs, 0);
     }
 
Index: gcc/testsuite/c-c++-common/Waddress-of-packed-member-1.c
===================================================================
--- gcc/testsuite/c-c++-common/Waddress-of-packed-member-1.c	(revision 268119)
+++ gcc/testsuite/c-c++-common/Waddress-of-packed-member-1.c	(working copy)
@@ -6,6 +6,7 @@ struct t {
   int b;
   int *c;
   int d[10];
+  int *e[1];
 } __attribute__((packed));
 
 struct t t0;
@@ -40,6 +41,7 @@ void foo (void)
   i1 = t10[0].c;               /* { dg-bogus "may result in an unaligned pointer value" } */
   u1 = (__UINTPTR_TYPE__) &t0; /* { dg-bogus "may result in an unaligned pointer value" } */
   u1 = (__UINTPTR_TYPE__) t1;  /* { dg-bogus "may result in an unaligned pointer value" } */
+  i1 = t10[0].e[0];            /* { dg-bogus "may result in an unaligned pointer value" } */
   t2 = (struct t**) t10;     /* { dg-warning "may result in an unaligned pointer value" } */
   t2 = (struct t**) t100;    /* { dg-warning "may result in an unaligned pointer value" } */
   t2 = (struct t**) t1;      /* { dg-warning "may result in an unaligned pointer value" } */
@@ -52,4 +54,6 @@ void foo (void)
   i1 = t0.d;                 /* { dg-warning "may result in an unaligned pointer value" } */
   i1 = t1->d;                /* { dg-warning "may result in an unaligned pointer value" } */
   i1 = t10[0].d;             /* { dg-warning "may result in an unaligned pointer value" } */
+  i1 = (int*) &t10[0].e[0];  /* { dg-warning "may result in an unaligned pointer value" } */
+  i1 = (int*) t10[0].e;      /* { dg-warning "may result in an unaligned pointer value" } */
 }
Index: gcc/testsuite/c-c++-common/Waddress-of-packed-member-2.c
===================================================================
--- gcc/testsuite/c-c++-common/Waddress-of-packed-member-2.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Waddress-of-packed-member-2.c	(working copy)
@@ -0,0 +1,38 @@
+/* { dg-do compile } */
+/* { dg-options "-Waddress-of-packed-member" } */
+
+struct r {
+  int a[10];
+  int b[10][10];
+};
+
+struct s {
+  char c;
+  struct r p;
+} __attribute__((packed));
+
+struct t {
+  char c;
+  struct r p __attribute__((packed));
+  struct r u;
+};
+
+struct s s0;
+struct t t0;
+int *i0;
+
+void foo (void)
+{
+  i0 = s0.p.a;               /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = t0.p.a;               /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = s0.p.b[0];            /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = t0.p.b[0];            /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = &s0.p.a[0];           /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = &t0.p.a[0];           /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = &s0.p.b[0][0];        /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = &t0.p.b[0][0];        /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = t0.u.a;                 /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = t0.u.b[0];              /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = &t0.u.a[0];             /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = &t0.u.b[0][0];          /* { dg-bogus "may result in an unaligned pointer value" } */
+}

Re: [PATCH] Refine -Waddress-of-packed-member once more

[H.J. Lu]

On Tue, Jan 22, 2019 at 6:10 AM Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
>
> Hi,
>
> unfortunately there are some more issues with -Waddress-of-packed-member, that show that my previous
> patch was not yet complete.
>
> That is a bogus warning, in C and C++ (see addition in test case 1), and a couple of missing warnings
> in C++ (test case 2).
>
> The latter warning regressions were accidentally introduced by my previous patch, sorry.
>
> As a side note, the generic tree for an expression like struct.array is folded differently by
> C and C++ FE.  While C folds that to &struct.array, C++ folds it to struct.array, without the
> ADDR_EXPR.  That might help to explain why this needs to be done in this slightly confusing way.
>
>
> Fixed the warning regression, and added some more test cases.

While working on it, I noticed that clang didn't warn for many cases.
Is that still true today?

Thanks.

-- 
H.J.

Re: [PATCH] Refine -Waddress-of-packed-member once more

[Jakub Jelinek]

On Tue, Jan 22, 2019 at 02:10:38PM +0000, Bernd Edlinger wrote:
> --- gcc/c-family/c-warn.c	(revision 268119)
> +++ gcc/c-family/c-warn.c	(working copy)
> @@ -2796,6 +2796,10 @@ check_address_or_pointer_of_packed_membe
>  	  if (context)
>  	    break;
>  	}
> +      if (TREE_CODE (TREE_TYPE (rhs)) == ARRAY_TYPE)
> +	rvalue = false;
> +      if (rvalue)
> +	return NULL_TREE;

That looks like ARRAY_REF specific stuff, isn't it?  We have various other
handled components, does e.g. REALPART_EXPR/IMAGPART_EXPR need that?
What about VIEW_CONVERT_EXPR?  Or is that something you want to do for
all of them?  In any case, there should be testcases with _Complex and
__real__/__imag__, address of that as well as value.

> @@ -52,4 +54,6 @@ void foo (void)
>    i1 = t0.d;                 /* { dg-warning "may result in an unaligned pointer value" } */
>    i1 = t1->d;                /* { dg-warning "may result in an unaligned pointer value" } */
>    i1 = t10[0].d;             /* { dg-warning "may result in an unaligned pointer value" } */
> +  i1 = (int*) &t10[0].e[0];  /* { dg-warning "may result in an unaligned pointer value" } */
> +  i1 = (int*) t10[0].e;      /* { dg-warning "may result in an unaligned pointer value" } */

Why not also i1 = &t10[0].e[0];
and i1 = t10[0].e; tests?

Unrelated to this patch, I'm worried e.g. about
  if (INDIRECT_REF_P (rhs))
    rhs = TREE_OPERAND (rhs, 0);
   
at the start of the check_address_or_pointer_of_packed_member
function, what if we have:
  i1 = *&t0.c;
Do we want to warn when in the end we don't care if the address is unaligned
or not, this is folded eventually into t0.c.

Plus as I said earlier to H.J., I think
  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;
should be really:
  bool nop_p;
  tree orig_rhs = rhs;
  STRIP_NOPS (rhs);
  nop_p = orig_rhs != rhs;

  while (TREE_CODE (rhs) == COMPOUND_EXPR)
    rhs = TREE_OPERAND (rhs, 1);

  orig_rhs = rhs;
  STRIP_NOPS (rhs);
  nop_p |= orig_rhs != rhs;

or similar, because if there are casts around COMPOUND_EXPR, it doesn't than
look through those COMPOUND_EXPRs, and if e.g. in the COMPOUND_EXPR rhs
there is then COND_EXPR or similar, it should handle that case.

	Jakub

Re: [PATCH] Refine -Waddress-of-packed-member once more

[Bernd Edlinger]

On 1/23/19 4:22 PM, Jakub Jelinek wrote:
> On Tue, Jan 22, 2019 at 02:10:38PM +0000, Bernd Edlinger wrote:
>> --- gcc/c-family/c-warn.c	(revision 268119)
>> +++ gcc/c-family/c-warn.c	(working copy)
>> @@ -2796,6 +2796,10 @@ check_address_or_pointer_of_packed_membe
>>  	  if (context)
>>  	    break;
>>  	}
>> +      if (TREE_CODE (TREE_TYPE (rhs)) == ARRAY_TYPE)
>> +	rvalue = false;
>> +      if (rvalue)
>> +	return NULL_TREE;
> 
> That looks like ARRAY_REF specific stuff, isn't it?  We have various other
> handled components, does e.g. REALPART_EXPR/IMAGPART_EXPR need that?
> What about VIEW_CONVERT_EXPR?  Or is that something you want to do for
> all of them?  In any case, there should be testcases with _Complex and
> __real__/__imag__, address of that as well as value.
> 

Okay, my guess it it will work with _Complex, but test cases would be
straight forward, and no reason not to have at least a few.

I have no idea what test case would be needed for VIEW_CONVERT_EXPR.

>> @@ -52,4 +54,6 @@ void foo (void)
>>    i1 = t0.d;                 /* { dg-warning "may result in an unaligned pointer value" } */
>>    i1 = t1->d;                /* { dg-warning "may result in an unaligned pointer value" } */
>>    i1 = t10[0].d;             /* { dg-warning "may result in an unaligned pointer value" } */
>> +  i1 = (int*) &t10[0].e[0];  /* { dg-warning "may result in an unaligned pointer value" } */
>> +  i1 = (int*) t10[0].e;      /* { dg-warning "may result in an unaligned pointer value" } */
> 
> Why not also i1 = &t10[0].e[0];
> and i1 = t10[0].e; tests?
> 

I will add that.

> Unrelated to this patch, I'm worried e.g. about
>   if (INDIRECT_REF_P (rhs))
>     rhs = TREE_OPERAND (rhs, 0);
>    
> at the start of the check_address_or_pointer_of_packed_member
> function, what if we have:
>   i1 = *&t0.c;
> Do we want to warn when in the end we don't care if the address is unaligned
> or not, this is folded eventually into t0.c.
> 

Porbably not. I tried your example, and find currently this is inconsistent
between C and C++, C folds *& away, before the warning, and C++ does not and
gets a warning.  I feel like I want to fix that too.

As a side note, your *&t0.c has a VIEW_CONVERT_EXPR in C++ but not in C:

Breakpoint 1, check_address_or_pointer_of_packed_member (type=0x7ffff6eef9d8, 
    rhs=0x7ffff702d760) at ../../gcc-trunk/gcc/c-family/c-warn.c:2727
2727	  bool rvalue = true;
(gdb) call debug(rhs)
*&VIEW_CONVERT_EXPR<struct t>(t0).c

... but it does not affect the warning.


> Plus as I said earlier to H.J., I think
>   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;
> should be really:
>   bool nop_p;
>   tree orig_rhs = rhs;
>   STRIP_NOPS (rhs);
>   nop_p = orig_rhs != rhs;
> 
>   while (TREE_CODE (rhs) == COMPOUND_EXPR)
>     rhs = TREE_OPERAND (rhs, 1);
> 
>   orig_rhs = rhs;
>   STRIP_NOPS (rhs);
>   nop_p |= orig_rhs != rhs;
> 
> or similar, because if there are casts around COMPOUND_EXPR, it doesn't than
> look through those COMPOUND_EXPRs, and if e.g. in the COMPOUND_EXPR rhs
> there is then COND_EXPR or similar, it should handle that case.
> 

Ah, yes.  That was not on my radar.

So this works in C and C++:
i1 = (0, (int*)&t0.c);

But this is again inconsistent between C and C++:
i1 = (int*) (0, &t0.c);

C gets a warning, and C++ no warning.

I agree and see that consistency betwenn C and C++ as important goal.
So, I will try to fix that as well.


Bernd.

Re: [PATCH] Refine -Waddress-of-packed-member once more

[Bernd Edlinger]

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

On 1/23/19 4:22 PM, Jakub Jelinek wrote:
> On Tue, Jan 22, 2019 at 02:10:38PM +0000, Bernd Edlinger wrote:
>> --- gcc/c-family/c-warn.c	(revision 268119)
>> +++ gcc/c-family/c-warn.c	(working copy)
>> @@ -2796,6 +2796,10 @@ check_address_or_pointer_of_packed_membe
>>  	  if (context)
>>  	    break;
>>  	}
>> +      if (TREE_CODE (TREE_TYPE (rhs)) == ARRAY_TYPE)
>> +	rvalue = false;
>> +      if (rvalue)
>> +	return NULL_TREE;
> 
> That looks like ARRAY_REF specific stuff, isn't it?  We have various other
> handled components, does e.g. REALPART_EXPR/IMAGPART_EXPR need that?
> What about VIEW_CONVERT_EXPR?  Or is that something you want to do for
> all of them?  In any case, there should be testcases with _Complex and
> __real__/__imag__, address of that as well as value.
> 
>> @@ -52,4 +54,6 @@ void foo (void)
>>    i1 = t0.d;                 /* { dg-warning "may result in an unaligned pointer value" } */
>>    i1 = t1->d;                /* { dg-warning "may result in an unaligned pointer value" } */
>>    i1 = t10[0].d;             /* { dg-warning "may result in an unaligned pointer value" } */
>> +  i1 = (int*) &t10[0].e[0];  /* { dg-warning "may result in an unaligned pointer value" } */
>> +  i1 = (int*) t10[0].e;      /* { dg-warning "may result in an unaligned pointer value" } */
> 
> Why not also i1 = &t10[0].e[0];
> and i1 = t10[0].e; tests?
> 
> Unrelated to this patch, I'm worried e.g. about
>   if (INDIRECT_REF_P (rhs))
>     rhs = TREE_OPERAND (rhs, 0);
>    
> at the start of the check_address_or_pointer_of_packed_member
> function, what if we have:
>   i1 = *&t0.c;
> Do we want to warn when in the end we don't care if the address is unaligned
> or not, this is folded eventually into t0.c.
> 
> Plus as I said earlier to H.J., I think
>   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;
> should be really:
>   bool nop_p;
>   tree orig_rhs = rhs;
>   STRIP_NOPS (rhs);
>   nop_p = orig_rhs != rhs;
> 
>   while (TREE_CODE (rhs) == COMPOUND_EXPR)
>     rhs = TREE_OPERAND (rhs, 1);
> 
>   orig_rhs = rhs;
>   STRIP_NOPS (rhs);
>   nop_p |= orig_rhs != rhs;
> 
> or similar, because if there are casts around COMPOUND_EXPR, it doesn't than
> look through those COMPOUND_EXPRs, and if e.g. in the COMPOUND_EXPR rhs
> there is then COND_EXPR or similar, it should handle that case.
> 

Okay, I updated the patch to address your comments.

Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-fix-packed-warning-v4.diff --]
[-- Type: text/x-patch; name="patch-fix-packed-warning-v4.diff", Size: 6462 bytes --]

Index: gcc/c-family/c-warn.c
===================================================================
--- gcc/c-family/c-warn.c	(revision 268195)
+++ gcc/c-family/c-warn.c	(working copy)
@@ -2725,14 +2725,18 @@ static tree
 check_address_or_pointer_of_packed_member (tree type, tree rhs)
 {
   bool rvalue = true;
+  bool indirect = false;
 
   if (INDIRECT_REF_P (rhs))
-    rhs = TREE_OPERAND (rhs, 0);
+    {
+      rhs = TREE_OPERAND (rhs, 0);
+      indirect = true;
+    }
 
   if (TREE_CODE (rhs) == ADDR_EXPR)
     {
       rhs = TREE_OPERAND (rhs, 0);
-      rvalue = false;
+      rvalue = indirect;
     }
 
   if (!POINTER_TYPE_P (type))
@@ -2796,6 +2800,10 @@ check_address_or_pointer_of_packed_member (tree ty
 	  if (context)
 	    break;
 	}
+      if (TREE_CODE (TREE_TYPE (rhs)) == ARRAY_TYPE)
+	rvalue = false;
+      if (rvalue)
+	return NULL_TREE;
       rhs = TREE_OPERAND (rhs, 0);
     }
 
@@ -2811,15 +2819,19 @@ check_address_or_pointer_of_packed_member (tree ty
 static void
 check_and_warn_address_or_pointer_of_packed_member (tree type, tree rhs)
 {
-  bool nop_p;
+  bool nop_p = false;
+  tree orig_rhs;
 
-  while (TREE_CODE (rhs) == COMPOUND_EXPR)
-    rhs = TREE_OPERAND (rhs, 1);
+  do
+    {
+      while (TREE_CODE (rhs) == COMPOUND_EXPR)
+	rhs = TREE_OPERAND (rhs, 1);
+      orig_rhs = rhs;
+      STRIP_NOPS (rhs);
+      nop_p |= orig_rhs != rhs;
+    }
+  while (orig_rhs != rhs);
 
-  tree orig_rhs = rhs;
-  STRIP_NOPS (rhs);
-  nop_p = orig_rhs != rhs;
-
   if (TREE_CODE (rhs) == COND_EXPR)
     {
       /* Check the THEN path.  */
Index: gcc/testsuite/c-c++-common/Waddress-of-packed-member-1.c
===================================================================
--- gcc/testsuite/c-c++-common/Waddress-of-packed-member-1.c	(revision 268195)
+++ gcc/testsuite/c-c++-common/Waddress-of-packed-member-1.c	(working copy)
@@ -6,6 +6,8 @@ struct t {
   int b;
   int *c;
   int d[10];
+  int *e[1];
+  _Complex float f;
 } __attribute__((packed));
 
 struct t t0;
@@ -17,6 +19,8 @@ struct t *bar();
 struct t (*baz())[10];
 struct t (*bazz())[10][10];
 int *i1;
+int **i2;
+float f0, *f1;
 __UINTPTR_TYPE__ u1;
 __UINTPTR_TYPE__ baa();
 
@@ -40,6 +44,13 @@ void foo (void)
   i1 = t10[0].c;               /* { dg-bogus "may result in an unaligned pointer value" } */
   u1 = (__UINTPTR_TYPE__) &t0; /* { dg-bogus "may result in an unaligned pointer value" } */
   u1 = (__UINTPTR_TYPE__) t1;  /* { dg-bogus "may result in an unaligned pointer value" } */
+  i1 = t10[0].e[0];            /* { dg-bogus "may result in an unaligned pointer value" } */
+  i1 = *&t0.c;                 /* { dg-bogus "may result in an unaligned pointer value" } */
+  f0 = __real__ t0.f;          /* { dg-bogus "may result in an unaligned pointer value" } */
+  f0 = __imag__ t0.f;          /* { dg-bogus "may result in an unaligned pointer value" } */
+  f0 = *&__real__ t0.f;        /* { dg-bogus "may result in an unaligned pointer value" } */
+  f0 = *&__imag__ t0.f;        /* { dg-bogus "may result in an unaligned pointer value" } */
+  i1 = (&t0.c, (int*) 0);      /* { dg-bogus "may result in an unaligned pointer value" } */
   t2 = (struct t**) t10;     /* { dg-warning "may result in an unaligned pointer value" } */
   t2 = (struct t**) t100;    /* { dg-warning "may result in an unaligned pointer value" } */
   t2 = (struct t**) t1;      /* { dg-warning "may result in an unaligned pointer value" } */
@@ -52,4 +63,14 @@ void foo (void)
   i1 = t0.d;                 /* { dg-warning "may result in an unaligned pointer value" } */
   i1 = t1->d;                /* { dg-warning "may result in an unaligned pointer value" } */
   i1 = t10[0].d;             /* { dg-warning "may result in an unaligned pointer value" } */
+  i1 = (int*) &t10[0].e[0];  /* { dg-warning "may result in an unaligned pointer value" } */
+  i1 = (int*) t10[0].e;      /* { dg-warning "may result in an unaligned pointer value" } */
+  i2 = &t10[0].e[0];         /* { dg-warning "may result in an unaligned pointer value" } */
+  i2 = t10[0].e;             /* { dg-warning "may result in an unaligned pointer value" } */
+  i2 = &*&t0.c;              /* { dg-warning "may result in an unaligned pointer value" } */
+  f1 = &__real__ t0.f;       /* { dg-warning "may result in an unaligned pointer value" } */
+  f1 = &__imag__ t0.f;       /* { dg-warning "may result in an unaligned pointer value" } */
+  i1 = (0, (int*) &t0.c);    /* { dg-warning "may result in an unaligned pointer value" } */
+  i1 = (int*) (0, &t0.c);    /* { dg-warning "may result in an unaligned pointer value" } */
+  i1 = (0, (int*)(0, &t0.c));/* { dg-warning "may result in an unaligned pointer value" } */
 }
Index: gcc/testsuite/c-c++-common/Waddress-of-packed-member-2.c
===================================================================
--- gcc/testsuite/c-c++-common/Waddress-of-packed-member-2.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Waddress-of-packed-member-2.c	(working copy)
@@ -0,0 +1,38 @@
+/* { dg-do compile } */
+/* { dg-options "-Waddress-of-packed-member" } */
+
+struct r {
+  int a[10];
+  int b[10][10];
+};
+
+struct s {
+  char c;
+  struct r p;
+} __attribute__((packed));
+
+struct t {
+  char c;
+  struct r p __attribute__((packed));
+  struct r u;
+};
+
+struct s s0;
+struct t t0;
+int *i0;
+
+void foo (void)
+{
+  i0 = s0.p.a;               /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = t0.p.a;               /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = s0.p.b[0];            /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = t0.p.b[0];            /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = &s0.p.a[0];           /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = &t0.p.a[0];           /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = &s0.p.b[0][0];        /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = &t0.p.b[0][0];        /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = t0.u.a;                 /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = t0.u.b[0];              /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = &t0.u.a[0];             /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = &t0.u.b[0][0];          /* { dg-bogus "may result in an unaligned pointer value" } */
+}

Re: [PATCH] Refine -Waddress-of-packed-member once more

[Jakub Jelinek]

On Thu, Jan 24, 2019 at 06:39:22AM +0000, Bernd Edlinger wrote:
> --- gcc/c-family/c-warn.c	(revision 268195)
> +++ gcc/c-family/c-warn.c	(working copy)
> @@ -2725,14 +2725,18 @@ static tree
>  check_address_or_pointer_of_packed_member (tree type, tree rhs)
>  {
>    bool rvalue = true;
> +  bool indirect = false;
>  
>    if (INDIRECT_REF_P (rhs))
> -    rhs = TREE_OPERAND (rhs, 0);
> +    {
> +      rhs = TREE_OPERAND (rhs, 0);
> +      indirect = true;
> +    }
>  
>    if (TREE_CODE (rhs) == ADDR_EXPR)
>      {
>        rhs = TREE_OPERAND (rhs, 0);
> -      rvalue = false;
> +      rvalue = indirect;
>      }

Given the unfolded *&, I wonder if the above actually shouldn't be
a loop with indirection integral counter instead of a boolean
and simply bump the indirection count on INDIRECT_REF_P and decrease on
ADDR_EXPR, and if indirection count is > 0 after the loop return NULL_TREE?
Say for *&*& or similar, and ***var.field always not warning etc.

Otherwise the patch looks good.

Though you might throw in a few test lines with the intermixed casts and
compound exprs multiple times (why you've added correctly the loop in
there).

	Jakub

Re: [PATCH] Refine -Waddress-of-packed-member once more

[Bernd Edlinger]

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

On 1/24/19 8:18 AM, Jakub Jelinek wrote:
> On Thu, Jan 24, 2019 at 06:39:22AM +0000, Bernd Edlinger wrote:
>> --- gcc/c-family/c-warn.c	(revision 268195)
>> +++ gcc/c-family/c-warn.c	(working copy)
>> @@ -2725,14 +2725,18 @@ static tree
>>  check_address_or_pointer_of_packed_member (tree type, tree rhs)
>>  {
>>    bool rvalue = true;
>> +  bool indirect = false;
>>  
>>    if (INDIRECT_REF_P (rhs))
>> -    rhs = TREE_OPERAND (rhs, 0);
>> +    {
>> +      rhs = TREE_OPERAND (rhs, 0);
>> +      indirect = true;
>> +    }
>>  
>>    if (TREE_CODE (rhs) == ADDR_EXPR)
>>      {
>>        rhs = TREE_OPERAND (rhs, 0);
>> -      rvalue = false;
>> +      rvalue = indirect;
>>      }
> 
> Given the unfolded *&, I wonder if the above actually shouldn't be
> a loop with indirection integral counter instead of a boolean
> and simply bump the indirection count on INDIRECT_REF_P and decrease on
> ADDR_EXPR, and if indirection count is > 0 after the loop return NULL_TREE?
> Say for *&*& or similar, and ***var.field always not warning etc.
> 

Okay, somehow the *&*&x is partially reduced, and arrives as *&x, so that works,
also &*&x is also partially reduced, and arrives as &x, works too.

Since in all test cases the INDIRECT_REFs are at the beginning, and only max. one
ADDR_EXPR at the end, the counting seems unnecessary, since neither the second
INDIRECT_REF nor the ADDR_EXPR are handled components, therefore there return
value is always NULL_TREE, and no warning, as expected.

Unfortunately **var.field should warn when field is a multi-dimensional array,
and again that did not work in C++, because it happens that after the
INDIRECT_REF there is a NOP_EXPR.  So I would like to add a SKIP_NOP, to make
the following additional test cases work:

struct r {
  int b[10][10];
  int ****i4;
}

  i0 = *s0.p.b;              /* { dg-warning "may result in an unaligned pointer value" } */
  i0 = *t0.p.b;              /* { dg-warning "may result in an unaligned pointer value" } */
  i0 = &**s0.p.b;            /* { dg-warning "may result in an unaligned pointer value" } */
  i0 = &**t0.p.b;            /* { dg-warning "may result in an unaligned pointer value" } */
  i0 = **&s0.p.b;            /* { dg-warning "may result in an unaligned pointer value" } */
  i0 = **&t0.p.b;            /* { dg-warning "may result in an unaligned pointer value" } */

  i0 = ***s0.p.i4;             /* { dg-bogus "may result in an unaligned pointer value" } */
  i0 = ***t0.p.i4;             /* { dg-bogus "may result in an unaligned pointer value" } */
  i0 = ****&s0.p.i4;           /* { dg-bogus "may result in an unaligned pointer value" } */
  i0 = ****&t0.p.i4;           /* { dg-bogus "may result in an unaligned pointer value" } */
  i0 = &****s0.p.i4;           /* { dg-bogus "may result in an unaligned pointer value" } */
  i0 = &****t0.p.i4;           /* { dg-bogus "may result in an unaligned pointer value" } */


> Otherwise the patch looks good.
> 
> Though you might throw in a few test lines with the intermixed casts and
> compound exprs multiple times (why you've added correctly the loop in
> there).
> 

So I've added one more, now have the following:

  i1 = (&t0.c, (int*) 0);      /* { dg-bogus "may result in an unaligned pointer value" } */

  i1 = (0, (int*) &t0.c);    /* { dg-warning "may result in an unaligned pointer value" } */
  i1 = (int*) (0, &t0.c);    /* { dg-warning "may result in an unaligned pointer value" } */
  i1 = (0, (int*)(0, &t0.c));/* { dg-warning "may result in an unaligned pointer value" } */
  i1 = (int*)(0, 1, (void*)(2, 3, (int*)(4, 5, &t0.c)));/* { dg-warning "may result in an unaligned pointer value" } */


Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-fix-packed-warning-v5.diff --]
[-- Type: text/x-patch; name="patch-fix-packed-warning-v5.diff", Size: 9190 bytes --]

2019-01-25  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-warn.c (check_address_or_pointer_of_packed_member): Handle the case
	when rhs is of array type correctly.  Fix handling of nested structures.
	Fix handling of indirect_ref together with nop_expr and/or addr_expr.
	(check_and_warn_address_or_pointer_of_packed_member): Fix handling of
	type casts within nested compound expressions.

testsuite:
2019-01-25  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/Waddress-of-packed-member-1.c: Extended test case.
	* c-c++-common/Waddress-of-packed-member-2.c: New test case.

Index: gcc/c-family/c-warn.c
===================================================================
--- gcc/c-family/c-warn.c	(revision 268195)
+++ gcc/c-family/c-warn.c	(working copy)
@@ -2725,14 +2725,19 @@ static tree
 check_address_or_pointer_of_packed_member (tree type, tree rhs)
 {
   bool rvalue = true;
+  bool indirect = false;
 
   if (INDIRECT_REF_P (rhs))
-    rhs = TREE_OPERAND (rhs, 0);
+    {
+      rhs = TREE_OPERAND (rhs, 0);
+      STRIP_NOPS (rhs);
+      indirect = true;
+    }
 
   if (TREE_CODE (rhs) == ADDR_EXPR)
     {
       rhs = TREE_OPERAND (rhs, 0);
-      rvalue = false;
+      rvalue = indirect;
     }
 
   if (!POINTER_TYPE_P (type))
@@ -2796,6 +2801,10 @@ check_address_or_pointer_of_packed_member (tree ty
 	  if (context)
 	    break;
 	}
+      if (TREE_CODE (TREE_TYPE (rhs)) == ARRAY_TYPE)
+	rvalue = false;
+      if (rvalue)
+	return NULL_TREE;
       rhs = TREE_OPERAND (rhs, 0);
     }
 
@@ -2811,15 +2820,19 @@ check_address_or_pointer_of_packed_member (tree ty
 static void
 check_and_warn_address_or_pointer_of_packed_member (tree type, tree rhs)
 {
-  bool nop_p;
+  bool nop_p = false;
+  tree orig_rhs;
 
-  while (TREE_CODE (rhs) == COMPOUND_EXPR)
-    rhs = TREE_OPERAND (rhs, 1);
+  do
+    {
+      while (TREE_CODE (rhs) == COMPOUND_EXPR)
+	rhs = TREE_OPERAND (rhs, 1);
+      orig_rhs = rhs;
+      STRIP_NOPS (rhs);
+      nop_p |= orig_rhs != rhs;
+    }
+  while (orig_rhs != rhs);
 
-  tree orig_rhs = rhs;
-  STRIP_NOPS (rhs);
-  nop_p = orig_rhs != rhs;
-
   if (TREE_CODE (rhs) == COND_EXPR)
     {
       /* Check the THEN path.  */
Index: gcc/testsuite/c-c++-common/Waddress-of-packed-member-1.c
===================================================================
--- gcc/testsuite/c-c++-common/Waddress-of-packed-member-1.c	(revision 268195)
+++ gcc/testsuite/c-c++-common/Waddress-of-packed-member-1.c	(working copy)
@@ -6,6 +6,8 @@ struct t {
   int b;
   int *c;
   int d[10];
+  int *e[1];
+  _Complex float f;
 } __attribute__((packed));
 
 struct t t0;
@@ -17,6 +19,8 @@ struct t *bar();
 struct t (*baz())[10];
 struct t (*bazz())[10][10];
 int *i1;
+int **i2;
+float f0, *f1;
 __UINTPTR_TYPE__ u1;
 __UINTPTR_TYPE__ baa();
 
@@ -40,6 +44,14 @@ void foo (void)
   i1 = t10[0].c;               /* { dg-bogus "may result in an unaligned pointer value" } */
   u1 = (__UINTPTR_TYPE__) &t0; /* { dg-bogus "may result in an unaligned pointer value" } */
   u1 = (__UINTPTR_TYPE__) t1;  /* { dg-bogus "may result in an unaligned pointer value" } */
+  i1 = t10[0].e[0];            /* { dg-bogus "may result in an unaligned pointer value" } */
+  i1 = *&t0.c;                 /* { dg-bogus "may result in an unaligned pointer value" } */
+  i1 = *&*&t0.c;               /* { dg-bogus "may result in an unaligned pointer value" } */
+  f0 = __real__ t0.f;          /* { dg-bogus "may result in an unaligned pointer value" } */
+  f0 = __imag__ t0.f;          /* { dg-bogus "may result in an unaligned pointer value" } */
+  f0 = *&__real__ t0.f;        /* { dg-bogus "may result in an unaligned pointer value" } */
+  f0 = *&__imag__ t0.f;        /* { dg-bogus "may result in an unaligned pointer value" } */
+  i1 = (&t0.c, (int*) 0);      /* { dg-bogus "may result in an unaligned pointer value" } */
   t2 = (struct t**) t10;     /* { dg-warning "may result in an unaligned pointer value" } */
   t2 = (struct t**) t100;    /* { dg-warning "may result in an unaligned pointer value" } */
   t2 = (struct t**) t1;      /* { dg-warning "may result in an unaligned pointer value" } */
@@ -52,4 +64,16 @@ void foo (void)
   i1 = t0.d;                 /* { dg-warning "may result in an unaligned pointer value" } */
   i1 = t1->d;                /* { dg-warning "may result in an unaligned pointer value" } */
   i1 = t10[0].d;             /* { dg-warning "may result in an unaligned pointer value" } */
+  i1 = (int*) &t10[0].e[0];  /* { dg-warning "may result in an unaligned pointer value" } */
+  i1 = (int*) t10[0].e;      /* { dg-warning "may result in an unaligned pointer value" } */
+  i2 = &t10[0].e[0];         /* { dg-warning "may result in an unaligned pointer value" } */
+  i2 = t10[0].e;             /* { dg-warning "may result in an unaligned pointer value" } */
+  i2 = &*&t0.c;              /* { dg-warning "may result in an unaligned pointer value" } */
+  i2 = &*&*&t0.c;            /* { dg-warning "may result in an unaligned pointer value" } */
+  f1 = &__real__ t0.f;       /* { dg-warning "may result in an unaligned pointer value" } */
+  f1 = &__imag__ t0.f;       /* { dg-warning "may result in an unaligned pointer value" } */
+  i1 = (0, (int*) &t0.c);    /* { dg-warning "may result in an unaligned pointer value" } */
+  i1 = (int*) (0, &t0.c);    /* { dg-warning "may result in an unaligned pointer value" } */
+  i1 = (0, (int*)(0, &t0.c));/* { dg-warning "may result in an unaligned pointer value" } */
+  i1 = (int*)(0, 1, (void*)(2, 3, (int*)(4, 5, &t0.c)));/* { dg-warning "may result in an unaligned pointer value" } */
 }
Index: gcc/testsuite/c-c++-common/Waddress-of-packed-member-2.c
===================================================================
--- gcc/testsuite/c-c++-common/Waddress-of-packed-member-2.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Waddress-of-packed-member-2.c	(working copy)
@@ -0,0 +1,58 @@
+/* { dg-do compile } */
+/* { dg-options "-Waddress-of-packed-member" } */
+
+struct r {
+  int a[10];
+  int b[10][10];
+  int ****i4;
+};
+
+struct s {
+  char c;
+  struct r p;
+} __attribute__((packed));
+
+struct t {
+  char c;
+  struct r p __attribute__((packed));
+  struct r u;
+};
+
+struct s s0;
+struct t t0;
+int *i0;
+
+void foo (void)
+{
+  i0 = s0.p.a;               /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = t0.p.a;               /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = s0.p.b[0];            /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = t0.p.b[0];            /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = &s0.p.a[0];           /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = &t0.p.a[0];           /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = &s0.p.b[0][0];        /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = &t0.p.b[0][0];        /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = *s0.p.b;              /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = *t0.p.b;              /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = &**s0.p.b;            /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = &**t0.p.b;            /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = **&s0.p.b;            /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = **&t0.p.b;            /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = &*s0.p.a;             /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = &*t0.p.a;             /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = *&s0.p.a;             /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = *&t0.p.a;             /* { dg-warning "may result in an unaligned pointer value" } */
+  i0 = t0.u.a;                 /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = t0.u.b[0];              /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = &t0.u.a[0];             /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = &t0.u.b[0][0];          /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = *t0.u.b;                /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = &*t0.u.a;               /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = &**t0.u.b;              /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = ***s0.p.i4;             /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = ***t0.p.i4;             /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = ****&s0.p.i4;           /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = ****&t0.p.i4;           /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = &****s0.p.i4;           /* { dg-bogus "may result in an unaligned pointer value" } */
+  i0 = &****t0.p.i4;           /* { dg-bogus "may result in an unaligned pointer value" } */
+}

Re: [PATCH] Refine -Waddress-of-packed-member once more

[Jakub Jelinek]

On Fri, Jan 25, 2019 at 06:36:53AM +0000, Bernd Edlinger wrote:
> 2019-01-25  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* c-warn.c (check_address_or_pointer_of_packed_member): Handle the case
> 	when rhs is of array type correctly.  Fix handling of nested structures.
> 	Fix handling of indirect_ref together with nop_expr and/or addr_expr.
> 	(check_and_warn_address_or_pointer_of_packed_member): Fix handling of
> 	type casts within nested compound expressions.
> 
> testsuite:
> 2019-01-25  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* c-c++-common/Waddress-of-packed-member-1.c: Extended test case.
> 	* c-c++-common/Waddress-of-packed-member-2.c: New test case.

Ok, thanks.

	Jakub