diff mbox series

improve constant string length folding (PR 89688)

Message ID 78dad3ef-fd7a-381b-6673-ae07f7fcf275@gmail.com
State New
Headers show
Series improve constant string length folding (PR 89688) | expand

Commit Message

Martin Sebor March 14, 2019, 2:17 a.m. UTC
PR 89688 points out a bogus warning about an unterminated
character array argument to strlen.  The root cause is
an oversight in the transformation of braced initializer lists
to STRING_CSTs where the solution implemented last summer only
considers one-dimensional arrays and skips more complex aggregates
such as multi-dimensional arrays or structs.

The folder (string_constant), on the other hand, assumes that
every a constant character array with an initializer is either
a properly nul-terminated string (i.e., STRING_CST), or
an unterminated array or a single character.  If the latter
then unless the character value is zero it indicates to its
caller that the constant is not a string.  As a result, we
end up with a warning.

To avoid the false positives the attached patch extends
the solution to those other kinds of aggregates.

Martin

Comments

Jeff Law March 19, 2019, 7:45 p.m. UTC | #1
On 3/13/19 8:17 PM, Martin Sebor wrote:
> PR 89688 points out a bogus warning about an unterminated
> character array argument to strlen.  The root cause is
> an oversight in the transformation of braced initializer lists
> to STRING_CSTs where the solution implemented last summer only
> considers one-dimensional arrays and skips more complex aggregates
> such as multi-dimensional arrays or structs.
> 
> The folder (string_constant), on the other hand, assumes that
> every a constant character array with an initializer is either
> a properly nul-terminated string (i.e., STRING_CST), or
> an unterminated array or a single character.  If the latter
> then unless the character value is zero it indicates to its
> caller that the constant is not a string.  As a result, we
> end up with a warning.
> 
> To avoid the false positives the attached patch extends
> the solution to those other kinds of aggregates.
> 
> Martin
> 
> gcc-89688.diff
> 
> PR tree-optimization/89688 - -Wstringop-overflow confused by const 2D array of char
> 
> gcc/c/ChangeLog:
> 
> 	PR tree-optimization/89688
> 	* c-decl.c (finish_decl): Call braced_lists_to_string for more
> 	kinds of initializers.
> 
> gcc/c-family/ChangeLog:
> 
> 	PR tree-optimization/89688
> 	* c-common.c (braced_list_to_string): Make static.
> 	(braced_lists_to_strings): Define new function.
> 	* c-common.h (braced_list_to_string): Remove.
> 	(braced_lists_to_strings): Declare.
> 
> gcc/cp/ChangeLog:
> 
> 	PR tree-optimization/89688
> 	* typeck2.c (store_init_value): Call braced_lists_to_string for more
> 	kinds of initializers.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/89688
> 	* gcc.dg/strlenopt-61.c: New test.
> 	* g++.dg/warn/Wstringop-overflow-2.C: New test.
OK.
jeff
>
diff mbox series

Patch

PR tree-optimization/89688 - -Wstringop-overflow confused by const 2D array of char

gcc/c/ChangeLog:

	PR tree-optimization/89688
	* c-decl.c (finish_decl): Call braced_lists_to_string for more
	kinds of initializers.

gcc/c-family/ChangeLog:

	PR tree-optimization/89688
	* c-common.c (braced_list_to_string): Make static.
	(braced_lists_to_strings): Define new function.
	* c-common.h (braced_list_to_string): Remove.
	(braced_lists_to_strings): Declare.

gcc/cp/ChangeLog:

	PR tree-optimization/89688
	* typeck2.c (store_init_value): Call braced_lists_to_string for more
	kinds of initializers.

gcc/testsuite/ChangeLog:

	PR tree-optimization/89688
	* gcc.dg/strlenopt-61.c: New test.
	* g++.dg/warn/Wstringop-overflow-2.C: New test.

Index: gcc/c/c-decl.c
===================================================================
--- gcc/c/c-decl.c	(revision 269657)
+++ gcc/c/c-decl.c	(working copy)
@@ -5165,11 +5165,10 @@  finish_decl (tree decl, location_t init_loc, tree
       relayout_decl (decl);
     }
 
