Re: [PATCH] handle MEM_REF with void* arguments (PR c++/95768)

[Martin Sebor <msebor@gmail.com>]

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

On 6/22/20 12:55 PM, Jason Merrill wrote:
> On 6/22/20 1:25 PM, Martin Sebor wrote:
>> The attached fix parallels the one for the equivalent C bug 95580
>> where the pretty printers don't correctly handle MEM_REF arguments
>> with type void* or other pointers to an incomplete type.
>>
>> The incorrect handling was exposed by the recent change to
>> -Wuninitialized which includes such expressions in diagnostics.
> 
>> +        if (tree size = TYPE_SIZE_UNIT (TREE_TYPE (argtype)))
>> +          if (!integer_onep (size))
>> +            {
>> +              pp_cxx_left_paren (pp);
>> +              dump_type (pp, ptr_type_node, flags);
>> +              pp_cxx_right_paren (pp);
>> +            }
> 
> Don't we want to print the cast if the pointer target type is incomplete?

I suppose, yes, although after some more testing I think what should
be output is the type of the access.  The target pointer type isn't
meaningful (at least not in this case).

Here's what the warning looks like in C for the test case in
gcc.dg/pr95580.c:

   warning: ‘*((void *)(p)+1)’ may be used uninitialized

and like this in C++:

   warning: ‘*(p +1)’ may be used uninitialized

The +1 is a byte offset, which is correct given that incrementing
a void* in GCC is the same as adding 1 to the byte address, but
dereferencing a void* doesn't correspond to what's going on in
the source.

Even for a complete type (with size greater than 1), printing
the type of the argument plus a byte offset is wrong.  It ends
up with this for the C++ test case from 95768:

   warning: ‘*((int*)<unknown> +4)’ is used uninitialized

when the access is actually ‘*((int*)<unknown> +1)’

So it seems to me for MEM_REF, to make the output meaningful,
it's the type of the access (i.e., the MEM_REF type) that should
be printed here, and the offset should either be in elements of
the accessed type, i.e.,

   warning: ‘*((int*)<unknown> +1)’ is used uninitialized

or, if the access is misaligned, the argument should first be
cast to char*, the offset added, and the result then cast to
the access type, like this:

   warning: ‘*(T*)((char*)<unknown> +1)’ is used uninitialized

The attached revised and less than fully tested patch implements
this for C++ only for now.  If we agree on this approach I'll see
about making the corresponding change in C.

Martin

