[PATCH] add support for strnlen (PR 81384)

public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed

* [PATCH] add support for strnlen (PR 81384)
@ 2018-06-05 21:43 Martin Sebor
  2018-06-05 22:20 ` Jakub Jelinek
                   ` (3 more replies)
  0 siblings, 4 replies; 22+ messages in thread
From: Martin Sebor @ 2018-06-05 21:43 UTC (permalink / raw)
  To: Gcc Patch List

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

The attached patch adds basic support for handling strnlen
as a built-in function.  It touches the strlen pass where
it folds constant results of the function, and builtins.c
to add simple support for expanding strnlen calls with known
results.  It also changes calls.c to detect excessive bounds
to the function and unsafe calls with arguments declared
attribute nonstring.

A side-effect of the strlen change I should call out is that
strlen() calls to all zero-length arrays that aren't considered
flexible array members (i.e., internal members or non-members)
are folded into zero.  No warning is issued for such invalid
uses of zero-length arrays but based on the responses to my
question Re: aliasing between internal zero-length-arrays and
other members(*) it sounds like one would be appropriate.
I will see about adding one in a separate patch.

Martin

[*] https://gcc.gnu.org/ml/gcc/2018-06/msg00046.html

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

PR tree-optimization/81384 - built-in form of strnlen missing

gcc/ChangeLog:

	PR tree-optimization/81384
	* builtin-types.def (BT_FN_SIZE_CONST_STRING_SIZE): New.
	* builtins.c (expand_builtin_strnlen): New function.
	(expand_builtin): Call it.
	(fold_builtin_n): Avoid setting TREE_NO_WARNING.
	* builtins.def (BUILT_IN_STRNLEN): New.
	* calls.c (maybe_warn_nonstring_arg): Handle BUILT_IN_STRNLEN.
	Warn for bounds in excess of maximum object size.
	* tree-ssa-strlen.c (maybe_set_strlen_range): Return tree representing
	single-value ranges.  Handle strnlen.
	(handle_builtin_strlen): Handle strnlen.
	(strlen_check_and_optimize_stmt): Same.

gcc/testsuite/ChangeLog:

	PR tree-optimization/81384
	* gcc.c-torture/execute/builtins/lib/strnlen.c: New test.
	* gcc.c-torture/execute/builtins/strnlen-lib.c: New test.
	* gcc.c-torture/execute/builtins/strnlen.c: New test.
	* gcc.dg/attr-nonstring-2.c: New test.
	* gcc.dg/attr-nonstring-3.c: New test.
	* gcc.dg/attr-nonstring-4.c: New test.
	* gcc.dg/strlenopt-44.c: New test.
	* gcc.dg/strlenopt.h (strnlen):  Declare.

diff --git a/gcc/builtin-types.def b/gcc/builtin-types.def
index 5365bef..1f15350 100644
--- a/gcc/builtin-types.def
+++ b/gcc/builtin-types.def
@@ -322,6 +322,8 @@ DEF_FUNCTION_TYPE_2 (BT_FN_STRING_CONST_STRING_INT,
 		     BT_STRING, BT_CONST_STRING, BT_INT)
 DEF_FUNCTION_TYPE_2 (BT_FN_STRING_CONST_STRING_SIZE,
 		     BT_STRING, BT_CONST_STRING, BT_SIZE)
+DEF_FUNCTION_TYPE_2 (BT_FN_SIZE_CONST_STRING_SIZE,
+		     BT_SIZE, BT_CONST_STRING, BT_SIZE)
 DEF_FUNCTION_TYPE_2 (BT_FN_INT_CONST_STRING_FILEPTR,
 		     BT_INT, BT_CONST_STRING, BT_FILEPTR)
 DEF_FUNCTION_TYPE_2 (BT_FN_INT_INT_FILEPTR,
diff --git a/gcc/builtins.c b/gcc/builtins.c
index c96ac38..e5a9bf2 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -145,6 +145,7 @@ static rtx expand_builtin_memset_with_bounds (tree, rtx, machine_mode);
 static rtx expand_builtin_memset_args (tree, tree, tree, rtx, machine_mode, tree);
 static rtx expand_builtin_bzero (tree);
 static rtx expand_builtin_strlen (tree, rtx, machine_mode);
+static rtx expand_builtin_strnlen (tree, rtx, machine_mode);
 static rtx expand_builtin_alloca (tree);
 static rtx expand_builtin_unop (machine_mode, tree, rtx, rtx, optab);
 static rtx expand_builtin_frame_address (tree, tree);
@@ -2825,7 +2826,7 @@ expand_builtin_powi (tree exp, rtx target)
 }
 
 /* Expand expression EXP which is a call to the strlen builtin.  Return
-   NULL_RTX if we failed the caller should emit a normal call, otherwise
+   NULL_RTX if we failed and the caller should emit a normal call, otherwise
    try to get the result in TARGET, if convenient.  */
 
 static rtx
@@ -2930,6 +2931,74 @@ expand_builtin_strlen (tree exp, rtx target,
   return target;
 }
 
+/* Expand call EXP to the strnlen built-in, returning the result
+   and setting it in TARGET.  Otherwise return NULL_RTX on failure.  */
+
+static rtx
+expand_builtin_strnlen (tree exp, rtx target, machine_mode target_mode)
+{
+  if (!validate_arglist (exp, POINTER_TYPE, INTEGER_TYPE, VOID_TYPE))
+    return NULL_RTX;
+
+  tree src = CALL_EXPR_ARG (exp, 0);
+  tree bound = CALL_EXPR_ARG (exp, 1);
+
+  if (!bound)
+    return NULL_RTX;
+
+  location_t loc = UNKNOWN_LOCATION;
+  if (EXPR_HAS_LOCATION (exp))
+    loc = EXPR_LOCATION (exp);
+
+  tree maxobjsize = max_object_size ();
+  tree func = get_callee_fndecl (exp);
+
+  tree len = c_strlen (src, 0);
+
+  if (TREE_CODE (bound) == INTEGER_CST)
+    {
+      if (!TREE_NO_WARNING (exp)
+	  && tree_int_cst_lt (maxobjsize, bound)
+	  && warning_at (loc, OPT_Wstringop_overflow_,
+			 "%K%qD specified bound %E "
+			 "exceeds maximum object size %E",
+			 exp, func, bound, maxobjsize))
+	  TREE_NO_WARNING (exp) = true;
+
+      if (!len || TREE_CODE (len) != INTEGER_CST)
+	return NULL_RTX;
+
+      len = fold_convert_loc (loc, size_type_node, len);
+      len = fold_build2_loc (loc, MIN_EXPR, size_type_node, len, bound);
+      return expand_expr (len, target, target_mode, EXPAND_NORMAL);
+    }
+
+  if (TREE_CODE (bound) != SSA_NAME)
+    return NULL_RTX;
+
+  wide_int min, max;
+  enum value_range_type rng = get_range_info (bound, &min, &max);
+  if (rng != VR_RANGE)
+    return NULL_RTX;
+
+  if (!TREE_NO_WARNING (exp)
+      && wi::ltu_p (wi::to_wide (maxobjsize), min)
+      && warning_at (loc, OPT_Wstringop_overflow_,
+		     "%K%qD specified bound [%wu, %wu] "
+		     "exceeds maximum object size %E",
+		     exp, func, min.to_uhwi (), max.to_uhwi (), maxobjsize))
+      TREE_NO_WARNING (exp) = true;
+
+  if (!len || TREE_CODE (len) != INTEGER_CST)
+    return NULL_RTX;
+
+  if (wi::gtu_p (min, wi::to_wide (len)))
+    return expand_expr (len, target, target_mode, EXPAND_NORMAL);
+
+  len = fold_build2_loc (loc, MIN_EXPR, TREE_TYPE (len), len, bound);
+  return expand_expr (len, target, target_mode, EXPAND_NORMAL);
+}
+
 /* Callback routine for store_by_pieces.  Read GET_MODE_BITSIZE (MODE)
    bytes from constant string DATA + OFFSET and return it as target
    constant.  */
@@ -3126,20 +3195,27 @@ check_access (tree exp, tree, tree, tree dstwrite,
      object size.  */
   if (range[0] && tree_int_cst_lt (maxobjsize, range[0]))
     {
+      if (TREE_NO_WARNING (exp))
+	return false;
+
       location_t loc = tree_nonartificial_location (exp);
       loc = expansion_point_location_if_in_system_header (loc);
 
+      bool warned;
       if (range[0] == range[1])
-	warning_at (loc, opt,
-		    "%K%qD specified size %E "
-		    "exceeds maximum object size %E",
-		    exp, func, range[0], maxobjsize);
-	  else
-	    warning_at (loc, opt,
-			"%K%qD specified size between %E and %E "
-			"exceeds maximum object size %E",
-			exp, func,
-			range[0], range[1], maxobjsize);
+	warned = warning_at (loc, opt,
+			     "%K%qD specified size %E "
+			     "exceeds maximum object size %E",
+			     exp, func, range[0], maxobjsize);
+      else
+	warned = warning_at (loc, opt,
+			     "%K%qD specified size between %E and %E "
+			     "exceeds maximum object size %E",
+			     exp, func,
+			     range[0], range[1], maxobjsize);
+      if (warned)
+	TREE_NO_WARNING (exp) = true;
+
       return false;
     }
 
@@ -7067,6 +7143,12 @@ expand_builtin (tree exp, rtx target, rtx subtarget, machine_mode mode,
 	return target;
       break;
 
+    case BUILT_IN_STRNLEN:
+      target = expand_builtin_strnlen (exp, target, target_mode);
+      if (target)
+	return target;
+      break;
+
     case BUILT_IN_STRCAT:
       target = expand_builtin_strcat (exp, target);
       if (target)
@@ -9384,7 +9466,6 @@ fold_builtin_n (location_t loc, tree fndecl, tree *args, int nargs, bool)
     {
       ret = build1 (NOP_EXPR, TREE_TYPE (ret), ret);
       SET_EXPR_LOCATION (ret, loc);
-      TREE_NO_WARNING (ret) = 1;
       return ret;
     }
   return NULL_TREE;
diff --git a/gcc/builtins.def b/gcc/builtins.def
index 58b4698..315c3f4 100644
--- a/gcc/builtins.def
+++ b/gcc/builtins.def
@@ -733,6 +733,7 @@ DEF_EXT_LIB_BUILTIN    (BUILT_IN_STRNCASECMP, "strncasecmp", BT_FN_INT_CONST_STR
 DEF_LIB_BUILTIN        (BUILT_IN_STRNCAT, "strncat", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF)
 DEF_LIB_BUILTIN        (BUILT_IN_STRNCMP, "strncmp", BT_FN_INT_CONST_STRING_CONST_STRING_SIZE, ATTR_PURE_NOTHROW_NONNULL_LEAF)
 DEF_LIB_BUILTIN        (BUILT_IN_STRNCPY, "strncpy", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF)
+DEF_LIB_BUILTIN_CHKP   (BUILT_IN_STRNLEN, "strnlen", BT_FN_SIZE_CONST_STRING_SIZE, ATTR_PURE_NOTHROW_NONNULL_LEAF)
 DEF_LIB_BUILTIN        (BUILT_IN_STRPBRK, "strpbrk", BT_FN_STRING_CONST_STRING_CONST_STRING, ATTR_PURE_NOTHROW_NONNULL_LEAF)
 DEF_LIB_BUILTIN        (BUILT_IN_STRRCHR, "strrchr", BT_FN_STRING_CONST_STRING_INT, ATTR_PURE_NOTHROW_NONNULL_LEAF)
 DEF_LIB_BUILTIN        (BUILT_IN_STRSPN, "strspn", BT_FN_SIZE_CONST_STRING_CONST_STRING, ATTR_PURE_NOTHROW_NONNULL_LEAF)
diff --git a/gcc/calls.c b/gcc/calls.c
index 6e1ea92..1952af1 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -1626,6 +1626,9 @@ maybe_warn_nonstring_arg (tree fndecl, tree exp)
   if (!fndecl || DECL_BUILT_IN_CLASS (fndecl) != BUILT_IN_NORMAL)
     return;
 
+  if (TREE_NO_WARNING (exp))
+    return;
+
   bool with_bounds = CALL_WITH_BOUNDS_P (exp);
 
   unsigned nargs = call_expr_nargs (exp);
@@ -1680,6 +1683,18 @@ maybe_warn_nonstring_arg (tree fndecl, tree exp)
 	break;
       }
 
+    case BUILT_IN_STRNLEN:
+      {
+	tree arg = CALL_EXPR_ARG (exp, 0);
+	if (!get_attr_nonstring_decl (arg))
+	  get_range_strlen (arg, lenrng);
+
+	unsigned argno = with_bounds ? 2 : 1;
+	if (argno < nargs)
+	  bound = CALL_EXPR_ARG (exp, argno);
+	break;
+      }
+
     default:
       break;
     }
@@ -1692,6 +1707,29 @@ maybe_warn_nonstring_arg (tree fndecl, tree exp)
       get_size_range (bound, bndrng);
     }
 