-  if (TREE_CODE (type) == ARRAY_TYPE
-      && TYPE_STRING_FLAG (TREE_TYPE (type))
-      && DECL_INITIAL (decl)
-      && TREE_CODE (DECL_INITIAL (decl)) == CONSTRUCTOR)
-    DECL_INITIAL (decl) = braced_list_to_string (type, DECL_INITIAL (decl));
+  /* Look for braced array initializers for character arrays and
+     recursively convert them into STRING_CSTs.  */
+  if (tree init = DECL_INITIAL (decl))
+    DECL_INITIAL (decl) = braced_lists_to_strings (type, init);
 
   if (VAR_P (decl))
     {
Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 269657)
+++ gcc/c-family/c-common.c	(working copy)
@@ -8814,7 +8814,7 @@  maybe_add_include_fixit (rich_location *richloc, c
    TYPE into a STRING_CST for convenience and efficiency.  Return
    the converted string on success or the original ctor on failure.  */
 
-tree
+static tree
 braced_list_to_string (tree type, tree ctor)
 {
   if (!tree_fits_uhwi_p (TYPE_SIZE_UNIT (type)))
@@ -8895,4 +8895,52 @@  braced_list_to_string (tree type, tree ctor)
   return res;
 }
 
+/* Attempt to convert a CTOR containing braced array initializer lists
+   for array TYPE into one containing STRING_CSTs, for convenience and
+   efficiency.  Recurse for arrays of arrays and member initializers.
+   Return the converted CTOR or STRING_CST on success or the original
+   CTOR otherwise.  */
+
+tree
+braced_lists_to_strings (tree type, tree ctor)
+{
+  if (TREE_CODE (ctor) != CONSTRUCTOR)
+    return ctor;
+
+  tree_code code = TREE_CODE (type);
+
+  tree ttp;
+  if (code == ARRAY_TYPE)
+    ttp = TREE_TYPE (type);
+  else if (code == RECORD_TYPE)
+    {
+      ttp = TREE_TYPE (ctor);
+      if (TREE_CODE (ttp) == ARRAY_TYPE)
+	{
+	  type = ttp;
+	  ttp = TREE_TYPE (ttp);
+	}
+    }
+  else
+    return ctor;
+
+  if (TYPE_STRING_FLAG (ttp))
+    return braced_list_to_string (type, ctor);
+
+  code = TREE_CODE (ttp);
+  if (code == ARRAY_TYPE || code == RECORD_TYPE)
+    {
+      /* Handle array of arrays or struct member initializers.  */
+      tree val;
+      unsigned HOST_WIDE_INT idx;
+      FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (ctor), idx, val)
+	{
+	  val = braced_lists_to_strings (ttp, val);
+	  CONSTRUCTOR_ELT (ctor, idx)->value = val;
+	}
+    }
+
+  return ctor;
+}
+
 #include "gt-c-family-c-common.h"
