On 11/29/18 4:07 PM, Jeff Law wrote:
On 11/29/18 1:34 PM, Martin Sebor wrote:
On 11/16/2018 02:07 AM, Richard Biener wrote:
On Fri, Nov 16, 2018 at 4:12 AM Martin Sebor <mse...@gmail.com> wrote:
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01818.html
Please let me know if there is something I need to change here
to make the fix acceptable or if I should stop trying.
I have one more comment about
+ /* Defer warning (and folding) until the next statement in the basic
+ block is reachable. */
+ if (!gimple_bb (stmt))
+ return false;
+
it's not about the next statement in the basic-block being "reachable"
(even w/o a CFG you can use gsi_next()) but rather that the next
stmt isn't yet gimplified and thus not inserted into the gimple sequence,
right?
No, it's about the current statement not being associated with
a basic block yet when the warning code runs for the first time
(during gimplify_expr), and so gsi_next() returning null.
You apply this to gimple_fold_builtin_strncpy but I'd rather
see us not sprinkling this over gimple-fold.c but instead do this
in gimplify.c:maybe_fold_stmt, delaying folding until say lowering.
See the attached (untested).
I would also prefer this solution. I had tested it (in response
to you first mentioning it back in September) and it causes quite
a bit of fallout in tests that look for the folding to take place
very early. See the end of my reply here:
https://gcc.gnu.org/ml/gcc-patches/2018-09/msg01248.html
But I'm willing to do the test suite cleanup if you think it's
suitable for GCC 9. (If you're thinking GCC 10 please let me
know now.)
The fallout on existing tests is minimal. What's more concerning is
that it doesn't actually pass the new test from Martin's original
submission. We get bogus warnings.
At least part of the problem is weakness in maybe_diag_stxncpy_trunc.
It can't handle something like this:
test_literal (char * d, struct S * s)
{
strncpy (d, "1234567890", 3);
_1 = d + 3;
*_1 = 0;
}
Note the pointer arithmetic between the strncpy and storing the NUL
terminator.
Right. I'm less concerned about this case because it involves
a literal that's obviously longer than the destination but it
would be nice if the suppression worked here as well in case
the literal comes from macro expansion. It will require
another tweak.
But the test from my patch passes with the changes to calls.c
from my patch, so that's an improvement.
I have done the test suite cleanup in the attached patch. It
was indeed minimal -- not sure why I saw so many failures with
my initial approach.
I can submit a patch to handle the literal case above as
a followup unless you would prefer it done at the same time.
Martin
PR tree-optimization/87028 - false positive -Wstringop-truncation strncpy with global variable source string
gcc/ChangeLog:
PR tree-optimization/87028
* calls.c (get_attr_nonstring_decl): Avoid setting *REF to
SSA_NAME_VAR.
* gcc/gimple-low.c (lower_stmt): Delay foldin built-ins.
* gimplify (maybe_fold_stmt): Avoid folding statements that
don't belong to a basic block.
* tree.h (SSA_NAME_VAR): Update comment.
* tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Simplify.
gcc/testsuite/ChangeLog:
PR tree-optimization/87028
* c-c++-common/Wstringop-truncation.c: Remove xfails.
* gcc.dg/Wstringop-truncation-5.c: New test.
* gcc.dg/strcmpopt_1.c: Adjust.
* gcc.dg/tree-ssa/pr79697.c: Same.
Index: gcc/calls.c
===================================================================
--- gcc/calls.c (revision 266637)
+++ gcc/calls.c (working copy)
@@ -1503,6 +1503,7 @@ tree
get_attr_nonstring_decl (tree expr, tree *ref)
{
tree decl = expr;
+ tree var = NULL_TREE;
if (TREE_CODE (decl) == SSA_NAME)
{
gimple *def = SSA_NAME_DEF_STMT (decl);
@@ -1515,17 +1516,25 @@ get_attr_nonstring_decl (tree expr, tree *ref)
|| code == VAR_DECL)
decl = gimple_assign_rhs1 (def);
}
- else if (tree var = SSA_NAME_VAR (decl))
- decl = var;
+ else
+ var = SSA_NAME_VAR (decl);
}
if (TREE_CODE (decl) == ADDR_EXPR)
decl = TREE_OPERAND (decl, 0);
+ /* To simplify calling code, store the referenced DECL regardless of
+ the attribute determined below, but avoid storing the SSA_NAME_VAR
+ obtained above (it's not useful for dataflow purposes). */
if (ref)
*ref = decl;
- if (TREE_CODE (decl) == ARRAY_REF)
+ /* Use the SSA_NAME_VAR that was determined above to see if it's
+ declared nonstring. Otherwise drill down into the referenced
+ DECL. */
+ if (var)
+ decl = var;
+ else if (TREE_CODE (decl) == ARRAY_REF)
decl = TREE_OPERAND (decl, 0);
else if (TREE_CODE (decl) == COMPONENT_REF)
decl = TREE_OPERAND (decl, 1);
Index: gcc/gimple-low.c
===================================================================
--- gcc/gimple-low.c (revision 266637)
+++ gcc/gimple-low.c (working copy)
@@ -32,6 +32,7 @@ along with GCC; see the file COPYING3. If not see
#include "gimple-low.h"
#include "predict.h"
#include "gimple-predict.h"
+#include "gimple-fold.h"
/* The differences between High GIMPLE and Low GIMPLE are the
following:
@@ -378,6 +379,12 @@ lower_stmt (gimple_stmt_iterator *gsi, struct lowe
gsi_next (gsi);
return;
}
+
+ /* We delay folding of built calls from gimplification to
+ here so the IL is in consistent state for the diagnostic
+ machineries job. */
+ if (gimple_call_builtin_p (stmt))
+ fold_stmt (gsi);
}
break;
Index: gcc/gimplify.c
===================================================================
--- gcc/gimplify.c (revision 266637)
+++ gcc/gimplify.c (working copy)
@@ -3192,6 +3192,10 @@ maybe_fold_stmt (gimple_stmt_iterator *gsi)
return false;
else if ((ctx->region_type & ORT_HOST_TEAMS) == ORT_HOST_TEAMS)
return false;
+ /* Delay folding of builtins until the IL is in consistent state
+ so the diagnostic machinery can do a better job. */
+ if (gimple_call_builtin_p (gsi_stmt (*gsi)))
+ return false;
return fold_stmt (gsi);
}
Index: gcc/testsuite/c-c++-common/Wstringop-truncation.c
===================================================================
--- gcc/testsuite/c-c++-common/Wstringop-truncation.c (revision 266637)
+++ gcc/testsuite/c-c++-common/Wstringop-truncation.c (working copy)
@@ -329,9 +329,8 @@ void test_strncpy_array (Dest *pd, int i, const ch
of the array to NUL is not diagnosed. */
{
/* 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 "\\\[-Wstringop-truncation]" "bug 81704" { xfail *-*-* } } */
+ it shouldn't be diagnosed. */
+ strncpy (dst7, "0123456", sizeof dst7); /* { dg-bogus "\\\[-Wstringop-truncation]" } */
dst7[sizeof dst7 - 1] = '\0';
sink (dst7);
}
@@ -350,7 +349,7 @@ void test_strncpy_array (Dest *pd, int i, const ch
}
{
- strncpy (pd->a5, "01234", sizeof pd->a5); /* { dg-bogus "\\\[-Wstringop-truncation]" "bug 81704" { xfail *-*-* } } */
+ strncpy (pd->a5, "01234", sizeof pd->a5); /* { dg-bogus "\\\[-Wstringop-truncation]" } */
pd->a5[sizeof pd->a5 - 1] = '\0';
sink (pd);
}
Index: gcc/testsuite/gcc.dg/Wstringop-truncation-5.c
===================================================================
--- gcc/testsuite/gcc.dg/Wstringop-truncation-5.c (nonexistent)
+++ gcc/testsuite/gcc.dg/Wstringop-truncation-5.c (working copy)
@@ -0,0 +1,64 @@
+/* PR tree-optimization/87028 - false positive -Wstringop-truncation
+ strncpy with global variable source string
+ { dg-do compile }
+ { dg-options "-O2 -Wstringop-truncation" } */
+
+char *strncpy (char *, const char *, __SIZE_TYPE__);
+
+#define STR "1234567890"
+
+struct S
+{
+ char a[5], b[5];
+};
+
+const char arr[] = STR;
+const char* const ptr = STR;
+
+const char arr2[][10] = { "123", STR };
+
+void test_literal (struct S *s)
+{
+ strncpy (s->a, STR, sizeof s->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation]" } */
+ s->a[sizeof s->a - 1] = '\0';
+}
+
+void test_global_arr (struct S *s)
+{
+ strncpy (s->a, arr, sizeof s->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation]" } */
+ s->a [sizeof s->a - 1] = '\0';
+}
+
+void test_global_arr2 (struct S *s)
+{
+ strncpy (s->a, arr2[1], sizeof s->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation]" } */
+ s->a [sizeof s->a - 1] = '\0';
+
+ strncpy (s->b, arr2[0], sizeof s->a - 1);
+}
+
+void test_global_ptr (struct S *s)
+{
+ strncpy (s->a, ptr, sizeof s->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation]" } */
+ s->a [sizeof s->a - 1] = '\0';
+}
+
+void test_local_arr (struct S *s)
+{
+ const char arr[] = STR;
+ strncpy (s->a, arr, sizeof s->a - 1);
+ s->a [sizeof s->a - 1] = '\0';
+}
+
+void test_local_ptr (struct S *s)
+{
+ const char* const ptr = STR;
+ strncpy (s->a, ptr, sizeof s->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation]" } */
+ s->a [sizeof s->a - 1] = '\0';
+}
+
+void test_compound_literal (struct S *s)
+{
+ strncpy (s->a, (char[]){ STR }, sizeof s->a - 1);
+ s->a [sizeof s->a - 1] = '\0';
+}
Index: gcc/testsuite/gcc.dg/fold-bcopy.c
===================================================================
--- gcc/testsuite/gcc.dg/fold-bcopy.c (revision 266637)
+++ gcc/testsuite/gcc.dg/fold-bcopy.c (working copy)
@@ -1,6 +1,6 @@
/* PR tree-optimization/80933 - redundant bzero/bcopy calls not eliminated
{ dg-do compile }
- { dg-options "-O0 -Wall -fdump-tree-gimple" } */
+ { dg-options "-O1 -Wall -fdump-tree-lower" } */
void f0 (void *dst, const void *src, unsigned n)
{
@@ -46,9 +46,9 @@ void f6 (void *p)
/* Verify that calls to bcmp, bcopy, and bzero have all been removed
and one of each replaced with memcmp, memmove, and memset, respectively.
The remaining three should be eliminated.
- { dg-final { scan-tree-dump-not "bcmp|bcopy|bzero" "gimple" } }
- { dg-final { scan-tree-dump-times "memcmp|memmove|memset" 3 "gimple" } }
+ { dg-final { scan-tree-dump-not "bcmp|bcopy|bzero" "lower" } }
+ { dg-final { scan-tree-dump-times "memcmp|memmove|memset" 3 "lower" } }
Verify that the bcopy to memmove transformation correctly transposed
the source and destination pointer arguments.
- { dg-final { scan-tree-dump-times "memmove \\(dst, src" 1 "gimple" } } */
+ { dg-final { scan-tree-dump-times "memmove \\(dst, src" 1 "lower" } } */
Index: gcc/testsuite/gcc.dg/strcmpopt_1.c
===================================================================
--- gcc/testsuite/gcc.dg/strcmpopt_1.c (revision 266637)
+++ gcc/testsuite/gcc.dg/strcmpopt_1.c (working copy)
@@ -1,5 +1,5 @@
/* { dg-do run } */
-/* { dg-options "-fdump-tree-gimple" } */
+/* { dg-options "-fdump-tree-lower" } */
#include <string.h>
#include <stdlib.h>
@@ -25,4 +25,4 @@ int main ()
return 0;
}
-/* { dg-final { scan-tree-dump-times "strcmp \\(" 2 "gimple" } } */
+/* { dg-final { scan-tree-dump-times "strcmp \\(" 2 "lower" } } */
Index: gcc/testsuite/gcc.dg/tree-ssa/pr79697.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/pr79697.c (revision 266637)
+++ gcc/testsuite/gcc.dg/tree-ssa/pr79697.c (working copy)
@@ -1,5 +1,5 @@
/* { dg-do compile } */
-/* { dg-options "-O2 -fdump-tree-gimple -fdump-tree-cddce-details -fdump-tree-optimized" } */
+/* { dg-options "-O2 -fdump-tree-lower -fdump-tree-cddce-details -fdump-tree-optimized" } */
void f(void)
{
@@ -18,4 +18,4 @@ void h(void)
/* { dg-final { scan-tree-dump "Deleting : __builtin_strdup" "cddce1" } } */
/* { dg-final { scan-tree-dump "Deleting : __builtin_strndup" "cddce1" } } */
-/* { dg-final { scan-tree-dump "__builtin_malloc" "gimple" } } */
+/* { dg-final { scan-tree-dump "__builtin_malloc" "lower" } } */