+  location_t loc = EXPR_LOCATION (exp);
+
+  if (bndrng[0])
+    {
+      /* Diagnose excessive bound prior the adjustment below and
+	 regardless of attribute nonstring.  */
+      tree maxobjsize = max_object_size ();
+      if (tree_int_cst_lt (maxobjsize, bndrng[0]))
+	{
+	  if (tree_int_cst_equal (bndrng[0], bndrng[1]))
+	    warning_at (loc, OPT_Wstringop_overflow_,
+			"%K%qD specified bound %E "
+			"exceeds maximum object size %E",
+			exp, fndecl, bndrng[0], maxobjsize);
+	  else
+	    warning_at (loc, OPT_Wstringop_overflow_,
+			"%K%qD specified bound [%E, %E] "
+			"exceeds maximum object size %E",
+			exp, fndecl, bndrng[0], bndrng[1], maxobjsize);
+	  return;
+	}
+    }
+
   if (*lenrng)
     {
       /* Add one for the nul.  */
@@ -1784,8 +1822,6 @@ maybe_warn_nonstring_arg (tree fndecl, tree exp)
       else if (bound == void_type_node)
 	bound = NULL_TREE;
 
-      location_t loc = EXPR_LOCATION (exp);
-
       bool warned = false;
 
       if (wi::ltu_p (asize, wibnd))
diff --git a/gcc/testsuite/gcc.c-torture/execute/builtins/lib/strnlen.c b/gcc/testsuite/gcc.c-torture/execute/builtins/lib/strnlen.c
new file mode 100644
index 0000000..73ada14
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/builtins/lib/strnlen.c
@@ -0,0 +1,22 @@
+typedef __SIZE_TYPE__ size_t;
+
+extern void abort (void);
+extern int inside_main;
+
+__attribute__ ((__noinline__))
+size_t
+strnlen (const char *s, size_t n)
+{
+  size_t i;
+
+#ifdef __OPTIMIZE__
+  if (inside_main)
+    abort ();
+#endif
+
+  i = 0;
+  while (s[i] != 0 && n--)
+    i++;
+
+  return i;
+}
diff --git a/gcc/testsuite/gcc.c-torture/execute/builtins/strnlen-lib.c b/gcc/testsuite/gcc.c-torture/execute/builtins/strnlen-lib.c
new file mode 100644
index 0000000..bf91940
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/builtins/strnlen-lib.c
@@ -0,0 +1 @@
+#include "lib/strnlen.c"
diff --git a/gcc/testsuite/gcc.c-torture/execute/builtins/strnlen.c b/gcc/testsuite/gcc.c-torture/execute/builtins/strnlen.c
new file mode 100644
index 0000000..2368d06
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/builtins/strnlen.c
@@ -0,0 +1,95 @@
+/* PR tree-optimization/81384 - built-in form of strnlen missing
+   Test to verify that strnlen built-in expansion works correctly
+   in the absence of tree strlen optimization.
+   { dg-do run }
+   { do-options "-O2 -fno-tree-strlen" }  */
+
+#define PTRDIFF_MAX __PTRDIFF_MAX__
+#define SIZE_MAX    __SIZE_MAX__
+#define NOIPA __attribute__ ((noipa))
+
+typedef __SIZE_TYPE__ size_t;
+
+extern void abort (void);
+extern size_t strnlen (const char *, size_t);
+
+#define A(expr)							\
+  ((expr) ? (void)0						\
+   : (__builtin_printf ("assertion on line %i failed: %s\n",	\
+			__LINE__, #expr),			\
+      abort ()))
+
+NOIPA void test_strnlen_str_cst (void)
+{
+  A (strnlen ("", 0) == 0);
+  A (strnlen ("", 1) == 0);
+  A (strnlen ("", 9) == 0);
+  A (strnlen ("", PTRDIFF_MAX) == 0);
+  A (strnlen ("", SIZE_MAX) == 0);
+  A (strnlen ("", -1) == 0);
+
+  A (strnlen ("1", 0) == 0);
+  A (strnlen ("1", 1) == 1);
+  A (strnlen ("1", 9) == 1);
+  A (strnlen ("1", PTRDIFF_MAX) == 1);
+  A (strnlen ("1", SIZE_MAX) == 1);
+  A (strnlen ("1", -2) == 1);
+
+  A (strnlen ("123", 0) == 0);
+  A (strnlen ("123", 1) == 1);
+  A (strnlen ("123", 2) == 2);
+  A (strnlen ("123", 3) == 3);
+  A (strnlen ("123", 9) == 3);
+  A (strnlen ("123", PTRDIFF_MAX) == 3);
+  A (strnlen ("123", SIZE_MAX) == 3);
+  A (strnlen ("123", -2) == 3);
+}
+
+NOIPA void test_strnlen_str_range (size_t x)
+{
+  size_t r_0_3 = x & 3;
+  size_t r_1_3 = r_0_3 | 1;
+  size_t r_2_3 = r_0_3 | 2;
+
+  A (strnlen ("",     r_0_3) == 0);
+  A (strnlen ("1",    r_0_3) <= 1);
+  A (strnlen ("12",   r_0_3) <= 2);
+  A (strnlen ("123",  r_0_3) <= 3);
+  A (strnlen ("1234", r_0_3) <= 3);
+
+  A (strnlen ("",     r_1_3) == 0);
+  A (strnlen ("1",    r_1_3) == 1);
+  A (strnlen ("12",   r_1_3) <= 2);
+  A (strnlen ("123",  r_1_3) <= 3);
+  A (strnlen ("1234", r_1_3) <= 3);
+
+  A (strnlen ("",     r_2_3) == 0);
+  A (strnlen ("1",    r_2_3) == 1);
+  A (strnlen ("12",   r_2_3) == 2);
+  A (strnlen ("123",  r_2_3) <= 3);
+  A (strnlen ("1234", r_2_3) <= 3);
+}
+
+NOIPA void test_strnlen_str_range_side_effect (size_t x)
+{
+  size_t r_0_3 = x & 3;
+  size_t r_1_3 = r_0_3 | 1;
+  size_t r_2_3 = r_0_3 | 2;
+  size_t n = r_2_3;
+
+  int i = 0;
+
+  A (strnlen ("1234" + i++, n) <= 3);
+  A (i == 1);
+
+  A (strnlen ("1234", n++) <= 3);
+  A (n == r_2_3 + 1);
+}
+
+void
+main_test (void)
+{
+  test_strnlen_str_cst ();
+  test_strnlen_str_range ((size_t)"");
+  test_strnlen_str_range_side_effect ((size_t)"");
+}
diff --git a/gcc/testsuite/gcc.dg/attr-nonstring-2.c b/gcc/testsuite/gcc.dg/attr-nonstring-2.c
new file mode 100644
index 0000000..e9edb66
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/attr-nonstring-2.c
@@ -0,0 +1,115 @@
+/* PR middle-end/81384 - built-in form of strnlen missing
+   { dg-do compile }
+   { dg-options "-O2 -Wall -ftrack-macro-expansion=0" } */
+
+#include "range.h"
+
+extern void* memcpy (void*, const void*, size_t);
+extern size_t strnlen (const char*, size_t);
+
+#define NONSTRING __attribute__ ((nonstring))
+
+#define _CAT(s, n)   s ## n
+#define CAT(s, n)    _CAT (s, n)
+#define UNIQ(n)      CAT (n, __LINE__)
+
+void sink (size_t, ...);
+
+#define T(expr)   sink (expr)
+
+void test_strnlen_array_cst (void)
+{
+  NONSTRING char ns3[3];
+  sink (0, ns3);    // "initialize" ns3
+
+  T (strnlen (ns3, 0));
+  T (strnlen (ns3, 1));
+  T (strnlen (ns3, 2));
+  T (strnlen (ns3, 3));
+  T (strnlen (ns3, 4));             /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound 4" } */
+  T (strnlen (ns3, DIFF_MAX));      /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound \[0-9\]+" } */
+  T (strnlen (ns3, SIZE_MAX));      /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+
+  NONSTRING char ns5[5];
+  sink (0, ns5);
+
+  T (strnlen (ns5, 0));
+  T (strnlen (ns5, 1));
+  T (strnlen (ns5, 2));
+  T (strnlen (ns5, 3));
+  T (strnlen (ns5, 6));             /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound 6" } */
+  T (strnlen (ns5, DIFF_MAX));      /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound \[0-9\]+" } */
+  T (strnlen (ns5, SIZE_MAX));      /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+}
+
+
+void test_strnlen_array_range (void)
+{
+  NONSTRING char ns3[3];
+  sink (0, ns3);    // "initialize" ns3
+
+  T (strnlen (ns3, UR (0, 3)));
+  T (strnlen (ns3, UR (0, 9)));
+  T (strnlen (ns3, UR (3, 4)));
+  T (strnlen (ns3, UR (3, DIFF_MAX)));
+  T (strnlen (ns3, UR (4, 5)));     /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound \[0-9\]+" } */
+  T (strnlen (ns3, UR (DIFF_MAX, SIZE_MAX)));  /* { dg-warning "argument 1 declared attribute .nonstring. is smaller " } */
+}
+
+
+#undef T
+#define T(N, init, nelts, bound)			\
+  do {							\
+    extern NONSTRING char UNIQ (arr)[N];		\
+    memcpy (UNIQ (arr), init, nelts);			\
+    sink (strnlen (UNIQ (arr), bound), UNIQ (arr));	\
+  } while (0)
+
+void test_strnlen_string_cst (void)
+{
+  T (3, "1",   2, 1);
+  T (3, "1",   2, 2);
+  T (3, "1",   2, 3);
+  T (3, "12",  3, 1);
+  T (3, "12",  3, 9);
+  T (3, "123", 3, 1);
+  T (3, "123", 3, 4);               /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound 4" } */
+  T (3, "123", 3, 9);               /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound 9" } */
+
+  T (5, "1",   2, 1);
+  T (5, "1",   2, 2);
+  T (5, "1",   2, 9);
+
+  T (5, "12",  3, 1);
+  T (5, "12",  3, 9);
+  T (5, "123", 3, 1);
+  T (5, "123", 3, 5);
+  T (5, "123", 3, 6);               /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound 6" } */
+
+  /* Strnlen shouldn't trigger a warning for arrays of unknown size
+     (except for accesses to uninitialized elements when those are
+     detected).  */
+  T (/* [] */, "1", 1, 1);
+  T (/* [] */, "1", 1, 2);
+  T (/* [] */, "1", 2, 1);
+  T (/* [] */, "1", 2, 2);
+  T (/* [] */, "1", 2, 3);
+  T (/* [] */, "1", 2, 9);
+  T (/* [] */, "1", 2, DIFF_MAX);
+  T (/* [] */, "1", 2, SIZE_MAX);
+
+  size_t n = DIFF_MAX;
+  T (/* [] */, "123", 3, n);
+  T (/* [] */, "123", 3, n + 1);    /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size " } */
+  n = SIZE_MAX;
+  T (/* [] */, "123", 3, n);        /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size " } */
+}
+
+
+void test_strnlen_string_range (void)
+{
+  T (3, "1",   2, UR (0, 1));
+  T (3, "1",   2, UR (3, 9));
+  T (3, "123", 3, UR (4, 5));       /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound 4" } */
+  T (3, "123", 3, UR (5, 9));       /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound 5" } */
+}
diff --git a/gcc/testsuite/gcc.dg/attr-nonstring-3.c b/gcc/testsuite/gcc.dg/attr-nonstring-3.c
new file mode 100644
index 0000000..4a10450
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/attr-nonstring-3.c
@@ -0,0 +1,117 @@
+/* PR middle-end/81384 - built-in form of strnlen missing
+
+   Since the strnlen patch affects the handling for strncmp and other
+   bounded functions, verify that a bound in excess of the maximum
+   object size specified for strncmp is diagnosed regardless of
+   attribute nonstring.  Also verify that a bound that's greater than
+   the size of a non-string array is diagnosed, even if it's not in
+   excess of the maximum object size.
+
+   { dg-do compile }
+   { dg-options "-O2 -Wall -ftrack-macro-expansion=0" } */
+
+#include "range.h"
+
+extern int strncmp (const char*, const char*, size_t);
+
+#define STR   /* not non-string */
+#define NS    __attribute__ ((nonstring))
+
+#define _CAT(s, n)   s ## n
+#define CAT(s, n)    _CAT (s, n)
+#define UNIQ(n)      CAT (n, __LINE__)
+
+void sink (int);
+
+#define T(at1, N1, at2, N2, bound)		\
+  do {						\
+    extern at1 char UNIQ (a)[N1];		\
+    extern at2 char UNIQ (b)[N2];		\
+    sink (strncmp (UNIQ (a), UNIQ (b), bound));	\
+  } while (0)
+
+void strncmp_cst (void)
+{
+  size_t n = DIFF_MAX;
+
+  T (STR, /* [] */, STR, /* [] */, n);
+  T (STR, /* [] */, STR, /* [] */, n + 1);    /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+
+  T (STR, 1, STR, /* [] */, n);
+  T (STR, 2, STR, /* [] */, n + 1);           /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+
+  T (STR, /* [] */, STR, 3, n);
+  T (STR, /* [] */, STR, 4, n + 1);           /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+
+  T (STR, /* [] */, NS, /* [] */, n);
+  T (STR, /* [] */, NS, /* [] */, n + 1);     /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+
+  T (STR, 5, NS, /* [] */, n);
+  T (STR, 6, NS, /* [] */, n + 1);            /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+
+  T (STR, /* [] */, NS, 7, n);                /* { dg-warning "argument 2 declared attribute .nonstring. is smaller than the specified bound" } */
+
+  T (STR, /* [] */, NS, 8, n + 1);            /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+
+  T (NS, /* [] */, STR, /* [] */, n);
+  T (NS, /* [] */, STR, /* [] */, n + 1);     /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+
+  T (NS, 9, STR, /* [] */, n);                /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound" } */
+  T (NS, 10, STR, /* [] */, n + 1);           /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+
+  T (NS, /* [] */, STR, 11, n);
+  T (NS, /* [] */, STR, 12, n + 1);           /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+
+  T (NS, /* [] */, NS, /* [] */, n);
+  T (NS, /* [] */, NS, /* [] */, n + 1);      /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+
+  T (NS, 13, NS, /* [] */, n);                /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound" } */
+  T (NS, 14, NS, /* [] */, n + 1);            /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+
+  T (NS, /* [] */, NS, 15, n);                /* { dg-warning "argument 2 declared attribute .nonstring. is smaller than the specified bound" } */
+  T (NS, /* [] */, NS, 16, n + 1);            /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+}
+
+
+void strncmp_range (void)
+{
+  size_t n = DIFF_MAX;
+  n = UR (n, n + 1);
+
+  T (STR, /* [] */, STR, /* [] */, n);
+  T (STR, /* [] */, STR, /* [] */, n + 1);    /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */
+
+  T (STR, 1, STR, /* [] */, n);
+  T (STR, 2, STR, /* [] */, n + 1);           /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */
+
+  T (STR, /* [] */, STR, 3, n);
+  T (STR, /* [] */, STR, 4, n + 1);           /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */
+
+  T (STR, /* [] */, NS, /* [] */, n);
+  T (STR, /* [] */, NS, /* [] */, n + 1);     /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */
+
+  T (STR, 5, NS, /* [] */, n);
+  T (STR, 6, NS, /* [] */, n + 1);            /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */
+
+  T (STR, /* [] */, NS, 7, n);                /* { dg-warning "argument 2 declared attribute .nonstring. is smaller than the specified bound" } */
+
+  T (STR, /* [] */, NS, 8, n + 1);            /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */
+
+  T (NS, /* [] */, STR, /* [] */, n);
+  T (NS, /* [] */, STR, /* [] */, n + 1);     /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */
+
+  T (NS, 9, STR, /* [] */, n);                /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound" } */
+  T (NS, 10, STR, /* [] */, n + 1);           /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */
+
+  T (NS, /* [] */, STR, 11, n);
+  T (NS, /* [] */, STR, 12, n + 1);           /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */
+
+  T (NS, /* [] */, NS, /* [] */, n);
+  T (NS, /* [] */, NS, /* [] */, n + 1);      /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */
+
+  T (NS, 13, NS, /* [] */, n);                /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound" } */
+  T (NS, 14, NS, /* [] */, n + 1);            /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */
+
+  T (NS, /* [] */, NS, 15, n);                /* { dg-warning "argument 2 declared attribute .nonstring. is smaller than the specified bound" } */
+  T (NS, /* [] */, NS, 16, n + 1);            /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */
+}
diff --git a/gcc/testsuite/gcc.dg/attr-nonstring-4.c b/gcc/testsuite/gcc.dg/attr-nonstring-4.c
new file mode 100644
index 0000000..7daff97
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/attr-nonstring-4.c
@@ -0,0 +1,64 @@
+/* PR middle-end/81384 - built-in form of strnlen missing
+
+   Verify that a strnlen bound in excess of the maximum object size
+   is diagnosed regardless of attribute nonstring.  Also verify that
+   a bound that's greater than the size of a non-string array is
+   diagnosed, even if it's not in excess of the maximum object size.
+
+   { dg-do compile }
+   { dg-options "-O2 -Wall -ftrack-macro-expansion=0" } */
+
+#include "range.h"
+
+extern size_t strnlen (const char*, size_t);
+
+#define STR   /* not non-string */
+#define NS    __attribute__ ((nonstring))
+
+#define _CAT(s, n)   s ## n
+#define CAT(s, n)    _CAT (s, n)
+#define UNIQ(n)      CAT (n, __LINE__)
+
+void sink (size_t);
+
+#define T(attr, N, bound)			\
+  do {						\
+    extern attr char UNIQ (a)[N];		\
+    sink (strnlen (UNIQ (a), bound));		\
+  } while (0)
+
+void strnlen_cst (void)
+{
+  size_t n = DIFF_MAX;
+
+  T (STR, /* [] */, n);
+  T (STR, /* [] */, n + 1);    /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+
+  T (STR, 1, n);
+  T (STR, 2, n + 1);           /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+
+  T (NS, /* [] */, n);
+  T (NS, /* [] */, n + 1);     /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+
+  T (NS, 9, n);                /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound" } */
+  T (NS, 10, n + 1);           /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size \[0-9\]+" } */
+}
+
+
+void strnlen_range (void)
+{
+  size_t n = DIFF_MAX;
+  n = UR (n, n + 1);
+
+  T (STR, /* [] */, n);
+  T (STR, /* [] */, n + 1);    /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */
+
+  T (STR, 1, n);
+  T (STR, 2, n + 1);           /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */
+
+  T (NS, /* [] */, n);
+  T (NS, /* [] */, n + 1);     /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */
+
+  T (NS, 9, n);                /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound" } */
+  T (NS, 10, n + 1);           /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */
+}
diff --git a/gcc/testsuite/gcc.dg/strlenopt-44.c b/gcc/testsuite/gcc.dg/strlenopt-44.c
new file mode 100644
index 0000000..bd9b197
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/strlenopt-44.c
@@ -0,0 +1,335 @@
+/* PR tree-optimization/81384 - built-in form of strnlen missing
+   Test to verify that strnlen built-in expansion works correctly
+   in the absence of tree strlen optimization.
+   { dg-do compile }
+   { dg-options "-O2 -Wall -fdump-tree-optimized" } */
+
+#include "strlenopt.h"
+
+#define PTRDIFF_MAX __PTRDIFF_MAX__
+#define SIZE_MAX    __SIZE_MAX__
+
+typedef __SIZE_TYPE__ size_t;
+
+extern void abort (void);
+extern size_t strnlen (const char *, size_t);
+
+#define CAT(x, y) x ## y
+#define CONCAT(x, y) CAT (x, y)
+#define FAILNAME(name) CONCAT (call_ ## name ##_on_line_, __LINE__)
+
+#define FAIL(name) do {				\
+    extern void FAILNAME (name) (void);		\
+    FAILNAME (name)();				\
+  } while (0)
+
+/* Macro to emit a call to funcation named
+     call_in_true_branch_not_eliminated_on_line_NNN()
+   for each call that's expected to be eliminated.  The dg-final
+   scan-tree-dump-time directive at the bottom of the test verifies
+   that no such call appears in output.  */
+#define ELIM(expr) \
+  if (!(expr)) FAIL (in_true_branch_not_eliminated); else (void)0
+
+/* Macro to emit a call to a function named
+     call_made_in_{true,false}_branch_on_line_NNN()
+   for each call that's expected to be retained.  The dg-final
+   scan-tree-dump-time directive at the bottom of the test verifies
+   that the expected number of both kinds of calls appears in output
+   (a pair for each line with the invocation of the KEEP() macro.  */
+#define KEEP(expr)				\
+  if (expr)					\
+    FAIL (made_in_true_branch);			\
+  else						\
+    FAIL (made_in_false_branch)
+
+extern char c;
+extern char a1[1];
+extern char a3[3];
+extern char a5[5];
+extern char a3_7[3][7];
+extern char ax[];
+
+void elim_strnlen_arr_cst (void)
+{
+  /* The length of a string stored in a one-element array must be zero.
+     The result reported by strnlen() for such an array can be non-zero
+     only when the bound is equal to 1 (in which case the result must
+     be one).  */
+  ELIM (strnlen (&c, 0) == 0);
+  ELIM (strnlen (&c, 1) < 2);
+  ELIM (strnlen (&c, 2) == 0);
+  ELIM (strnlen (&c, 9) == 0);
+  ELIM (strnlen (&c, PTRDIFF_MAX) == 0);
+  ELIM (strnlen (&c, SIZE_MAX) == 0);
+  ELIM (strnlen (&c, -1) == 0);
+
+  ELIM (strnlen (a1, 0) == 0);
+  ELIM (strnlen (a1, 1) < 2);
+  ELIM (strnlen (a1, 2) == 0);
+  ELIM (strnlen (a1, 9) == 0);
+  ELIM (strnlen (a1, PTRDIFF_MAX) == 0);
+  ELIM (strnlen (a1, SIZE_MAX) == 0);
+  ELIM (strnlen (a1, -1) == 0);
+
+  ELIM (strnlen (a3, 0) == 0);
+  ELIM (strnlen (a3, 1) < 2);
+  ELIM (strnlen (a3, 2) < 3);
+  ELIM (strnlen (a3, 3) < 4);
+  ELIM (strnlen (a3, 9) < 4);
+  ELIM (strnlen (a3, PTRDIFF_MAX) < 4);
+  ELIM (strnlen (a3, SIZE_MAX) < 4);
+  ELIM (strnlen (a3, -1) < 4);
+
+  ELIM (strnlen (a3_7[0], 0) == 0);
+  ELIM (strnlen (a3_7[0], 1) < 2);
+  ELIM (strnlen (a3_7[0], 2) < 3);
+  ELIM (strnlen (a3_7[0], 3) < 4);
+  ELIM (strnlen (a3_7[0], 9) < 8);
+  ELIM (strnlen (a3_7[0], PTRDIFF_MAX) < 8);
+  ELIM (strnlen (a3_7[0], SIZE_MAX) < 8);
+  ELIM (strnlen (a3_7[0], -1) < 8);
+
+  ELIM (strnlen (a3_7[2], 0) == 0);
+  ELIM (strnlen (a3_7[2], 1) < 2);
+  ELIM (strnlen (a3_7[2], 2) < 3);
+  ELIM (strnlen (a3_7[2], 3) < 4);
+  ELIM (strnlen (a3_7[2], 9) < 8);
+  ELIM (strnlen (a3_7[2], PTRDIFF_MAX) < 8);
+  ELIM (strnlen (a3_7[2], SIZE_MAX) < 8);
+  ELIM (strnlen (a3_7[2], -1) < 8);
+
+  ELIM (strnlen ((char*)a3_7, 0) == 0);
+  ELIM (strnlen ((char*)a3_7, 1) < 2);
+  ELIM (strnlen ((char*)a3_7, 2) < 3);
+  ELIM (strnlen ((char*)a3_7, 3) < 4);
+  ELIM (strnlen ((char*)a3_7, 9) < 10);
+  ELIM (strnlen ((char*)a3_7, 19) < 20);
+  ELIM (strnlen ((char*)a3_7, 21) < 22);
+  ELIM (strnlen ((char*)a3_7, 23) < 22);
+  ELIM (strnlen ((char*)a3_7, PTRDIFF_MAX) < 22);
+  ELIM (strnlen ((char*)a3_7, SIZE_MAX) < 22);
+  ELIM (strnlen ((char*)a3_7, -1) < 22);
+
+  ELIM (strnlen (ax, 0) == 0);
+  ELIM (strnlen (ax, 1) < 2);
+  ELIM (strnlen (ax, 2) < 3);
+  ELIM (strnlen (ax, 9) < 10);
+  ELIM (strnlen (a3, PTRDIFF_MAX) <= PTRDIFF_MAX);
+  ELIM (strnlen (a3, SIZE_MAX) < PTRDIFF_MAX);
+  ELIM (strnlen (a3, -1) < PTRDIFF_MAX);
+}
+
+struct MemArrays
+{
+  char c;
+  char a0[0];
+  char a1[1];
+  char a3[3];
+  char a5[5];
+  char a3_7[3][7];
+  char ax[1];
+};
+
+void elim_strnlen_memarr_cst (struct MemArrays *p, int i)
+{
+  ELIM (strnlen (&p->c, 0) == 0);
+  ELIM (strnlen (&p->c, 1) < 2);
+  ELIM (strnlen (&p->c, 9) == 0);
+  ELIM (strnlen (&p->c, PTRDIFF_MAX) == 0);
+  ELIM (strnlen (&p->c, SIZE_MAX) == 0);
+  ELIM (strnlen (&p->c, -1) == 0);
+
+  /* Other accesses to internal zero-length arrays are undefined.  */
+  ELIM (strnlen (p->a0, 0) == 0);
+
+  ELIM (strnlen (p->a1, 0) == 0);
+  ELIM (strnlen (p->a1, 1) < 2);
+  ELIM (strnlen (p->a1, 9) == 0);
+  ELIM (strnlen (p->a1, PTRDIFF_MAX) == 0);
+  ELIM (strnlen (p->a1, SIZE_MAX) == 0);
+  ELIM (strnlen (p->a1, -1) == 0);
+
+  ELIM (strnlen (p->a3, 0) == 0);
+  ELIM (strnlen (p->a3, 1) < 2);
+  ELIM (strnlen (p->a3, 2) < 3);
+  ELIM (strnlen (p->a3, 3) < 4);
+  ELIM (strnlen (p->a3, 9) < 4);
+  ELIM (strnlen (p->a3, PTRDIFF_MAX) < 4);
+  ELIM (strnlen (p->a3, SIZE_MAX) < 4);
+  ELIM (strnlen (p->a3, -1) < 4);
+
+  ELIM (strnlen (p[i].a3, 0) == 0);
+  ELIM (strnlen (p[i].a3, 1) < 2);
+  ELIM (strnlen (p[i].a3, 2) < 3);
+  ELIM (strnlen (p[i].a3, 3) < 4);
+  ELIM (strnlen (p[i].a3, 9) < 4);
+  ELIM (strnlen (p[i].a3, PTRDIFF_MAX) < 4);
+  ELIM (strnlen (p[i].a3, SIZE_MAX) < 4);
+  ELIM (strnlen (p[i].a3, -1) < 4);
+
+  ELIM (strnlen (p->a3_7[0], 0) == 0);
+  ELIM (strnlen (p->a3_7[0], 1) < 2);
+  ELIM (strnlen (p->a3_7[0], 2) < 3);
+  ELIM (strnlen (p->a3_7[0], 3) < 4);
+  ELIM (strnlen (p->a3_7[0], 9) < 8);
+  ELIM (strnlen (p->a3_7[0], PTRDIFF_MAX) < 8);
+  ELIM (strnlen (p->a3_7[0], SIZE_MAX) < 8);
+  ELIM (strnlen (p->a3_7[0], -1) < 8);
+
+  ELIM (strnlen (p->a3_7[2], 0) == 0);
+  ELIM (strnlen (p->a3_7[2], 1) < 2);
+  ELIM (strnlen (p->a3_7[2], 2) < 3);
+  ELIM (strnlen (p->a3_7[2], 3) < 4);
+  ELIM (strnlen (p->a3_7[2], 9) < 8);
+  ELIM (strnlen (p->a3_7[2], PTRDIFF_MAX) < 8);
+  ELIM (strnlen (p->a3_7[2], SIZE_MAX) < 8);
+  ELIM (strnlen (p->a3_7[2], -1) < 8);
+
+  ELIM (strnlen (p->a3_7[i], 0) == 0);
+  ELIM (strnlen (p->a3_7[i], 1) < 2);
+  ELIM (strnlen (p->a3_7[i], 2) < 3);
+  ELIM (strnlen (p->a3_7[i], 3) < 4);
+
+#if 0
+  /* This is tranformed into strnlen ((char*)p + offsetof (a3_7[i]), N)
+     which makes it impssible to determine the size of the array.  */
+  ELIM (strnlen (p->a3_7[i], 9) < 8);
+  ELIM (strnlen (p->a3_7[i], PTRDIFF_MAX) < 8);
+  ELIM (strnlen (p->a3_7[i], SIZE_MAX) < 8);
+  ELIM (strnlen (p->a3_7[i], -1) < 8);
+#else
+  ELIM (strnlen (p->a3_7[i], 9) < 10);
+  ELIM (strnlen (p->a3_7[i], 19) < 20);
+#endif
+
+  ELIM (strnlen ((char*)p->a3_7, 0) == 0);
+  ELIM (strnlen ((char*)p->a3_7, 1) < 2);
+  ELIM (strnlen ((char*)p->a3_7, 2) < 3);
+  ELIM (strnlen ((char*)p->a3_7, 3) < 4);
+  ELIM (strnlen ((char*)p->a3_7, 9) < 10);
+  ELIM (strnlen ((char*)p->a3_7, 19) < 20);
+  ELIM (strnlen ((char*)p->a3_7, 21) < 22);
+  ELIM (strnlen ((char*)p->a3_7, 23) < 22);
+  ELIM (strnlen ((char*)p->a3_7, PTRDIFF_MAX) < 22);
+  ELIM (strnlen ((char*)p->a3_7, SIZE_MAX) < 22);
+  ELIM (strnlen ((char*)p->a3_7, -1) < 22);
+
+  ELIM (strnlen (p->ax, 0) == 0);
+  ELIM (strnlen (p->ax, 1) < 2);
+  ELIM (strnlen (p->ax, 2) < 3);
+  ELIM (strnlen (p->ax, 9) < 10);
+  ELIM (strnlen (p->a3, PTRDIFF_MAX) <= PTRDIFF_MAX);
+  ELIM (strnlen (p->a3, SIZE_MAX) < PTRDIFF_MAX);
+  ELIM (strnlen (p->a3, -1) < PTRDIFF_MAX);
+}
+
+
+void elim_strnlen_str_cst (void)
+{
+  const char *s0 = "";
+  const char *s1 = "1";
+  const char *s3 = "123";
+
+  ELIM (strnlen (s0, 0) == 0);
+  ELIM (strnlen (s0, 1) == 0);
+  ELIM (strnlen (s0, 9) == 0);
+  ELIM (strnlen (s0, PTRDIFF_MAX) == 0);
+  ELIM (strnlen (s0, SIZE_MAX) == 0);
+  ELIM (strnlen (s0, -1) == 0);
+
+  ELIM (strnlen (s1, 0) == 0);
+  ELIM (strnlen (s1, 1) == 1);
+  ELIM (strnlen (s1, 9) == 1);
+  ELIM (strnlen (s1, PTRDIFF_MAX) == 1);
+  ELIM (strnlen (s1, SIZE_MAX) == 1);
+  ELIM (strnlen (s1, -2) == 1);
+
+  ELIM (strnlen (s3, 0) == 0);
+  ELIM (strnlen (s3, 1) == 1);
+  ELIM (strnlen (s3, 2) == 2);
+  ELIM (strnlen (s3, 3) == 3);
+  ELIM (strnlen (s3, 9) == 3);
+  ELIM (strnlen (s3, PTRDIFF_MAX) == 3);
+  ELIM (strnlen (s3, SIZE_MAX) == 3);
+  ELIM (strnlen (s3, -2) == 3);
+}
+
+void elim_strnlen_range (char *s)
+{
+  const char *s0 = "";
+  const char *s1 = "1";
+  const char *s3 = "123";
+
+  size_t n_0_1 = (size_t)s & 1;
+  size_t n_0_2 = ((size_t)s & 3) < 3 ? ((size_t)s & 3) : 2;
+  size_t n_0_3 = (size_t)s & 3;
+  size_t n_1_2 = n_0_1 + 1;
+
+  ELIM (strnlen (s0, n_0_1) == 0);
+  ELIM (strnlen (s0, n_0_2) == 0);
+  ELIM (strnlen (s0, n_1_2) == 0);
+
+  ELIM (strnlen (s1, n_0_1) < 2);
+  ELIM (strnlen (s1, n_0_2) < 2);
+  ELIM (strnlen (s1, n_0_3) < 2);
+
+  ELIM (strnlen (s1, n_1_2) > 0);
+  ELIM (strnlen (s1, n_1_2) < 2);
+
+  ELIM (strnlen (s3, n_0_1) < 2);
+  ELIM (strnlen (s3, n_0_2) < 3);
+  ELIM (strnlen (s3, n_0_3) < 4);
+
+  ELIM (strnlen (s3, n_1_2) > 0);
+  ELIM (strnlen (s3, n_1_2) < 4);
+}
+
+
+#line 1000
+
+void keep_strnlen_arr_cst (void)
+{
+  KEEP (strnlen (&c, 1) == 0);
+  KEEP (strnlen (&c, 1) == 1);
+
+  KEEP (strnlen (a1, 1) == 0);
+  KEEP (strnlen (a1, 1) == 1);
+
+  KEEP (strnlen (ax, 9) < 9);
+}
+
+struct FlexArrays
+{
+  char c;
+  char a0[0];   /* Access to internal zero-length arrays are undefined.  */
+  char a1[1];
+};
+
+void keep_strnlen_memarr_cst (struct FlexArrays *p)
+{
+  KEEP (strnlen (&p->c, 1) == 0);
+  KEEP (strnlen (&p->c, 1) == 1);
+
+#if 0
+  /* Accesses to internal zero-length arrays are undefined so avoid
+     exercising them.  */
+  KEEP (strnlen (p->a0, 1) == 0);
+  KEEP (strnlen (p->a0, 1) == 1);
+  KEEP (strnlen (p->a0, 9) < 9);
+#endif
+
+  KEEP (strnlen (p->a1, 1) == 0);
+  KEEP (strnlen (p->a1, 1) == 1);
+
+  KEEP (strnlen (p->a1, 2) == 0);
+  KEEP (strnlen (p->a1, 2) == 1);
+  KEEP (strnlen (p->a1, 2) == 2);
+
+  KEEP (strnlen (p->a1, 9) < 9);
+}
+
+/* { dg-final { scan-tree-dump-times "call_in_true_branch_not_eliminated_" 0 "optimized" } }
+
+   { dg-final { scan-tree-dump-times "call_made_in_true_branch_on_line_1\[0-9\]\[0-9\]\[0-9\]" 13 "optimized" } }
+   { dg-final { scan-tree-dump-times "call_made_in_false_branch_on_line_1\[0-9\]\[0-9\]\[0-9\]" 13 "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/strlenopt.h b/gcc/testsuite/gcc.dg/strlenopt.h
index 8f69940..a4044fd 100644
--- a/gcc/testsuite/gcc.dg/strlenopt.h
+++ b/gcc/testsuite/gcc.dg/strlenopt.h
@@ -9,6 +9,7 @@ void *malloc (size_t);
 void free (void *);
 char *strdup (const char *);
 size_t strlen (const char *);
+size_t strnlen (const char *, size_t);
 void *memcpy (void *__restrict, const void *__restrict, size_t);
 void *memmove (void *, const void *, size_t);
 char *strcpy (char *__restrict, const char *__restrict);
diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
index 7a89174..567fcf3 100644
--- a/gcc/tree-ssa-strlen.c
+++ b/gcc/tree-ssa-strlen.c
@@ -1157,15 +1157,16 @@ adjust_last_stmt (strinfo *si, gimple *stmt, bool is_strcat)
   update_stmt (last.stmt);
 }
 
-/* For an LHS that is an SSA_NAME and for strlen() argument SRC, set
-   LHS range info to [0, N] if SRC refers to a character array A[N]
-   with unknown length bounded by N.  */
+/* For an LHS that is an SSA_NAME and for strlen() or strnlen() argument
+   SRC, set LHS range info to [0, min (N, BOUND)] if SRC refers to
+   a character array A[N] with unknown length bounded by N, and for
+   strnlen(), by min (N, BOUND).  */
 
-static void
-maybe_set_strlen_range (tree lhs, tree src)
+static tree
+maybe_set_strlen_range (tree lhs, tree src, tree bound)
 {
   if (TREE_CODE (lhs) != SSA_NAME)
-    return;
+    return NULL_TREE;
 
   if (TREE_CODE (src) == SSA_NAME)
     {
@@ -1175,24 +1176,87 @@ maybe_set_strlen_range (tree lhs, tree src)
 	src = gimple_assign_rhs1 (def);
     }
 
-  if (TREE_CODE (src) != ADDR_EXPR)
-    return;
+  wide_int max = wi::to_wide (TYPE_MAX_VALUE (ptrdiff_type_node));
+  wide_int min = wi::zero (max.get_precision ());
 
-  /* The last array member of a struct can be bigger than its size
-     suggests if it's treated as a poor-man's flexible array member.  */
-  src = TREE_OPERAND (src, 0);
-  if (TREE_CODE (TREE_TYPE (src)) != ARRAY_TYPE
-      || array_at_struct_end_p (src))
-    return;
+  if (TREE_CODE (src) == ADDR_EXPR)
+    {
+      /* The last array member of a struct can be bigger than its size
+	 suggests if it's treated as a poor-man's flexible array member.  */
+      src = TREE_OPERAND (src, 0);
+      bool src_is_array = TREE_CODE (TREE_TYPE (src)) == ARRAY_TYPE;
+      if (src_is_array && !array_at_struct_end_p (src))
+	{
+	  tree type = TREE_TYPE (src);
+	  if (tree dom = TYPE_DOMAIN (type))
+	    {
+	      tree maxval = TYPE_MAX_VALUE (dom);
+	      if (maxval)
+		max = wi::to_wide (maxval);
+	      else
+		max = wi::zero (min.get_precision ());
+
+	      /* For strlen() the upper bound above is equal to
+		 the longest string that can be stored in the array
+		 (i.e., it accounts for the terminating nul.  For
+		 strnlen() bump up the maximum by one since the array
+		 need not be nul-terminated.  */
+	      if (bound)
+		++max;
+	    }
+	}
+      else
+	{
+	  if (TREE_CODE (src) == COMPONENT_REF && !src_is_array)
+	    src = TREE_OPERAND (src, 1);
+	  if (DECL_P (src))
+	    {
+	      /* Handle the unlikely case of strlen (&c) where c is some
+		 variable.  */
+	      if (tree size = DECL_SIZE_UNIT (src))
+		if (TREE_CODE (size) == INTEGER_CST)
+		  max = wi::to_wide (size);
+	    }
+	}
+    }
 
-  tree type = TREE_TYPE (src);
-  if (tree dom = TYPE_DOMAIN (type))
-    if (tree maxval = TYPE_MAX_VALUE (dom))
-      {
-	wide_int max = wi::to_wide (maxval);
-	wide_int min = wi::zero (max.get_precision ());
-	set_range_info (lhs, VR_RANGE, min, max);
-      }
+  if (bound)
+    {
+      /* For strnlen, adjust MIN and MAX as necessary.  If the bound
+	 is less than the size of the array set MAX to it.  It it's
+	 greater than MAX and MAX is non-zero bump MAX down to account
+	 for the necessary terminating nul.  Otherwise leave it alone.  */
+      if (TREE_CODE (bound) == INTEGER_CST)
+	{
+	  wide_int wibnd = wi::to_wide (bound);
+	  int cmp = wi::cmpu (wibnd, max);
+	  if (cmp < 0)
+	    max = wibnd;
+	  else if (cmp && wi::ne_p (max, min))
+	    --max;
+	}
+      else if (TREE_CODE (bound) == SSA_NAME)
+	{
+	  wide_int minbound, maxbound;
+	  value_range_type rng = get_range_info (bound, &minbound, &maxbound);
+	  if (rng == VR_RANGE)
+	    {
+	      /* For a bound in a known range, adjust the range determined
+		 above as necessary.  For a bound in some anti-range or
+		 in an unknown range, use the range determined above.  */
+	      if (wi::ltu_p (minbound, min))
+		min = minbound;
+	      if (wi::ltu_p (maxbound, max))
+		max = maxbound;
+	    }
+	}
+    }
+
+  if (min == max)
+    return wide_int_to_tree (size_type_node, min);
+
+  set_range_info (lhs, VR_RANGE, min, max);
+  return lhs;
 }
 
 /* Handle a strlen call.  If strlen of the argument is known, replace
@@ -1202,16 +1266,18 @@ maybe_set_strlen_range (tree lhs, tree src)
 static void
 handle_builtin_strlen (gimple_stmt_iterator *gsi)
 {
-  int idx;
-  tree src;
   gimple *stmt = gsi_stmt (*gsi);
   tree lhs = gimple_call_lhs (stmt);
 
   if (lhs == NULL_TREE)
     return;
 
-  src = gimple_call_arg (stmt, 0);
-  idx = get_stridx (src);
+  location_t loc = gimple_location (stmt);
+  tree callee = gimple_call_fndecl (stmt);
+  tree src = gimple_call_arg (stmt, 0);
+  tree bound = (DECL_FUNCTION_CODE (callee) == BUILT_IN_STRNLEN
+		? gimple_call_arg (stmt, 1) : NULL_TREE);
+  int idx = get_stridx (src);
   if (idx)
     {
       strinfo *si = NULL;
@@ -1235,8 +1301,9 @@ handle_builtin_strlen (gimple_stmt_iterator *gsi)
 	    }
 	  rhs = unshare_expr (rhs);
 	  if (!useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE (rhs)))
-	    rhs = fold_convert_loc (gimple_location (stmt),
-				    TREE_TYPE (lhs), rhs);
+	    rhs = fold_convert_loc (loc, TREE_TYPE (lhs), rhs);
+	  if (bound)
+	    rhs = fold_build2_loc (loc, MIN_EXPR, TREE_TYPE (rhs), rhs, bound);
 	  if (!update_call_from_tree (gsi, rhs))
 	    gimplify_and_update_call_from_tree (gsi, rhs);
 	  stmt = gsi_stmt (*gsi);
@@ -1257,10 +1324,8 @@ handle_builtin_strlen (gimple_stmt_iterator *gsi)
 	    }
 
 	  if (strlen_to_stridx)
-	    {
-	      location_t loc = gimple_location (stmt);
-	      strlen_to_stridx->put (lhs, stridx_strlenloc (idx, loc));
-	    }
+	    strlen_to_stridx->put (lhs, stridx_strlenloc (idx, loc));
+
 	  return;
 	}
     }
@@ -1283,7 +1348,6 @@ handle_builtin_strlen (gimple_stmt_iterator *gsi)
 	      si->full_string_p = true;
 	      if (TREE_CODE (old) == INTEGER_CST)
 		{
-		  location_t loc = gimple_location (stmt);
 		  old = fold_convert_loc (loc, TREE_TYPE (lhs), old);
 		  tree adj = fold_build2_loc (loc, MINUS_EXPR,
 					      TREE_TYPE (lhs), lhs, old);
@@ -1306,14 +1370,32 @@ handle_builtin_strlen (gimple_stmt_iterator *gsi)
       find_equal_ptrs (src, idx);
 
       /* For SRC that is an array of N elements, set LHS's range
-	 to [0, N].  */
-      maybe_set_strlen_range (lhs, src);
+	 to [0, min (N, BOUND)].  A constant return value means
+	 the range would have consisted of a single value.  In
+	 that case, fold the result into the returned constant.  */
+      if (tree ret = maybe_set_strlen_range (lhs, src, bound))
+	if (TREE_CODE (ret) == INTEGER_CST)
+	  {
+	    if (dump_file && (dump_flags & TDF_DETAILS) != 0)
+	      {
+		fprintf (dump_file, "Optimizing: ");
+		print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
+	      }
+	    if (!useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE (ret)))
+	      ret = fold_convert_loc (loc, TREE_TYPE (lhs), ret);
+	    if (!update_call_from_tree (gsi, ret))
+	      gimplify_and_update_call_from_tree (gsi, ret);
+	    stmt = gsi_stmt (*gsi);
+	    update_stmt (stmt);
+	    if (dump_file && (dump_flags & TDF_DETAILS) != 0)
+	      {
+		fprintf (dump_file, "into: ");
+		print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
+	      }
+	  }
 
       if (strlen_to_stridx)
