[PATCH] avoid bogus -Wstringop-truncation when inlining (PR 84480)

[PATCH] avoid bogus -Wstringop-truncation when inlining (PR 84480)

[Martin Sebor]

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

The attached patch eliminates -Wstringop-truncation false
positives reported in bug 84480 - bogus -Wstringop-truncation
despite assignment with an inlined string literal.  It does
that by delegating early strncpy checks during folding to
the same machinery in tree-ssa-strlen that looks for a NUL
assignment to the destination the next statement.

The patch also adds inlining context to the warnings via
the %G directive.

Tested on x86_64-linux with no regressions.

Martin

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

PR tree-optimization/84480 - bogus -Wstringop-truncation despite assignment with an inlined string literal

gcc/ChangeLog:

	PR tree-optimization/84480
	* gimple-fold.c (gimple_fold_builtin_strcpy): Move warnings
	to maybe_diag_stxncpy_trunc.  Call it.
	* tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Integrate warnings
	from gimple_fold_builtin_strcpy.  Print inlining stack.
	(handle_builtin_stxncpy): Print inlining stack.
	* tree-ssa-strlen.h (maybe_diag_stxncpy_trunc): Declare.

gcc/testsuite/ChangeLog:

	PR tree-optimization/84480
	* c-c++-common/Wstringop-truncation.c: Adjust text of expected warnings.
	* g++.dg/warn/Wstringop-truncation-1.C: New test.
Index: gcc/gimple-fold.c
===================================================================
--- gcc/gimple-fold.c	(revision 257875)
+++ gcc/gimple-fold.c	(working copy)
@@ -65,6 +65,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "intl.h"
 #include "calls.h"
 #include "tree-vector-builder.h"
+#include "tree-ssa-strlen.h"
 
 /* Return true when DECL can be referenced from current unit.
    FROM_DECL (if non-null) specify constructor of variable DECL was taken from.
@@ -1710,38 +1711,9 @@ gimple_fold_builtin_strncpy (gimple_stmt_iterator
   if (tree_int_cst_lt (ssize, len))
     return false;
 
-  if (!nonstring)
-    {
-      if (tree_int_cst_lt (len, slen))
-	{
-	  tree fndecl = gimple_call_fndecl (stmt);
-	  gcall *call = as_a <gcall *> (stmt);
+  /* Diagnose truncation that leaves the copy unterminated.  */
+  maybe_diag_stxncpy_trunc (*gsi, src, len);
 
-	  warning_at (loc, OPT_Wstringop_truncation,
-		      (tree_int_cst_equal (size_one_node, len)
-		       ? G_("%G%qD output truncated copying %E byte "
-			    "from a string of length %E")
-		       : G_("%G%qD output truncated copying %E bytes "
-			    "from a string of length %E")),
-		      call, fndecl, len, slen);
-	}
-      else if (tree_int_cst_equal (len, slen))
-	{
-	  tree fndecl = gimple_call_fndecl (stmt);
-	  gcall *call = as_a <gcall *> (stmt);
-
-	  warning_at (loc, OPT_Wstringop_truncation,
-		      (tree_int_cst_equal (size_one_node, len)
-		       ? G_("%G%qD output truncated before terminating nul "
-			    "copying %E byte from a string of the same "
-			    "length")
-		       : G_("%G%qD output truncated before terminating nul "
-			    "copying %E bytes from a string of the same "
-			    "length")),
-		      call, fndecl, len);
-	}
-    }
-
   /* OK transform into builtin memcpy.  */
   tree fn = builtin_decl_implicit (BUILT_IN_MEMCPY);
   if (!fn)