[-- Attachment #2: gcc-95768.diff --]
[-- Type: text/x-patch, Size: 5229 bytes --]

PR c++/95768 - pretty-printer ICE on -Wuninitialized with allocated storage

gcc/cp/ChangeLog:

	PR c++/95768
	* error.c (dump_expr): Handle sizeless operand types such as void*.

gcc/testsuite/ChangeLog:

	PR c++/95768
	* g++.dg/pr95768.C: New test.

diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index 0d6375e5e14..ea1f3232e3d 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -2374,32 +2374,63 @@ dump_expr (cxx_pretty_printer *pp, tree t, int flags)
       break;
 
     case MEM_REF:
-      if (TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR
-	  && integer_zerop (TREE_OPERAND (t, 1)))
-	dump_expr (pp, TREE_OPERAND (TREE_OPERAND (t, 0), 0), flags);
-      else
-	{
-	  pp_cxx_star (pp);
-	  if (!integer_zerop (TREE_OPERAND (t, 1)))
-	    {
-	      pp_cxx_left_paren (pp);
-	      if (!integer_onep (TYPE_SIZE_UNIT
-				 (TREE_TYPE (TREE_TYPE (TREE_OPERAND (t, 0))))))
-		{
-		  pp_cxx_left_paren (pp);
-		  dump_type (pp, ptr_type_node, flags);
-		  pp_cxx_right_paren (pp);
-		}
-	    }
-	  dump_expr (pp, TREE_OPERAND (t, 0), flags);
-	  if (!integer_zerop (TREE_OPERAND (t, 1)))
-	    {
-	      pp_cxx_ws_string (pp, "+");
-	      dump_expr (pp, fold_convert (ssizetype, TREE_OPERAND (t, 1)),
-                         flags);
-	      pp_cxx_right_paren (pp);
-	    }
-	}
+      {
+	tree type = TREE_TYPE (t);
+	tree arg = TREE_OPERAND (t, 0);
+	tree offset = TREE_OPERAND (t, 1);
+	bool zero_offset = integer_zerop (offset);
+
+	if (TREE_CODE (arg) == ADDR_EXPR && zero_offset)
+	  dump_expr (pp, TREE_OPERAND (arg, 0), flags);
+	else
+	  {
+	    pp_cxx_star (pp);
+	    if (!zero_offset)
+	      {
+		pp_cxx_left_paren (pp);
+		pp_cxx_left_paren (pp);
+		dump_type (pp, type, flags);
+		pp_cxx_star (pp);
+		pp_cxx_right_paren (pp);
+
+		bool byte_offset = true;
+
+		if (tree size = TYPE_SIZE_UNIT (type))
+		  {
+		    /* For naturally aligned accesses print the nonzero
+		       offset in units of the access type.  For unaligned
+		       accesses print a byte offset instead.  */
+		    offset_int wsiz = wi::to_offset (size);
+		    offset_int woff = wi::to_offset (offset);
+		    offset_int szlg2 = wi::floor_log2 (wsiz);
+		    offset_int eltoff = woff >> szlg2;
+		    if (eltoff << szlg2 == woff)
+		      {
+			offset = wide_int_to_tree (ssizetype, eltoff);
+			byte_offset = false;
+		      }
+		  }
+
+		if (byte_offset)
+		  {
+		    /* When printing the byte offset include a cast to
+		       a character type first, before printing the cast
+		       to the access type.  */
+		    pp_cxx_left_paren (pp);
+		    dump_type (pp, char_type_node, flags);
+		    pp_cxx_star (pp);
+		    pp_cxx_right_paren (pp);
+		  }
+	      }
+	    dump_expr (pp, arg, flags);
+	    if (!zero_offset)
+	      {
+		pp_cxx_ws_string (pp, "+");
+		dump_expr (pp, fold_convert (ssizetype, offset), flags);
+		pp_cxx_right_paren (pp);
+	      }
+	  }
+      }
       break;
 
     case NEGATE_EXPR:
diff --git a/gcc/testsuite/g++.dg/pr95768.C b/gcc/testsuite/g++.dg/pr95768.C
new file mode 100644
index 00000000000..23e6988b410
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr95768.C
@@ -0,0 +1,32 @@
+/* PR c++/95768 - pretty-printer ICE on -Wuninitialized with allocated storage
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+extern "C" void *malloc (__SIZE_TYPE__);
+
+struct f
+{
+  int i;
+  static int e (int);
+  void operator= (int) { e (i); }
+};
+
+struct m {
+  int i;
+  f length;
+};
+
+struct n {
+  m *o() { return (m *)this; }
+};
+
+struct p {
+  n *header;
+  p () {
+    header = (n *)malloc (0);
+    m b = *header->o();       // { dg-warning "'\\*\\(\\(int\\*\\)<\[a-z\]+> \\+1\\)' is used uninitialized" }
+    b.length = 0;
+  }
+};
+
+void detach2() { p(); }
diff --git a/gcc/testsuite/g++.dg/warn/Wuninitialized-11.C b/gcc/testsuite/g++.dg/warn/Wuninitialized-11.C
new file mode 100644
index 00000000000..6f8a4586adf
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wuninitialized-11.C
@@ -0,0 +1,40 @@
+/* Verify that -Wuninitialized warnings about accesses to objects via
+   pointers and offsets mention valid expressions.
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+typedef __INT16_TYPE__ int16_t;
+typedef __INT32_TYPE__ int32_t;
+
+void sink (int);
+
+/* Verify properly aligned accesses at offsets that are multiples of
+   the access size.  */
+
+void test_aligned (void)
+{
+  char *p1 = (char*)__builtin_malloc (32);
+  p1 += sizeof (int32_t);
+
+  int16_t *p2 = (int16_t*)p1;
+  sink (p2[1]);               // { dg-warning "'\\\*\\\(\\\(int16_t\\\*\\\)p1 \\\+ *3\\\)' is used uninitialized" }
+
+  int32_t *p4 = (int32_t*)p1;
+  sink (p4[1]);               // { dg-warning "'\\\*\\\(\\\(int32_t\\\*\\\)p1 \\\+ *2\\\)' is used uninitialized" }
+}
+
+
+/* Verify misaligned accesses at offsets that aren't multiples of
+   the access size.  */
+
+void test_misaligned (void)
+{
+  char *p1 = (char*)__builtin_malloc (32);
+  p1 += 1;
+
+  int16_t *p2 = (int16_t*)p1;
+  sink (p2[1]);               // { dg-warning "'\\\*\\\(\\\(int16_t\\\*\\\)\\\(char\\\*\\\)p1 \\\+ *3\\\)' is used uninitialized" }
+
+  int32_t *p4 = (int32_t*)p1;
+  sink (p4[1]);               // { dg-warning "'\\\*\\\(\\\(int32_t\\\*\\)\\\(char\\\*\\\)p1 \\\+ *5\\\)' is used uninitialized" }
+}