Re: [PATCH] Fix pdftex miscompilation due to get_range_strlen (PR tree-optimization/84478)

[Martin Sebor <msebor at gmail dot com>]

On 02/20/2018 05:14 PM, Jakub Jelinek wrote:
> On Tue, Feb 20, 2018 at 04:59:12PM -0700, Martin Sebor wrote:
> > > It would help if you explained why you think it is a good idea
> > > ignoring the other phi arguments if you have one (or more) where you can
> > > determine length.
> >
> > It's a heuristic that was meant just for the -Wformat-overflow
> > warning.  When making decisions that affect code generation it's
> > obviously not correct to ignore the possibility that unknown
> > arguments may be shorter than the minimum or longer than
> > the maximum.  The fuzzy argument was meant to differentiate
> > between two got but I forgot about it when I added the fix
> > for PR 83671.
>
> get_range_strlen (the 2 argument one) is right now called:
> 3 times from builtins.c for -Wstringop-overflow
> once from gimple-ssa-sprintf.c for -Wformat-overflow
> once from tree-ssa-strlen.c for -Wstringop-truncation

Right.  The warnings depend on the heuristic and on using
a non-zero lower bound when some of the strings is unknown
or has unknown length but other don't.

> once from gimple-fold.c for gimple_fold_builtin_strlen value ranges

Right.  This depends on it too, except it needs to avoid
using a nonzero lower bound when any of the strings is
unknown/has unknown length.  Let's fix that.

> So, which of these do you want the heuristics for?  All 3 warnings,
> just one of them, something else?  In the 2 patches I've posted last
> all the 3 different warnings are treated the same (fuzzy == 2).

All of them.  The bug is in how strlen is folded.  There is
nothing wrong with the warnings, so fix the bug and leave
the warnings alone.  My second patch did that and it passed
all tests.  (I forgot to include the change to
gimple-ssa-sprintf.c but it was the same as the one in
tree-ssa-strlen.c:  call get_range_strlen with true as
the third argument. Attached is the same patch with that
bit added.)

> Looking at strlenopt-40.c testcase, in the test you clearly rely on
> fuzzy argument being set for the value ranges case
> (gimple_fold_builtin_strlen), otherwise many of the
> subtests would fail.

Right, that needs to continue to work.  It does with my patch.
If your solution regresses any of the tests then use the second
patch I posted.

What am I missing?

Martin

PR tree-optimization/84478 - [8 Regression] pdftex miscompilation on i386

gcc/ChangeLog:

	PR tree-optimization/84478
	* gimple-fold.h (get_range_strlen): Add argument.
	* gimple-fold.c (get_range_strlen): Set *MINLEN to zero.
	(get_range_strlen): Reset range on failure.
	* gimple-ssa-sprintf.c (get_string_length): Add third argument
	to the call to get_range_strlen.
	* tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Same.

gcc/testsuite/ChangeLog:

	PR tree-optimization/84478
	* gcc.c-torture/execute/pr84478.c: New test.