Index: gcc/testsuite/c-c++-common/Wstringop-truncation.c
===================================================================
--- gcc/testsuite/c-c++-common/Wstringop-truncation.c	(revision 257875)
+++ gcc/testsuite/c-c++-common/Wstringop-truncation.c	(working copy)
@@ -188,7 +188,7 @@ void test_strncpy_ptr (char *d, const char* s, con
   CPY (d, CHOOSE (s, t), 2);
 
   CPY (d, CHOOSE ("", "123"), 1);   /* { dg-warning ".strncpy\[^\n\r\]* output may be truncated copying 1 byte from a string of length 3" } */
-  CPY (d, CHOOSE ("1", "123"), 1);  /* { dg-warning ".strncpy\[^\n\r\]* output truncated copying 1 byte from a string of length 1" } */
+  CPY (d, CHOOSE ("1", "123"), 1);  /* { dg-warning ".strncpy\[^\n\r\]* output truncated before terminating nul copying 1 byte from a string of the same length" } */
   CPY (d, CHOOSE ("12", "123"), 1); /* { dg-warning ".strncpy\[^\n\r\]* output truncated copying 1 byte from a string of length 2" } */
   CPY (d, CHOOSE ("123", "12"), 1); /* { dg-warning ".strncpy\[^\n\r\]* output truncated copying 1 byte from a string of length 2" } */
 
@@ -331,7 +331,7 @@ void test_strncpy_array (Dest *pd, int i, const ch
     /* This might be better written using memcpy() but it's safe so
        it probably shouldn't be diagnosed.  It currently triggers
        a warning because of bug 81704.  */
-    strncpy (dst7, "0123456", sizeof dst7);   /* { dg-bogus "truncated" "bug 81704" { xfail *-*-* } } */
+    strncpy (dst7, "0123456", sizeof dst7);   /* { dg-bogus "\\\[-Wstringop-truncation]" "bug 81704" { xfail *-*-* } } */
     dst7[sizeof dst7 - 1] = '\0';
     sink (dst7);
   }
@@ -350,7 +350,7 @@ void test_strncpy_array (Dest *pd, int i, const ch
   }
 
   {
-    strncpy (pd->a5, "01234", sizeof pd->a5);   /* { dg-bogus "truncated" "bug 81704" { xfail *-*-* } } */
+    strncpy (pd->a5, "01234", sizeof pd->a5);   /* { dg-bogus "\\\[-Wstringop-truncation]" "bug 81704" { xfail *-*-* } } */
     pd->a5[sizeof pd->a5 - 1] = '\0';
     sink (pd);
   }
Index: gcc/testsuite/g++.dg/warn/Wstringop-truncation-1.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wstringop-truncation-1.C	(nonexistent)
+++ gcc/testsuite/g++.dg/warn/Wstringop-truncation-1.C	(working copy)
@@ -0,0 +1,126 @@
+/* PR/tree-optimization/84480 - bogus -Wstringop-truncation despite
+   assignment with an inlined string literal
+   { dg-do compile }
+   { dg-options "-O2 -Wstringop-truncation" }  */
+
+#include <string.h>
+
+template <size_t N>
+class GoodString
+{
+public:
+  GoodString (const char *s, size_t slen = N)
+  {
+    if (slen > N)
+      slen = N;
+
+    strncpy (str, s, slen);
+
+    str[slen] = '\0';
+  }
+
+private:
+  char str[N + 1];
+};
+
+void sink (void*);
+
+void good_nowarn_size_m2 ()
+{
+  GoodString<3> str ("12");
+  sink (&str);
+}
+
+void good_nowarn_size_m1 ()
+{
+  GoodString<3> str ("123");    // { dg-bogus "\\\[-Wstringop-truncation]" }
+  sink (&str);
+}
+
+void good_nowarn_size_m1_var (const char* s)
+{
+  GoodString<3> str (s);        // { dg-bogus "\\\[-Wstringop-truncation]" }
+  sink (&str);
+}
+
+void call_good_nowarn_size_m1_var ()
+{
+  good_nowarn_size_m1_var ("456");
+}
+
+
+template <size_t N>
+class BadString1
+{
+public:
+  BadString1 (const char *s, size_t slen = N)
+  {
+    if (slen > N)
+      slen = N;
+
+    strncpy (str, s, slen);
+  }
+
+private:
+  char str[N + 1];
+};
+
+void bad1_nowarn_size_m2 ()
+{
+  BadString1<3> str ("12");
+  sink (&str);
+}
+
+
+template <size_t N>
+class BadString2
+{
+public:
+  BadString2 (const char *s, size_t slen = N)
+  {
+    if (slen > N)
+      slen = N;
+
+    strncpy (str, s, slen);     // { dg-warning "\\\[-Wstringop-truncation]" }
+  }
+
+private:
+  char str[N + 1];
+};
+
+void bad2_warn_size_m1 ()
+{
+  BadString2<3> str ("123");
+  sink (&str);
+}
+
+// { dg-message "inlined from .void bad2_warn_size_m1." "" { target *-*-* } 0 }
+
+template <size_t N>
+class BadString3
+{
+public:
+  BadString3 (const char *s, size_t slen = N)
+  {
+    if (slen > N)
+      slen = N;
+
+    strncpy (str, s, slen);     // { dg-warning "\\\[-Wstringop-truncation]" }
+  }
+
+private:
+  char str[N + 1];
+};
+
+void bad3_warn_size_m1_var (const char *s)
+{
+  BadString3<3> str (s);
+  sink (&str);
+}
+
+void call_bad3_warn_size_m1_var ()
+{
+  bad3_warn_size_m1_var ("456");
+}
+
+// { dg-message "inlined from .void call_bad3_warn_size_m1_var." "" { target *-*-* } 0 }
Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c	(revision 257875)
+++ gcc/tree-ssa-strlen.c	(working copy)
@@ -44,6 +44,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "domwalk.h"
 #include "tree-ssa-alias.h"
 #include "tree-ssa-propagate.h"
+#include "tree-ssa-strlen.h"
 #include "params.h"
 #include "ipa-chkp.h"
 #include "tree-hash-traits.h"
@@ -1780,11 +1781,12 @@ is_strlen_related_p (tree src, tree len)
   return false;
 }
 
