[PATCH] Cleanup strcpy/stpcpy no nul warning code

[PATCH] Cleanup strcpy/stpcpy no nul warning code

[Bernd Edlinger]

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

Hi,

this is a cleanup of the recently added strlen/strcpy/stpcpy
no nul warning code.

Most importantly it moves the SSA_NAME handling from
unterminated_array to string_constant, thereby fixing
another round of xfails in the strlen and stpcpy test cases.

I need to say that the fix for bug 86622 is relying in
type info on the pointer which is probably not safe in
GIMPLE in the light of the recent discussion.

I had to add two further exceptions, which should
be safe in general: that is a pointer arithmentic on a string
literal is okay, and arithmetic on a string constant
that is exactly the size of the whole DECL, cannot
access an adjacent member.


Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-cleanup-no-nul.diff --]
[-- Type: text/x-patch; name="patch-cleanup-no-nul.diff", Size: 12368 bytes --]

gcc:
2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* builtins.h (unterminated_array): Remove prototype.
        * builtins.c (unterminated_array): Simplify.  Make static.
        (expand_builtin_strcpy_args): Don't use TREE_NO_WARNING here.
	(expand_builtin_stpcpy_1): Remove warn_string_no_nul without effect.
	* expr.c (string_constant): Handle SSA_NAME.  Add more exceptions
	where pointer arithmetic is safe.
	* gimple-fold.c (get_range_strlen): Handle nonstr like in c_strlen.
	(get_max_strlen): Remove the unnecessary mynonstr handling.
	(gimple_fold_builtin_strcpy): Simplify.
	(gimple_fold_builtin_stpcpy): Simplify.
	(gimple_fold_builtin_sprintf): Remove NO_WARNING propagation
	without effect.
	(gimple_fold_builtin_strlen): Simplify.

testsuite:
2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* gcc.dg/warn-stpcpy-no-nul.c: Remove xfails.
	* gcc.dg/warn-strlen-no-nul.c: Remove xfails.

Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c	(revision 264342)
+++ gcc/builtins.c	(working copy)
@@ -567,31 +567,12 @@ warn_string_no_nul (location_t loc, const char *fn
    the declaration of the object of which the array is a member or
    element.  Otherwise return null.  */
 
-tree
+static tree
 unterminated_array (tree exp)
 {
-  if (TREE_CODE (exp) == SSA_NAME)
-    {
-      gimple *stmt = SSA_NAME_DEF_STMT (exp);
-      if (!is_gimple_assign (stmt))
-	return NULL_TREE;
-
-      tree rhs1 = gimple_assign_rhs1 (stmt);
-      tree_code code = gimple_assign_rhs_code (stmt);
-      if (code == ADDR_EXPR
-	  && TREE_CODE (TREE_OPERAND (rhs1, 0)) == ARRAY_REF)
-	rhs1 = rhs1;
-      else if (code != POINTER_PLUS_EXPR)
-	return NULL_TREE;
-
-      exp = rhs1;
-    }
-
   tree nonstr = NULL;
-  if (c_strlen (exp, 1, &nonstr, 1) == NULL && nonstr)
-    return nonstr;
-
-  return NULL_TREE;
+  c_strlen (exp, 1, &nonstr);
+  return nonstr;
 }
 
 /* Compute the length of a null-terminated character string or wide
@@ -3936,8 +3917,7 @@ expand_builtin_strcpy_args (tree exp, tree dest, t
   if (tree nonstr = unterminated_array (src))
     {
       /* NONSTR refers to the non-nul terminated constant array.  */
-      if (!TREE_NO_WARNING (exp))
-	warn_string_no_nul (EXPR_LOCATION (exp), "strcpy", src, nonstr);
+      warn_string_no_nul (EXPR_LOCATION (exp), "strcpy", src, nonstr);
       return NULL_RTX;
     }
 
@@ -3984,14 +3964,10 @@ expand_builtin_stpcpy_1 (tree exp, rtx target, mac
 	 compile-time, not an expression containing a string.  This is
 	 because the latter will potentially produce pessimized code
 	 when used to produce the return value.  */
-      tree nonstr = NULL_TREE;
       if (!c_getstr (src, NULL)
-	  || !(len = c_strlen (src, 0, &nonstr, 1)))
+	  || !(len = c_strlen (src, 0)))
 	return expand_movstr (dst, src, target, /*endp=*/2);
 
-      if (nonstr && !TREE_NO_WARNING (exp))
-	warn_string_no_nul (EXPR_LOCATION (exp), "stpcpy", src, nonstr);
-
       lenp1 = size_binop_loc (loc, PLUS_EXPR, len, ssize_int (1));
       ret = expand_builtin_mempcpy_args (dst, src, lenp1,
 					 target, exp, /*endp=*/2);
Index: gcc/builtins.h
===================================================================
--- gcc/builtins.h	(revision 264342)
+++ gcc/builtins.h	(working copy)
@@ -104,7 +104,6 @@ extern internal_fn associated_internal_fn (tree);
 extern internal_fn replacement_internal_fn (gcall *);
 
 extern void warn_string_no_nul (location_t, const char *, tree, tree);
-extern tree unterminated_array (tree);
 extern tree max_object_size ();
 
 #endif /* GCC_BUILTINS_H */
Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 264342)
+++ gcc/expr.c	(working copy)
@@ -11372,7 +11372,10 @@ string_constant (tree arg, tree *ptr_offset, tree
 	  /* Avoid pointers to arrays (see bug 86622).  */
 	  if (POINTER_TYPE_P (TREE_TYPE (arg))
 	      && TREE_CODE (TREE_TYPE (TREE_TYPE (arg))) == ARRAY_TYPE
-	      && TREE_CODE (TREE_OPERAND (arg0, 0)) == ARRAY_REF)
+	      && !(decl && !*decl)
+	      && !(decl && tree_fits_uhwi_p (DECL_SIZE_UNIT (*decl))
+		   && mem_size && tree_fits_uhwi_p (*mem_size)
+		   && tree_int_cst_equal (*mem_size, DECL_SIZE_UNIT (*decl))))
 	    return NULL_TREE;
 
 	  tree type = TREE_TYPE (arg1);
@@ -11381,6 +11384,38 @@ string_constant (tree arg, tree *ptr_offset, tree
 	}
       return NULL_TREE;
     }
+  else if (TREE_CODE (arg) == SSA_NAME)
+    {
+      gimple *stmt = SSA_NAME_DEF_STMT (arg);
+      if (!is_gimple_assign (stmt))
+	return NULL_TREE;
+
+      tree rhs1 = gimple_assign_rhs1 (stmt);
+      tree_code code = gimple_assign_rhs_code (stmt);
+      if (code == ADDR_EXPR)
+	return string_constant (rhs1, ptr_offset, mem_size, decl);
+      else if (code != POINTER_PLUS_EXPR)
+	return NULL_TREE;
+
+      tree offset;
+      if (tree str = string_constant (rhs1, &offset, mem_size, decl))
+	{
+	  /* Avoid pointers to arrays (see bug 86622).  */
+	  if (POINTER_TYPE_P (TREE_TYPE (rhs1))
+	      && TREE_CODE (TREE_TYPE (TREE_TYPE (rhs1))) == ARRAY_TYPE
+	      && !(decl && !*decl)
+	      && !(decl && tree_fits_uhwi_p (DECL_SIZE_UNIT (*decl))
+		   && mem_size && tree_fits_uhwi_p (*mem_size)
+		   && tree_int_cst_equal (*mem_size, DECL_SIZE_UNIT (*decl))))
+	    return NULL_TREE;
+
+	  tree rhs2 = gimple_assign_rhs2 (stmt);
+	  tree type = TREE_TYPE (rhs2);
+	  *ptr_offset = fold_build2 (PLUS_EXPR, type, offset, rhs2);
+	  return str;
+	}
+      return NULL_TREE;
+    }
   else if (DECL_P (arg))
     array = arg;
   else
