This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PING] [PATCH] consider successor blocks when avoiding -Wstringop-truncation (PR 84468)
On 02/26/2018 12:13 PM, Jeff Law wrote:
On 02/24/2018 05:11 PM, Martin Sebor wrote:
Attached is an updated patch with a fix for a bad assumption
exposed by building the linux kernel.
On 02/19/2018 07:50 PM, Martin Sebor wrote:
PR 84468 points out a false positive in -Wstringop-truncation
in code like this:
struct A { char a[4]; };
void f (struct A *p, const struct A *q)
{
if (p->a)
strncpy (p->a, q->a, sizeof p->a - 1); // warning here
p->a[3] = '\0';
}
The warning is due to the code checking only the same basic block
as the one with the strncpy call for an assignment to the destination
to avoid it, but failing to check the successor basic block if there
is no subsequent statement in the current block. (Eliminating
the conditional is being tracked in PR 21474.)
The attached test case adds logic to avoid this false positive.
I don't know under what circumstances there could be more than
one successor block here so I don't handle that case.
So this is feeling more and more like we need to go back to the ideas
behind checking the virtual operand chains.
The patch as-written does not properly handle the case where BB has
multiple outgoing edges. For gcc-8 you could probably get away with
checking that you have precisely one outgoing edge without EDGE_ABNORMAL
set in its flags in addition to the checks you're already doing.
But again, it's feeling more and more like the right fix is to go back
and walk the virtual operands.
I intentionally kept the patch as simple as possible to minimize
risk at this late stage.
Attached is a more robust version that handles multiple outgoing
edges and avoids those with the EDGE_ABNORMAL bit set. Retested
on x86_64 and with the Linux kernel.
Enhancing this code to handle more complex cases is on my to-do
list for stage 1 (e.g., to handle bug 84561 where MEM_REF defeats
the detection of the nul assignment).
Martin
PR tree-optimization/84468 - bogus -Wstringop-truncation despite assignment after conditional strncpy
gcc/ChangeLog:
PR tree-optimization/84468
* tree-ssa-strlen.c (suppress_stringop_trunc): New function to also
consider successor basic blocks.
(maybe_diag_stxncpy_trunc): Call it.
gcc/testsuite/ChangeLog:
PR tree-optimization/84468
* gcc.dg/Wstringop-truncation.c: New test.
* gcc.dg/Wstringop-truncation-2.c: New test.
Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c (revision 258016)
+++ gcc/tree-ssa-strlen.c (working copy)
@@ -1781,6 +1781,83 @@ is_strlen_related_p (tree src, tree len)
return false;
}
+/* Return true if -Wstringop-truncation for statement STMT should be
+ suppressed. GSI is an interator initially pointing at STMT and
+ set to subsequent statements on recursive calls by the function.
+ REF refers to the object being referred to. VISITED is a bitmap
+ of visited basic blocks to prevent infinite recursion. */
+
+static bool
+suppress_stringop_trunc (gimple *stmt, gimple_stmt_iterator gsi, tree ref,
+ bitmap visited)
+{
+ if (gsi_end_p (gsi))
+ return false;
+
+ gimple *cur_stmt = gsi_stmt (gsi);
+ if (is_gimple_debug (cur_stmt) || stmt == cur_stmt)
+ gsi_next_nondebug (&gsi);
+
+ gimple *next_stmt = gsi_stmt (gsi);
+ if (!next_stmt)
+ {
+ /* When there is no statement in the same basic block check
+ the immediate successor block. */
+ if (basic_block bb = gimple_bb (cur_stmt))
+ {
+ /* Return if the basic block has already been visited. */
+ if (!bitmap_set_bit (visited, bb->index))
+ return false;
+
+ edge e;
+ edge_iterator ei;
+ FOR_EACH_EDGE (e, ei, bb->succs)
+ {
+ if (e->flags & EDGE_ABNORMAL)
+ continue;
+
+ basic_block nextbb = e->dest;
+ gimple_stmt_iterator it = gsi_start_bb (nextbb);
+ if (suppress_stringop_trunc (stmt, it, ref, visited))
+ return true;
+ }
+ }
+
+ return false;
+ }
+
+ if (!is_gimple_assign (next_stmt))
+ return false;
+
+ tree lhs = gimple_assign_lhs (next_stmt);
+ tree_code code = TREE_CODE (lhs);
+ if (code == ARRAY_REF || code == MEM_REF)
+ lhs = TREE_OPERAND (lhs, 0);
+
+ tree func = gimple_call_fndecl (stmt);
+ if (DECL_FUNCTION_CODE (func) == BUILT_IN_STPNCPY)
+ {
+ tree ret = gimple_call_lhs (stmt);
+ if (ret && operand_equal_p (ret, lhs, 0))
+ return true;
+ }
+
+ /* Determine the base address and offset of the reference,
+ ignoring the innermost array index. */
+ if (TREE_CODE (ref) == ARRAY_REF)
+ ref = TREE_OPERAND (ref, 0);
+
+ poly_int64 dstoff;
+ tree dstbase = get_addr_base_and_unit_offset (ref, &dstoff);
+
+ poly_int64 lhsoff;
+ tree lhsbase = get_addr_base_and_unit_offset (lhs, &lhsoff);
+ return (lhsbase
+ && dstbase
+ && known_eq (dstoff, lhsoff)
+ && operand_equal_p (dstbase, lhsbase, 0));
+}
+
/* 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
@@ -1846,49 +1923,22 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi
if (TREE_CODE (dstdecl) == ADDR_EXPR)
dstdecl = TREE_OPERAND (dstdecl, 0);
- /* If the destination refers to a an array/pointer declared nonstring
+ /* If the destination refers to an array/pointer declared nonstring
return early. */
tree ref = NULL_TREE;
if (get_attr_nonstring_decl (dstdecl, &ref))
return false;
- /* Look for dst[i] = '\0'; after the stxncpy() call and if found
- avoid the truncation warning. */
- gsi_next_nondebug (&gsi);
- gimple *next_stmt = gsi_stmt (gsi);
+ {
+ /* Look for dst[i] = '\0'; after the stxncpy() call and if found
+ avoid the truncation warning. */
+ bitmap visited = BITMAP_ALLOC (NULL);
+ bool suppress = suppress_stringop_trunc (stmt, gsi, ref, visited);
+ BITMAP_FREE (visited);
+ if (suppress)
+ return false;
+ }
- if (!gsi_end_p (gsi) && is_gimple_assign (next_stmt))
- {
- tree lhs = gimple_assign_lhs (next_stmt);
- tree_code code = TREE_CODE (lhs);
- if (code == ARRAY_REF || code == MEM_REF)
- lhs = TREE_OPERAND (lhs, 0);
-
- tree func = gimple_call_fndecl (stmt);
- if (DECL_FUNCTION_CODE (func) == BUILT_IN_STPNCPY)
- {
- tree ret = gimple_call_lhs (stmt);
- if (ret && operand_equal_p (ret, lhs, 0))
- return false;
- }
-
- /* Determine the base address and offset of the reference,
- ignoring the innermost array index. */
- if (TREE_CODE (ref) == ARRAY_REF)
- ref = TREE_OPERAND (ref, 0);
-
- poly_int64 dstoff;
- tree dstbase = get_addr_base_and_unit_offset (ref, &dstoff);
-
- poly_int64 lhsoff;
- tree lhsbase = get_addr_base_and_unit_offset (lhs, &lhsoff);
- if (lhsbase
- && dstbase
- && known_eq (dstoff, lhsoff)
- && operand_equal_p (dstbase, lhsbase, 0))
- return false;
- }
-
int prec = TYPE_PRECISION (TREE_TYPE (cnt));
wide_int lenrange[2];
if (strinfo *sisrc = sidx > 0 ? get_strinfo (sidx) : NULL)
Index: gcc/testsuite/gcc.dg/Wstringop-truncation.c
===================================================================
--- gcc/testsuite/gcc.dg/Wstringop-truncation.c (nonexistent)
+++ gcc/testsuite/gcc.dg/Wstringop-truncation.c (working copy)
@@ -0,0 +1,131 @@
+/* PR tree-optimization/84468 - Inconsistent -Wstringop-truncation warnings
+ with -O2
+ { dg-do compile }
+ { dg-options "-O2 -Wstringop-truncation -ftrack-macro-expansion=0 -g" } */
+
+#define strncpy __builtin_strncpy
+
+struct A
+{
+ char a[4];
+};
+
+void no_pred_succ_lit (struct A *p)
+{
+ /* The following is folded early on, before the strncpy statement
+ has a basic block. Verify that the case is handled gracefully
+ (i.e., there's no assumption that the statement does have
+ a basic block). */
+ strncpy (p->a, "1234", sizeof p->a - 1); /* { dg-warning "\\\[-Wstringop-truncation" } */
+}
+
+/* Verify a strncpy call in a basic block with no predecessor or
+ successor. */
+void no_pred_succ (struct A *p, const struct A *q)
+{
+ strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-warning "\\\[-Wstringop-truncation" } */
+}
+
+
+/* Verify a strncpy call in a basic block with no successor. */
+void no_succ (struct A *p, const struct A *q)
+{
+ if (q->a)
+ strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-warning "\\\[-Wstringop-truncation" } */
+}
+
+/* Verify a strncpy call in a basic block with nul assignment in
+ a successor block. */
+void succ (struct A *p, const struct A *q)
+{
+ /* Verify that the assignment suppresses the warning for the conditional
+ strcnpy call. The conditional should be folded to true since the
+ address of an array can never be null (see bug 84470). */
+ if (q->a)
+ strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation" } */
+
+ p->a[sizeof p->a - 1] = 0;
+}
+
+
+void succ_2 (struct A *p, const struct A *q, int i)
+{
+ /* Same as above but with a conditional that cannot be eliminated. */
+ if (i < 0)
+ strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation" } */
+
+ p->a[sizeof p->a - 1] = 0;
+}
+
+
+/* Verify a strncpy call in a basic block with nul assignment in
+ the next successor block. */
+int next_succ (struct A *p, const struct A *q, int i, int j)
+{
+ /* Same as above but with a nested conditionals with else clauses. */
+ if (i < 0)
+ {
+ if (j < 0)
+ strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation" } */
+ }
+ else
+ __builtin_strcpy (p->a, q->a);
+
+ p->a[sizeof p->a - 1] = 0;
+ return 0;
+}
+
+
+int next_succ_1 (struct A *p, const struct A *q, int i, int j)
+{
+ /* Same as above but with a nested conditionals with else clauses. */
+ if (i < 0)
+ {
+ if (j < 0)
+ strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation" } */
+ else
+ strncpy (p->a, q->a, sizeof p->a - 2); /* { dg-bogus "\\\[-Wstringop-truncation" } */
+ }
+
+ p->a[sizeof p->a - 2] = 0;
+ return 1;
+}
+
+
+int next_succ_2 (struct A *p, const struct A *q, int i, int j)
+{
+ /* Same as above but with a nested conditionals with else clauses. */
+ if (i < 0)
+ {
+ if (j < 0)
+ strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation" } */
+ else
+ strncpy (p->a, q->a, sizeof p->a - 2); /* { dg-bogus "\\\[-Wstringop-truncation" } */
+ }
+ else
+ __builtin_strcpy (p->a, q->a);
+
+ p->a[sizeof p->a - 2] = 0;
+ return 2;
+}
+
+
+void cond_succ_warn (struct A *p, const struct A *q, int i)
+{
+ /* Verify that a conditional assignment doesn't suppress the warning. */
+ strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-warning "\\\[-Wstringop-truncation" } */
+
+ if (i < 0)
+ p->a[sizeof p->a - 1] = 0;
+}
+
+void cond_succ_nowarn (struct A *p, const struct A *q)
+{
+ /* Verify that distinct but provably equivalent conditionals are
+ recognized and don't trigger the warning. */
+ if (p != q)
+ strncpy (p->a, q->a, sizeof p->a - 1);
+
+ if (p->a != q->a)
+ p->a[sizeof p->a - 1] = 0;
+}
Index: gcc/testsuite/gcc.dg/Wstringop-truncation-2.c
===================================================================
--- gcc/testsuite/gcc.dg/Wstringop-truncation-2.c (nonexistent)
+++ gcc/testsuite/gcc.dg/Wstringop-truncation-2.c (working copy)
@@ -0,0 +1,126 @@
+/* PR tree-optimization/84468 - bogus -Wstringop-truncation despite
+ assignment after conditional strncpy
+ { dg-do compile }
+ { dg-options "-O2 -Wstringop-truncation -g" } */
+
+extern char* strncpy (char*, const char*, __SIZE_TYPE__);
+
+char a[4];
+
+void f1 (char *s)
+{
+ int i = 0;
+
+ if (s[0] == '0')
+ {
+ i += 1;
+ strncpy (a, s, sizeof a); /* { dg-bogus "\\\[-Wstringop-truncation]" } */
+ }
+ else
+ i += 2;
+
+ a[sizeof a - 1] = 0;
+}
+
+void f2 (char *s)
+{
+ int i = 0;
+
+ if (s[0] == '0')
+ {
+ i += 1;
+ if (s[1] == '1')
+ {
+ i += 2;
+ strncpy (a, s, sizeof a); /* { dg-bogus "\\\[-Wstringop-truncation]" } */
+ }
+ else
+ i += 3;
+ }
+ else
+ i += 4;
+
+ a[sizeof a - 1] = 0;
+}
+
+void f3 (char *s)
+{
+ int i = 0;
+
+ if (s[0] == '0')
+ {
+ i += 1;
+ if (s[1] == '1')
+ {
+ i += 2;
+ if (s[2] == '2')
+ strncpy (a, s, sizeof a); /* { dg-bogus "\\\[-Wstringop-truncation]" } */
+ else
+ i += 3;
+ }
+ else
+ i += 4;
+ }
+ else
+ i += 5;
+
+ a[sizeof a - 1] = 0;
+}
+
+void f4 (char *s)
+{
+ int i = 0;
+
+ if (s[0] == '0')
+ {
+ i += 1;
+ if (s[1] == '1')
+ {
+ i += 2;
+ if (s[2] == '2')
+ {
+ i += 3;
+ if (s[3] == '3')
+ strncpy (a, s, sizeof a); /* { dg-bogus "\\\[-Wstringop-truncation]" } */
+ else
+ i += 4;
+ }
+ else
+ i += 5;
+ }
+ else
+ i += 6;
+ }
+ else
+ i += 7;
+
+ a[sizeof a - 1] = 0;
+}
+
+void f4_warn (char *s)
+{
+ int i = 0;
+
+ if (s[0] == '0')
+ {
+ i += 1;
+ if (s[1] == '1')
+ {
+ i += 2;
+ if (s[2] == '2')
+ {
+ i += 3;
+ if (s[3] == '3')
+ strncpy (a, s, sizeof a); /* { dg-warning "\\\[-Wstringop-truncation]" } */
+ else
+ i += 4;
+ }
+ else
+ i += 5;
+ }
+ else
+ i += 6;
+ }
+ else
+ i += 7;
+}