-/* A helper of handle_builtin_stxncpy.  Check to see if the specified
-   bound is a) equal to the size of the destination DST and if so, b)
-   if it's immediately followed by DST[CNT - 1] = '\0'.  If a) holds
-   and b) does not, warn.  Otherwise, do nothing.  Return true if
-   diagnostic has been issued.
+/* Called by handle_builtin_stxncpy and by gimple_fold_builtin_strncpy
+   in gimple-fold.c.
+   Check to see if the specified bound is a) equal to the size of
+   the destination DST and if so, b) if it's immediately followed by
+   DST[CNT - 1] = '\0'.  If a) holds and b) does not, warn.  Otherwise,
+   do nothing.  Return true if diagnostic has been issued.
 
    The purpose is to diagnose calls to strncpy and stpncpy that do
    not nul-terminate the copy while allowing for the idiom where
@@ -1795,7 +1797,7 @@ is_strlen_related_p (tree src, tree len)
      a[sizeof a - 1] = '\0';
 */
 
-static bool
+bool
 maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi, tree src, tree cnt)
 {
   gimple *stmt = gsi_stmt (gsi);
@@ -1831,8 +1833,11 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi
     return false;
 
   /* Negative value is the constant string length.  If it's less than
-     the lower bound there is no truncation.  */
-  int sidx = get_stridx (src);
+     the lower bound there is no truncation.  Avoid calling get_stridx()
+     when ssa_ver_to_stridx is empty.  That implies the caller isn't
+     running under the control of this pass and ssa_ver_to_stridx hasn't
+     been created yet.  */
+  int sidx = ssa_ver_to_stridx.length () ? get_stridx (src) : 0;
   if (sidx < 0 && wi::gtu_p (cntrange[0], ~sidx))
     return false;
 
@@ -1935,7 +1940,19 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi
 	  lenrange[0] = wi::shwi (0, prec);
 	}
 
