[PATCH] consider successor blocks when avoiding -Wstringop-truncation (PR 84468)

[PATCH] consider successor blocks when avoiding -Wstringop-truncation (PR 84468)

[Martin Sebor]

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

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.

Tested on x86_64-linux.

Martin

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

PR tree-optimization/84468 - bogus -Wstringop-truncation despite assignment after conditional strncpy

gcc/testsuite/ChangeLog:

	PR tree-optimization/84468
	* gcc.dg/Wstringop-truncation.c: New test.

gcc/ChangeLog:

	PR tree-optimization/84468
	* tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Also consider successor
	basic blocks.

Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c	(revision 257796)
+++ gcc/tree-ssa-strlen.c	(working copy)
@@ -1851,8 +1851,18 @@ 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.  */
+      basic_block bb = gimple_bb (stmt);
+      basic_block nextbb
+	= EDGE_COUNT (bb->succs) ? EDGE_SUCC (bb, 0)->dest : NULL;
+      gimple_stmt_iterator it = gsi_start_bb (nextbb);
+      next_stmt = gsi_stmt (it);
+    }
 
-  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);
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,116 @@
+/* PR tree-optimization/84468 - Inconsistent -Wstringop-truncation warnings
+   with -O2
+   { dg-do compile }
+   { dg-options "-O2 -Wstringop-truncation -ftrack-macro-expansion=0" }  */
+
+#define strncpy __builtin_strncpy
+
+struct A
+{
+  char a[4];
+};
+
+void no_pred_succ (struct A *p, const struct A *q)
+{
+  strncpy (p->a, q->a, sizeof p->a - 1);      /* { dg-warning "\\\[-Wstringop-truncation" } */
+}
+
+
+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" } */
+}
+
+
+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;
+}
+
+
+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;
+}

[PING] [PATCH] consider successor blocks when avoiding -Wstringop-truncation (PR 84468)

[Martin Sebor]

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

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.
>
> Tested on x86_64-linux.
>
> Martin


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

PR tree-optimization/84468 - bogus -Wstringop-truncation despite assignment after conditional strncpy

gcc/testsuite/ChangeLog:

	PR tree-optimization/84468
	* gcc.dg/Wstringop-truncation.c: New test.

gcc/ChangeLog:

	PR tree-optimization/84468
	* tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Also consider successor
	basic blocks.

Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c	(revision 257963)
+++ gcc/tree-ssa-strlen.c	(working copy)
@@ -1856,8 +1856,20 @@ 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))
+	{
+	  basic_block nextbb
+	    = EDGE_COUNT (bb->succs) ? EDGE_SUCC (bb, 0)->dest : NULL;
+	  gimple_stmt_iterator it = gsi_start_bb (nextbb);
+	  next_stmt = gsi_stmt (it);
+	}
+    }
 
-  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);
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,132 @@
+/* PR tree-optimization/84468 - Inconsistent -Wstringop-truncation warnings
+   with -O2
+   { dg-do compile }
+   { dg-options "-O2 -Wstringop-truncation -ftrack-macro-expansion=0" }  */
+
+#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;
+}

Re: [PING] [PATCH] consider successor blocks when avoiding -Wstringop-truncation (PR 84468)

[Jeff Law]

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.

Jeff

Re: [PING] [PATCH] consider successor blocks when avoiding -Wstringop-truncation (PR 84468)

[Martin Sebor]

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

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

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

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;
+}

Re: [PING] [PATCH] consider successor blocks when avoiding -Wstringop-truncation (PR 84468)

[Jeff Law]

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 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).

jeff

Re: [PING] [PATCH] consider successor blocks when avoiding -Wstringop-truncation (PR 84468)

[Martin Sebor]

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

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%]

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

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);

Re: [PING] [PATCH] consider successor blocks when avoiding -Wstringop-truncation (PR 84468)

[Jeff Law]

On 03/01/2018 02:17 PM, Martin Sebor wrote:
> 
> I read you last reply as asking me to handle multiple edges.
Sorry, I should have been clearer.

You need to be prepared for the possibility of multiple edges and handle
them in a conservatively correct way.  The most likely way to get
multiple outgoing edges is going to be via exception handling.

In this code I think that's easily accomplished by making the code which
walks into the successor block(s) conditional on single_succ_p (bb) and
that the edge is not marked as EDGE_ABNORMAL.

> The original patch handled just one edge and didn't bother
> with EDGE_ABNORMAL because I wanted to keep it simple.
Understood.  But that's not safe in the sense that once you have
multiple successors, you don't know which one you will transfer control
to -- thus you have to check all of them for a suitable store.  If any
doesn't have a suitable store, then you'd have to warn.

Essentially the question you need to answer is "given I'm in block X, do
all paths from block X have a suitable store to terminate the string
prior to a use".  You can handle that in the trivial case with the code
you're proposing in this patch, and that's fine for gcc-8.

But the "right" way to answer that question is to look at the virtual
operand chains.  Though as we've discussed that may not necessarily be a
good thing.





  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). 
Correct.  I think your original patch with a check for single_succ_p is
the direction I think we want for gcc-8.



 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've actually got a fair amount of data here on kernel builds using the
trunk on a variety of architectures.  I haven't gone through it all yet,
but there's certainly some string-op truncation warnings and a few
others.  I haven't figured out how best to aggregate that info and I
don't want to duplicate Arnd's work.




> 
> 
>> 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?
EH is the best bet to create them.  You might also be able to get one if
we've got fake edges in the CFG (necessary for things like
post-dominator computation).  async exceptions could likely create them
fairly easily.


My comment was more about not seeing the need to look into successor
blocks if there's more than one.


Can you file a bug report on the false positive so that we have a
reminder to revisit looking at the virtual operand chains as a better
solution here?

I think this version is OK.

Thanks, and again sorry for the confusion.


Jeff