Index: gcc/gimple-fold.c
===================================================================
--- gcc/gimple-fold.c	(revision 257859)
+++ gcc/gimple-fold.c	(working copy)
@@ -1290,6 +1290,9 @@ gimple_fold_builtin_memset (gimple_stmt_iterator *
    When FUZZY is set and the length of a string cannot be determined,
    the function instead considers as the maximum possible length the
    size of a character array it may refer to.
+   When STRICT_MIN is set the function will clear MINMAXLEN[0] if
+   the length of any of the referenced strings cannot be determined
+   (and thus can be zero).
    Set *FLEXP to true if the range of the string lengths has been
    obtained from the upper bound of an array at the end of a struct.
    Such an array may hold a string that's longer than its upper bound
@@ -1297,7 +1300,7 @@ gimple_fold_builtin_memset (gimple_stmt_iterator *
 
 static bool
 get_range_strlen (tree arg, tree length[2], bitmap *visited, int type,
-		  bool fuzzy, bool *flexp)
+		  bool fuzzy, bool strict_min, bool *flexp)
 {
   tree var, val = NULL_TREE;
   gimple *def_stmt;
@@ -1320,7 +1323,8 @@ get_range_strlen (tree arg, tree length[2], bitmap
 	      if (TREE_CODE (aop0) == INDIRECT_REF
 		  && TREE_CODE (TREE_OPERAND (aop0, 0)) == SSA_NAME)
 		return get_range_strlen (TREE_OPERAND (aop0, 0),
-					 length, visited, type, fuzzy, flexp);
+					 length, visited, type, fuzzy,
+					 strict_min, flexp);
 	    }
 	  else if (TREE_CODE (TREE_OPERAND (op, 0)) == COMPONENT_REF && fuzzy)
 	    {
@@ -1354,7 +1358,7 @@ get_range_strlen (tree arg, tree length[2], bitmap
 	{
 	  if (TREE_CODE (arg) == ADDR_EXPR)
 	    return get_range_strlen (TREE_OPERAND (arg, 0), length,
-				     visited, type, fuzzy, flexp);
+				     visited, type, fuzzy, strict_min, flexp);
 
 	  if (TREE_CODE (arg) == ARRAY_REF)
 	    {
@@ -1497,14 +1501,17 @@ get_range_strlen (tree arg, tree length[2], bitmap
             || gimple_assign_unary_nop_p (def_stmt))
           {
             tree rhs = gimple_assign_rhs1 (def_stmt);
-	    return get_range_strlen (rhs, length, visited, type, fuzzy, flexp);
+	    return get_range_strlen (rhs, length, visited, type, fuzzy,
+				     strict_min, flexp);
           }
 	else if (gimple_assign_rhs_code (def_stmt) == COND_EXPR)
 	  {
 	    tree op2 = gimple_assign_rhs2 (def_stmt);
 	    tree op3 = gimple_assign_rhs3 (def_stmt);
-	    return get_range_strlen (op2, length, visited, type, fuzzy, flexp)
-	      && get_range_strlen (op3, length, visited, type, fuzzy, flexp);
+	    return (get_range_strlen (op2, length, visited, type, fuzzy,
+				      strict_min, flexp)
+		    && get_range_strlen (op3, length, visited, type, fuzzy,
+					 strict_min, flexp));
 	  }
         return false;
 
@@ -1527,10 +1534,15 @@ get_range_strlen (tree arg, tree length[2], bitmap
             if (arg == gimple_phi_result (def_stmt))
               continue;
 
-	    if (!get_range_strlen (arg, length, visited, type, fuzzy, flexp))
+	    if (!get_range_strlen (arg, length, visited, type, fuzzy,
+				   strict_min, flexp))
 	      {
 		if (fuzzy)
-		  *maxlen = build_all_ones_cst (size_type_node);
+		  {
+		    if (strict_min)
+		      *minlen = ssize_int (0);
+		    *maxlen = build_all_ones_cst (size_type_node);
+		  }
 		else
 		  return false;
 	      }
@@ -1551,6 +1563,9 @@ get_range_strlen (tree arg, tree length[2], bitmap
    and array declared as 'char array[8]', MINMAXLEN[0] will be set
    to 3 and MINMAXLEN[1] to 7, the longest string that could be
    stored in array.
+   When STRICT_MIN is set the function will clear MINMAXLEN[0] if
+   the length of any of the referenced strings cannot be determined
+   (and thus can be zero).
    Return true if the range of the string lengths has been obtained
    from the upper bound of an array at the end of a struct.  Such
    an array may hold a string that's longer than its upper bound
@@ -1557,7 +1572,7 @@ get_range_strlen (tree arg, tree length[2], bitmap
    due to it being used as a poor-man's flexible array member.  */
 
 bool
-get_range_strlen (tree arg, tree minmaxlen[2])
+get_range_strlen (tree arg, tree minmaxlen[2], bool strict_min /* = true */)
 {
   bitmap visited = NULL;
 
@@ -1565,7 +1580,12 @@ bool
   minmaxlen[1] = NULL_TREE;
 
   bool flexarray = false;
-  get_range_strlen (arg, minmaxlen, &visited, 1, true, &flexarray);
+  if (!get_range_strlen (arg, minmaxlen, &visited, 1, true, strict_min,
+			 &flexarray))
+    {
+      minmaxlen[0] = NULL_TREE;
+      minmaxlen[1] = NULL_TREE;
+    }
 
   if (visited)
     BITMAP_FREE (visited);
@@ -1580,7 +1600,7 @@ get_maxval_strlen (tree arg, int type)
   tree len[2] = { NULL_TREE, NULL_TREE };
 
   bool dummy;
-  if (!get_range_strlen (arg, len, &visited, type, false, &dummy))
+  if (!get_range_strlen (arg, len, &visited, type, false, true, &dummy))
     len[1] = NULL_TREE;
   if (visited)
     BITMAP_FREE (visited);
Index: gcc/gimple-fold.h
===================================================================
--- gcc/gimple-fold.h	(revision 257859)
+++ gcc/gimple-fold.h	(working copy)
@@ -25,7 +25,7 @@ along with GCC; see the file COPYING3.  If not see
 extern tree create_tmp_reg_or_ssa_name (tree, gimple *stmt = NULL);
 extern tree canonicalize_constructor_val (tree, tree);
 extern tree get_symbol_constant_value (tree);
-extern bool get_range_strlen (tree, tree[2]);
+extern bool get_range_strlen (tree, tree[2], bool = true);
 extern tree get_maxval_strlen (tree, int);
 extern void gimplify_and_update_call_from_tree (gimple_stmt_iterator *, tree);
 extern bool fold_stmt (gimple_stmt_iterator *);
Index: gcc/gimple-ssa-sprintf.c
===================================================================
--- gcc/gimple-ssa-sprintf.c	(revision 257859)
+++ gcc/gimple-ssa-sprintf.c	(working copy)
@@ -2075,7 +2075,7 @@ get_string_length (tree str)
      aren't known to point any such arrays result in LENRANGE[1] set
      to SIZE_MAX.  */
   tree lenrange[2];
-  bool flexarray = get_range_strlen (str, lenrange);
+  bool flexarray = get_range_strlen (str, lenrange, true);
 
   if (lenrange [0] || lenrange [1])
     {