-      if (wi::geu_p (lenrange[0], cntrange[1]))
+      gcall *call = as_a <gcall *> (stmt);
+
+      if (lenrange[0] == cntrange[1] && cntrange[0] == cntrange[1])
+	return warning_at (callloc, OPT_Wstringop_truncation,
+			   (integer_onep (cnt)
+			    ? G_("%G%qD output truncated before terminating "
+				 "nul copying %E byte from a string of the "
+				 "same length")
+			    : G_("%G%qD output truncated before terminating nul "
+				 "copying %E bytes from a string of the same "
+				 "length")),
+			   call, func, cnt);
+      else if (wi::geu_p (lenrange[0], cntrange[1]))
 	{
 	  /* The shortest string is longer than the upper bound of
 	     the count so the truncation is certain.  */
@@ -1942,16 +1959,16 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi
 	  if (cntrange[0] == cntrange[1])
 	    return warning_at (callloc, OPT_Wstringop_truncation,
 			       integer_onep (cnt)
-			       ? G_("%qD output truncated copying %E byte "
+			       ? G_("%G%qD output truncated copying %E byte "
 				    "from a string of length %wu")
-			       : G_("%qD output truncated copying %E bytes "
+			       : G_("%G%qD output truncated copying %E bytes "
 				    "from a string of length %wu"),
-			       func, cnt, lenrange[0].to_uhwi ());
+			       call, func, cnt, lenrange[0].to_uhwi ());
 
 	  return warning_at (callloc, OPT_Wstringop_truncation,
-			     "%qD output truncated copying between %wu "
+			     "%G%qD output truncated copying between %wu "
 			     "and %wu bytes from a string of length %wu",
-			     func, cntrange[0].to_uhwi (),
+			     call, func, cntrange[0].to_uhwi (),
 			     cntrange[1].to_uhwi (), lenrange[0].to_uhwi ());
 	}
       else if (wi::geu_p (lenrange[1], cntrange[1]))
@@ -1961,16 +1978,16 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi
 	  if (cntrange[0] == cntrange[1])
 	    return warning_at (callloc, OPT_Wstringop_truncation,
 			       integer_onep (cnt)
-			       ? G_("%qD output may be truncated copying %E "
+			       ? G_("%G%qD output may be truncated copying %E "
 				    "byte from a string of length %wu")
-			       : G_("%qD output may be truncated copying %E "
+			       : G_("%G%qD output may be truncated copying %E "
 				    "bytes from a string of length %wu"),
-			       func, cnt, lenrange[1].to_uhwi ());
+			       call, func, cnt, lenrange[1].to_uhwi ());
 
 	  return warning_at (callloc, OPT_Wstringop_truncation,
-			     "%qD output may be truncated copying between %wu "
+			     "%G%qD output may be truncated copying between %wu "
 			     "and %wu bytes from a string of length %wu",
-			     func, cntrange[0].to_uhwi (),
+			     call, func, cntrange[0].to_uhwi (),
 			     cntrange[1].to_uhwi (), lenrange[1].to_uhwi ());
 	}
 
@@ -1982,9 +1999,9 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi
 	     the lower bound of the specified count but shorter than the
 	     upper bound the copy may (but need not) be truncated.  */
 	  return warning_at (callloc, OPT_Wstringop_truncation,
-			     "%qD output may be truncated copying between %wu "
-			     "and %wu bytes from a string of length %wu",
-			     func, cntrange[0].to_uhwi (),
+			     "%G%qD output may be truncated copying between "
+			     "%wu and %wu bytes from a string of length %wu",
+			     call, func, cntrange[0].to_uhwi (),
 			     cntrange[1].to_uhwi (), lenrange[0].to_uhwi ());
 	}
     }
@@ -2003,8 +2020,8 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi
 
       if (cntrange[0] == cntrange[1])
 	return warning_at (callloc, OPT_Wstringop_truncation,
-			   "%qD specified bound %E equals destination size",
-			   func, cnt);
+			   "%G%qD specified bound %E equals destination size",
+			   as_a <gcall *> (stmt), func, cnt);
     }
 
   return false;