Index: gcc/c-family/c-common.h
===================================================================
--- gcc/c-family/c-common.h	(revision 269657)
+++ gcc/c-family/c-common.h	(working copy)
@@ -1372,7 +1372,8 @@  extern void maybe_add_include_fixit (rich_location
 extern void maybe_suggest_missing_token_insertion (rich_location *richloc,
 						   enum cpp_ttype token_type,
 						   location_t prev_token_loc);
-extern tree braced_list_to_string (tree, tree);
+extern tree braced_lists_to_strings (tree, tree);
+
 extern bool has_attribute (location_t, tree, tree, tree (*)(tree));
 
 #if CHECKING_P
Index: gcc/cp/typeck2.c
===================================================================
--- gcc/cp/typeck2.c	(revision 269657)
+++ gcc/cp/typeck2.c	(working copy)
@@ -824,10 +824,9 @@  store_init_value (tree decl, tree init, vec<tree,
       value = digest_init_flags (type, init, flags, tf_warning_or_error);
     }
 
-  if (TREE_CODE (type) == ARRAY_TYPE
-      && TYPE_STRING_FLAG (TREE_TYPE (type))
-      && TREE_CODE (value) == CONSTRUCTOR)
-    value = braced_list_to_string (type, value);
+  /* Look for braced array initializers for character arrays and
+     recursively convert them into STRING_CSTs.  */
+  value = braced_lists_to_strings (type, value);
 
   current_ref_temp_count = 0;
   value = extend_ref_init_temps (decl, value, cleanups);
Index: gcc/testsuite/gcc.dg/strlenopt-61.c
===================================================================
--- gcc/testsuite/gcc.dg/strlenopt-61.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/strlenopt-61.c	(working copy)
@@ -0,0 +1,218 @@ 
+/* PR tree-optimization/89688 - -Wstringop-overflow confused by const
+   2D array of char
+   { dg-do compile }
+   { dg-options "-Wall -fdump-tree-gimple -fdump-tree-optimized" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+size_t strlen (const char*);
+#define CAT(x, y) x ## y
+#define CONCAT(x, y) CAT (x, y)
+#define FAILNAME(name) CONCAT (call_ ## name ##_on_line_, __LINE__)
+
+#define FAIL(name) do {                         \
+    extern __attribute__ ((noreturn)) void FAILNAME (name) (void);	\
+    FAILNAME (name)();                          \
+  } while (0)
+
+#define A(ref, len)					\
+  if (strlen (ref) != len) FAIL (failure); else (void)0
+
+const char a3_4[3][4] = { { 1 }, { 1, 2 }, { 1, 2, 3 } };
+
+void test_a4_4 (void)
+{
+  A (a3_4[0], 1);
+  A (a3_4[1], 2);
+  A (a3_4[2], 3);
+
+  A (&a3_4[0][0], 1);
+  A (&a3_4[0][1], 0);
+  A (&a3_4[0][2], 0);
+  A (&a3_4[0][3], 0);
+
+  A (&a3_4[1][0], 2);
+  A (&a3_4[1][1], 1);
+  A (&a3_4[1][2], 0);
+  A (&a3_4[1][3], 0);
+
+  A (&a3_4[2][0], 3);
+  A (&a3_4[2][1], 2);
+  A (&a3_4[2][2], 1);
+  A (&a3_4[2][3], 0);
+}
+
+
+const char a3_4_5[3][4][5] =
+  {
+   { { 1 }, { 1, 2 }, { 1, 2, 3 }, { 1, 2, 3, 4 } },
+   { { 1, 2 }, { 1, 2, 3 }, { 1, 2, 3, 4 }, { 1 } },
+   { { 1, 2, 3 }, { 1, 2, 3, 4 }, { 1 }, { 1, 2 } },
+  };
+
+void test_a3_4_5 (void)
+{
+  A (a3_4_5[0][0], 1);
+  A (a3_4_5[0][1], 2);
+  A (a3_4_5[0][2], 3);
+  A (a3_4_5[0][3], 4);
+
+  A (a3_4_5[1][0], 2);
+  A (a3_4_5[1][1], 3);
+  A (a3_4_5[1][2], 4);
+  A (a3_4_5[1][3], 1);
+
+  A (a3_4_5[2][0], 3);
+  A (a3_4_5[2][1], 4);
+  A (a3_4_5[2][2], 1);
+  A (a3_4_5[2][3], 2);
+}
+
+
+struct S
+{
+  char a3[3];
+  char a4_5[4][5];
+};
+
+const struct S sa4[4] =
+  {
+   { .a3 = { 0 },
+     .a4_5 =
+     {
+      { 1 }, { 1, 2 }, { 1, 2, 3 }, { 1, 2, 3, 4 }
+     }
+   },
+   { .a3 = { 1 },
+     .a4_5 =
+     {
+      { 1, 2 }, { 1, 2, 3 }, { 1, 2, 3, 4 }, { 1 }
+     }
+   },
+   { .a3 = { 1, 2 },
+     .a4_5 =
+     {
+      { 1, 2, 3 }, { 1, 2, 3, 4 }, { 1 }, { 1, 2 }
+     }
+   },
+   { .a3 = { 1 },
+     .a4_5 =
+     {
+      { 1, 2, 3, 4 }, "1", { 1, 2 }, "123"
+     }
+   }
+  };
+
+void test_sa4 (void)
+{
+  A (sa4[0].a3, 0);
+  A (sa4[0].a4_5[0], 1);
+  A (sa4[0].a4_5[1], 2);
+  A (sa4[0].a4_5[2], 3);
+  A (sa4[0].a4_5[3], 4);
+
+  A (sa4[1].a3, 1);
+  A (sa4[1].a4_5[0], 2);
+  A (sa4[1].a4_5[1], 3);
+  A (sa4[1].a4_5[2], 4);
+  A (sa4[1].a4_5[3], 1);
+
+  A (sa4[2].a3, 2);
+  A (sa4[2].a4_5[0], 3);
+  A (sa4[2].a4_5[1], 4);
+  A (sa4[2].a4_5[2], 1);
+  A (sa4[2].a4_5[3], 2);
+
+  A (sa4[3].a3, 1);
+  A (sa4[3].a4_5[0], 4);
+  A (sa4[3].a4_5[1], 1);
+  A (sa4[3].a4_5[2], 2);
+  A (sa4[3].a4_5[3], 3);
+}
+
+
+struct T
+{
+  struct S sa2[2];
+  char a4[4];
+};
+
+const struct T ta2[2] =
+  {
+   [0] =
+   {
+    .sa2 =
+    {
+     [0] =
+     { .a3 = { 0 },
+       .a4_5 =
+       {
+	{ 1 }, { 1, 2 }, { 1, 2, 3 }, { 1, 2, 3, 4 }
+       }
+     },
+     [1] =
+     { .a3 = { 1 },
+       .a4_5 =
+       {
+	{ 1, 2 }, { 1, 2, 3 }, { 1, 2, 3, 4 }, { 1 }
+       }
+     },
+    },
+    .a4 = "12"
+   },
+
+   [1] =
+   {
+    .sa2 =
+    {
+     [0] =
+     { .a3 = { 1, 2 },
+       .a4_5 =
+       {
+	{ 1, 2, 3 }, { 1, 2, 3, 4 }, { 1 }, { 1, 2 }
+       }
+     },
+     { .a3 = { 1 },
+       .a4_5 =
+       {
+	{ 1, 2, 3, 4 }, "1", { 1, 2 }, "123"
+       }
+     }
+    },
+    .a4 = "123"
+   }
+  };
+
+void test_ta2 (void)
+{
+  A (ta2[0].sa2[0].a3, 0);
+  A (ta2[0].sa2[0].a4_5[0], 1);
+  A (ta2[0].sa2[0].a4_5[1], 2);
+  A (ta2[0].sa2[0].a4_5[2], 3);
+  A (ta2[0].sa2[0].a4_5[3], 4);
+
+  A (ta2[0].sa2[1].a3, 1);
+  A (ta2[0].sa2[1].a4_5[0], 2);
+  A (ta2[0].sa2[1].a4_5[1], 3);
+  A (ta2[0].sa2[1].a4_5[2], 4);
+  A (ta2[0].sa2[1].a4_5[3], 1);
+
+  A (ta2[0].a4, 2);
+
+  A (ta2[1].sa2[0].a3, 2);
+  A (ta2[1].sa2[0].a4_5[0], 3);
+  A (ta2[1].sa2[0].a4_5[1], 4);
+  A (ta2[1].sa2[0].a4_5[2], 1);
+  A (ta2[1].sa2[0].a4_5[3], 2);
+
+  A (ta2[1].sa2[1].a3, 1);
+  A (ta2[1].sa2[1].a4_5[0], 4);
+  A (ta2[1].sa2[1].a4_5[1], 1);
+  A (ta2[1].sa2[1].a4_5[2], 2);
+  A (ta2[1].sa2[1].a4_5[3], 3);
+
+  A (ta2[1].a4, 3);
+}
+
+/* { dg-final { scan-tree-dump-not "failure" "optimized" } }
+   { dg-final { scan-tree-dump-not "strlen" "gimple" } } */
Index: gcc/testsuite/g++.dg/warn/Wstringop-overflow-2.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wstringop-overflow-2.C	(nonexistent)
+++ gcc/testsuite/g++.dg/warn/Wstringop-overflow-2.C	(working copy)
@@ -0,0 +1,29 @@ 
+/* PR tree-optimization/89688 - -Wstringop-overflow confused by const 2D
+   array of char
+   { dg-do compile }
+   { dg-options "-Wall -fdump-tree-gimple -fdump-tree-optimized" } */
+
+extern "C" __SIZE_TYPE__ strlen (const char*);
+
+const char a2[2] = { '1' };
+
+void a2_len ()
+{
+  if (strlen (a2) != 1)
+    __builtin_abort ();
+}
+
+const char a2_2[2][3] = { { '1' }, { '1', '2' } };
+
+void a2_2_len ()
+{
+  if  (strlen (a2_2[0]) != 1)   // { dg-bogus "-Wstringop-overflow" }
+    __builtin_abort ();
+
+  if  (strlen (a2_2[1]) != 2)   // { dg-bogus "-Wstringop-overflow" }
+    __builtin_abort ();
+}
+
+
+/* { dg-final { scan-tree-dump-not "abort" "optimized" } }
+   { dg-final { scan-tree-dump-not "strlen" "gimple" } } */