On 02/27/2018 06:50 PM, Jeff Law wrote:
On 02/26/2018 05:47 PM, Martin Sebor wrote:
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).
I don't think handling multiple outgoing edges is advisable here. To do
that you have to start thinking about post-dominator analysis at which
point you're better off walking the memory web via VUSE/VDEFs.
Just verify the block has a single outgoing edge and that the edge is
not marked with EDGE_ABNORMAL. Don't bother with the recursive call.
Assuming you get a suitable block, then look inside.
I read you last reply as asking me to handle multiple edges.
The original patch handled just one edge and didn't bother
with EDGE_ABNORMAL because I wanted to keep it simple. But
it sounds like you want me to go back to the first version
and just add a check for EDGE_ABNORMAL. The attached patch
does that (plus it skips over any non-debug statements in
the successor block). Besides the usual I retested it with
the Linux kernel. There are just three instances of
the warning in my default configuration but I know there
are a lot more in more extensive builds.
I glanced over the tests and I didn't see any that would benefit from
handling multiple edges or the recursion (every one of the dg-bogus
markers should be immediately transferring control to the null
termination statement AFAICT).
I've added some more test cases involving C++ exceptions (and
found one false positive(*)) but I don't have a test to exercise
blocks with multiple outgoing edges. For future reference, how
would I create one?
Martin
[*] From bug 84624:
char d[3];
void f ();
void g (const char *s)
{
try
{
f ();
}
catch (...)
{
__builtin_strncpy (d, s, sizeof d); // bogus warning
}
d[sizeof d - 1] = 0; // because of this
}
The warning sees the following and triggers because the strncpy
call is followed by __cxa_end_catch().
<bb 4> [count: 0]:
<L1>:
_1 = __builtin_eh_pointer (1);
__cxa_begin_catch (_1);
__builtin_strncpy (&d, s_7(D), 3);
__cxa_end_catch ();
goto <bb 3>; [100.00%]
Index: gcc/testsuite/g++.dg/warn/Wstringop-truncation-2.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wstringop-truncation-2.C (nonexistent)
+++ gcc/testsuite/g++.dg/warn/Wstringop-truncation-2.C (working copy)
@@ -0,0 +1,164 @@
+// PR tree-optimization/84468 - bogus -Wstringop-truncation despite
+// assignment after conditional strncpy
+// Compile with -g to verify the warning deals properly with debug
+// statements.
+// { dg-do compile }
+// { dg-options "-O2 -Wstringop-truncation -g" }
+
+extern "C" char* strncpy (char*, const char*, __SIZE_TYPE__);
+
+char d[3];
+
+void g ();
+
+void fnowarn1 (const char *s)
+{
+ // Update dummy but never actually use it so it's eliminated
+ // but causes debugging statements to be emitted for each
+ // modification.
+ int dummy = 0;
+
+ try
+ {
+ g ();
+ strncpy (d, s, sizeof d); // { dg-bogus "\\\[-Wstringop-truncation]" }
+ ++dummy;
+ }
+ catch (...)
+ {
+ ++dummy;
+ d[0] = 0;
+ }
+
+ ++dummy;
+ d[sizeof d - 1] = 0;
+}
+
+void fnowarn2 (const char *s)
+{
+ int dummy = 0;
+
+ try
+ {
+ g ();
+ strncpy (d, s, sizeof d);
+ ++dummy;
+ }
+ catch (...)
+ {
+ ++dummy;
+ return;
+ }
+
+ ++dummy;
+ d[sizeof d - 1] = 0;
+}
+
+void fnowarn3 (const char *s)
+{
+ int dummy = 0;
+
+ try
+ {
+ g ();
+ strncpy (d, s, sizeof d);
+ ++dummy;
+ try
+ {
+ ++dummy;
+ d[sizeof d - 1] = 0;
+ g ();
+ }
+ catch (...)
+ {
+ ++dummy;
+ }
+ }
+ catch (...)
+ {
+ ++dummy;
+ return;
+ }
+
+ ++dummy;
+ d[sizeof d - 1] = 0;
+}
+
+void fnowarn4 (const char *s)
+{
+ int dummy = 0;
+
+ try
+ {
+ g ();
+ }
+ catch (...)
+ {
+ strncpy (d, s, sizeof d); // { dg-bogus "\\\[-Wstringop-truncation]" "bug 84468" { xfail *-*-*} }
+ ++dummy;
+ }
+
+ ++dummy;
+ d[sizeof d - 1] = 0;
+}
+
+void fwarn1 (const char *s)
+{
+ int dummy = 0;
+
+ try
+ {
+ ++dummy;
+ g ();
+ ++dummy;
+ strncpy (d, s, sizeof d); // { dg-warning "\\\[-Wstringop-truncation]" }
+ ++dummy;
+ }
+ catch (...)
+ {
+ ++dummy;
+ }
+
+ ++dummy;
+}
+
+void fwarn2 (const char *s)
+{
+ int dummy = 0;
+
+ try
+ {
+ ++dummy;
+ strncpy (d, s, sizeof d); // { dg-warning "\\\[-Wstringop-truncation]" }
+ ++dummy;
+ g ();
+ ++dummy;
+ }
+ catch (...)
+ {
+ ++dummy;
+ }
+
+ ++dummy;
+}
+
+void fwarn3 (const char *s)
+{
+ int dummy = 0;
+
+ try
+ {
+ ++dummy;
+ g ();
+ ++dummy;
+ strncpy (d, s, sizeof d); // { dg-warning "\\\[-Wstringop-truncation]" }
+ ++dummy;
+ }
+ catch (...)
+ {
+ ++dummy;
+ d[0] = 0;
+ }
+
+ ++dummy;
+}
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;
+}
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/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c (revision 258088)
+++ gcc/tree-ssa-strlen.c (working copy)
@@ -1856,8 +1856,33 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi
avoid the truncation warning. */
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 (stmt))
+ {
+ if (single_succ_p (bb))
+ {
+ /* For simplicity, ignore blocks with multiple outgoing
+ edges for now and only consider successor blocks along
+ normal edges. */
+ edge e = EDGE_SUCC (bb, 0);
+ if (!(e->flags & EDGE_ABNORMAL))
+ {
+ gsi = gsi_start_bb (e->dest);
+ next_stmt = gsi_stmt (gsi);
+ if (next_stmt && is_gimple_debug (next_stmt))
+ {
+ gsi_next_nondebug (&gsi);
+ next_stmt = gsi_stmt (gsi);
+ }
+ }
+ }
+ }
+ }
- if (!gsi_end_p (gsi) && is_gimple_assign (next_stmt))
+ if (next_stmt && is_gimple_assign (next_stmt))
{
tree lhs = gimple_assign_lhs (next_stmt);
tree_code code = TREE_CODE (lhs);