@@ -2103,14 +2120,15 @@ handle_builtin_stxncpy (built_in_function, gimple_
   if (sisrc == silen
       && is_strlen_related_p (src, len)
       && warning_at (callloc, OPT_Wstringop_truncation,
-		     "%qD output truncated before terminating nul "
+		     "%G%qD output truncated before terminating nul "
 		     "copying as many bytes from a string as its length",
-		     func))
+		     as_a <gcall *>(stmt), func))
     warned = true;
   else if (silen && is_strlen_related_p (src, silen->ptr))
     warned = warning_at (callloc, OPT_Wstringop_overflow_,
-			 "%qD specified bound depends on the length "
-			 "of the source argument", func);
+			 "%G%qD specified bound depends on the length "
+			 "of the source argument",
+			 as_a <gcall *>(stmt), func);
   if (warned)
     {
       location_t strlenloc = pss->second;
Index: gcc/tree-ssa-strlen.h
===================================================================
--- gcc/tree-ssa-strlen.h	(nonexistent)
+++ gcc/tree-ssa-strlen.h	(working copy)
@@ -0,0 +1,29 @@
+/* Declarations of tree-ssa-strlen API.
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+
+   This file is part of GCC.
+
+   GCC is free software; you can redistribute it and/or modify it under
+   the terms of the GNU General Public License as published by the Free
+   Software Foundation; either version 3, or (at your option) any later
+   version.
+
+   GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+   WARRANTY; without even the implied warranty of MERCHANTABILITY or
+   FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+   for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with GCC; see the file COPYING3.  If not see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef GCC_TREE_SSA_STRLEN_H
+#define GCC_TREE_SSA_STRLEN_H
+
+#include "gimple-iterator.h"
+#include "tree.h"
+
+extern bool maybe_diag_stxncpy_trunc (gimple_stmt_iterator, tree, tree);
+
+#endif   // GCC_TREE_SSA_STRLEN_H

Re: [PATCH] avoid bogus -Wstringop-truncation when inlining (PR 84480)

[Jeff Law]

On 02/21/2018 02:19 PM, Martin Sebor wrote:
> The attached patch eliminates -Wstringop-truncation false
> positives reported in bug 84480 - bogus -Wstringop-truncation
> despite assignment with an inlined string literal.  It does
> that by delegating early strncpy checks during folding to
> the same machinery in tree-ssa-strlen that looks for a NUL
> assignment to the destination the next statement.
> 
> The patch also adds inlining context to the warnings via
> the %G directive.
> 
> Tested on x86_64-linux with no regressions.
> 
> Martin
> 
> gcc-84480.diff
> 
> 
> PR tree-optimization/84480 - bogus -Wstringop-truncation despite assignment with an inlined string literal
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/84480
> 	* gimple-fold.c (gimple_fold_builtin_strcpy): Move warnings
> 	to maybe_diag_stxncpy_trunc.  Call it.
> 	* tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Integrate warnings
> 	from gimple_fold_builtin_strcpy.  Print inlining stack.
> 	(handle_builtin_stxncpy): Print inlining stack.
> 	* tree-ssa-strlen.h (maybe_diag_stxncpy_trunc): Declare.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/84480
> 	* c-c++-common/Wstringop-truncation.c: Adjust text of expected warnings.
> 	* g++.dg/warn/Wstringop-truncation-1.C: New test.
In general our guidelines are that the users of a .h file should include
any dependencies rather than having the .h file itself include other .h
files (Now that we've detangled the header files we may want to revisit
that guideline, but that's not a gcc-8 item).

It looks like gimple-fold.c and tree-ssa-strlen.c already have the
prereqs.  So in theory you should be able to just remove the bogus
#includes from tree-ssa-strlen.h.

In general we want to avoid adding more warnings to folding code.  But I
think the argument here is that we're already trying to warn within the
folder and just doing a poor job -- so we're removing that
implementation and delegating the warning to a better implementation.
Right?

So I think you just need to remove the bogus #includes from
tree-ssa-strlen and this is OK.

jeff

Re: [PATCH] avoid bogus -Wstringop-truncation when inlining (PR 84480)

[Martin Sebor]

On 02/21/2018 07:53 PM, Jeff Law wrote:
> On 02/21/2018 02:19 PM, Martin Sebor wrote:
>> The attached patch eliminates -Wstringop-truncation false
>> positives reported in bug 84480 - bogus -Wstringop-truncation
>> despite assignment with an inlined string literal.  It does
>> that by delegating early strncpy checks during folding to
>> the same machinery in tree-ssa-strlen that looks for a NUL
>> assignment to the destination the next statement.
>>
>> The patch also adds inlining context to the warnings via
>> the %G directive.
>>
>> Tested on x86_64-linux with no regressions.
>>
>> Martin
>>
>> gcc-84480.diff
>>
>>
>> PR tree-optimization/84480 - bogus -Wstringop-truncation despite assignment with an inlined string literal
>>
>> gcc/ChangeLog:
>>
>> 	PR tree-optimization/84480
>> 	* gimple-fold.c (gimple_fold_builtin_strcpy): Move warnings
>> 	to maybe_diag_stxncpy_trunc.  Call it.
>> 	* tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Integrate warnings
>> 	from gimple_fold_builtin_strcpy.  Print inlining stack.
>> 	(handle_builtin_stxncpy): Print inlining stack.
>> 	* tree-ssa-strlen.h (maybe_diag_stxncpy_trunc): Declare.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR tree-optimization/84480
>> 	* c-c++-common/Wstringop-truncation.c: Adjust text of expected warnings.
>> 	* g++.dg/warn/Wstringop-truncation-1.C: New test.
> In general our guidelines are that the users of a .h file should include
> any dependencies rather than having the .h file itself include other .h
> files (Now that we've detangled the header files we may want to revisit
> that guideline, but that's not a gcc-8 item).
>
> It looks like gimple-fold.c and tree-ssa-strlen.c already have the
> prereqs.  So in theory you should be able to just remove the bogus
> #includes from tree-ssa-strlen.h.

Done.

>
> In general we want to avoid adding more warnings to folding code.  But I
> think the argument here is that we're already trying to warn within the
> folder and just doing a poor job -- so we're removing that
> implementation and delegating the warning to a better implementation.
> Right?

Exactly.

>
> So I think you just need to remove the bogus #includes from
> tree-ssa-strlen and this is OK.

Thanks.  Committed in r257910.

Martin