-	{
-	  location_t loc = gimple_location (stmt);
-	  strlen_to_stridx->put (lhs, stridx_strlenloc (idx, loc));
-	}
+	strlen_to_stridx->put (lhs, stridx_strlenloc (idx, loc));
     }
 }
 
@@ -3424,6 +3506,8 @@ strlen_check_and_optimize_stmt (gimple_stmt_iterator *gsi, bool *cleanup_eh)
 	  {
 	  case BUILT_IN_STRLEN:
 	  case BUILT_IN_STRLEN_CHKP:
+	  case BUILT_IN_STRNLEN:
+	  case BUILT_IN_STRNLEN_CHKP:
 	    handle_builtin_strlen (gsi);
 	    break;
 	  case BUILT_IN_STRCHR:

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-06-05 21:43 [PATCH] add support for strnlen (PR 81384) Martin Sebor
@ 2018-06-05 22:20 ` Jakub Jelinek
  2018-06-06 15:14   ` Martin Sebor
  2018-06-06  0:39 ` Eric Gallager
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 22+ messages in thread
From: Jakub Jelinek @ 2018-06-05 22:20 UTC (permalink / raw)
  To: Martin Sebor; +Cc: Gcc Patch List

On Tue, Jun 05, 2018 at 03:43:17PM -0600, Martin Sebor wrote:
> --- a/gcc/builtins.def
> +++ b/gcc/builtins.def
> @@ -733,6 +733,7 @@ DEF_EXT_LIB_BUILTIN    (BUILT_IN_STRNCASECMP, "strncasecmp", BT_FN_INT_CONST_STR
>  DEF_LIB_BUILTIN        (BUILT_IN_STRNCAT, "strncat", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF)
>  DEF_LIB_BUILTIN        (BUILT_IN_STRNCMP, "strncmp", BT_FN_INT_CONST_STRING_CONST_STRING_SIZE, ATTR_PURE_NOTHROW_NONNULL_LEAF)
>  DEF_LIB_BUILTIN        (BUILT_IN_STRNCPY, "strncpy", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF)
> +DEF_LIB_BUILTIN_CHKP   (BUILT_IN_STRNLEN, "strnlen", BT_FN_SIZE_CONST_STRING_SIZE, ATTR_PURE_NOTHROW_NONNULL_LEAF)

strnlen isn't a C89, C99 nor C11 function, so I think it should be
DEF_EXT_LIB_BUILTIN like e.g. stpcpy, not DEF_LIB_BUILTIN.
And not really sure it is worth adding the CHKP stuff when it will be just
more work for Martin Liska to remove it again.

	Jakub

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-06-05 22:20 ` Jakub Jelinek
@ 2018-06-06 15:14   ` Martin Sebor
  2018-06-06 15:49     ` Jakub Jelinek
  0 siblings, 1 reply; 22+ messages in thread
From: Martin Sebor @ 2018-06-06 15:14 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Gcc Patch List

On 06/05/2018 04:19 PM, Jakub Jelinek wrote:
> On Tue, Jun 05, 2018 at 03:43:17PM -0600, Martin Sebor wrote:
>> --- a/gcc/builtins.def
>> +++ b/gcc/builtins.def
>> @@ -733,6 +733,7 @@ DEF_EXT_LIB_BUILTIN    (BUILT_IN_STRNCASECMP, "strncasecmp", BT_FN_INT_CONST_STR
>>  DEF_LIB_BUILTIN        (BUILT_IN_STRNCAT, "strncat", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF)
>>  DEF_LIB_BUILTIN        (BUILT_IN_STRNCMP, "strncmp", BT_FN_INT_CONST_STRING_CONST_STRING_SIZE, ATTR_PURE_NOTHROW_NONNULL_LEAF)
>>  DEF_LIB_BUILTIN        (BUILT_IN_STRNCPY, "strncpy", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF)
>> +DEF_LIB_BUILTIN_CHKP   (BUILT_IN_STRNLEN, "strnlen", BT_FN_SIZE_CONST_STRING_SIZE, ATTR_PURE_NOTHROW_NONNULL_LEAF)
>
> strnlen isn't a C89, C99 nor C11 function, so I think it should be
> DEF_EXT_LIB_BUILTIN like e.g. stpcpy, not DEF_LIB_BUILTIN.

Thanks.  Let me change that.

> And not really sure it is worth adding the CHKP stuff when it will be just
> more work for Martin Liska to remove it again.

Sure, let me take that out.

Is there anything else?

Martin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-06-06 15:14   ` Martin Sebor
@ 2018-06-06 15:49     ` Jakub Jelinek
  0 siblings, 0 replies; 22+ messages in thread
From: Jakub Jelinek @ 2018-06-06 15:49 UTC (permalink / raw)
  To: Martin Sebor; +Cc: Gcc Patch List

On Wed, Jun 06, 2018 at 09:14:11AM -0600, Martin Sebor wrote:
> On 06/05/2018 04:19 PM, Jakub Jelinek wrote:
> > On Tue, Jun 05, 2018 at 03:43:17PM -0600, Martin Sebor wrote:
> > > --- a/gcc/builtins.def
> > > +++ b/gcc/builtins.def
> > > @@ -733,6 +733,7 @@ DEF_EXT_LIB_BUILTIN    (BUILT_IN_STRNCASECMP, "strncasecmp", BT_FN_INT_CONST_STR
> > >  DEF_LIB_BUILTIN        (BUILT_IN_STRNCAT, "strncat", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF)
> > >  DEF_LIB_BUILTIN        (BUILT_IN_STRNCMP, "strncmp", BT_FN_INT_CONST_STRING_CONST_STRING_SIZE, ATTR_PURE_NOTHROW_NONNULL_LEAF)
> > >  DEF_LIB_BUILTIN        (BUILT_IN_STRNCPY, "strncpy", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF)
> > > +DEF_LIB_BUILTIN_CHKP   (BUILT_IN_STRNLEN, "strnlen", BT_FN_SIZE_CONST_STRING_SIZE, ATTR_PURE_NOTHROW_NONNULL_LEAF)
> > 
> > strnlen isn't a C89, C99 nor C11 function, so I think it should be
> > DEF_EXT_LIB_BUILTIN like e.g. stpcpy, not DEF_LIB_BUILTIN.
> 
> Thanks.  Let me change that.
> 
> > And not really sure it is worth adding the CHKP stuff when it will be just
> > more work for Martin Liska to remove it again.
> 
> Sure, let me take that out.
> 
> Is there anything else?

This wasn't a review, I just wanted to point out these two things.

	Jakub

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-06-05 21:43 [PATCH] add support for strnlen (PR 81384) Martin Sebor
  2018-06-05 22:20 ` Jakub Jelinek
@ 2018-06-06  0:39 ` Eric Gallager
  2018-06-06 14:44   ` Martin Sebor
  2018-06-12 15:10 ` PING " Martin Sebor
  2018-06-12 21:11 ` Jeff Law
  3 siblings, 1 reply; 22+ messages in thread
From: Eric Gallager @ 2018-06-06  0:39 UTC (permalink / raw)
  To: Martin Sebor; +Cc: Gcc Patch List

On 6/5/18, Martin Sebor <msebor@gmail.com> wrote:
> The attached patch adds basic support for handling strnlen
> as a built-in function.  It touches the strlen pass where
> it folds constant results of the function, and builtins.c
> to add simple support for expanding strnlen calls with known
> results.  It also changes calls.c to detect excessive bounds
> to the function and unsafe calls with arguments declared
> attribute nonstring.
>
> A side-effect of the strlen change I should call out is that
> strlen() calls to all zero-length arrays that aren't considered
> flexible array members (i.e., internal members or non-members)
> are folded into zero.  No warning is issued for such invalid
> uses of zero-length arrays but based on the responses to my
> question Re: aliasing between internal zero-length-arrays and
> other members(*) it sounds like one would be appropriate.
> I will see about adding one in a separate patch.
>
> Martin
>
> [*] https://gcc.gnu.org/ml/gcc/2018-06/msg00046.html
>

I see stuff in the patch mentioning the library version of strnlen;
have you tested on platforms both with and without strnlen in their
libcs? The gnulib documentation lists these platforms as having
missing or buggy strnlens:

This function is missing on some platforms: Mac OS X 10.5, FreeBSD
6.0, NetBSD 5.0, OpenBSD 3.8, AIX 4.3.2, HP-UX 11, IRIX 6.5, OSF/1
5.1, Solaris 10, mingw, Interix 3.5.
This function is buggy on some platforms: AIX 4.3.

https://www.gnu.org/software/gnulib/manual/html_node/strnlen.html

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-06-06  0:39 ` Eric Gallager
@ 2018-06-06 14:44   ` Martin Sebor
  0 siblings, 0 replies; 22+ messages in thread
From: Martin Sebor @ 2018-06-06 14:44 UTC (permalink / raw)
  To: Eric Gallager; +Cc: Gcc Patch List

On 06/05/2018 06:39 PM, Eric Gallager wrote:
> On 6/5/18, Martin Sebor <msebor@gmail.com> wrote:
>> The attached patch adds basic support for handling strnlen
>> as a built-in function.  It touches the strlen pass where
>> it folds constant results of the function, and builtins.c
>> to add simple support for expanding strnlen calls with known
>> results.  It also changes calls.c to detect excessive bounds
>> to the function and unsafe calls with arguments declared
>> attribute nonstring.
>>
>> A side-effect of the strlen change I should call out is that
>> strlen() calls to all zero-length arrays that aren't considered
>> flexible array members (i.e., internal members or non-members)
>> are folded into zero.  No warning is issued for such invalid
>> uses of zero-length arrays but based on the responses to my
>> question Re: aliasing between internal zero-length-arrays and
>> other members(*) it sounds like one would be appropriate.
>> I will see about adding one in a separate patch.
>>
>> Martin
>>
>> [*] https://gcc.gnu.org/ml/gcc/2018-06/msg00046.html
>>
>
> I see stuff in the patch mentioning the library version of strnlen;
> have you tested on platforms both with and without strnlen in their
> libcs? The gnulib documentation lists these platforms as having
> missing or buggy strnlens:
>
> This function is missing on some platforms: Mac OS X 10.5, FreeBSD
> 6.0, NetBSD 5.0, OpenBSD 3.8, AIX 4.3.2, HP-UX 11, IRIX 6.5, OSF/1
> 5.1, Solaris 10, mingw, Interix 3.5.
> This function is buggy on some platforms: AIX 4.3.
>
> https://www.gnu.org/software/gnulib/manual/html_node/strnlen.html

The patch doesn't make GCC synthesize calls to strnlen if they
don't appear in source code.  It's the responsibility of
programmers for those targets to work around their limitations,
bugs, or absence of support for the function (e.g., by avoiding
making calls to it).  This is the same as for most other library
functions, except a few exceptions like memcpy and memset where
GCC relies on the target to provide conforming implementations.

I only tested the patch on x86_64-linux.

Martin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* PING [PATCH] add support for strnlen (PR 81384)
  2018-06-05 21:43 [PATCH] add support for strnlen (PR 81384) Martin Sebor
  2018-06-05 22:20 ` Jakub Jelinek
  2018-06-06  0:39 ` Eric Gallager
@ 2018-06-12 15:10 ` Martin Sebor
  2018-06-12 21:11 ` Jeff Law
  3 siblings, 0 replies; 22+ messages in thread
From: Martin Sebor @ 2018-06-12 15:10 UTC (permalink / raw)
  To: Gcc Patch List

I'm looking for a review of the patch below (beyond using
DEF_EXT_LIB_BUILTIN to declare the built-in rather than
DEF_LIB_BUILTIN, as already pointed out by Jakub):

   https://gcc.gnu.org/ml/gcc-patches/2018-06/msg00267.html

On 06/05/2018 03:43 PM, Martin Sebor wrote:
> The attached patch adds basic support for handling strnlen
> as a built-in function.  It touches the strlen pass where
> it folds constant results of the function, and builtins.c
> to add simple support for expanding strnlen calls with known
> results.  It also changes calls.c to detect excessive bounds
> to the function and unsafe calls with arguments declared
> attribute nonstring.
>
> A side-effect of the strlen change I should call out is that
> strlen() calls to all zero-length arrays that aren't considered
> flexible array members (i.e., internal members or non-members)
> are folded into zero.  No warning is issued for such invalid
> uses of zero-length arrays but based on the responses to my
> question Re: aliasing between internal zero-length-arrays and
> other members(*) it sounds like one would be appropriate.
> I will see about adding one in a separate patch.
>
> Martin
>
> [*] https://gcc.gnu.org/ml/gcc/2018-06/msg00046.html

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-06-05 21:43 [PATCH] add support for strnlen (PR 81384) Martin Sebor
                   ` (2 preceding siblings ...)
  2018-06-12 15:10 ` PING " Martin Sebor
@ 2018-06-12 21:11 ` Jeff Law
  2018-06-18 16:35   ` Martin Sebor
  3 siblings, 1 reply; 22+ messages in thread
From: Jeff Law @ 2018-06-12 21:11 UTC (permalink / raw)
  To: Martin Sebor, Gcc Patch List

On 06/05/2018 03:43 PM, Martin Sebor wrote:
> TheÂ attachedÂ patchÂ addsÂ basicÂ supportÂ forÂ handlingÂ strnlen
> asÂ aÂ built-inÂ function.Â Â ItÂ touchesÂ theÂ strlenÂ passÂ where
> itÂ foldsÂ constantÂ resultsÂ ofÂ theÂ function,Â andÂ builtins.c
> toÂ addÂ simpleÂ supportÂ forÂ expandingÂ strnlenÂ callsÂ withÂ known
> results.Â Â ItÂ alsoÂ changesÂ calls.cÂ toÂ detectÂ excessiveÂ bounds
> toÂ theÂ functionÂ andÂ unsafeÂ callsÂ withÂ argumentsÂ declared
> attributeÂ nonstring.
> 
> AÂ side-effectÂ ofÂ theÂ strlenÂ changeÂ IÂ shouldÂ callÂ outÂ isÂ that
> strlen()Â callsÂ toÂ allÂ zero-lengthÂ arraysÂ thatÂ aren'tÂ considered
> flexibleÂ arrayÂ membersÂ (i.e.,Â internalÂ membersÂ orÂ non-members)
> areÂ foldedÂ intoÂ zero.Â Â NoÂ warningÂ isÂ issuedÂ forÂ suchÂ invalid
> usesÂ ofÂ zero-lengthÂ arraysÂ butÂ basedÂ onÂ theÂ responsesÂ toÂ my
> questionÂ Re:Â aliasingÂ betweenÂ internalÂ zero-length-arraysÂ and
> otherÂ members(*)Â itÂ soundsÂ likeÂ oneÂ wouldÂ beÂ appropriate.
> IÂ willÂ seeÂ aboutÂ addingÂ oneÂ inÂ aÂ separateÂ patch.
> 
> Martin
> 
> [*]Â https://gcc.gnu.org/ml/gcc/2018-06/msg00046.html
> 
> gcc-81384.diff
> 
> 
> PR tree-optimization/81384 - built-in form of strnlen missing
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/81384
> 	* builtin-types.def (BT_FN_SIZE_CONST_STRING_SIZE): New.
> 	* builtins.c (expand_builtin_strnlen): New function.
> 	(expand_builtin): Call it.
> 	(fold_builtin_n): Avoid setting TREE_NO_WARNING.
> 	* builtins.def (BUILT_IN_STRNLEN): New.
> 	* calls.c (maybe_warn_nonstring_arg): Handle BUILT_IN_STRNLEN.
> 	Warn for bounds in excess of maximum object size.
> 	* tree-ssa-strlen.c (maybe_set_strlen_range): Return tree representing
> 	single-value ranges.  Handle strnlen.
> 	(handle_builtin_strlen): Handle strnlen.
> 	(strlen_check_and_optimize_stmt): Same.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/81384
> 	* gcc.c-torture/execute/builtins/lib/strnlen.c: New test.
> 	* gcc.c-torture/execute/builtins/strnlen-lib.c: New test.
> 	* gcc.c-torture/execute/builtins/strnlen.c: New test.
> 	* gcc.dg/attr-nonstring-2.c: New test.
> 	* gcc.dg/attr-nonstring-3.c: New test.
> 	* gcc.dg/attr-nonstring-4.c: New test.
> 	* gcc.dg/strlenopt-44.c: New test.
> 	* gcc.dg/strlenopt.h (strnlen):  Declare.
> 
> diff --git a/gcc/builtin-types.def b/gcc/builtin-types.def
> index 5365bef..1f15350 100644
> --- a/gcc/builtin-types.def
> +++ b/gcc/builtin-types.def
> @@ -322,6 +322,8 @@ DEF_FUNCTION_TYPE_2 (BT_FN_STRING_CONST_STRING_INT,
>  		     BT_STRING, BT_CONST_STRING, BT_INT)
>  DEF_FUNCTION_TYPE_2 (BT_FN_STRING_CONST_STRING_SIZE,
>  		     BT_STRING, BT_CONST_STRING, BT_SIZE)
> +DEF_FUNCTION_TYPE_2 (BT_FN_SIZE_CONST_STRING_SIZE,
> +		     BT_SIZE, BT_CONST_STRING, BT_SIZE)
>  DEF_FUNCTION_TYPE_2 (BT_FN_INT_CONST_STRING_FILEPTR,
>  		     BT_INT, BT_CONST_STRING, BT_FILEPTR)
>  DEF_FUNCTION_TYPE_2 (BT_FN_INT_INT_FILEPTR,
I believe Jakub already suggested these change and you ack'd that.

You have some hunks which will need updating now that the CHKP/MPX bits
are gone.

So OK after the cleanups noted above and a fresh bootstrap & regression
test cycle.

jeff

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-06-12 21:11 ` Jeff Law
@ 2018-06-18 16:35   ` Martin Sebor
  2018-07-09 14:36     ` Aldy Hernandez
  0 siblings, 1 reply; 22+ messages in thread
From: Martin Sebor @ 2018-06-18 16:35 UTC (permalink / raw)
  To: Jeff Law, Gcc Patch List

On 06/12/2018 03:11 PM, Jeff Law wrote:
> On 06/05/2018 03:43 PM, Martin Sebor wrote:
>> The attached patch adds basic support for handling strnlen
>> as a built-in function.  It touches the strlen pass where
>> it folds constant results of the function, and builtins.c
>> to add simple support for expanding strnlen calls with known
>> results.  It also changes calls.c to detect excessive bounds
>> to the function and unsafe calls with arguments declared
>> attribute nonstring.
>>
>> A side-effect of the strlen change I should call out is that
>> strlen() calls to all zero-length arrays that aren't considered
>> flexible array members (i.e., internal members or non-members)
>> are folded into zero.  No warning is issued for such invalid
>> uses of zero-length arrays but based on the responses to my
>> question Re: aliasing between internal zero-length-arrays and
>> other members(*) it sounds like one would be appropriate.
>> I will see about adding one in a separate patch.
>>
>> Martin
>>
>> [*] https://gcc.gnu.org/ml/gcc/2018-06/msg00046.html
>>
>> gcc-81384.diff
>>
>>
>> PR tree-optimization/81384 - built-in form of strnlen missing
>>
>> gcc/ChangeLog:
>>
>> 	PR tree-optimization/81384
>> 	* builtin-types.def (BT_FN_SIZE_CONST_STRING_SIZE): New.
>> 	* builtins.c (expand_builtin_strnlen): New function.
>> 	(expand_builtin): Call it.
>> 	(fold_builtin_n): Avoid setting TREE_NO_WARNING.
>> 	* builtins.def (BUILT_IN_STRNLEN): New.
>> 	* calls.c (maybe_warn_nonstring_arg): Handle BUILT_IN_STRNLEN.
>> 	Warn for bounds in excess of maximum object size.
>> 	* tree-ssa-strlen.c (maybe_set_strlen_range): Return tree representing
>> 	single-value ranges.  Handle strnlen.
>> 	(handle_builtin_strlen): Handle strnlen.
>> 	(strlen_check_and_optimize_stmt): Same.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR tree-optimization/81384
>> 	* gcc.c-torture/execute/builtins/lib/strnlen.c: New test.
>> 	* gcc.c-torture/execute/builtins/strnlen-lib.c: New test.
>> 	* gcc.c-torture/execute/builtins/strnlen.c: New test.
>> 	* gcc.dg/attr-nonstring-2.c: New test.
>> 	* gcc.dg/attr-nonstring-3.c: New test.
>> 	* gcc.dg/attr-nonstring-4.c: New test.
>> 	* gcc.dg/strlenopt-44.c: New test.
>> 	* gcc.dg/strlenopt.h (strnlen):  Declare.
>>
>> diff --git a/gcc/builtin-types.def b/gcc/builtin-types.def
>> index 5365bef..1f15350 100644
>> --- a/gcc/builtin-types.def
>> +++ b/gcc/builtin-types.def
>> @@ -322,6 +322,8 @@ DEF_FUNCTION_TYPE_2 (BT_FN_STRING_CONST_STRING_INT,
>>  		     BT_STRING, BT_CONST_STRING, BT_INT)
>>  DEF_FUNCTION_TYPE_2 (BT_FN_STRING_CONST_STRING_SIZE,
>>  		     BT_STRING, BT_CONST_STRING, BT_SIZE)
>> +DEF_FUNCTION_TYPE_2 (BT_FN_SIZE_CONST_STRING_SIZE,
>> +		     BT_SIZE, BT_CONST_STRING, BT_SIZE)
>>  DEF_FUNCTION_TYPE_2 (BT_FN_INT_CONST_STRING_FILEPTR,
>>  		     BT_INT, BT_CONST_STRING, BT_FILEPTR)
>>  DEF_FUNCTION_TYPE_2 (BT_FN_INT_INT_FILEPTR,
> I believe Jakub already suggested these change and you ack'd that.
>
> You have some hunks which will need updating now that the CHKP/MPX bits
> are gone.
>
> So OK after the cleanups noted above and a fresh bootstrap & regression
> test cycle.
>

Done.  I also added documentation for the built-in, reran GCC
and Glibc tests with no regressions, and committed 261705.

Martin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-06-18 16:35   ` Martin Sebor
@ 2018-07-09 14:36     ` Aldy Hernandez
  2018-07-09 19:51       ` Jeff Law
  2018-07-09 21:26       ` Martin Sebor
  0 siblings, 2 replies; 22+ messages in thread
From: Aldy Hernandez @ 2018-07-09 14:36 UTC (permalink / raw)
  To: Martin Sebor; +Cc: Jeff Law, gcc-patches

   { dg-do run }
   { do-options "-O2 -fno-tree-strlen" }  */

^^^^ I don't think this is doing anything.

If you look at the test run you can see that -fno-tree-strlen is never
passed (I think you actually mean -fno-optimize-strlen for that
matter).  Also, the builtins.exp harness runs your test for an
assortment of other flags, not just -O2.

This test is failing on my range branch for -Og, because
expand_builtin_strnlen() needs range info:

+  wide_int min, max;
+  enum value_range_type rng = get_range_info (bound, &min, &max);
+  if (rng != VR_RANGE)
+    return NULL_RTX;

but interestingly enough, it seems to be calculated in the sprintf
pass as part of the DOM walk:

      /* First record ranges generated by this statement.  */
      evrp_range_analyzer.record_ranges_from_stmt (stmt, false);

It feels wrong that the sprintf warning pass is generating range info
that you may later depend on at rtl expansion time (and for a totally
unrelated thing-- strlen expansion).

I don't know if this is just a quirk of builtins.exp calling your test
with flags you didn't intend, but the inconsistency could cause
problems in the future.  Errr, or my present ;-).

Would it be too much to ask for you to either fix the flags being
passed down to the test, or better yet, find some non-sprintf
dependent way of calculating range info earlier?

Aldy
On Mon, Jun 18, 2018 at 6:35 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 06/12/2018 03:11 PM, Jeff Law wrote:
> > On 06/05/2018 03:43 PM, Martin Sebor wrote:
> >> The attached patch adds basic support for handling strnlen
> >> as a built-in function.  It touches the strlen pass where
> >> it folds constant results of the function, and builtins.c
> >> to add simple support for expanding strnlen calls with known
> >> results.  It also changes calls.c to detect excessive bounds
> >> to the function and unsafe calls with arguments declared
> >> attribute nonstring.
> >>
> >> A side-effect of the strlen change I should call out is that
> >> strlen() calls to all zero-length arrays that aren't considered
> >> flexible array members (i.e., internal members or non-members)
> >> are folded into zero.  No warning is issued for such invalid
> >> uses of zero-length arrays but based on the responses to my
> >> question Re: aliasing between internal zero-length-arrays and
> >> other members(*) it sounds like one would be appropriate.
> >> I will see about adding one in a separate patch.
> >>
> >> Martin
> >>
> >> [*] https://gcc.gnu.org/ml/gcc/2018-06/msg00046.html
> >>
> >> gcc-81384.diff
> >>
> >>
> >> PR tree-optimization/81384 - built-in form of strnlen missing
> >>
> >> gcc/ChangeLog:
> >>
> >>      PR tree-optimization/81384
> >>      * builtin-types.def (BT_FN_SIZE_CONST_STRING_SIZE): New.
> >>      * builtins.c (expand_builtin_strnlen): New function.
> >>      (expand_builtin): Call it.
> >>      (fold_builtin_n): Avoid setting TREE_NO_WARNING.
> >>      * builtins.def (BUILT_IN_STRNLEN): New.
> >>      * calls.c (maybe_warn_nonstring_arg): Handle BUILT_IN_STRNLEN.
> >>      Warn for bounds in excess of maximum object size.
> >>      * tree-ssa-strlen.c (maybe_set_strlen_range): Return tree representing
> >>      single-value ranges.  Handle strnlen.
> >>      (handle_builtin_strlen): Handle strnlen.
> >>      (strlen_check_and_optimize_stmt): Same.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >>      PR tree-optimization/81384
> >>      * gcc.c-torture/execute/builtins/lib/strnlen.c: New test.
> >>      * gcc.c-torture/execute/builtins/strnlen-lib.c: New test.
> >>      * gcc.c-torture/execute/builtins/strnlen.c: New test.
> >>      * gcc.dg/attr-nonstring-2.c: New test.
> >>      * gcc.dg/attr-nonstring-3.c: New test.
> >>      * gcc.dg/attr-nonstring-4.c: New test.
> >>      * gcc.dg/strlenopt-44.c: New test.
> >>      * gcc.dg/strlenopt.h (strnlen):  Declare.
> >>
> >> diff --git a/gcc/builtin-types.def b/gcc/builtin-types.def
> >> index 5365bef..1f15350 100644
> >> --- a/gcc/builtin-types.def
> >> +++ b/gcc/builtin-types.def
> >> @@ -322,6 +322,8 @@ DEF_FUNCTION_TYPE_2 (BT_FN_STRING_CONST_STRING_INT,
> >>                   BT_STRING, BT_CONST_STRING, BT_INT)
> >>  DEF_FUNCTION_TYPE_2 (BT_FN_STRING_CONST_STRING_SIZE,
> >>                   BT_STRING, BT_CONST_STRING, BT_SIZE)
> >> +DEF_FUNCTION_TYPE_2 (BT_FN_SIZE_CONST_STRING_SIZE,
> >> +                 BT_SIZE, BT_CONST_STRING, BT_SIZE)
> >>  DEF_FUNCTION_TYPE_2 (BT_FN_INT_CONST_STRING_FILEPTR,
> >>                   BT_INT, BT_CONST_STRING, BT_FILEPTR)
> >>  DEF_FUNCTION_TYPE_2 (BT_FN_INT_INT_FILEPTR,
> > I believe Jakub already suggested these change and you ack'd that.
> >
> > You have some hunks which will need updating now that the CHKP/MPX bits
> > are gone.
> >
> > So OK after the cleanups noted above and a fresh bootstrap & regression
> > test cycle.
> >
>
> Done.  I also added documentation for the built-in, reran GCC
> and Glibc tests with no regressions, and committed 261705.
>
> Martin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-07-09 14:36     ` Aldy Hernandez
@ 2018-07-09 19:51       ` Jeff Law
  2018-07-09 20:51         ` Martin Sebor
  2018-07-09 21:26       ` Martin Sebor
  1 sibling, 1 reply; 22+ messages in thread
From: Jeff Law @ 2018-07-09 19:51 UTC (permalink / raw)
  To: Aldy Hernandez, Martin Sebor; +Cc: gcc-patches

On 07/09/2018 08:36 AM, Aldy Hernandez wrote:
>    { dg-do run }
>    { do-options "-O2 -fno-tree-strlen" }  */
> 
> ^^^^ I don't think this is doing anything.
> 
> If you look at the test run you can see that -fno-tree-strlen is never
> passed (I think you actually mean -fno-optimize-strlen for that
> matter).  Also, the builtins.exp harness runs your test for an
> assortment of other flags, not just -O2.
> 
> This test is failing on my range branch for -Og, because
> expand_builtin_strnlen() needs range info:
> 
> +  wide_int min, max;
> +  enum value_range_type rng = get_range_info (bound, &min, &max);
> +  if (rng != VR_RANGE)
> +    return NULL_RTX;
> 
> but interestingly enough, it seems to be calculated in the sprintf
> pass as part of the DOM walk:
> 
>       /* First record ranges generated by this statement.  */
>       evrp_range_analyzer.record_ranges_from_stmt (stmt, false);
> 
> It feels wrong that the sprintf warning pass is generating range info
> that you may later depend on at rtl expansion time (and for a totally
> unrelated thing-- strlen expansion).
We have a fair amount of expansion code these days that will use
globally valid range information if it's been computed.


> 
> I don't know if this is just a quirk of builtins.exp calling your test
> with flags you didn't intend, but the inconsistency could cause
> problems in the future.  Errr, or my present ;-).
> 
> Would it be too much to ask for you to either fix the flags being
> passed down to the test, or better yet, find some non-sprintf
> dependent way of calculating range info earlier?
I think the general issue here is if we do something like
-O <-fblahblahblah> -Wsprintf-blah

Where <blahblahblah> is whatever aggregate of -f options are need to
disable any optimizer that generates range information.

In that case the sprintf warning pass becomes the only pass that
generates ranges.  So whether or not we run the sprintf warning pass
would affect the generated code.  And I think that's really the bigger
issue here -- warnings should affect code generation.

The question is what to do about it.  It probably wouldn't be too
terrible to have clients of the EVRP analyzer to specify if any
discovered ranges should be mirrored into the global range information.

Optimization passes would ask for the mirroring, the sprintf pass or any
other warning pass would not ask for mirroring.

Thoughts?

jeff

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-07-09 19:51       ` Jeff Law
@ 2018-07-09 20:51         ` Martin Sebor
  0 siblings, 0 replies; 22+ messages in thread
From: Martin Sebor @ 2018-07-09 20:51 UTC (permalink / raw)
  To: Jeff Law, Aldy Hernandez; +Cc: gcc-patches

On 07/09/2018 01:51 PM, Jeff Law wrote:
> On 07/09/2018 08:36 AM, Aldy Hernandez wrote:
>>    { dg-do run }
>>    { do-options "-O2 -fno-tree-strlen" }  */
>>
>> ^^^^ I don't think this is doing anything.
>>
>> If you look at the test run you can see that -fno-tree-strlen is never
>> passed (I think you actually mean -fno-optimize-strlen for that
>> matter).  Also, the builtins.exp harness runs your test for an
>> assortment of other flags, not just -O2.
>>
>> This test is failing on my range branch for -Og, because
>> expand_builtin_strnlen() needs range info:
>>
>> +  wide_int min, max;
>> +  enum value_range_type rng = get_range_info (bound, &min, &max);
>> +  if (rng != VR_RANGE)
>> +    return NULL_RTX;
>>
>> but interestingly enough, it seems to be calculated in the sprintf
>> pass as part of the DOM walk:
>>
>>       /* First record ranges generated by this statement.  */
>>       evrp_range_analyzer.record_ranges_from_stmt (stmt, false);
>>
>> It feels wrong that the sprintf warning pass is generating range info
>> that you may later depend on at rtl expansion time (and for a totally
>> unrelated thing-- strlen expansion).
> We have a fair amount of expansion code these days that will use
> globally valid range information if it's been computed.
>
>
>>
>> I don't know if this is just a quirk of builtins.exp calling your test
>> with flags you didn't intend, but the inconsistency could cause
>> problems in the future.  Errr, or my present ;-).
>>
>> Would it be too much to ask for you to either fix the flags being
>> passed down to the test, or better yet, find some non-sprintf
>> dependent way of calculating range info earlier?
> I think the general issue here is if we do something like
> -O <-fblahblahblah> -Wsprintf-blah
>
> Where <blahblahblah> is whatever aggregate of -f options are need to
> disable any optimizer that generates range information.
>
> In that case the sprintf warning pass becomes the only pass that
> generates ranges.  So whether or not we run the sprintf warning pass
> would affect the generated code.  And I think that's really the bigger
> issue here -- warnings should affect code generation.
>
> The question is what to do about it.  It probably wouldn't be too
> terrible to have clients of the EVRP analyzer to specify if any
> discovered ranges should be mirrored into the global range information.
>
> Optimization passes would ask for the mirroring, the sprintf pass or any
> other warning pass would not ask for mirroring.
>
> Thoughts?

The sprintf pass doesn't just emit warnings -- it also performs
the sprintf optimizations, and the two functions are independent
of one another.  I'm pretty sure there are tests to verify that
it's so.

The sprintf optimization include replacing constant calls with
their results and setting ranges for others.  As you mentioned,
the sprintf pass isn't the only one in GCC that does the latter.
The strlen pass does as well, and so does gimple-fold.c.  There
is also a call to set_range_info() in tree-vect-loop-manip.c
though I'm not familiar with that one yet.

Martin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-07-09 14:36     ` Aldy Hernandez
  2018-07-09 19:51       ` Jeff Law
@ 2018-07-09 21:26       ` Martin Sebor
  2018-07-10  7:13         ` Richard Biener
  1 sibling, 1 reply; 22+ messages in thread
From: Martin Sebor @ 2018-07-09 21:26 UTC (permalink / raw)
  To: Aldy Hernandez; +Cc: Jeff Law, gcc-patches

On 07/09/2018 08:36 AM, Aldy Hernandez wrote:
>    { dg-do run }
>    { do-options "-O2 -fno-tree-strlen" }  */
>
> ^^^^ I don't think this is doing anything.
>
> If you look at the test run you can see that -fno-tree-strlen is never
> passed (I think you actually mean -fno-optimize-strlen for that
> matter).  Also, the builtins.exp harness runs your test for an
> assortment of other flags, not just -O2.

I didn't know the harness ignores dg-options specified in these
tests.  That's surprising and feels like a bug in the harness
not to complain about it.  The purpose of the test is to verify
that the strnlen expansion in builtins.c does the right thing
and it deliberately tries to disable the earlier strlen
optimizations to make sure the expansion in builtins.c is fully
exercised.  By not pointing out my mistake the harness effectively
let me commit a change without making sure it's thoroughly tested
(I tested it manually before committing the patch but things could
regress without us noticing).  I'll look into fixing this somehow.

>
> This test is failing on my range branch for -Og, because
> expand_builtin_strnlen() needs range info:
>
> +  wide_int min, max;
> +  enum value_range_type rng = get_range_info (bound, &min, &max);
> +  if (rng != VR_RANGE)
> +    return NULL_RTX;
>
> but interestingly enough, it seems to be calculated in the sprintf
> pass as part of the DOM walk:
>
>       /* First record ranges generated by this statement.  */
>       evrp_range_analyzer.record_ranges_from_stmt (stmt, false);
>
> It feels wrong that the sprintf warning pass is generating range info
> that you may later depend on at rtl expansion time (and for a totally
> unrelated thing-- strlen expansion).

Any pass that records ranges for statements will have this
effect.  The sprintf pass seems to be the first one to make
use of this utility (and it's not just a warning pass but also
an optimization pass) but it would be a shame to put it off
limits to warning-only passes only because it happens to set
ranges.

>
> I don't know if this is just a quirk of builtins.exp calling your test
> with flags you didn't intend, but the inconsistency could cause
> problems in the future.  Errr, or my present ;-).
>
> Would it be too much to ask for you to either fix the flags being
> passed down to the test, or better yet, find some non-sprintf
> dependent way of calculating range info earlier?

At the time I wrote the test I didn't realize the statement
range info was being computed only in the sprintf pass --
I thought it was done as "a basic service for the greater
good" by VRP.  It seems that it should be such a service.

Let me look into tweaking the test.

Martin

>
> Aldy
> On Mon, Jun 18, 2018 at 6:35 PM Martin Sebor <msebor@gmail.com> wrote:
>>
>> On 06/12/2018 03:11 PM, Jeff Law wrote:
>>> On 06/05/2018 03:43 PM, Martin Sebor wrote:
>>>> The attached patch adds basic support for handling strnlen
>>>> as a built-in function.  It touches the strlen pass where
>>>> it folds constant results of the function, and builtins.c
>>>> to add simple support for expanding strnlen calls with known
>>>> results.  It also changes calls.c to detect excessive bounds
>>>> to the function and unsafe calls with arguments declared
>>>> attribute nonstring.
>>>>
>>>> A side-effect of the strlen change I should call out is that
>>>> strlen() calls to all zero-length arrays that aren't considered
>>>> flexible array members (i.e., internal members or non-members)
>>>> are folded into zero.  No warning is issued for such invalid
>>>> uses of zero-length arrays but based on the responses to my
>>>> question Re: aliasing between internal zero-length-arrays and
>>>> other members(*) it sounds like one would be appropriate.
>>>> I will see about adding one in a separate patch.
>>>>
>>>> Martin
>>>>
>>>> [*] https://gcc.gnu.org/ml/gcc/2018-06/msg00046.html
>>>>
>>>> gcc-81384.diff
>>>>
>>>>
>>>> PR tree-optimization/81384 - built-in form of strnlen missing
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>>      PR tree-optimization/81384
>>>>      * builtin-types.def (BT_FN_SIZE_CONST_STRING_SIZE): New.
>>>>      * builtins.c (expand_builtin_strnlen): New function.
>>>>      (expand_builtin): Call it.
>>>>      (fold_builtin_n): Avoid setting TREE_NO_WARNING.
>>>>      * builtins.def (BUILT_IN_STRNLEN): New.
>>>>      * calls.c (maybe_warn_nonstring_arg): Handle BUILT_IN_STRNLEN.
>>>>      Warn for bounds in excess of maximum object size.
>>>>      * tree-ssa-strlen.c (maybe_set_strlen_range): Return tree representing
>>>>      single-value ranges.  Handle strnlen.
>>>>      (handle_builtin_strlen): Handle strnlen.
>>>>      (strlen_check_and_optimize_stmt): Same.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>>      PR tree-optimization/81384
>>>>      * gcc.c-torture/execute/builtins/lib/strnlen.c: New test.
>>>>      * gcc.c-torture/execute/builtins/strnlen-lib.c: New test.
>>>>      * gcc.c-torture/execute/builtins/strnlen.c: New test.
>>>>      * gcc.dg/attr-nonstring-2.c: New test.
>>>>      * gcc.dg/attr-nonstring-3.c: New test.
>>>>      * gcc.dg/attr-nonstring-4.c: New test.
>>>>      * gcc.dg/strlenopt-44.c: New test.
>>>>      * gcc.dg/strlenopt.h (strnlen):  Declare.
>>>>
>>>> diff --git a/gcc/builtin-types.def b/gcc/builtin-types.def
>>>> index 5365bef..1f15350 100644
>>>> --- a/gcc/builtin-types.def
>>>> +++ b/gcc/builtin-types.def
>>>> @@ -322,6 +322,8 @@ DEF_FUNCTION_TYPE_2 (BT_FN_STRING_CONST_STRING_INT,
>>>>                   BT_STRING, BT_CONST_STRING, BT_INT)
>>>>  DEF_FUNCTION_TYPE_2 (BT_FN_STRING_CONST_STRING_SIZE,
>>>>                   BT_STRING, BT_CONST_STRING, BT_SIZE)
>>>> +DEF_FUNCTION_TYPE_2 (BT_FN_SIZE_CONST_STRING_SIZE,
>>>> +                 BT_SIZE, BT_CONST_STRING, BT_SIZE)
>>>>  DEF_FUNCTION_TYPE_2 (BT_FN_INT_CONST_STRING_FILEPTR,
>>>>                   BT_INT, BT_CONST_STRING, BT_FILEPTR)
>>>>  DEF_FUNCTION_TYPE_2 (BT_FN_INT_INT_FILEPTR,
>>> I believe Jakub already suggested these change and you ack'd that.
>>>
>>> You have some hunks which will need updating now that the CHKP/MPX bits
>>> are gone.
>>>
>>> So OK after the cleanups noted above and a fresh bootstrap & regression
>>> test cycle.
>>>
>>
>> Done.  I also added documentation for the built-in, reran GCC
>> and Glibc tests with no regressions, and committed 261705.
>>
>> Martin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-07-09 21:26       ` Martin Sebor
@ 2018-07-10  7:13         ` Richard Biener
  2018-07-10 14:10           ` Martin Sebor
  0 siblings, 1 reply; 22+ messages in thread
From: Richard Biener @ 2018-07-10  7:13 UTC (permalink / raw)
  To: Martin Sebor; +Cc: Aldy Hernandez, Jeff Law, GCC Patches

On Mon, Jul 9, 2018 at 11:26 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 07/09/2018 08:36 AM, Aldy Hernandez wrote:
> >    { dg-do run }
> >    { do-options "-O2 -fno-tree-strlen" }  */
> >
> > ^^^^ I don't think this is doing anything.
> >
> > If you look at the test run you can see that -fno-tree-strlen is never
> > passed (I think you actually mean -fno-optimize-strlen for that
> > matter).  Also, the builtins.exp harness runs your test for an
> > assortment of other flags, not just -O2.
>
> I didn't know the harness ignores dg-options specified in these
> tests.  That's surprising and feels like a bug in the harness
> not to complain about it.  The purpose of the test is to verify
> that the strnlen expansion in builtins.c does the right thing
> and it deliberately tries to disable the earlier strlen
> optimizations to make sure the expansion in builtins.c is fully
> exercised.  By not pointing out my mistake the harness effectively
> let me commit a change without making sure it's thoroughly tested
> (I tested it manually before committing the patch but things could
> regress without us noticing).  I'll look into fixing this somehow.
>
> >
> > This test is failing on my range branch for -Og, because
> > expand_builtin_strnlen() needs range info:
> >
> > +  wide_int min, max;
> > +  enum value_range_type rng = get_range_info (bound, &min, &max);
> > +  if (rng != VR_RANGE)
> > +    return NULL_RTX;
> >
> > but interestingly enough, it seems to be calculated in the sprintf
> > pass as part of the DOM walk:
> >
> >       /* First record ranges generated by this statement.  */
> >       evrp_range_analyzer.record_ranges_from_stmt (stmt, false);
> >
> > It feels wrong that the sprintf warning pass is generating range info
> > that you may later depend on at rtl expansion time (and for a totally
> > unrelated thing-- strlen expansion).
>
> Any pass that records ranges for statements will have this
> effect.  The sprintf pass seems to be the first one to make
> use of this utility (and it's not just a warning pass but also
> an optimization pass) but it would be a shame to put it off
> limits to warning-only passes only because it happens to set
> ranges.

As you noted elsewhere warning options shouldn't change code-generation.
This means that ranges may not be set to the IL in case they are only
computed when a warning option is enabled.

Richard.

> >
> > I don't know if this is just a quirk of builtins.exp calling your test
> > with flags you didn't intend, but the inconsistency could cause
> > problems in the future.  Errr, or my present ;-).
> >
> > Would it be too much to ask for you to either fix the flags being
> > passed down to the test, or better yet, find some non-sprintf
> > dependent way of calculating range info earlier?
>
> At the time I wrote the test I didn't realize the statement
> range info was being computed only in the sprintf pass --
> I thought it was done as "a basic service for the greater
> good" by VRP.  It seems that it should be such a service.
>
> Let me look into tweaking the test.
>
> Martin
>
> >
> > Aldy
> > On Mon, Jun 18, 2018 at 6:35 PM Martin Sebor <msebor@gmail.com> wrote:
> >>
> >> On 06/12/2018 03:11 PM, Jeff Law wrote:
> >>> On 06/05/2018 03:43 PM, Martin Sebor wrote:
> >>>> The attached patch adds basic support for handling strnlen
> >>>> as a built-in function.  It touches the strlen pass where
> >>>> it folds constant results of the function, and builtins.c
> >>>> to add simple support for expanding strnlen calls with known
> >>>> results.  It also changes calls.c to detect excessive bounds
> >>>> to the function and unsafe calls with arguments declared
> >>>> attribute nonstring.
> >>>>
> >>>> A side-effect of the strlen change I should call out is that
> >>>> strlen() calls to all zero-length arrays that aren't considered
> >>>> flexible array members (i.e., internal members or non-members)
> >>>> are folded into zero.  No warning is issued for such invalid
> >>>> uses of zero-length arrays but based on the responses to my
> >>>> question Re: aliasing between internal zero-length-arrays and
> >>>> other members(*) it sounds like one would be appropriate.
> >>>> I will see about adding one in a separate patch.
> >>>>
> >>>> Martin
> >>>>
> >>>> [*] https://gcc.gnu.org/ml/gcc/2018-06/msg00046.html
> >>>>
> >>>> gcc-81384.diff
> >>>>
> >>>>
> >>>> PR tree-optimization/81384 - built-in form of strnlen missing
> >>>>
> >>>> gcc/ChangeLog:
> >>>>
> >>>>      PR tree-optimization/81384
> >>>>      * builtin-types.def (BT_FN_SIZE_CONST_STRING_SIZE): New.
> >>>>      * builtins.c (expand_builtin_strnlen): New function.
> >>>>      (expand_builtin): Call it.
> >>>>      (fold_builtin_n): Avoid setting TREE_NO_WARNING.
> >>>>      * builtins.def (BUILT_IN_STRNLEN): New.
> >>>>      * calls.c (maybe_warn_nonstring_arg): Handle BUILT_IN_STRNLEN.
> >>>>      Warn for bounds in excess of maximum object size.
> >>>>      * tree-ssa-strlen.c (maybe_set_strlen_range): Return tree representing
> >>>>      single-value ranges.  Handle strnlen.
> >>>>      (handle_builtin_strlen): Handle strnlen.
> >>>>      (strlen_check_and_optimize_stmt): Same.
> >>>>
> >>>> gcc/testsuite/ChangeLog:
> >>>>
> >>>>      PR tree-optimization/81384
> >>>>      * gcc.c-torture/execute/builtins/lib/strnlen.c: New test.
> >>>>      * gcc.c-torture/execute/builtins/strnlen-lib.c: New test.
> >>>>      * gcc.c-torture/execute/builtins/strnlen.c: New test.
> >>>>      * gcc.dg/attr-nonstring-2.c: New test.
> >>>>      * gcc.dg/attr-nonstring-3.c: New test.
> >>>>      * gcc.dg/attr-nonstring-4.c: New test.
> >>>>      * gcc.dg/strlenopt-44.c: New test.
> >>>>      * gcc.dg/strlenopt.h (strnlen):  Declare.
> >>>>
> >>>> diff --git a/gcc/builtin-types.def b/gcc/builtin-types.def
> >>>> index 5365bef..1f15350 100644
> >>>> --- a/gcc/builtin-types.def
> >>>> +++ b/gcc/builtin-types.def
> >>>> @@ -322,6 +322,8 @@ DEF_FUNCTION_TYPE_2 (BT_FN_STRING_CONST_STRING_INT,
> >>>>                   BT_STRING, BT_CONST_STRING, BT_INT)
> >>>>  DEF_FUNCTION_TYPE_2 (BT_FN_STRING_CONST_STRING_SIZE,
> >>>>                   BT_STRING, BT_CONST_STRING, BT_SIZE)
> >>>> +DEF_FUNCTION_TYPE_2 (BT_FN_SIZE_CONST_STRING_SIZE,
> >>>> +                 BT_SIZE, BT_CONST_STRING, BT_SIZE)
> >>>>  DEF_FUNCTION_TYPE_2 (BT_FN_INT_CONST_STRING_FILEPTR,
> >>>>                   BT_INT, BT_CONST_STRING, BT_FILEPTR)
> >>>>  DEF_FUNCTION_TYPE_2 (BT_FN_INT_INT_FILEPTR,
> >>> I believe Jakub already suggested these change and you ack'd that.
> >>>
> >>> You have some hunks which will need updating now that the CHKP/MPX bits
> >>> are gone.
> >>>
> >>> So OK after the cleanups noted above and a fresh bootstrap & regression
> >>> test cycle.
> >>>
> >>
> >> Done.  I also added documentation for the built-in, reran GCC
> >> and Glibc tests with no regressions, and committed 261705.
> >>
> >> Martin
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-07-10  7:13         ` Richard Biener
@ 2018-07-10 14:10           ` Martin Sebor
  2018-07-10 14:25             ` Richard Biener
  0 siblings, 1 reply; 22+ messages in thread
From: Martin Sebor @ 2018-07-10 14:10 UTC (permalink / raw)
  To: Richard Biener; +Cc: Aldy Hernandez, Jeff Law, GCC Patches

On 07/10/2018 01:12 AM, Richard Biener wrote:
> On Mon, Jul 9, 2018 at 11:26 PM Martin Sebor <msebor@gmail.com> wrote:
>>
>> On 07/09/2018 08:36 AM, Aldy Hernandez wrote:
>>>    { dg-do run }
>>>    { do-options "-O2 -fno-tree-strlen" }  */
>>>
>>> ^^^^ I don't think this is doing anything.
>>>
>>> If you look at the test run you can see that -fno-tree-strlen is never
>>> passed (I think you actually mean -fno-optimize-strlen for that
>>> matter).  Also, the builtins.exp harness runs your test for an
>>> assortment of other flags, not just -O2.
>>
>> I didn't know the harness ignores dg-options specified in these
>> tests.  That's surprising and feels like a bug in the harness
>> not to complain about it.  The purpose of the test is to verify
>> that the strnlen expansion in builtins.c does the right thing
>> and it deliberately tries to disable the earlier strlen
>> optimizations to make sure the expansion in builtins.c is fully
>> exercised.  By not pointing out my mistake the harness effectively
>> let me commit a change without making sure it's thoroughly tested
>> (I tested it manually before committing the patch but things could
>> regress without us noticing).  I'll look into fixing this somehow.
>>
>>>
>>> This test is failing on my range branch for -Og, because
>>> expand_builtin_strnlen() needs range info:
>>>
>>> +  wide_int min, max;
>>> +  enum value_range_type rng = get_range_info (bound, &min, &max);
>>> +  if (rng != VR_RANGE)
>>> +    return NULL_RTX;
>>>
>>> but interestingly enough, it seems to be calculated in the sprintf
>>> pass as part of the DOM walk:
>>>
>>>       /* First record ranges generated by this statement.  */
>>>       evrp_range_analyzer.record_ranges_from_stmt (stmt, false);
>>>
>>> It feels wrong that the sprintf warning pass is generating range info
>>> that you may later depend on at rtl expansion time (and for a totally
>>> unrelated thing-- strlen expansion).
>>
>> Any pass that records ranges for statements will have this
>> effect.  The sprintf pass seems to be the first one to make
>> use of this utility (and it's not just a warning pass but also
>> an optimization pass) but it would be a shame to put it off
>> limits to warning-only passes only because it happens to set
>> ranges.
>
> As you noted elsewhere warning options shouldn't change code-generation.
> This means that ranges may not be set to the IL in case they are only
> computed when a warning option is enabled.

I agree.  That's also why I think it should be a basic service
rather than a side-effect of tree traversal, either one done
to implement a particular optimization, or one done by a warning.

The main reason for the work Jeff put in to extracting it out
of EVRP, if I recall correctly, was to expose more accurate
range information to warning passes.  Would setting statement
ranges make sense as part of EVRP (or some other similar pass)?
If not, the only way to conform to the policy that I can think
of is to have warning-only  passes that need it make
the traversal and call record_ranges regardless of whether or
not the warning itself is enabled.  That would seem needlessly
inefficient, even if the code implementing the warning itself
were disabled.

Martin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-07-10 14:10           ` Martin Sebor
@ 2018-07-10 14:25             ` Richard Biener
  2018-07-10 14:34               ` Jeff Law
  0 siblings, 1 reply; 22+ messages in thread
From: Richard Biener @ 2018-07-10 14:25 UTC (permalink / raw)
  To: Martin Sebor; +Cc: Aldy Hernandez, Jeff Law, GCC Patches

On Tue, Jul 10, 2018 at 4:10 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 07/10/2018 01:12 AM, Richard Biener wrote:
> > On Mon, Jul 9, 2018 at 11:26 PM Martin Sebor <msebor@gmail.com> wrote:
> >>
> >> On 07/09/2018 08:36 AM, Aldy Hernandez wrote:
> >>>    { dg-do run }
> >>>    { do-options "-O2 -fno-tree-strlen" }  */
> >>>
> >>> ^^^^ I don't think this is doing anything.
> >>>
> >>> If you look at the test run you can see that -fno-tree-strlen is never
> >>> passed (I think you actually mean -fno-optimize-strlen for that
> >>> matter).  Also, the builtins.exp harness runs your test for an
> >>> assortment of other flags, not just -O2.
> >>
> >> I didn't know the harness ignores dg-options specified in these
> >> tests.  That's surprising and feels like a bug in the harness
> >> not to complain about it.  The purpose of the test is to verify
> >> that the strnlen expansion in builtins.c does the right thing
> >> and it deliberately tries to disable the earlier strlen
> >> optimizations to make sure the expansion in builtins.c is fully
> >> exercised.  By not pointing out my mistake the harness effectively
> >> let me commit a change without making sure it's thoroughly tested
> >> (I tested it manually before committing the patch but things could
> >> regress without us noticing).  I'll look into fixing this somehow.
> >>
> >>>
> >>> This test is failing on my range branch for -Og, because
> >>> expand_builtin_strnlen() needs range info:
> >>>
> >>> +  wide_int min, max;
> >>> +  enum value_range_type rng = get_range_info (bound, &min, &max);
> >>> +  if (rng != VR_RANGE)
> >>> +    return NULL_RTX;
> >>>
> >>> but interestingly enough, it seems to be calculated in the sprintf
> >>> pass as part of the DOM walk:
> >>>
> >>>       /* First record ranges generated by this statement.  */
> >>>       evrp_range_analyzer.record_ranges_from_stmt (stmt, false);
> >>>
> >>> It feels wrong that the sprintf warning pass is generating range info
> >>> that you may later depend on at rtl expansion time (and for a totally
> >>> unrelated thing-- strlen expansion).
> >>
> >> Any pass that records ranges for statements will have this
> >> effect.  The sprintf pass seems to be the first one to make
> >> use of this utility (and it's not just a warning pass but also
> >> an optimization pass) but it would be a shame to put it off
> >> limits to warning-only passes only because it happens to set
> >> ranges.
> >
> > As you noted elsewhere warning options shouldn't change code-generation.
> > This means that ranges may not be set to the IL in case they are only
> > computed when a warning option is enabled.
>
> I agree.  That's also why I think it should be a basic service
> rather than a side-effect of tree traversal, either one done
> to implement a particular optimization, or one done by a warning.
>
> The main reason for the work Jeff put in to extracting it out
> of EVRP, if I recall correctly, was to expose more accurate
> range information to warning passes.  Would setting statement
> ranges make sense as part of EVRP (or some other similar pass)?
> If not, the only way to conform to the policy that I can think
> of is to have warning-only  passes that need it make
> the traversal and call record_ranges regardless of whether or
> not the warning itself is enabled.  That would seem needlessly
> inefficient, even if the code implementing the warning itself
> were disabled.

Well, simply not set range-info on SSA names but use the
lattice values?  Should be easy to key that to a flag.

Richard.

>
> Martin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-07-10 14:25             ` Richard Biener
@ 2018-07-10 14:34               ` Jeff Law
  2018-07-10 14:57                 ` Martin Sebor
  0 siblings, 1 reply; 22+ messages in thread
From: Jeff Law @ 2018-07-10 14:34 UTC (permalink / raw)
  To: Richard Biener, Martin Sebor; +Cc: Aldy Hernandez, GCC Patches

On 07/10/2018 08:25 AM, Richard Biener wrote:
> On Tue, Jul 10, 2018 at 4:10 PM Martin Sebor <msebor@gmail.com> wrote:
>>
>> On 07/10/2018 01:12 AM, Richard Biener wrote:
>>> On Mon, Jul 9, 2018 at 11:26 PM Martin Sebor <msebor@gmail.com> wrote:
>>>>
>>>> On 07/09/2018 08:36 AM, Aldy Hernandez wrote:
>>>>>    { dg-do run }
>>>>>    { do-options "-O2 -fno-tree-strlen" }  */
>>>>>
>>>>> ^^^^ I don't think this is doing anything.
>>>>>
>>>>> If you look at the test run you can see that -fno-tree-strlen is never
>>>>> passed (I think you actually mean -fno-optimize-strlen for that
>>>>> matter).  Also, the builtins.exp harness runs your test for an
>>>>> assortment of other flags, not just -O2.
>>>>
>>>> I didn't know the harness ignores dg-options specified in these
>>>> tests.  That's surprising and feels like a bug in the harness
>>>> not to complain about it.  The purpose of the test is to verify
>>>> that the strnlen expansion in builtins.c does the right thing
>>>> and it deliberately tries to disable the earlier strlen
>>>> optimizations to make sure the expansion in builtins.c is fully
>>>> exercised.  By not pointing out my mistake the harness effectively
>>>> let me commit a change without making sure it's thoroughly tested
>>>> (I tested it manually before committing the patch but things could
>>>> regress without us noticing).  I'll look into fixing this somehow.
>>>>
>>>>>
>>>>> This test is failing on my range branch for -Og, because
>>>>> expand_builtin_strnlen() needs range info:
>>>>>
>>>>> +  wide_int min, max;
>>>>> +  enum value_range_type rng = get_range_info (bound, &min, &max);
>>>>> +  if (rng != VR_RANGE)
>>>>> +    return NULL_RTX;
>>>>>
>>>>> but interestingly enough, it seems to be calculated in the sprintf
>>>>> pass as part of the DOM walk:
>>>>>
>>>>>       /* First record ranges generated by this statement.  */
>>>>>       evrp_range_analyzer.record_ranges_from_stmt (stmt, false);
>>>>>
>>>>> It feels wrong that the sprintf warning pass is generating range info
>>>>> that you may later depend on at rtl expansion time (and for a totally
>>>>> unrelated thing-- strlen expansion).
>>>>
>>>> Any pass that records ranges for statements will have this
>>>> effect.  The sprintf pass seems to be the first one to make
>>>> use of this utility (and it's not just a warning pass but also
>>>> an optimization pass) but it would be a shame to put it off
>>>> limits to warning-only passes only because it happens to set
>>>> ranges.
>>>
>>> As you noted elsewhere warning options shouldn't change code-generation.
>>> This means that ranges may not be set to the IL in case they are only
>>> computed when a warning option is enabled.
>>
>> I agree.  That's also why I think it should be a basic service
>> rather than a side-effect of tree traversal, either one done
>> to implement a particular optimization, or one done by a warning.
>>
>> The main reason for the work Jeff put in to extracting it out
>> of EVRP, if I recall correctly, was to expose more accurate
>> range information to warning passes.  Would setting statement
>> ranges make sense as part of EVRP (or some other similar pass)?
>> If not, the only way to conform to the policy that I can think
>> of is to have warning-only  passes that need it make
>> the traversal and call record_ranges regardless of whether or
>> not the warning itself is enabled.  That would seem needlessly
>> inefficient, even if the code implementing the warning itself
>> were disabled.
> 
> Well, simply not set range-info on SSA names but use the
> lattice values?  Should be easy to key that to a flag.
Right.  That was essentially my suggestion.  For a client that uses EVRP
analysis to drive warnings, they do not mirror results into the global
range info we attach to SSA_NAMEs.  An optimization pass which utilizes
EVRP can (of course) set the global range info.

THe sprintf bits are a bit tricky as it's a mix of warning and
optimization, but I think there's enough separation that we can arrange
to do the right thing.

Since I introduced EVRP into the sprintf bits, I'm happy to own fixing
this up.  I just wanted to get general agreement on the approach.

jeff

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-07-10 14:34               ` Jeff Law
@ 2018-07-10 14:57                 ` Martin Sebor
  0 siblings, 0 replies; 22+ messages in thread
From: Martin Sebor @ 2018-07-10 14:57 UTC (permalink / raw)
  To: Jeff Law, Richard Biener; +Cc: Aldy Hernandez, GCC Patches

On 07/10/2018 08:34 AM, Jeff Law wrote:
> On 07/10/2018 08:25 AM, Richard Biener wrote:
>> On Tue, Jul 10, 2018 at 4:10 PM Martin Sebor <msebor@gmail.com> wrote:
>>>
>>> On 07/10/2018 01:12 AM, Richard Biener wrote:
>>>> On Mon, Jul 9, 2018 at 11:26 PM Martin Sebor <msebor@gmail.com> wrote:
>>>>>
>>>>> On 07/09/2018 08:36 AM, Aldy Hernandez wrote:
>>>>>>    { dg-do run }
>>>>>>    { do-options "-O2 -fno-tree-strlen" }  */
>>>>>>
>>>>>> ^^^^ I don't think this is doing anything.
>>>>>>
>>>>>> If you look at the test run you can see that -fno-tree-strlen is never
>>>>>> passed (I think you actually mean -fno-optimize-strlen for that
>>>>>> matter).  Also, the builtins.exp harness runs your test for an
>>>>>> assortment of other flags, not just -O2.
>>>>>
>>>>> I didn't know the harness ignores dg-options specified in these
>>>>> tests.  That's surprising and feels like a bug in the harness
>>>>> not to complain about it.  The purpose of the test is to verify
>>>>> that the strnlen expansion in builtins.c does the right thing
>>>>> and it deliberately tries to disable the earlier strlen
>>>>> optimizations to make sure the expansion in builtins.c is fully
>>>>> exercised.  By not pointing out my mistake the harness effectively
>>>>> let me commit a change without making sure it's thoroughly tested
>>>>> (I tested it manually before committing the patch but things could
>>>>> regress without us noticing).  I'll look into fixing this somehow.
>>>>>
>>>>>>
>>>>>> This test is failing on my range branch for -Og, because
>>>>>> expand_builtin_strnlen() needs range info:
>>>>>>
>>>>>> +  wide_int min, max;
>>>>>> +  enum value_range_type rng = get_range_info (bound, &min, &max);
>>>>>> +  if (rng != VR_RANGE)
>>>>>> +    return NULL_RTX;
>>>>>>
>>>>>> but interestingly enough, it seems to be calculated in the sprintf
>>>>>> pass as part of the DOM walk:
>>>>>>
>>>>>>       /* First record ranges generated by this statement.  */
>>>>>>       evrp_range_analyzer.record_ranges_from_stmt (stmt, false);
>>>>>>
>>>>>> It feels wrong that the sprintf warning pass is generating range info
>>>>>> that you may later depend on at rtl expansion time (and for a totally
>>>>>> unrelated thing-- strlen expansion).
>>>>>
>>>>> Any pass that records ranges for statements will have this
>>>>> effect.  The sprintf pass seems to be the first one to make
>>>>> use of this utility (and it's not just a warning pass but also
>>>>> an optimization pass) but it would be a shame to put it off
>>>>> limits to warning-only passes only because it happens to set
>>>>> ranges.
>>>>
>>>> As you noted elsewhere warning options shouldn't change code-generation.
>>>> This means that ranges may not be set to the IL in case they are only
>>>> computed when a warning option is enabled.
>>>
>>> I agree.  That's also why I think it should be a basic service
>>> rather than a side-effect of tree traversal, either one done
>>> to implement a particular optimization, or one done by a warning.
>>>
>>> The main reason for the work Jeff put in to extracting it out
>>> of EVRP, if I recall correctly, was to expose more accurate
>>> range information to warning passes.  Would setting statement
>>> ranges make sense as part of EVRP (or some other similar pass)?
>>> If not, the only way to conform to the policy that I can think
>>> of is to have warning-only  passes that need it make
>>> the traversal and call record_ranges regardless of whether or
>>> not the warning itself is enabled.  That would seem needlessly
>>> inefficient, even if the code implementing the warning itself
>>> were disabled.
>>
>> Well, simply not set range-info on SSA names but use the
>> lattice values?  Should be easy to key that to a flag.
> Right.  That was essentially my suggestion.  For a client that uses EVRP
> analysis to drive warnings, they do not mirror results into the global
> range info we attach to SSA_NAMEs.  An optimization pass which utilizes
> EVRP can (of course) set the global range info.
>
> THe sprintf bits are a bit tricky as it's a mix of warning and
> optimization, but I think there's enough separation that we can arrange
> to do the right thing.
>
> Since I introduced EVRP into the sprintf bits, I'm happy to own fixing
> this up.  I just wanted to get general agreement on the approach.

I'm not sure I understand what about the sprintf pass needs
changing since that the warning is independent of the optimization.
The gate function makes the intent clear:

pass_sprintf_length::gate (function *)
{
   /* Run the pass iff -Warn-format-overflow or -Warn-format-truncation
      is specified and either not optimizing and the pass is being invoked
      early, or when optimizing and the pass is being invoked during
      optimization (i.e., "late").  */
   return ((warn_format_overflow > 0
	   || warn_format_trunc > 0
	   || flag_printf_return_value)
	  && (optimize > 0) == fold_return_value);
}

The only other use of the warn_format_overflow and
warn_format_trunc variables is to set the warn_level variable,
and that one is only used to affect the LIKELY counter which
is used for warnings only but not for optimization.

Am I missing something?

Martin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
@ 2018-06-19 19:33 David Edelsohn
  2018-06-19 20:10 ` Martin Sebor
  0 siblings, 1 reply; 22+ messages in thread
From: David Edelsohn @ 2018-06-19 19:33 UTC (permalink / raw)
  To: Martin Sebor; +Cc: GCC Patches, Jeffrey Law

Martin,

Does attr-nonstring-3.c assume a 64 bit environment?

I'm seeing new errors on the rs6000 port

FAIL: gcc.dg/attr-nonstring-3.c (internal compiler error)
/nasfarm/edelsohn/src/src/gcc/testsuite/gcc.dg/attr-nonstring-3.c:73:1:
error: could not split insn
(insn 3244 3245 3246 (set (reg:SI 5 5)
        (const_int -2147483680 [0xffffffff7fffffe0]))
"/nasfarm/edelsohn/src/src/gcc/testsuite/gcc.dg/attr-nonstring-3.c":72
446 {*movsi_internal1}
     (nil))
during RTL pass: final
/nasfarm/edelsohn/src/src/gcc/testsuite/gcc.dg/attr-nonstring-3.c:73:1:
internal compiler error: in final_scan_insn_1, at final.c:3140
ranges offset out of range

Something is assuming that a 64 bit value can fit in a 32 bit register.

Thanks, David

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-06-19 19:33 David Edelsohn
@ 2018-06-19 20:10 ` Martin Sebor
  2018-06-19 20:25   ` Martin Sebor
  2018-06-20  5:24   ` Jeff Law
  0 siblings, 2 replies; 22+ messages in thread
From: Martin Sebor @ 2018-06-19 20:10 UTC (permalink / raw)
  To: David Edelsohn; +Cc: GCC Patches, Jeffrey Law

On 06/19/2018 01:33 PM, David Edelsohn wrote:
> Martin,
>
> Does attr-nonstring-3.c assume a 64 bit environment?

I don't think so.  The error below suggests a problem during
the expansion of the strncmp built-in.  The patch didn't
change that.  I see GCC 8 ICE on the test as well so it must
be a latent bug that the test somehow manages to tickle.  Let
me try to reduce it to a small test case and open a bug.

Martin

> I'm seeing new errors on the rs6000 port
>
> FAIL: gcc.dg/attr-nonstring-3.c (internal compiler error)
> /nasfarm/edelsohn/src/src/gcc/testsuite/gcc.dg/attr-nonstring-3.c:73:1:
> error: could not split insn
> (insn 3244 3245 3246 (set (reg:SI 5 5)
>         (const_int -2147483680 [0xffffffff7fffffe0]))
> "/nasfarm/edelsohn/src/src/gcc/testsuite/gcc.dg/attr-nonstring-3.c":72
> 446 {*movsi_internal1}
>      (nil))
> during RTL pass: final
> /nasfarm/edelsohn/src/src/gcc/testsuite/gcc.dg/attr-nonstring-3.c:73:1:
> internal compiler error: in final_scan_insn_1, at final.c:3140
> ranges offset out of range
>
> Something is assuming that a 64 bit value can fit in a 32 bit register.
>
> Thanks, David
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-06-19 20:10 ` Martin Sebor
@ 2018-06-19 20:25   ` Martin Sebor
  2018-06-20  5:24   ` Jeff Law
  1 sibling, 0 replies; 22+ messages in thread
From: Martin Sebor @ 2018-06-19 20:25 UTC (permalink / raw)
  To: David Edelsohn; +Cc: GCC Patches, Jeffrey Law

On 06/19/2018 02:10 PM, Martin Sebor wrote:
> On 06/19/2018 01:33 PM, David Edelsohn wrote:
>> Martin,
>>
>> Does attr-nonstring-3.c assume a 64 bit environment?
>
> I don't think so.  The error below suggests a problem during
> the expansion of the strncmp built-in.  The patch didn't
> change that.  I see GCC 8 ICE on the test as well so it must
> be a latent bug that the test somehow manages to tickle.  Let
> me try to reduce it to a small test case and open a bug.

I opened 86222 and CC'd you on it.  The problem can be triggered
by calling strncpy() with a constant bound between PTRDIFF_MAX
+ 1 and PTRDIFF_MAX + 32.

Martin

>
> Martin
>
>> I'm seeing new errors on the rs6000 port
>>
>> FAIL: gcc.dg/attr-nonstring-3.c (internal compiler error)
>> /nasfarm/edelsohn/src/src/gcc/testsuite/gcc.dg/attr-nonstring-3.c:73:1:
>> error: could not split insn
>> (insn 3244 3245 3246 (set (reg:SI 5 5)
>>         (const_int -2147483680 [0xffffffff7fffffe0]))
>> "/nasfarm/edelsohn/src/src/gcc/testsuite/gcc.dg/attr-nonstring-3.c":72
>> 446 {*movsi_internal1}
>>      (nil))
>> during RTL pass: final
>> /nasfarm/edelsohn/src/src/gcc/testsuite/gcc.dg/attr-nonstring-3.c:73:1:
>> internal compiler error: in final_scan_insn_1, at final.c:3140
>> ranges offset out of range
>>
>> Something is assuming that a 64 bit value can fit in a 32 bit register.
>>
>> Thanks, David
>>
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] add support for strnlen (PR 81384)
  2018-06-19 20:10 ` Martin Sebor
  2018-06-19 20:25   ` Martin Sebor
@ 2018-06-20  5:24   ` Jeff Law
  1 sibling, 0 replies; 22+ messages in thread
From: Jeff Law @ 2018-06-20  5:24 UTC (permalink / raw)
  To: Martin Sebor, David Edelsohn; +Cc: GCC Patches

On 06/19/2018 02:10 PM, Martin Sebor wrote:
> On 06/19/2018 01:33 PM, David Edelsohn wrote:
>> Martin,
>>
>> Does attr-nonstring-3.c assume a 64 bit environment?
> 
> I don't think so.Â  The error below suggests a problem during
> the expansion of the strncmp built-in.Â  The patch didn't
> change that.Â  I see GCC 8 ICE on the test as well so it must
> be a latent bug that the test somehow manages to tickle.Â  Let
> me try to reduce it to a small test case and open a bug.
Looks like a backend bug to me.  Most likely there's a pattern that was
matched, but is force-splitted via "#" in its output template.  If the
splitting process fails, then this is the error I'd expect to see.


> 
> Martin
> 
>> I'm seeing new errors on the rs6000 port
>>
>> FAIL: gcc.dg/attr-nonstring-3.c (internal compiler error)
>> /nasfarm/edelsohn/src/src/gcc/testsuite/gcc.dg/attr-nonstring-3.c:73:1:
>> error: could not split insn
>> (insn 3244 3245 3246 (set (reg:SI 5 5)
>> Â Â Â Â Â Â Â  (const_int -2147483680 [0xffffffff7fffffe0]))
>> "/nasfarm/edelsohn/src/src/gcc/testsuite/gcc.dg/attr-nonstring-3.c":72
>> 446 {*movsi_internal1}
>> Â Â Â Â  (nil))
>> during RTL pass: final
>> /nasfarm/edelsohn/src/src/gcc/testsuite/gcc.dg/attr-nonstring-3.c:73:1:
>> internal compiler error: in final_scan_insn_1, at final.c:3140
>> ranges offset out of range
>>
>> Something is assuming that a 64 bit value can fit in a 32 bit register.
>>
>> Thanks, David
>>
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2018-07-10 14:57 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-05 21:43 [PATCH] add support for strnlen (PR 81384) Martin Sebor
2018-06-05 22:20 ` Jakub Jelinek
2018-06-06 15:14   ` Martin Sebor
2018-06-06 15:49     ` Jakub Jelinek
2018-06-06  0:39 ` Eric Gallager
2018-06-06 14:44   ` Martin Sebor
2018-06-12 15:10 ` PING " Martin Sebor
2018-06-12 21:11 ` Jeff Law
2018-06-18 16:35   ` Martin Sebor
2018-07-09 14:36     ` Aldy Hernandez
2018-07-09 19:51       ` Jeff Law
2018-07-09 20:51         ` Martin Sebor
2018-07-09 21:26       ` Martin Sebor
2018-07-10  7:13         ` Richard Biener
2018-07-10 14:10           ` Martin Sebor
2018-07-10 14:25             ` Richard Biener
2018-07-10 14:34               ` Jeff Law
2018-07-10 14:57                 ` Martin Sebor
2018-06-19 19:33 David Edelsohn
2018-06-19 20:10 ` Martin Sebor
2018-06-19 20:25   ` Martin Sebor
2018-06-20  5:24   ` Jeff Law

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).