Index: gcc/gimple-fold.c
===================================================================
--- gcc/gimple-fold.c	(revision 264342)
+++ gcc/gimple-fold.c	(working copy)
@@ -1573,11 +1573,6 @@ get_range_strlen (tree arg, tree minmaxlen[2], uns
   minmaxlen[0] = NULL_TREE;
   minmaxlen[1] = NULL_TREE;
 
-  tree nonstrbuf;
-  if (!nonstr)
-    nonstr = &nonstrbuf;
-  *nonstr = NULL_TREE;
-
   bool flexarray = false;
   if (!get_range_strlen (arg, minmaxlen, &visited, 1, strict ? 1 : 2,
 			 &flexarray, eltsize, nonstr))
@@ -1607,24 +1602,12 @@ get_maxval_strlen (tree arg, int type, tree *nonst
   tree len[2] = { NULL_TREE, NULL_TREE };
 
   bool dummy;
-  /* Set to non-null if ARG refers to an untermianted array.  */
-  tree mynonstr = NULL_TREE;
-  if (!get_range_strlen (arg, len, &visited, type, 0, &dummy, 1, &mynonstr))
+  if (!get_range_strlen (arg, len, &visited, type, 0, &dummy, 1, nonstr))
     len[1] = NULL_TREE;
   if (visited)
     BITMAP_FREE (visited);
 
-  if (nonstr)
-    {
-      /* For callers prepared to handle unterminated arrays set
-	 *NONSTR to point to the declaration of the array and return
-	 the maximum length/size. */
-      *nonstr = mynonstr;
-      return len[1];
-    }
-
-  /* Fail if the constant array isn't nul-terminated.  */
-  return mynonstr ? NULL_TREE : len[1];
+  return len[1];
 }
 
 
@@ -1672,13 +1655,7 @@ gimple_fold_builtin_strcpy (gimple_stmt_iterator *
   tree len = get_maxval_strlen (src, 0, &nonstr);
 
   if (nonstr)
-    {
-      /* Avoid folding calls with unterminated arrays.  */
-      if (!gimple_no_warning_p (stmt))
-	warn_string_no_nul (loc, "strcpy", src, nonstr);
-      gimple_set_no_warning (stmt, true);
-      return false;
-    }
+    warn_string_no_nul (loc, "strcpy", src, nonstr);
 
   if (!len)
     return false;
@@ -2813,24 +2790,15 @@ gimple_fold_builtin_stpcpy (gimple_stmt_iterator *
 
   /* Set to non-null if ARG refers to an unterminated array.  */
   tree nonstr = NULL;
-  tree len = c_strlen (src, 1, &nonstr, 1);
+  tree len = c_strlen (src, 1, &nonstr);
+
+  if (nonstr)
+    warn_string_no_nul (loc, "stpcpy", src, nonstr);
+
   if (!len
       || TREE_CODE (len) != INTEGER_CST)
-    {
-      nonstr = unterminated_array (src);
-      if (!nonstr)
-	return false;
-    }
+    return false;
 
-  if (nonstr)
-    {
-      /* Avoid folding calls with unterminated arrays.  */
-      if (!gimple_no_warning_p (stmt))
-	warn_string_no_nul (loc, "stpcpy", src, nonstr);
-      gimple_set_no_warning (stmt, true);
-      return false;
-    }
-
   if (optimize_function_for_size_p (cfun)
       /* If length is zero it's small enough.  */
       && !integer_zerop (len))
@@ -3093,11 +3061,6 @@ gimple_fold_builtin_sprintf (gimple_stmt_iterator
       gimple_seq stmts = NULL;
       gimple *repl = gimple_build_call (fn, 2, dest, fmt);
 
-      /* Propagate the NO_WARNING bit to avoid issuing the same
-	 warning more than once.  */
-      if (gimple_no_warning_p (stmt))
-	gimple_set_no_warning (repl, true);
-
       gimple_seq_add_stmt_without_update (&stmts, repl);
       if (gimple_call_lhs (stmt))
 	{
@@ -3147,11 +3110,6 @@ gimple_fold_builtin_sprintf (gimple_stmt_iterator
       gimple_seq stmts = NULL;
       gimple *repl = gimple_build_call (fn, 2, dest, orig);
 
-      /* Propagate the NO_WARNING bit to avoid issuing the same
-	 warning more than once.  */
-      if (gimple_no_warning_p (stmt))
-	gimple_set_no_warning (repl, true);
-
       gimple_seq_add_stmt_without_update (&stmts, repl);
       if (gimple_call_lhs (stmt))
 	{
@@ -3582,10 +3540,8 @@ gimple_fold_builtin_strlen (gimple_stmt_iterator *
   wide_int minlen;
   wide_int maxlen;
 
-  /* Set to non-null if ARG refers to an unterminated array.  */
-  tree nonstr;
   tree lenrange[2];
-  if (!get_range_strlen (arg, lenrange, 1, true, &nonstr)
+  if (!get_range_strlen (arg, lenrange, 1, true)
       && lenrange[0] && TREE_CODE (lenrange[0]) == INTEGER_CST
       && lenrange[1] && TREE_CODE (lenrange[1]) == INTEGER_CST)
     {
Index: gcc/testsuite/gcc.dg/warn-stpcpy-no-nul.c
===================================================================
--- gcc/testsuite/gcc.dg/warn-stpcpy-no-nul.c	(revision 264342)
+++ gcc/testsuite/gcc.dg/warn-stpcpy-no-nul.c	(working copy)
@@ -71,13 +71,13 @@ void test_two_dim_array (char *d)
   T (&b[3][1] + 1);     /* { dg-warning "nul" }  */
   T (&b[3][v0]);        /* { dg-warning "nul" }  */
   T (&b[3][1] + v0);    /* { dg-warning "nul" }  */
-  T (&b[3][v0] + v1);   /* { dg-warning "nul" "bug ???" { xfail *-*-* } }  */
+  T (&b[3][v0] + v1);   /* { dg-warning "nul" }  */
 
   T (&b[i3][i1]);       /* { dg-warning "nul" }  */
   T (&b[i3][i1] + i1);  /* { dg-warning "nul" }  */
   T (&b[i3][v0]);       /* { dg-warning "nul" }  */
   T (&b[i3][i1] + v0);  /* { dg-warning "nul" }  */
-  T (&b[i3][v0] + v1);  /* { dg-warning "nul" "bug ???" { xfail *-*-* } }  */
+  T (&b[i3][v0] + v1);  /* { dg-warning "nul" }  */
 
   T (v0 ? "" : b[0]);
   T (v0 ? "" : b[1]);
Index: gcc/testsuite/gcc.dg/warn-strlen-no-nul.c
===================================================================
--- gcc/testsuite/gcc.dg/warn-strlen-no-nul.c	(revision 264342)
+++ gcc/testsuite/gcc.dg/warn-strlen-no-nul.c	(working copy)
@@ -71,9 +71,9 @@ T (&b[3][v0] + v1);     /* { dg-warning "nul" }  *
 T (&b[i3][i1]);         /* { dg-warning "nul" }  */
 T (&b[i3][i1] + 1);     /* { dg-warning "nul" }  */
 T (&b[i3][i1] + i1);    /* { dg-warning "nul" }  */
-T (&b[i3][v0]);         /* { dg-warning "nul" "pr86919" { xfail *-*-* } }  */
-T (&b[i3][i1] + v0);    /* { dg-warning "nul" "pr86919" { xfail *-*-* } }  */
-T (&b[i3][v0] + v1);    /* { dg-warning "nul" "pr86919" { xfail *-*-* } }  */
+T (&b[i3][v0]);         /* { dg-warning "nul" }  */
+T (&b[i3][i1] + v0);    /* { dg-warning "nul" }  */
+T (&b[i3][v0] + v1);    /* { dg-warning "nul" }  */
 
 T (v0 ? "" : b[0]);
 T (v0 ? "" : b[1]);
@@ -152,10 +152,10 @@ T (&s.b[1] + v0);     /* { dg-warning "nul" }  */
 
 T (&s.b[i0]);         /* { dg-warning "nul" }  */
 T (&s.b[i0] + i1);    /* { dg-warning "nul" }  */
-T (&s.b[i0] + v0);    /* { dg-warning "nul" "pr86919" { xfail *-*-* } }  */
+T (&s.b[i0] + v0);    /* { dg-warning "nul" }  */
 T (&s.b[i1]);         /* { dg-warning "nul" }  */
 T (&s.b[i1] + i1);    /* { dg-warning "nul" }  */
-T (&s.b[i1] + v0);    /* { dg-warning "nul" "pr86919" { xfail *-*-* } }  */
+T (&s.b[i1] + v0);    /* { dg-warning "nul" }  */
 
 struct B { struct A a[2]; };
 

Re: [PATCH] Cleanup strcpy/stpcpy no nul warning code

[Jeff Law]

On 9/16/18 1:58 PM, Bernd Edlinger wrote:
> Hi,
> 
> this is a cleanup of the recently added strlen/strcpy/stpcpy
> no nul warning code.
> 
> Most importantly it moves the SSA_NAME handling from
> unterminated_array to string_constant, thereby fixing
> another round of xfails in the strlen and stpcpy test cases.
> 
> I need to say that the fix for bug 86622 is relying in
> type info on the pointer which is probably not safe in
> GIMPLE in the light of the recent discussion.
> 
> I had to add two further exceptions, which should
> be safe in general: that is a pointer arithmentic on a string
> literal is okay, and arithmetic on a string constant
> that is exactly the size of the whole DECL, cannot
> access an adjacent member.
> 
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
> 
> 
> Thanks
> Bernd.
> 
> 
> patch-cleanup-no-nul.diff
> 
> gcc:
> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* builtins.h (unterminated_array): Remove prototype.
>         * builtins.c (unterminated_array): Simplify.  Make static.
>         (expand_builtin_strcpy_args): Don't use TREE_NO_WARNING here.
> 	(expand_builtin_stpcpy_1): Remove warn_string_no_nul without effect.
> 	* expr.c (string_constant): Handle SSA_NAME.  Add more exceptions
> 	where pointer arithmetic is safe.
> 	* gimple-fold.c (get_range_strlen): Handle nonstr like in c_strlen.
> 	(get_max_strlen): Remove the unnecessary mynonstr handling.
> 	(gimple_fold_builtin_strcpy): Simplify.
> 	(gimple_fold_builtin_stpcpy): Simplify.
> 	(gimple_fold_builtin_sprintf): Remove NO_WARNING propagation
> 	without effect.
> 	(gimple_fold_builtin_strlen): Simplify.
> 
> testsuite:
> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* gcc.dg/warn-stpcpy-no-nul.c: Remove xfails.
> 	* gcc.dg/warn-strlen-no-nul.c: Remove xfails.
> 
> Index: gcc/builtins.c
> ===================================================================
> --- gcc/builtins.c	(revision 264342)
> +++ gcc/builtins.c	(working copy)
> @@ -567,31 +567,12 @@ warn_string_no_nul (location_t loc, const char *fn
>     the declaration of the object of which the array is a member or
>     element.  Otherwise return null.  */
>  
> -tree
> +static tree
>  unterminated_array (tree exp)
>  {
> -  if (TREE_CODE (exp) == SSA_NAME)
> -    {
> -      gimple *stmt = SSA_NAME_DEF_STMT (exp);
> -      if (!is_gimple_assign (stmt))
> -	return NULL_TREE;
> -
> -      tree rhs1 = gimple_assign_rhs1 (stmt);
> -      tree_code code = gimple_assign_rhs_code (stmt);
> -      if (code == ADDR_EXPR
> -	  && TREE_CODE (TREE_OPERAND (rhs1, 0)) == ARRAY_REF)
> -	rhs1 = rhs1;
> -      else if (code != POINTER_PLUS_EXPR)
> -	return NULL_TREE;
> -
> -      exp = rhs1;
> -    }
> -
>    tree nonstr = NULL;
> -  if (c_strlen (exp, 1, &nonstr, 1) == NULL && nonstr)
> -    return nonstr;
> -
> -  return NULL_TREE;
> +  c_strlen (exp, 1, &nonstr);
> +  return nonstr;
>  }
Sigh.  This is going to conflict with patch #6 from Martin's kit.



>  
>  /* Compute the length of a null-terminated character string or wide
> @@ -3936,8 +3917,7 @@ expand_builtin_strcpy_args (tree exp, tree dest, t
>    if (tree nonstr = unterminated_array (src))
>      {
>        /* NONSTR refers to the non-nul terminated constant array.  */
> -      if (!TREE_NO_WARNING (exp))
> -	warn_string_no_nul (EXPR_LOCATION (exp), "strcpy", src, nonstr);
> +      warn_string_no_nul (EXPR_LOCATION (exp), "strcpy", src, nonstr);
>        return NULL_RTX;
>      }
There's also some target dependencies to be concerned about.  I'll have
to dig out the thread, but in general removing these TREE_NO_WARNING
checks is probably a bad idea -- you're likely introducing redundant
warnings on one or more targets (but not x86).


There may be things in there we want to take.  But my focus is going to
be on getting the #4 and #6 patches from Martin's kit resolved (while he
works on the string length range stuff).

Jeff

Re: [PATCH] Cleanup strcpy/stpcpy no nul warning code

[Bernd Edlinger]

On 09/17/18 19:33, Jeff Law wrote:
> On 9/16/18 1:58 PM, Bernd Edlinger wrote:
>> Hi,
>>
>> this is a cleanup of the recently added strlen/strcpy/stpcpy
>> no nul warning code.
>>
>> Most importantly it moves the SSA_NAME handling from
>> unterminated_array to string_constant, thereby fixing
>> another round of xfails in the strlen and stpcpy test cases.
>>
>> I need to say that the fix for bug 86622 is relying in
>> type info on the pointer which is probably not safe in
>> GIMPLE in the light of the recent discussion.
>>
>> I had to add two further exceptions, which should
>> be safe in general: that is a pointer arithmentic on a string
>> literal is okay, and arithmetic on a string constant
>> that is exactly the size of the whole DECL, cannot
>> access an adjacent member.
>>
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
>>
>>
>> Thanks
>> Bernd.
>>
>>
>> patch-cleanup-no-nul.diff
>>
>> gcc:
>> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	* builtins.h (unterminated_array): Remove prototype.
>>          * builtins.c (unterminated_array): Simplify.  Make static.
>>          (expand_builtin_strcpy_args): Don't use TREE_NO_WARNING here.
>> 	(expand_builtin_stpcpy_1): Remove warn_string_no_nul without effect.
>> 	* expr.c (string_constant): Handle SSA_NAME.  Add more exceptions
>> 	where pointer arithmetic is safe.
>> 	* gimple-fold.c (get_range_strlen): Handle nonstr like in c_strlen.
>> 	(get_max_strlen): Remove the unnecessary mynonstr handling.
>> 	(gimple_fold_builtin_strcpy): Simplify.
>> 	(gimple_fold_builtin_stpcpy): Simplify.
>> 	(gimple_fold_builtin_sprintf): Remove NO_WARNING propagation
>> 	without effect.
>> 	(gimple_fold_builtin_strlen): Simplify.
>>
>> testsuite:
>> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	* gcc.dg/warn-stpcpy-no-nul.c: Remove xfails.
>> 	* gcc.dg/warn-strlen-no-nul.c: Remove xfails.
>>
>> Index: gcc/builtins.c
>> ===================================================================
>> --- gcc/builtins.c	(revision 264342)
>> +++ gcc/builtins.c	(working copy)
>> @@ -567,31 +567,12 @@ warn_string_no_nul (location_t loc, const char *fn
>>      the declaration of the object of which the array is a member or
>>      element.  Otherwise return null.  */
>>   
>> -tree
>> +static tree
>>   unterminated_array (tree exp)
>>   {
>> -  if (TREE_CODE (exp) == SSA_NAME)
>> -    {
>> -      gimple *stmt = SSA_NAME_DEF_STMT (exp);
>> -      if (!is_gimple_assign (stmt))
>> -	return NULL_TREE;
>> -
>> -      tree rhs1 = gimple_assign_rhs1 (stmt);
>> -      tree_code code = gimple_assign_rhs_code (stmt);
>> -      if (code == ADDR_EXPR
>> -	  && TREE_CODE (TREE_OPERAND (rhs1, 0)) == ARRAY_REF)
>> -	rhs1 = rhs1;
>> -      else if (code != POINTER_PLUS_EXPR)
>> -	return NULL_TREE;
>> -
>> -      exp = rhs1;
>> -    }
>> -
>>     tree nonstr = NULL;
>> -  if (c_strlen (exp, 1, &nonstr, 1) == NULL && nonstr)
>> -    return nonstr;
>> -
>> -  return NULL_TREE;
>> +  c_strlen (exp, 1, &nonstr);
>> +  return nonstr;
>>   }
> Sigh.  This is going to conflict with patch #6 from Martin's kit.
> 

Sorry, it just feels wrong to do this folding here instead of in
string_constant.

I think the handling of POINTER_PLUS_EXPR above, is faulty,
because it is ignoring the offset parameter, which typically
contains the offset part, may add an offset to a different
structure member or another array index.  That is what happened
in PR 86622.

So you are likely looking at the wrong index, or even the wrong
structure member.


> 
>>   
>>   /* Compute the length of a null-terminated character string or wide
>> @@ -3936,8 +3917,7 @@ expand_builtin_strcpy_args (tree exp, tree dest, t
>>     if (tree nonstr = unterminated_array (src))
>>       {
>>         /* NONSTR refers to the non-nul terminated constant array.  */
>> -      if (!TREE_NO_WARNING (exp))
>> -	warn_string_no_nul (EXPR_LOCATION (exp), "strcpy", src, nonstr);
>> +      warn_string_no_nul (EXPR_LOCATION (exp), "strcpy", src, nonstr);
>>         return NULL_RTX;
>>       }
> There's also some target dependencies to be concerned about.  I'll have
> to dig out the thread, but in general removing these TREE_NO_WARNING
> checks is probably a bad idea -- you're likely introducing redundant
> warnings on one or more targets (but not x86).
> 

I just wondered why use if (!TREE_NO_WARNING(exp)) when warn_string_no_nul
uses if (!TREE_NO_WARNING(src)), and sets TREE_NO_WARNING(src) on success.

> 
> There may be things in there we want to take.  But my focus is going to
> be on getting the #4 and #6 patches from Martin's kit resolved (while he
> works on the string length range stuff).
> 

Oh, you are aware that I have a proposed patch on the string length range
vs. gimple semantics here:

https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00805.html


Bernd.

Re: [PATCH] Cleanup strcpy/stpcpy no nul warning code

[Jeff Law]

On 9/17/18 12:20 PM, Bernd Edlinger wrote:
> On 09/17/18 19:33, Jeff Law wrote:
>> On 9/16/18 1:58 PM, Bernd Edlinger wrote:
>>> Hi,
>>>
>>> this is a cleanup of the recently added strlen/strcpy/stpcpy
>>> no nul warning code.
>>>
>>> Most importantly it moves the SSA_NAME handling from
>>> unterminated_array to string_constant, thereby fixing
>>> another round of xfails in the strlen and stpcpy test cases.
>>>
>>> I need to say that the fix for bug 86622 is relying in
>>> type info on the pointer which is probably not safe in
>>> GIMPLE in the light of the recent discussion.
>>>
>>> I had to add two further exceptions, which should
>>> be safe in general: that is a pointer arithmentic on a string
>>> literal is okay, and arithmetic on a string constant
>>> that is exactly the size of the whole DECL, cannot
>>> access an adjacent member.
>>>
>>>
>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>> Is it OK for trunk?
>>>
>>>
>>> Thanks
>>> Bernd.
>>>
>>>
>>> patch-cleanup-no-nul.diff
>>>
>>> gcc:
>>> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>
>>> 	* builtins.h (unterminated_array): Remove prototype.
>>>          * builtins.c (unterminated_array): Simplify.  Make static.
>>>          (expand_builtin_strcpy_args): Don't use TREE_NO_WARNING here.
>>> 	(expand_builtin_stpcpy_1): Remove warn_string_no_nul without effect.
>>> 	* expr.c (string_constant): Handle SSA_NAME.  Add more exceptions
>>> 	where pointer arithmetic is safe.
>>> 	* gimple-fold.c (get_range_strlen): Handle nonstr like in c_strlen.
>>> 	(get_max_strlen): Remove the unnecessary mynonstr handling.
>>> 	(gimple_fold_builtin_strcpy): Simplify.
>>> 	(gimple_fold_builtin_stpcpy): Simplify.
>>> 	(gimple_fold_builtin_sprintf): Remove NO_WARNING propagation
>>> 	without effect.
>>> 	(gimple_fold_builtin_strlen): Simplify.
>>>
>>> testsuite:
>>> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>
>>> 	* gcc.dg/warn-stpcpy-no-nul.c: Remove xfails.
>>> 	* gcc.dg/warn-strlen-no-nul.c: Remove xfails.
>>>
>>> Index: gcc/builtins.c
>>> ===================================================================
>>> --- gcc/builtins.c	(revision 264342)
>>> +++ gcc/builtins.c	(working copy)
>>> @@ -567,31 +567,12 @@ warn_string_no_nul (location_t loc, const char *fn
>>>      the declaration of the object of which the array is a member or
>>>      element.  Otherwise return null.  */
>>>   
>>> -tree
>>> +static tree
>>>   unterminated_array (tree exp)
>>>   {
>>> -  if (TREE_CODE (exp) == SSA_NAME)
>>> -    {
>>> -      gimple *stmt = SSA_NAME_DEF_STMT (exp);
>>> -      if (!is_gimple_assign (stmt))
>>> -	return NULL_TREE;
>>> -
>>> -      tree rhs1 = gimple_assign_rhs1 (stmt);
>>> -      tree_code code = gimple_assign_rhs_code (stmt);
>>> -      if (code == ADDR_EXPR
>>> -	  && TREE_CODE (TREE_OPERAND (rhs1, 0)) == ARRAY_REF)
>>> -	rhs1 = rhs1;
>>> -      else if (code != POINTER_PLUS_EXPR)
>>> -	return NULL_TREE;
>>> -
>>> -      exp = rhs1;
>>> -    }
>>> -
>>>     tree nonstr = NULL;
>>> -  if (c_strlen (exp, 1, &nonstr, 1) == NULL && nonstr)
>>> -    return nonstr;
>>> -
>>> -  return NULL_TREE;
>>> +  c_strlen (exp, 1, &nonstr);
>>> +  return nonstr;
>>>   }
>> Sigh.  This is going to conflict with patch #6 from Martin's kit.
>>
> 
> Sorry, it just feels wrong to do this folding here instead of in
> string_constant.
> 
> I think the handling of POINTER_PLUS_EXPR above, is faulty,
> because it is ignoring the offset parameter, which typically
> contains the offset part, may add an offset to a different
> structure member or another array index.  That is what happened
> in PR 86622.
> 
> So you are likely looking at the wrong index, or even the wrong
> structure member.
I'm not disagreeing that something's wrong here -- the whole concept
that we extract the rhs and totally ignore the offset seems wrong.  I've
stumbled over it working through issues with either patch #4 or #6 from
Martin.  But I felt I needed to go back and reevaluate any assumptions I
had about how the code was supposed to be used before I called it out.


Your expr.c changes may be worth pushing through independently of the
rest.    AFAICT they're really just exposing more cases where we can
determine that we're working with a stirng constant.

> 
> 
>>
>>>   
>>>   /* Compute the length of a null-terminated character string or wide
>>> @@ -3936,8 +3917,7 @@ expand_builtin_strcpy_args (tree exp, tree dest, t
>>>     if (tree nonstr = unterminated_array (src))
>>>       {
>>>         /* NONSTR refers to the non-nul terminated constant array.  */
>>> -      if (!TREE_NO_WARNING (exp))
>>> -	warn_string_no_nul (EXPR_LOCATION (exp), "strcpy", src, nonstr);
>>> +      warn_string_no_nul (EXPR_LOCATION (exp), "strcpy", src, nonstr);
>>>         return NULL_RTX;
>>>       }
>> There's also some target dependencies to be concerned about.  I'll have
>> to dig out the thread, but in general removing these TREE_NO_WARNING
>> checks is probably a bad idea -- you're likely introducing redundant
>> warnings on one or more targets (but not x86).
>>
> 
> I just wondered why use if (!TREE_NO_WARNING(exp)) when warn_string_no_nul
> uses if (!TREE_NO_WARNING(src)), and sets TREE_NO_WARNING(src) on success.
We may be doing too much here in some cases, but the general idea is to
avoid duplicated warnings on one or more targets.  IIRC it depends on
how they handle the various cases where you lower from one builtin to
another at expansion time.  Hence my comment that we'd need to go back
and look at that discussion before ripping out so much nowarning stuff.
 It's there for a reason (though I'm not convinced we're using it
consistently).


> 
>>
>> There may be things in there we want to take.  But my focus is going to
>> be on getting the #4 and #6 patches from Martin's kit resolved (while he
>> works on the string length range stuff).
>>
> 
> Oh, you are aware that I have a proposed patch on the string length range
> vs. gimple semantics here:
> 
> https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00805.html
Yes,  I'm aware of it.

jeff

Re: [PATCH] Cleanup strcpy/stpcpy no nul warning code

[Bernd Edlinger]

On 09/17/18 20:32, Jeff Law wrote:
> On 9/17/18 12:20 PM, Bernd Edlinger wrote:
>> On 09/17/18 19:33, Jeff Law wrote:
>>> On 9/16/18 1:58 PM, Bernd Edlinger wrote:
>>>> Hi,
>>>>
>>>> this is a cleanup of the recently added strlen/strcpy/stpcpy
>>>> no nul warning code.
>>>>
>>>> Most importantly it moves the SSA_NAME handling from
>>>> unterminated_array to string_constant, thereby fixing
>>>> another round of xfails in the strlen and stpcpy test cases.
>>>>
>>>> I need to say that the fix for bug 86622 is relying in
>>>> type info on the pointer which is probably not safe in
>>>> GIMPLE in the light of the recent discussion.
>>>>
>>>> I had to add two further exceptions, which should
>>>> be safe in general: that is a pointer arithmentic on a string
>>>> literal is okay, and arithmetic on a string constant
>>>> that is exactly the size of the whole DECL, cannot
>>>> access an adjacent member.
>>>>
>>>>
>>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>>> Is it OK for trunk?
>>>>
>>>>
>>>> Thanks
>>>> Bernd.
>>>>
>>>>
>>>> patch-cleanup-no-nul.diff
>>>>
>>>> gcc:
>>>> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>>
>>>> 	* builtins.h (unterminated_array): Remove prototype.
>>>>           * builtins.c (unterminated_array): Simplify.  Make static.
>>>>           (expand_builtin_strcpy_args): Don't use TREE_NO_WARNING here.
>>>> 	(expand_builtin_stpcpy_1): Remove warn_string_no_nul without effect.
>>>> 	* expr.c (string_constant): Handle SSA_NAME.  Add more exceptions
>>>> 	where pointer arithmetic is safe.
>>>> 	* gimple-fold.c (get_range_strlen): Handle nonstr like in c_strlen.
>>>> 	(get_max_strlen): Remove the unnecessary mynonstr handling.
>>>> 	(gimple_fold_builtin_strcpy): Simplify.
>>>> 	(gimple_fold_builtin_stpcpy): Simplify.
>>>> 	(gimple_fold_builtin_sprintf): Remove NO_WARNING propagation
>>>> 	without effect.
>>>> 	(gimple_fold_builtin_strlen): Simplify.
>>>>
>>>> testsuite:
>>>> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>>
>>>> 	* gcc.dg/warn-stpcpy-no-nul.c: Remove xfails.
>>>> 	* gcc.dg/warn-strlen-no-nul.c: Remove xfails.
>>>>
>>>> Index: gcc/builtins.c
>>>> ===================================================================
>>>> --- gcc/builtins.c	(revision 264342)
>>>> +++ gcc/builtins.c	(working copy)
>>>> @@ -567,31 +567,12 @@ warn_string_no_nul (location_t loc, const char *fn
>>>>       the declaration of the object of which the array is a member or
>>>>       element.  Otherwise return null.  */
>>>>    
>>>> -tree
>>>> +static tree
>>>>    unterminated_array (tree exp)
>>>>    {
>>>> -  if (TREE_CODE (exp) == SSA_NAME)
>>>> -    {
>>>> -      gimple *stmt = SSA_NAME_DEF_STMT (exp);
>>>> -      if (!is_gimple_assign (stmt))
>>>> -	return NULL_TREE;
>>>> -
>>>> -      tree rhs1 = gimple_assign_rhs1 (stmt);
>>>> -      tree_code code = gimple_assign_rhs_code (stmt);
>>>> -      if (code == ADDR_EXPR
>>>> -	  && TREE_CODE (TREE_OPERAND (rhs1, 0)) == ARRAY_REF)
>>>> -	rhs1 = rhs1;
>>>> -      else if (code != POINTER_PLUS_EXPR)
>>>> -	return NULL_TREE;
>>>> -
>>>> -      exp = rhs1;
>>>> -    }
>>>> -
>>>>      tree nonstr = NULL;
>>>> -  if (c_strlen (exp, 1, &nonstr, 1) == NULL && nonstr)
>>>> -    return nonstr;
>>>> -
>>>> -  return NULL_TREE;
>>>> +  c_strlen (exp, 1, &nonstr);
>>>> +  return nonstr;
>>>>    }
>>> Sigh.  This is going to conflict with patch #6 from Martin's kit.
>>>
>>
>> Sorry, it just feels wrong to do this folding here instead of in
>> string_constant.
>>
>> I think the handling of POINTER_PLUS_EXPR above, is faulty,
>> because it is ignoring the offset parameter, which typically
>> contains the offset part, may add an offset to a different
>> structure member or another array index.  That is what happened
>> in PR 86622.
>>
>> So you are likely looking at the wrong index, or even the wrong
>> structure member.
> I'm not disagreeing that something's wrong here -- the whole concept
> that we extract the rhs and totally ignore the offset seems wrong.  I've
> stumbled over it working through issues with either patch #4 or #6 from
> Martin.  But I felt I needed to go back and reevaluate any assumptions I
> had about how the code was supposed to be used before I called it out.
> 
> 
> Your expr.c changes may be worth pushing through independently of the
> rest.    AFAICT they're really just exposing more cases where we can
> determine that we're working with a stirng constant.
> 

I think that moves just the folding from here to expr.c,
I don't see how the change in part #6 on unterminated_array
is supposed to work, I quote it here and add some comments:

>  tree
> -unterminated_array (tree exp)
> +unterminated_array (tree exp, tree *size /* = NULL */, bool *exact /* = NULL */)
>  {
> +  /* Offset from the beginning of the array or null.  */
> +  tree off = NULL_TREE;
> +
>    if (TREE_CODE (exp) == SSA_NAME)
>      {
>        gimple *stmt = SSA_NAME_DEF_STMT (exp);
> @@ -595,18 +600,43 @@ unterminated_array (tree exp)
>  
>        tree rhs1 = gimple_assign_rhs1 (stmt);
>        tree_code code = gimple_assign_rhs_code (stmt);
> -      if (code == ADDR_EXPR
> -	  && TREE_CODE (TREE_OPERAND (rhs1, 0)) == ARRAY_REF)
> -	rhs1 = rhs1;
> -      else if (code != POINTER_PLUS_EXPR)
> +      if ((code == ADDR_EXPR
> +	   && TREE_CODE (TREE_OPERAND (rhs1, 0)) == ARRAY_REF)
> +	  || code == POINTER_PLUS_EXPR)
> +	{
> +	  /* Store the index or offset.  */
> +	  off = gimple_assign_rhs2 (stmt);

ADDR_EXPR does not have rhs2, right?

> +	  exp = rhs1;
> +	}
> +      else
>  	return NULL_TREE;
> -
> -      exp = rhs1;
>      }
>  
>    tree nonstr;
> -  if (c_strlen (exp, 1, &nonstr) && nonstr)
> -    return nonstr;
> +  tree len = c_strlen (exp, 1, &nonstr);
> +  if (len && nonstr)
> +    {
> +      if (size)
> +	{
> +	  if (off)
> +	    {
> +	      if (TREE_CODE (off) == INTEGER_CST)
> +		{
> +		  /* Subtract the offset from the size of the array.  */
> +		  *exact = true;
> +		  off = fold_convert (ssizetype, off);
> +		  len = fold_build2 (MINUS_EXPR, ssizetype, len, off);

Underflow could happen here, off could even be negative.

Something similar to the check in string_constant should be done to avoid
that off might be moving the access to another array member:

           if (POINTER_TYPE_P (TREE_TYPE (rhs1))
               && TREE_CODE (TREE_TYPE (TREE_TYPE (rhs1))) == ARRAY_TYPE
               && !(decl && !*decl)
               && !(decl && tree_fits_uhwi_p (DECL_SIZE_UNIT (*decl))
                    && mem_size && tree_fits_uhwi_p (*mem_size)
                    && tree_int_cst_equal (*mem_size, DECL_SIZE_UNIT (*decl))))


> +		}
> +	      else
> +		*exact = false;
> +	    }
> +	  else
> +	    *exact = true;
> +
> +	  *size = len;
> +	}
> +      return nonstr;
> +    }
>  
>    return NULL_TREE;
>  }

Yeah, sorry, but this looks like a hack.

I would just use c_strlen if I need the length
and optionally nonstr.

The previous version was a convenience when you
are only interested in nonstr, which may happen.

I suppose the expr.c change will likely work alone, but
it was designed to replace the stuff here.

So I will try to split the expr.c part out, and post it
tomorrow.


Bernd.

Re: [PATCH] Cleanup strcpy/stpcpy no nul warning code

[Jeff Law]

On 9/17/18 1:18 PM, Bernd Edlinger wrote:
> 
> I suppose the expr.c change will likely work alone, but
> it was designed to replace the stuff here.
> 
> So I will try to split the expr.c part out, and post it
> tomorrow.
It does work alone.  I've already verified that here :-)  BUt will
probably call it a night without pushing it to the trunk.

jeff

Re: [PATCH] Cleanup strcpy/stpcpy no nul warning code

[Jeff Law]

On 9/17/18 1:18 PM, Bernd Edlinger wrote:
> On 09/17/18 20:32, Jeff Law wrote:
>> On 9/17/18 12:20 PM, Bernd Edlinger wrote:
>>> On 09/17/18 19:33, Jeff Law wrote:
>>>> On 9/16/18 1:58 PM, Bernd Edlinger wrote:
>>>>> Hi,
>>>>>
>>>>> this is a cleanup of the recently added strlen/strcpy/stpcpy
>>>>> no nul warning code.
>>>>>
>>>>> Most importantly it moves the SSA_NAME handling from
>>>>> unterminated_array to string_constant, thereby fixing
>>>>> another round of xfails in the strlen and stpcpy test cases.
>>>>>
>>>>> I need to say that the fix for bug 86622 is relying in
>>>>> type info on the pointer which is probably not safe in
>>>>> GIMPLE in the light of the recent discussion.
>>>>>
>>>>> I had to add two further exceptions, which should
>>>>> be safe in general: that is a pointer arithmentic on a string
>>>>> literal is okay, and arithmetic on a string constant
>>>>> that is exactly the size of the whole DECL, cannot
>>>>> access an adjacent member.
>>>>>
>>>>>
>>>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>>>> Is it OK for trunk?
>>>>>
>>>>>
>>>>> Thanks
>>>>> Bernd.
>>>>>
>>>>>
>>>>> patch-cleanup-no-nul.diff
>>>>>
>>>>> gcc:
>>>>> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>>>
>>>>> 	* builtins.h (unterminated_array): Remove prototype.
>>>>>           * builtins.c (unterminated_array): Simplify.  Make static.
>>>>>           (expand_builtin_strcpy_args): Don't use TREE_NO_WARNING here.
>>>>> 	(expand_builtin_stpcpy_1): Remove warn_string_no_nul without effect.
>>>>> 	* expr.c (string_constant): Handle SSA_NAME.  Add more exceptions
>>>>> 	where pointer arithmetic is safe.
>>>>> 	* gimple-fold.c (get_range_strlen): Handle nonstr like in c_strlen.
>>>>> 	(get_max_strlen): Remove the unnecessary mynonstr handling.
>>>>> 	(gimple_fold_builtin_strcpy): Simplify.
>>>>> 	(gimple_fold_builtin_stpcpy): Simplify.
>>>>> 	(gimple_fold_builtin_sprintf): Remove NO_WARNING propagation
>>>>> 	without effect.
>>>>> 	(gimple_fold_builtin_strlen): Simplify.
>>>>>
>>>>> testsuite:
>>>>> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>>>
>>>>> 	* gcc.dg/warn-stpcpy-no-nul.c: Remove xfails.
>>>>> 	* gcc.dg/warn-strlen-no-nul.c: Remove xfails.
>>>>>
>>>>> Index: gcc/builtins.c
>>>>> ===================================================================
>>>>> --- gcc/builtins.c	(revision 264342)
>>>>> +++ gcc/builtins.c	(working copy)
>>>>> @@ -567,31 +567,12 @@ warn_string_no_nul (location_t loc, const char *fn
>>>>>       the declaration of the object of which the array is a member or
>>>>>       element.  Otherwise return null.  */
>>>>>    
>>>>> -tree
>>>>> +static tree
>>>>>    unterminated_array (tree exp)
>>>>>    {
>>>>> -  if (TREE_CODE (exp) == SSA_NAME)
>>>>> -    {
>>>>> -      gimple *stmt = SSA_NAME_DEF_STMT (exp);
>>>>> -      if (!is_gimple_assign (stmt))
>>>>> -	return NULL_TREE;
>>>>> -
>>>>> -      tree rhs1 = gimple_assign_rhs1 (stmt);
>>>>> -      tree_code code = gimple_assign_rhs_code (stmt);
>>>>> -      if (code == ADDR_EXPR
>>>>> -	  && TREE_CODE (TREE_OPERAND (rhs1, 0)) == ARRAY_REF)
>>>>> -	rhs1 = rhs1;
>>>>> -      else if (code != POINTER_PLUS_EXPR)
>>>>> -	return NULL_TREE;
>>>>> -
>>>>> -      exp = rhs1;
>>>>> -    }
>>>>> -
>>>>>      tree nonstr = NULL;
>>>>> -  if (c_strlen (exp, 1, &nonstr, 1) == NULL && nonstr)
>>>>> -    return nonstr;
>>>>> -
>>>>> -  return NULL_TREE;
>>>>> +  c_strlen (exp, 1, &nonstr);
>>>>> +  return nonstr;
>>>>>    }
>>>> Sigh.  This is going to conflict with patch #6 from Martin's kit.
>>>>
>>>
>>> Sorry, it just feels wrong to do this folding here instead of in
>>> string_constant.
>>>
>>> I think the handling of POINTER_PLUS_EXPR above, is faulty,
>>> because it is ignoring the offset parameter, which typically
>>> contains the offset part, may add an offset to a different
>>> structure member or another array index.  That is what happened
>>> in PR 86622.
>>>
>>> So you are likely looking at the wrong index, or even the wrong
>>> structure member.
>> I'm not disagreeing that something's wrong here -- the whole concept
>> that we extract the rhs and totally ignore the offset seems wrong.  I've
>> stumbled over it working through issues with either patch #4 or #6 from
>> Martin.  But I felt I needed to go back and reevaluate any assumptions I
>> had about how the code was supposed to be used before I called it out.
>>
>>
>> Your expr.c changes may be worth pushing through independently of the
>> rest.    AFAICT they're really just exposing more cases where we can
>> determine that we're working with a stirng constant.
>>
> 
> I think that moves just the folding from here to expr.c,
> I don't see how the change in part #6 on unterminated_array
> is supposed to work, I quote it here and add some comments:
> 
Essentially there's a couple of concepts he wants to get in
unterminated_array.

First, given an address, if there's a variable part we want to clear
exact so we can adjust the text of the message in the caller.  ie
"exceeds the size" vs "may exceed the size".

Second, he really wants LEN to be set to the length of the string for
whatever the constant part of the address might be, even if it's
potentially not properly terminated.

So given:

const char b[][5] = { /* { dg-message "declared here" } */
  "12", "345", "6789", "abcde"
};

A reference like:

T (&b[3][1] + v0, bsz);

We want the length of the string at &b[3][1] into *LEN.  That's used to
provide the potential length of the string in the message.

I don't think we can get that from c_strlen right now.  Right now we've
defined c_strlen as (reasonably) returning NULL for the length when
we've got an string without proper termination.

I'm hesitant to provide a boolean argument that essentially says give me
whatever length you've computed, even if the string isn't properly
terminated.   But conceptually that's one of the things we'd need.  It's
also needed for patch #4.

Jeff

ps.  Yes there's still some issues in the code.  I'm mostly trying to
highlight how it's being used and what requirements we'd have if we were
going to do something like you've suggested in your patch to
unterminated_array.

Re: [PATCH] Cleanup strcpy/stpcpy no nul warning code

[Bernd Edlinger]

On 09/18/18 07:31, Jeff Law wrote:
> On 9/17/18 1:18 PM, Bernd Edlinger wrote:
>> On 09/17/18 20:32, Jeff Law wrote:
>>> On 9/17/18 12:20 PM, Bernd Edlinger wrote:
>>>> On 09/17/18 19:33, Jeff Law wrote:
>>>>> On 9/16/18 1:58 PM, Bernd Edlinger wrote:
>>>>>> Hi,
>>>>>>
>>>>>> this is a cleanup of the recently added strlen/strcpy/stpcpy
>>>>>> no nul warning code.
>>>>>>
>>>>>> Most importantly it moves the SSA_NAME handling from
>>>>>> unterminated_array to string_constant, thereby fixing
>>>>>> another round of xfails in the strlen and stpcpy test cases.
>>>>>>
>>>>>> I need to say that the fix for bug 86622 is relying in
>>>>>> type info on the pointer which is probably not safe in
>>>>>> GIMPLE in the light of the recent discussion.
>>>>>>
>>>>>> I had to add two further exceptions, which should
>>>>>> be safe in general: that is a pointer arithmentic on a string
>>>>>> literal is okay, and arithmetic on a string constant
>>>>>> that is exactly the size of the whole DECL, cannot
>>>>>> access an adjacent member.
>>>>>>
>>>>>>
>>>>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>>>>> Is it OK for trunk?
>>>>>>
>>>>>>
>>>>>> Thanks
>>>>>> Bernd.
>>>>>>
>>>>>>
>>>>>> patch-cleanup-no-nul.diff
>>>>>>
>>>>>> gcc:
>>>>>> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>>>>
>>>>>> 	* builtins.h (unterminated_array): Remove prototype.
>>>>>>            * builtins.c (unterminated_array): Simplify.  Make static.
>>>>>>            (expand_builtin_strcpy_args): Don't use TREE_NO_WARNING here.
>>>>>> 	(expand_builtin_stpcpy_1): Remove warn_string_no_nul without effect.
>>>>>> 	* expr.c (string_constant): Handle SSA_NAME.  Add more exceptions
>>>>>> 	where pointer arithmetic is safe.
>>>>>> 	* gimple-fold.c (get_range_strlen): Handle nonstr like in c_strlen.
>>>>>> 	(get_max_strlen): Remove the unnecessary mynonstr handling.
>>>>>> 	(gimple_fold_builtin_strcpy): Simplify.
>>>>>> 	(gimple_fold_builtin_stpcpy): Simplify.
>>>>>> 	(gimple_fold_builtin_sprintf): Remove NO_WARNING propagation
>>>>>> 	without effect.
>>>>>> 	(gimple_fold_builtin_strlen): Simplify.
>>>>>>
>>>>>> testsuite:
>>>>>> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>>>>
>>>>>> 	* gcc.dg/warn-stpcpy-no-nul.c: Remove xfails.
>>>>>> 	* gcc.dg/warn-strlen-no-nul.c: Remove xfails.
>>>>>>
>>>>>> Index: gcc/builtins.c
>>>>>> ===================================================================
>>>>>> --- gcc/builtins.c	(revision 264342)
>>>>>> +++ gcc/builtins.c	(working copy)
>>>>>> @@ -567,31 +567,12 @@ warn_string_no_nul (location_t loc, const char *fn
>>>>>>        the declaration of the object of which the array is a member or
>>>>>>        element.  Otherwise return null.  */
>>>>>>     
>>>>>> -tree
>>>>>> +static tree
>>>>>>     unterminated_array (tree exp)
>>>>>>     {
>>>>>> -  if (TREE_CODE (exp) == SSA_NAME)
>>>>>> -    {
>>>>>> -      gimple *stmt = SSA_NAME_DEF_STMT (exp);
>>>>>> -      if (!is_gimple_assign (stmt))
>>>>>> -	return NULL_TREE;
>>>>>> -
>>>>>> -      tree rhs1 = gimple_assign_rhs1 (stmt);
>>>>>> -      tree_code code = gimple_assign_rhs_code (stmt);
>>>>>> -      if (code == ADDR_EXPR
>>>>>> -	  && TREE_CODE (TREE_OPERAND (rhs1, 0)) == ARRAY_REF)
>>>>>> -	rhs1 = rhs1;
>>>>>> -      else if (code != POINTER_PLUS_EXPR)
>>>>>> -	return NULL_TREE;
>>>>>> -
>>>>>> -      exp = rhs1;
>>>>>> -    }
>>>>>> -
>>>>>>       tree nonstr = NULL;
>>>>>> -  if (c_strlen (exp, 1, &nonstr, 1) == NULL && nonstr)
>>>>>> -    return nonstr;
>>>>>> -
>>>>>> -  return NULL_TREE;
>>>>>> +  c_strlen (exp, 1, &nonstr);
>>>>>> +  return nonstr;
>>>>>>     }
>>>>> Sigh.  This is going to conflict with patch #6 from Martin's kit.
>>>>>
>>>>
>>>> Sorry, it just feels wrong to do this folding here instead of in
>>>> string_constant.
>>>>
>>>> I think the handling of POINTER_PLUS_EXPR above, is faulty,
>>>> because it is ignoring the offset parameter, which typically
>>>> contains the offset part, may add an offset to a different
>>>> structure member or another array index.  That is what happened
>>>> in PR 86622.
>>>>
>>>> So you are likely looking at the wrong index, or even the wrong
>>>> structure member.
>>> I'm not disagreeing that something's wrong here -- the whole concept
>>> that we extract the rhs and totally ignore the offset seems wrong.  I've
>>> stumbled over it working through issues with either patch #4 or #6 from
>>> Martin.  But I felt I needed to go back and reevaluate any assumptions I
>>> had about how the code was supposed to be used before I called it out.
>>>
>>>
>>> Your expr.c changes may be worth pushing through independently of the
>>> rest.    AFAICT they're really just exposing more cases where we can
>>> determine that we're working with a stirng constant.
>>>
>>
>> I think that moves just the folding from here to expr.c,
>> I don't see how the change in part #6 on unterminated_array
>> is supposed to work, I quote it here and add some comments:
>>
> Essentially there's a couple of concepts he wants to get in
> unterminated_array.
> 
> First, given an address, if there's a variable part we want to clear
> exact so we can adjust the text of the message in the caller.  ie
> "exceeds the size" vs "may exceed the size".
> 
> Second, he really wants LEN to be set to the length of the string for
> whatever the constant part of the address might be, even if it's
> potentially not properly terminated.
> 
> So given:
> 
> const char b[][5] = { /* { dg-message "declared here" } */
>    "12", "345", "6789", "abcde"
> };
> 
> A reference like:
> 
> T (&b[3][1] + v0, bsz);
> 
> We want the length of the string at &b[3][1] into *LEN.  That's used to
> provide the potential length of the string in the message.
> 
> I don't think we can get that from c_strlen right now.  Right now we've
> defined c_strlen as (reasonably) returning NULL for the length when
> we've got an string without proper termination.
> 
> I'm hesitant to provide a boolean argument that essentially says give me
> whatever length you've computed, even if the string isn't properly
> terminated.   But conceptually that's one of the things we'd need.  It's
> also needed for patch #4.
> 
> Jeff
> 
> ps.  Yes there's still some issues in the code.  I'm mostly trying to
> highlight how it's being used and what requirements we'd have if we were
> going to do something like you've suggested in your patch to
> unterminated_array.
> 

Hmm, you know, I wrote a while ago the following:
https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00411.html

Where I suggested to change c_strlen parameter nonstr to a
structure, where additional information could go.

There are a lot more informations that could be relevant
for warnings.

So it would be easy to convey the unterminated string length
there, also I could imagine to diagnose unaligned wide string
constants, if we have a decl with a too low alignment factor,
or a known unaligned offset for instance.  Also the location
of the expression on which c_strlen found the unterminated
string, could be of interest, especially when get_range_strlen
follows phi nodes.

Well, I have the impression that the 6/6 patch should be split
up in a part that uses c_strlen in its current form, and a
follow-up patch that adds any additional info.
I prefer doing things step-by-step, you know.

Note that the example given is probably unsafe:
> T (&b[3][1] + v0, bsz);

This does not pass the check in string_constant because it
looks like _2 = (char*)&b + _1; _3 = strlen (_2);
I have no way to prove that v0 is not larger than sizeof (b[3]).

However T(&a[1] + v0, bsz) will definitely work.

So the warning will likely get completely bogus data out
of unterminated_array.

By the way test test case for pr87053 is an example
of a false positive warning (it comes twice even though
TREE_NO_WARNING is used).

gcc pr87053.c
pr87053.c: In function 'main':
pr87053.c:15:26: warning: 'strlen' argument missing terminating nul [-Wstringop-overflow=]
15 |   if (__builtin_strlen (u.z) != 7)
    |                         ~^~
pr87053.c:11:3: note: referenced argument declared here
11 | } u = {{"1234", "567"}};
    |   ^
pr87053.c:15:26: warning: 'strlen' argument missing terminating nul [-Wstringop-overflow=]
15 |   if (__builtin_strlen (u.z) != 7)
    |                         ~^~
pr87053.c:11:3: note: referenced argument declared here
11 | } u = {{"1234", "567"}};
    |   ^

I think the duplicate warnings will not go away unless
this warning is only diagnosed in the tree-ssa-strlen
pass instead of the folding code, which happens repeatedly.



Bernd.

Re: [PATCH] Cleanup strcpy/stpcpy no nul warning code

[Jeff Law]

On 9/18/18 5:44 AM, Bernd Edlinger wrote:
>>
> 
> Hmm, you know, I wrote a while ago the following:
> https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00411.html
> 
> Where I suggested to change c_strlen parameter nonstr to a
> structure, where additional information could go.
It certainly seems better than continually adding more parameters and/or
having a given parameter have multiple overloaded meanings.  We'd fill
in everything we can and let the caller determine what bits they need.

Martin has suggested something similar.

> 
> Well, I have the impression that the 6/6 patch should be split
> up in a part that uses c_strlen in its current form, and a
> follow-up patch that adds any additional info.
> I prefer doing things step-by-step, you know.
I'm pretty sure it's not going to be able to use c_strlen in its current
form.  There are times when it really wants the length of the string,
even if it's not terminated.


> 
> Note that the example given is probably unsafe:
>> T (&b[3][1] + v0, bsz);
> 
> This does not pass the check in string_constant because it
> looks like _2 = (char*)&b + _1; _3 = strlen (_2);
> I have no way to prove that v0 is not larger than sizeof (b[3]).
That's OK.   The existence of the variable part turns "must" warnings
into "may" warnings precisely because we don't know how v0 impacts things.

> 
> By the way test test case for pr87053 is an example
> of a false positive warning (it comes twice even though
> TREE_NO_WARNING is used).
> 
> gcc pr87053.c
> pr87053.c: In function 'main':
> pr87053.c:15:26: warning: 'strlen' argument missing terminating nul [-Wstringop-overflow=]
> 15 |   if (__builtin_strlen (u.z) != 7)
>     |                         ~^~
> pr87053.c:11:3: note: referenced argument declared here
> 11 | } u = {{"1234", "567"}};
>     |   ^
> pr87053.c:15:26: warning: 'strlen' argument missing terminating nul [-Wstringop-overflow=]
> 15 |   if (__builtin_strlen (u.z) != 7)
>     |                         ~^~
> pr87053.c:11:3: note: referenced argument declared here
> 11 | } u = {{"1234", "567"}};
>     |   ^
> 
> I think the duplicate warnings will not go away unless
> this warning is only diagnosed in the tree-ssa-strlen
> pass instead of the folding code, which happens repeatedly.
Please file a bug.  FWIW, I wouldn't necessarily rely on on the strlen
pass not being repeated either.

jeff

Re: [PATCH] Cleanup strcpy/stpcpy no nul warning code

[Martin Liška]

Hi Jeff.

I noticed that your commit r264328 introduced this:

gcc/builtins.c:
...
    579        tree rhs1 = gimple_assign_rhs1 (stmt);
    580        tree_code code = gimple_assign_rhs_code (stmt);
    581        if (code == ADDR_EXPR
    582            && TREE_CODE (TREE_OPERAND (rhs1, 0)) == ARRAY_REF)
    583          rhs1 = rhs1; <---- here
    584        else if (code != POINTER_PLUS_EXPR)
    585          return NULL_TREE;
...

which is reported by LLVM as warning:
gcc/builtins.c:583:2:Semantic Issue: explicitly assigning value of variable of type 'tree' (aka 'tree_node *') to itself: -Wself-assign

Can you please fix that?
Thanks,
Martin

Re: [PATCH] Cleanup strcpy/stpcpy no nul warning code

[Martin Liška]

On 9/22/18 8:32 PM, Martin Liška wrote:
> Hi Jeff.
> 
> I noticed that your commit r264328 introduced this:
> 
> gcc/builtins.c:
> ...
>     579        tree rhs1 = gimple_assign_rhs1 (stmt);
>     580        tree_code code = gimple_assign_rhs_code (stmt);
>     581        if (code == ADDR_EXPR
>     582            && TREE_CODE (TREE_OPERAND (rhs1, 0)) == ARRAY_REF)
>     583          rhs1 = rhs1; <---- here
>     584        else if (code != POINTER_PLUS_EXPR)
>     585          return NULL_TREE;
> ...
> 
> which is reported by LLVM as warning:
> gcc/builtins.c:583:2:Semantic Issue: explicitly assigning value of variable of type 'tree' (aka 'tree_node *') to itself: -Wself-assign
> 
> Can you please fix that?
> Thanks,
> Martin

Apparently the same was already reported here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87387

Martin

Re: [PATCH] Cleanup strcpy/stpcpy no nul warning code

[Jeff Law]

On 9/22/18 12:32 PM, Martin Liška wrote:
> Hi Jeff.
> 
> I noticed that your commit r264328 introduced this:
> 
> gcc/builtins.c:
> ...
>    579        tree rhs1 = gimple_assign_rhs1 (stmt);
>    580        tree_code code = gimple_assign_rhs_code (stmt);
>    581        if (code == ADDR_EXPR
>    582            && TREE_CODE (TREE_OPERAND (rhs1, 0)) == ARRAY_REF)
>    583          rhs1 = rhs1; <---- here
>    584        else if (code != POINTER_PLUS_EXPR)
>    585          return NULL_TREE;
> ...
> 
> which is reported by LLVM as warning:
> gcc/builtins.c:583:2:Semantic Issue: explicitly assigning value of
> variable of type 'tree' (aka 'tree_node *') to itself: -Wself-assign
> 
> Can you please fix that?
Yes.  I'll be addressed with the changes I'm going to install from
Bernd/Martin.

jeff

Re: [PATCH] Cleanup strcpy/stpcpy no nul warning code

[Jeff Law]

On 9/16/18 1:58 PM, Bernd Edlinger wrote:
> Hi,
> 
> this is a cleanup of the recently added strlen/strcpy/stpcpy
> no nul warning code.
> 
> Most importantly it moves the SSA_NAME handling from
> unterminated_array to string_constant, thereby fixing
> another round of xfails in the strlen and stpcpy test cases.
> 
> I need to say that the fix for bug 86622 is relying in
> type info on the pointer which is probably not safe in
> GIMPLE in the light of the recent discussion.
> 
> I had to add two further exceptions, which should
> be safe in general: that is a pointer arithmentic on a string
> literal is okay, and arithmetic on a string constant
> that is exactly the size of the whole DECL, cannot
> access an adjacent member.
> 
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
> 
> 
> Thanks
> Bernd.
> 
> 
> patch-cleanup-no-nul.diff
> 
> gcc:
> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* builtins.h (unterminated_array): Remove prototype.
>         * builtins.c (unterminated_array): Simplify.  Make static.
>         (expand_builtin_strcpy_args): Don't use TREE_NO_WARNING here.
> 	(expand_builtin_stpcpy_1): Remove warn_string_no_nul without effect.
> 	* expr.c (string_constant): Handle SSA_NAME.  Add more exceptions
> 	where pointer arithmetic is safe.
> 	* gimple-fold.c (get_range_strlen): Handle nonstr like in c_strlen.
> 	(get_max_strlen): Remove the unnecessary mynonstr handling.
> 	(gimple_fold_builtin_strcpy): Simplify.
> 	(gimple_fold_builtin_stpcpy): Simplify.
> 	(gimple_fold_builtin_sprintf): Remove NO_WARNING propagation
> 	without effect.
> 	(gimple_fold_builtin_strlen): Simplify.
So my thinking right now is to go forward with the API change to allow
c_strlen to fill in a structure with relevant tidbits about the string.

That in turn allows us to use simplify unterminated_array in a manner
similar to what you've done in your patch -- while carrying forward the
capabilities we need for Martin's nul terminator warnings.  This would
be combined with the expr.c chunks from your patch.

However, most of the changes to drop NO_WARNING stuff should be handled
separately.  I don't think they're safe as-is.  I'm also pretty sure the
stpcpy changes in builtins.c aren't correct as-is.


Jeff

Re: [PATCH] Cleanup strcpy/stpcpy no nul warning code

[Bernd Edlinger]

On 09/24/18 19:48, Jeff Law wrote:
> On 9/16/18 1:58 PM, Bernd Edlinger wrote:
>> Hi,
>>
>> this is a cleanup of the recently added strlen/strcpy/stpcpy
>> no nul warning code.
>>
>> Most importantly it moves the SSA_NAME handling from
>> unterminated_array to string_constant, thereby fixing
>> another round of xfails in the strlen and stpcpy test cases.
>>
>> I need to say that the fix for bug 86622 is relying in
>> type info on the pointer which is probably not safe in
>> GIMPLE in the light of the recent discussion.
>>
>> I had to add two further exceptions, which should
>> be safe in general: that is a pointer arithmentic on a string
>> literal is okay, and arithmetic on a string constant
>> that is exactly the size of the whole DECL, cannot
>> access an adjacent member.
>>
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
>>
>>
>> Thanks
>> Bernd.
>>
>>
>> patch-cleanup-no-nul.diff
>>
>> gcc:
>> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	* builtins.h (unterminated_array): Remove prototype.
>>          * builtins.c (unterminated_array): Simplify.  Make static.
>>          (expand_builtin_strcpy_args): Don't use TREE_NO_WARNING here.
>> 	(expand_builtin_stpcpy_1): Remove warn_string_no_nul without effect.
>> 	* expr.c (string_constant): Handle SSA_NAME.  Add more exceptions
>> 	where pointer arithmetic is safe.
>> 	* gimple-fold.c (get_range_strlen): Handle nonstr like in c_strlen.
>> 	(get_max_strlen): Remove the unnecessary mynonstr handling.
>> 	(gimple_fold_builtin_strcpy): Simplify.
>> 	(gimple_fold_builtin_stpcpy): Simplify.
>> 	(gimple_fold_builtin_sprintf): Remove NO_WARNING propagation
>> 	without effect.
>> 	(gimple_fold_builtin_strlen): Simplify.
> So my thinking right now is to go forward with the API change to allow
> c_strlen to fill in a structure with relevant tidbits about the string.
> 
> That in turn allows us to use simplify unterminated_array in a manner
> similar to what you've done in your patch -- while carrying forward the
> capabilities we need for Martin's nul terminator warnings.  This would
> be combined with the expr.c chunks from your patch.
> 

Do you want me to elaborate that idea?

> However, most of the changes to drop NO_WARNING stuff should be handled
> separately.  I don't think they're safe as-is.  I'm also pretty sure the
> stpcpy changes in builtins.c aren't correct as-is.
> 
> 

Well, I think you must be referring to this:

@@ -3984,14 +3964,10 @@ expand_builtin_stpcpy_1 (tree exp, rtx target, mac
          compile-time, not an expression containing a string.  This is
          because the latter will potentially produce pessimized code
          when used to produce the return value.  */
-      tree nonstr = NULL_TREE;
        if (!c_getstr (src, NULL)
-         || !(len = c_strlen (src, 0, &nonstr, 1)))
+         || !(len = c_strlen (src, 0)))
         return expand_movstr (dst, src, target, /*endp=*/2);

-      if (nonstr && !TREE_NO_WARNING (exp))
-       warn_string_no_nul (EXPR_LOCATION (exp), "stpcpy", src, nonstr);
-
        lenp1 = size_binop_loc (loc, PLUS_EXPR, len, ssize_int (1));
        ret = expand_builtin_mempcpy_args (dst, src, lenp1,
                                          target, exp, /*endp=*/2);


My observation is: If that one is necessary and does not only emit some
duplicated warnings, then the test case must be incomplete, at least it did not
regress when this code is removed.

It is possible that there are cases where this expansion produces a warning
where previous folding steps did not catch the issue, however they
are probably rare, and come at a cost, which means duplicated warnings.

Maybe there could a better way than TREE_NO_WARNING to get rid
of the duplicated warnings.

Maybe it will be best to concentrate the warnings on a single pass,
which means expand will it not be, right?


Bernd.

Re: [PATCH] Cleanup strcpy/stpcpy no nul warning code

[Jeff Law]

On 9/24/18 12:18 PM, Bernd Edlinger wrote:
> On 09/24/18 19:48, Jeff Law wrote:
>> On 9/16/18 1:58 PM, Bernd Edlinger wrote:
>>> Hi,
>>>
>>> this is a cleanup of the recently added strlen/strcpy/stpcpy
>>> no nul warning code.
>>>
>>> Most importantly it moves the SSA_NAME handling from
>>> unterminated_array to string_constant, thereby fixing
>>> another round of xfails in the strlen and stpcpy test cases.
>>>
>>> I need to say that the fix for bug 86622 is relying in
>>> type info on the pointer which is probably not safe in
>>> GIMPLE in the light of the recent discussion.
>>>
>>> I had to add two further exceptions, which should
>>> be safe in general: that is a pointer arithmentic on a string
>>> literal is okay, and arithmetic on a string constant
>>> that is exactly the size of the whole DECL, cannot
>>> access an adjacent member.
>>>
>>>
>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>> Is it OK for trunk?
>>>
>>>
>>> Thanks
>>> Bernd.
>>>
>>>
>>> patch-cleanup-no-nul.diff
>>>
>>> gcc:
>>> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>
>>> 	* builtins.h (unterminated_array): Remove prototype.
>>>          * builtins.c (unterminated_array): Simplify.  Make static.
>>>          (expand_builtin_strcpy_args): Don't use TREE_NO_WARNING here.
>>> 	(expand_builtin_stpcpy_1): Remove warn_string_no_nul without effect.
>>> 	* expr.c (string_constant): Handle SSA_NAME.  Add more exceptions
>>> 	where pointer arithmetic is safe.
>>> 	* gimple-fold.c (get_range_strlen): Handle nonstr like in c_strlen.
>>> 	(get_max_strlen): Remove the unnecessary mynonstr handling.
>>> 	(gimple_fold_builtin_strcpy): Simplify.
>>> 	(gimple_fold_builtin_stpcpy): Simplify.
>>> 	(gimple_fold_builtin_sprintf): Remove NO_WARNING propagation
>>> 	without effect.
>>> 	(gimple_fold_builtin_strlen): Simplify.
>> So my thinking right now is to go forward with the API change to allow
>> c_strlen to fill in a structure with relevant tidbits about the string.
>>
>> That in turn allows us to use simplify unterminated_array in a manner
>> similar to what you've done in your patch -- while carrying forward the
>> capabilities we need for Martin's nul terminator warnings.  This would
>> be combined with the expr.c chunks from your patch.
>>
> 
> Do you want me to elaborate that idea?
No need.  I've already got those bits here.

> 
>> However, most of the changes to drop NO_WARNING stuff should be handled
>> separately.  I don't think they're safe as-is.  I'm also pretty sure the
>> stpcpy changes in builtins.c aren't correct as-is.
>>
>>
> 
> Well, I think you must be referring to this:
> 
> @@ -3984,14 +3964,10 @@ expand_builtin_stpcpy_1 (tree exp, rtx target, mac
>           compile-time, not an expression containing a string.  This is
>           because the latter will potentially produce pessimized code
>           when used to produce the return value.  */
> -      tree nonstr = NULL_TREE;
>         if (!c_getstr (src, NULL)
> -         || !(len = c_strlen (src, 0, &nonstr, 1)))
> +         || !(len = c_strlen (src, 0)))
>          return expand_movstr (dst, src, target, /*endp=*/2);
> 
> -      if (nonstr && !TREE_NO_WARNING (exp))
> -       warn_string_no_nul (EXPR_LOCATION (exp), "stpcpy", src, nonstr);
> -
>         lenp1 = size_binop_loc (loc, PLUS_EXPR, len, ssize_int (1));
>         ret = expand_builtin_mempcpy_args (dst, src, lenp1,
>                                           target, exp, /*endp=*/2);
> 
> 
> My observation is: If that one is necessary and does not only emit some
> duplicated warnings, then the test case must be incomplete, at least it did not
> regress when this code is removed.
I'm pretty sure the test is incomplete.

> 
> Maybe there could a better way than TREE_NO_WARNING to get rid
> of the duplicated warnings.
> 
> Maybe it will be best to concentrate the warnings on a single pass,
> which means expand will it not be, right?
COnceptually getting all the warnings out of the folder is a good start,
but insufficient.  You'd need to look at the thread for the duplicated
warnings due to how the expanders call into each other.

jeff

Re: [PATCH] Cleanup strcpy/stpcpy no nul warning code

[Jeff Law]

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

On 9/16/18 1:58 PM, Bernd Edlinger wrote:
> Hi,
> 
> this is a cleanup of the recently added strlen/strcpy/stpcpy
> no nul warning code.
> 
> Most importantly it moves the SSA_NAME handling from
> unterminated_array to string_constant, thereby fixing
> another round of xfails in the strlen and stpcpy test cases.
> 
> I need to say that the fix for bug 86622 is relying in
> type info on the pointer which is probably not safe in
> GIMPLE in the light of the recent discussion.
> 
> I had to add two further exceptions, which should
> be safe in general: that is a pointer arithmentic on a string
> literal is okay, and arithmetic on a string constant
> that is exactly the size of the whole DECL, cannot
> access an adjacent member.
> 
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
> 
> 
> Thanks
> Bernd.
> 
> 
> patch-cleanup-no-nul.diff
> 
> gcc:
> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* builtins.h (unterminated_array): Remove prototype.
>         * builtins.c (unterminated_array): Simplify.  Make static.
>         (expand_builtin_strcpy_args): Don't use TREE_NO_WARNING here.
> 	(expand_builtin_stpcpy_1): Remove warn_string_no_nul without effect.
> 	* expr.c (string_constant): Handle SSA_NAME.  Add more exceptions
> 	where pointer arithmetic is safe.
> 	* gimple-fold.c (get_range_strlen): Handle nonstr like in c_strlen.
> 	(get_max_strlen): Remove the unnecessary mynonstr handling.
> 	(gimple_fold_builtin_strcpy): Simplify.
> 	(gimple_fold_builtin_stpcpy): Simplify.
> 	(gimple_fold_builtin_sprintf): Remove NO_WARNING propagation
> 	without effect.
> 	(gimple_fold_builtin_strlen): Simplify.
> 
> testsuite:
> 2018-09-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* gcc.dg/warn-stpcpy-no-nul.c: Remove xfails.
> 	* gcc.dg/warn-strlen-no-nul.c: Remove xfails.
I extracted the unterminated_array simplifications and the changes to
string_constant and committed them to the trunk.

I left unterminated_array with external scope as a patch from Martin
wants to call it from outside builtins.c.

jeff

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 6686 bytes --]

commit c1451dd20f9064084df7b45c4635c1bcb57a712d
Author: Jeff Law <law@torsion.usersys.redhat.com>
Date:   Mon Sep 17 14:09:55 2018 -0400

            PR c/87387
            * builtins.c (unterminated_array): Simplify.
            * expr.c (string_constant): Handle SSA_NAME.  Add more exceptions
            where pointer arithmetic is safe.
    
            * gcc.dg/warn-stpcpy-no-nul.c: Drop unnecessary xfails.
            * gcc.dg/warn-stplen-no-nul.c: Likewise.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 6be143e9f18..8ff607e4d23 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,10 @@
+2018-09-25  Bernd Edlinger  <bernd.edlinger@hotmail.de>
+
+	PR c/87387
+        * builtins.c (unterminated_array): Simplify.
+	* expr.c (string_constant): Handle SSA_NAME.  Add more exceptions
+	where pointer arithmetic is safe.
+
 2018-09-25  Richard Biener  <rguenther@suse.de>
 
 	PR debug/83941
diff --git a/gcc/builtins.c b/gcc/builtins.c
index 1d4de099726..5f00208ff09 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -570,28 +570,9 @@ warn_string_no_nul (location_t loc, const char *fn, tree arg, tree decl)
 tree
 unterminated_array (tree exp)
 {
-  if (TREE_CODE (exp) == SSA_NAME)
-    {
-      gimple *stmt = SSA_NAME_DEF_STMT (exp);
-      if (!is_gimple_assign (stmt))
-	return NULL_TREE;
-
-      tree rhs1 = gimple_assign_rhs1 (stmt);
-      tree_code code = gimple_assign_rhs_code (stmt);
-      if (code == ADDR_EXPR
-	  && TREE_CODE (TREE_OPERAND (rhs1, 0)) == ARRAY_REF)
-	rhs1 = rhs1;
-      else if (code != POINTER_PLUS_EXPR)
-	return NULL_TREE;
-
-      exp = rhs1;
-    }
-
   tree nonstr = NULL;
-  if (c_strlen (exp, 1, &nonstr, 1) == NULL && nonstr)
-    return nonstr;
-
-  return NULL_TREE;
+  c_strlen (exp, 1, &nonstr);
+  return nonstr;
 }
 
 /* Compute the length of a null-terminated character string or wide
diff --git a/gcc/expr.c b/gcc/expr.c
index b8782b9b133..583c7f008ff 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -11372,7 +11372,10 @@ string_constant (tree arg, tree *ptr_offset, tree *mem_size, tree *decl)
 	  /* Avoid pointers to arrays (see bug 86622).  */
 	  if (POINTER_TYPE_P (TREE_TYPE (arg))
 	      && TREE_CODE (TREE_TYPE (TREE_TYPE (arg))) == ARRAY_TYPE
-	      && TREE_CODE (TREE_OPERAND (arg0, 0)) == ARRAY_REF)
+	      && !(decl && !*decl)
+	      && !(decl && tree_fits_uhwi_p (DECL_SIZE_UNIT (*decl))
+		   && mem_size && tree_fits_uhwi_p (*mem_size)
+		   && tree_int_cst_equal (*mem_size, DECL_SIZE_UNIT (*decl))))
 	    return NULL_TREE;
 
 	  tree type = TREE_TYPE (arg1);
@@ -11381,6 +11384,38 @@ string_constant (tree arg, tree *ptr_offset, tree *mem_size, tree *decl)
 	}
       return NULL_TREE;
     }
+  else if (TREE_CODE (arg) == SSA_NAME)
+    {
+      gimple *stmt = SSA_NAME_DEF_STMT (arg);
+      if (!is_gimple_assign (stmt))
+	return NULL_TREE;
+
+      tree rhs1 = gimple_assign_rhs1 (stmt);
+      tree_code code = gimple_assign_rhs_code (stmt);
+      if (code == ADDR_EXPR)
+	return string_constant (rhs1, ptr_offset, mem_size, decl);
+      else if (code != POINTER_PLUS_EXPR)
+	return NULL_TREE;
+
+      tree offset;
+      if (tree str = string_constant (rhs1, &offset, mem_size, decl))
+	{
+	  /* Avoid pointers to arrays (see bug 86622).  */
+	  if (POINTER_TYPE_P (TREE_TYPE (rhs1))
+	      && TREE_CODE (TREE_TYPE (TREE_TYPE (rhs1))) == ARRAY_TYPE
+	      && !(decl && !*decl)
+	      && !(decl && tree_fits_uhwi_p (DECL_SIZE_UNIT (*decl))
+		   && mem_size && tree_fits_uhwi_p (*mem_size)
+		   && tree_int_cst_equal (*mem_size, DECL_SIZE_UNIT (*decl))))
+	    return NULL_TREE;
+
+	  tree rhs2 = gimple_assign_rhs2 (stmt);
+	  tree type = TREE_TYPE (rhs2);
+	  *ptr_offset = fold_build2 (PLUS_EXPR, type, offset, rhs2);
+	  return str;
+	}
+      return NULL_TREE;
+    }
   else if (DECL_P (arg))
     array = arg;
   else
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 05dbb0d40ea..7b26b894b15 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2018-09-25  Jeff Law  <law@redhat.com>
+
+	* gcc.dg/warn-stpcpy-no-nul.c: Drop unnecessary xfails.
+	* gcc.dg/warn-stplen-no-nul.c: Likewise.
+
 2018-09-25  Alexandre Oliva <oliva@adacore.com>
 
 	* gnat.dg/dinst.adb: Adjust for locviews.
diff --git a/gcc/testsuite/gcc.dg/warn-stpcpy-no-nul.c b/gcc/testsuite/gcc.dg/warn-stpcpy-no-nul.c
index 78c4a7f9286..e718010ec2a 100644
--- a/gcc/testsuite/gcc.dg/warn-stpcpy-no-nul.c
+++ b/gcc/testsuite/gcc.dg/warn-stpcpy-no-nul.c
@@ -71,13 +71,13 @@ void test_two_dim_array (char *d)
   T (&b[3][1] + 1);     /* { dg-warning "nul" }  */
   T (&b[3][v0]);        /* { dg-warning "nul" }  */
   T (&b[3][1] + v0);    /* { dg-warning "nul" }  */
-  T (&b[3][v0] + v1);   /* { dg-warning "nul" "bug ???" { xfail *-*-* } }  */
+  T (&b[3][v0] + v1);   /* { dg-warning "nul" }  */
 
   T (&b[i3][i1]);       /* { dg-warning "nul" }  */
   T (&b[i3][i1] + i1);  /* { dg-warning "nul" }  */
   T (&b[i3][v0]);       /* { dg-warning "nul" }  */
   T (&b[i3][i1] + v0);  /* { dg-warning "nul" }  */
-  T (&b[i3][v0] + v1);  /* { dg-warning "nul" "bug ???" { xfail *-*-* } }  */
+  T (&b[i3][v0] + v1);  /* { dg-warning "nul" }  */
 
   T (v0 ? "" : b[0]);
   T (v0 ? "" : b[1]);
diff --git a/gcc/testsuite/gcc.dg/warn-strlen-no-nul.c b/gcc/testsuite/gcc.dg/warn-strlen-no-nul.c
index 997dfc3540f..b716aa44770 100644
--- a/gcc/testsuite/gcc.dg/warn-strlen-no-nul.c
+++ b/gcc/testsuite/gcc.dg/warn-strlen-no-nul.c
@@ -71,9 +71,9 @@ T (&b[3][v0] + v1);     /* { dg-warning "nul" }  */
 T (&b[i3][i1]);         /* { dg-warning "nul" }  */
 T (&b[i3][i1] + 1);     /* { dg-warning "nul" }  */
 T (&b[i3][i1] + i1);    /* { dg-warning "nul" }  */
-T (&b[i3][v0]);         /* { dg-warning "nul" "pr86919" { xfail *-*-* } }  */
-T (&b[i3][i1] + v0);    /* { dg-warning "nul" "pr86919" { xfail *-*-* } }  */
-T (&b[i3][v0] + v1);    /* { dg-warning "nul" "pr86919" { xfail *-*-* } }  */
+T (&b[i3][v0]);         /* { dg-warning "nul" }  */
+T (&b[i3][i1] + v0);    /* { dg-warning "nul" }  */
+T (&b[i3][v0] + v1);    /* { dg-warning "nul" }  */
 
 T (v0 ? "" : b[0]);
 T (v0 ? "" : b[1]);
@@ -152,10 +152,10 @@ T (&s.b[1] + v0);     /* { dg-warning "nul" }  */
 
 T (&s.b[i0]);         /* { dg-warning "nul" }  */
 T (&s.b[i0] + i1);    /* { dg-warning "nul" }  */
-T (&s.b[i0] + v0);    /* { dg-warning "nul" "pr86919" { xfail *-*-* } }  */
+T (&s.b[i0] + v0);    /* { dg-warning "nul" }  */
 T (&s.b[i1]);         /* { dg-warning "nul" }  */
 T (&s.b[i1] + i1);    /* { dg-warning "nul" }  */
-T (&s.b[i1] + v0);    /* { dg-warning "nul" "pr86919" { xfail *-*-* } }  */
+T (&s.b[i1] + v0);    /* { dg-warning "nul" }  */
 
 struct B { struct A a[2]; };