[PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245)

[PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245)

[Martin Sebor]

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

The -Wformat-length checker relies on the compute_builtin_object_size
function to determine the size of the buffer it checks for overflow.
The function returns either a size computed by the tree-object-size
pass for objects referenced by the __builtin_object_size intrinsic
(if it's used in the program) or it tries to compute it for a small
subset of expressions otherwise.  This subset doesn't include objects
allocated by either malloc or alloca, and so for those the function
returns "unknown" or (size_t)-1 in the case of -Wformat-length.  As
a consequence, -Wformat-length is unable to detect overflows
involving such objects.

The attached patch adds a new function, compute_object_size, that
uses the existing algorithms to compute and return the sizes of
allocated objects as well, as if they were referenced by
__builtin_object_size in the program source, enabling the
-Wformat-length checker to detect more buffer overflows.

Martin

PS The function makes use of the init_function_sizes API that is
otherwise unused outside the tree-object-size pass to initialize
the internal structures, but then calls fini_object_sizes to
release them before returning.  That seems wasteful because
the size of the same object or one related to it might need
to computed again in the context of the same function.  I
experimented with allocating and releasing the structures only
when current_function_decl changes but that led to crashes.
I suspect I'm missing something about the management of memory
allocated for these structures.  Does anyone have any suggestions
how to make this work?  (Do I perhaps need to allocate them using
a special allocator so they don't get garbage collected?)

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

PR middle-end/78245 - missing -Wformat-length on an overflow of a dynamically allocated buffer

gcc/testsuite/ChangeLog:

	PR middle-end/78245
	* gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: Add tests.

gcc/ChangeLog:

	PR middle-end/78245
	* gimple-ssa-sprintf.c (get_destination_size): Call compute_object_size.
	* tree-object-size.c (addr_object_size): Adjust.
	(pass_through_call): Adjust.
	(compute_object_size, internal_object_size): New functions.
	(compute_builtin_object_size): Call internal_object_size.
	(pass_object_sizes::execute): Adjust.
	* tree-object-size.h (compute_object_size): Declare.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index 3138ad3..f360711 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -2471,7 +2471,7 @@ get_destination_size (tree dest)
      object (the function fails without optimization in this type).  */
   int ost = optimize > 0;
   unsigned HOST_WIDE_INT size;
-  if (compute_builtin_object_size (dest, ost, &size))
+  if (compute_object_size (dest, ost, &size))
     return size;
 
   return HOST_WIDE_INT_M1U;
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
index 8d97fa8..9874332 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
@@ -1,5 +1,10 @@
 /* { dg-do compile } */
-/* { dg-options "-std=c99 -O2 -Wformat -Wformat-length=1 -ftrack-macro-expansion=0" } */
+/* { dg-options "-O2 -Wformat -Wformat-length=1 -ftrack-macro-expansion=0" } */
+/* Verify that all sprintf built-ins detect overflow involving directives
+   with non-constant arguments known to be constrained by some range of
+   values, and even when writing into dynamically allocated buffers.
+   -O2 (-ftree-vrp) is necessary for the tests involving ranges to pass,
+   otherwise -O1 is sufficient.  */
 
 #ifndef LINE
 #  define LINE 0
@@ -7,18 +12,26 @@
 
 #define bos(x) __builtin_object_size (x, 0)
 
-#define T(bufsize, fmt, ...)						\
-    do {								\
-      if (!LINE || __LINE__ == LINE)					\
-	{								\
-	  char *d = (char *)__builtin_malloc (bufsize);			\
-	  __builtin___sprintf_chk (d, 0, bos (d), fmt, __VA_ARGS__);	\
-	  sink (d);							\
-	}								\
-    } while (0)
+/* Defined (and redefined) to the allocation function to use, either
+   malloc, or alloca, or a VLA.  */
+#define ALLOC(p, n)   (p) = __builtin_malloc (n)
 
-void
-sink (void*);
+/* Defined (and redefined) to the sprintf function to exercise.  */
+#define TEST_SPRINTF(d, maxsize, objsize, fmt, ...)		\
+  __builtin___sprintf_chk (d, 0, objsize, fmt, __VA_ARGS__)
+
+#define T(bufsize, fmt, ...)				\
+  do {							\
+    if (!LINE || __LINE__ == LINE)			\
+      {							\
+	char *d;					\
+	ALLOC (d, bufsize);				\
+	TEST_SPRINTF (d, 0, bos (d), fmt, __VA_ARGS__);	\
+	sink (d);					\
+      }							\
+  } while (0)
+
+void sink (void*);
 
 /* Identity function to verify that the checker figures out the value
    of the operand even when it's not constant (i.e., makes use of
@@ -232,3 +245,88 @@ void test_sprintf_chk_range_sshort (signed short *a, signed short *b)
   T ( 4, "%i",  Ra (998,  999));
   T ( 4, "%i",  Ra (999, 1000)); /* { dg-warning "may write a terminating nul past the end of the destination" } */
 }
+
+/* Exercise ordinary sprintf with malloc.  */
+#undef TEST_SPRINTF
+#define TEST_SPRINTF(d, maxsize, objsize, fmt, ...)	\
+  __builtin_sprintf (d, fmt, __VA_ARGS__)
+
+void test_sprintf_malloc (const char *s, const char *t)
+{
+#define x x ()
+
+  T (1, "%-s", x ? "" : "1");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : "");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : "1");        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : s);        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : t);
+
+  T (2, "%-s", x ? "" : "1");
+  T (2, "%-s", x ? "" : s);
+  T (2, "%-s", x ? "1" : "");
+  T (2, "%-s", x ? s : "");
+  T (2, "%-s", x ? "1" : "2");
+  T (2, "%-s", x ? "" : "12");      /* { dg-warning "nul past the end" } */
+  T (2, "%-s", x ? "12" : "");      /* { dg-warning "nul past the end" } */
+
+  T (2, "%-s", x ? "" : "123");     /* { dg-warning "into a region" } */
+  T (2, "%-s", x ? "123" : "");     /* { dg-warning "into a region" } */
+
+#undef x
+}
+
+/* Exercise ordinary sprintf with alloca.  */
+#undef ALLOC
+#define ALLOC(p, n) (p) = __builtin_alloca (n)
+
+void test_sprintf_alloca (const char *s, const char *t)
+{
+#define x x ()
+
+  T (1, "%-s", x ? "" : "1");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : "");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : "1");        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : s);        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : t);
+
+  T (2, "%-s", x ? "" : "1");
+  T (2, "%-s", x ? "" : s);
+  T (2, "%-s", x ? "1" : "");
+  T (2, "%-s", x ? s : "");
+  T (2, "%-s", x ? "1" : "2");
+  T (2, "%-s", x ? "" : "12");      /* { dg-warning "nul past the end" } */
+  T (2, "%-s", x ? "12" : "");      /* { dg-warning "nul past the end" } */
+
+  T (2, "%-s", x ? "" : "123");     /* { dg-warning "into a region" } */
+  T (2, "%-s", x ? "123" : "");     /* { dg-warning "into a region" } */
+
+#undef x
+}
+
+/* Exercise ordinary sprintf with a VLA.  */
+#undef ALLOC
+#define ALLOC(p, n) char vla [i (n)]; (p) = vla
+
+void test_sprintf_vla (const char *s, const char *t)
+{
+#define x x ()
+
+  T (1, "%-s", x ? "" : "1");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : "");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : "1");        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : s);        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : t);
+
+  T (2, "%-s", x ? "" : "1");
+  T (2, "%-s", x ? "" : s);
+  T (2, "%-s", x ? "1" : "");
+  T (2, "%-s", x ? s : "");
+  T (2, "%-s", x ? "1" : "2");
+  T (2, "%-s", x ? "" : "12");      /* { dg-warning "nul past the end" } */
+  T (2, "%-s", x ? "12" : "");      /* { dg-warning "nul past the end" } */
+
+  T (2, "%-s", x ? "" : "123");     /* { dg-warning "into a region" } */
+  T (2, "%-s", x ? "123" : "");     /* { dg-warning "into a region" } */
+
+#undef x
+}
diff --git a/gcc/tree-object-size.c b/gcc/tree-object-size.c
index 1317ad7..59ff90c 100644
--- a/gcc/tree-object-size.c
+++ b/gcc/tree-object-size.c
@@ -50,6 +50,8 @@ static const unsigned HOST_WIDE_INT unknown[4] = {
   0
 };
 
+static bool internal_object_size (tree, int, unsigned HOST_WIDE_INT *);
+static void fini_object_sizes (void);
 static tree compute_object_offset (const_tree, const_tree);
 static bool addr_object_size (struct object_size_info *,
 			      const_tree, int, unsigned HOST_WIDE_INT *);
@@ -187,7 +189,7 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
       if (!osi || (object_size_type & 1) != 0
 	  || TREE_CODE (TREE_OPERAND (pt_var, 0)) != SSA_NAME)
 	{
-	  compute_builtin_object_size (TREE_OPERAND (pt_var, 0),
+	  internal_object_size (TREE_OPERAND (pt_var, 0),
 				       object_size_type & ~1, &sz);
 	}
       else
@@ -491,14 +493,14 @@ pass_through_call (const gcall *call)
 }
 
 
-/* Compute __builtin_object_size value for PTR and set *PSIZE to
-   the resulting value.  OBJECT_SIZE_TYPE is the second argument
-   to __builtin_object_size.  Return true on success and false
-   when the object size could not be determined.  */
+/* Compute the size of the object pointed to by PTR and set *PSIZE
+   to the resulting value.  OBJECT_SIZE_TYPE is the second argument
+   to __builtin_object_size.  Return true on success and false when
+   the object size could not be determined.  */
 
 bool
-compute_builtin_object_size (tree ptr, int object_size_type,
-			     unsigned HOST_WIDE_INT *psize)
+internal_object_size (tree ptr, int object_size_type,
+		      unsigned HOST_WIDE_INT *psize)
 {
   gcc_assert (object_size_type >= 0 && object_size_type <= 3);
 
@@ -534,7 +536,7 @@ compute_builtin_object_size (tree ptr, int object_size_type,
 	      ptr = gimple_assign_rhs1 (def);
 
 	      if (cst_and_fits_in_hwi (offset)
-		  && compute_builtin_object_size (ptr, object_size_type, psize))
+		  && internal_object_size (ptr, object_size_type, psize))
 		{
 		  /* Return zero when the offset is out of bounds.  */
 		  unsigned HOST_WIDE_INT off = tree_to_shwi (offset);
@@ -664,6 +666,38 @@ compute_builtin_object_size (tree ptr, int object_size_type,
   return *psize != unknown[object_size_type];
 }
 
+/* Compute __builtin_object_size value for PTR and set *PSIZE to
+   the resulting value.  OBJECT_SIZE_TYPE is the second argument
+   to __builtin_object_size.  Return true on success and false
+   when the object size could not be determined.  */
+
+bool
+compute_builtin_object_size (tree ptr, int object_size_type,
+			     unsigned HOST_WIDE_INT *psize)
+{
+  return internal_object_size (ptr, object_size_type, psize);
+}
+
+
+/* Like compute_builtin_object_size but intended to be called
+   without a corresponding __builtin_object_size in the program.  */
+
+bool
+compute_object_size (tree ptr, int object_size_type,
+		     unsigned HOST_WIDE_INT *psize)
+{
+  bool init = computed[0] == NULL;
+  if (init)
+    init_object_sizes ();
+
+  bool retval = internal_object_size (ptr, object_size_type, psize);
+
+  if (init)
+    fini_object_sizes ();
+
+  return retval;
+}
+
 /* Compute object_sizes for PTR, defined to VALUE, which is not an SSA_NAME.  */
 
 static void
@@ -1325,7 +1359,7 @@ pass_object_sizes::execute (function *fun)
 		    {
 		      tree type = TREE_TYPE (lhs);
 		      unsigned HOST_WIDE_INT bytes;
-		      if (compute_builtin_object_size (ptr, object_size_type,
+		      if (internal_object_size (ptr, object_size_type,
 						       &bytes)
 			  && wi::fits_to_tree_p (bytes, type))
 			{
diff --git a/gcc/tree-object-size.h b/gcc/tree-object-size.h
index 38c3e07..60256e6 100644
--- a/gcc/tree-object-size.h
+++ b/gcc/tree-object-size.h
@@ -21,6 +21,7 @@ along with GCC; see the file COPYING3.  If not see
 #define GCC_TREE_OBJECT_SIZE_H
 
 extern void init_object_sizes (void);
+extern bool compute_object_size (tree, int, unsigned HOST_WIDE_INT *);
 extern bool compute_builtin_object_size (tree, int, unsigned HOST_WIDE_INT *);
 
 #endif  // GCC_TREE_OBJECT_SIZE_H

PING [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245)

[Martin Sebor]

I'm looking for a review of the patch below:

   https://gcc.gnu.org/ml/gcc-patches/2016-11/msg00779.html

Thanks

On 11/08/2016 05:09 PM, Martin Sebor wrote:
> The -Wformat-length checker relies on the compute_builtin_object_size
> function to determine the size of the buffer it checks for overflow.
> The function returns either a size computed by the tree-object-size
> pass for objects referenced by the __builtin_object_size intrinsic
> (if it's used in the program) or it tries to compute it for a small
> subset of expressions otherwise.  This subset doesn't include objects
> allocated by either malloc or alloca, and so for those the function
> returns "unknown" or (size_t)-1 in the case of -Wformat-length.  As
> a consequence, -Wformat-length is unable to detect overflows
> involving such objects.
>
> The attached patch adds a new function, compute_object_size, that
> uses the existing algorithms to compute and return the sizes of
> allocated objects as well, as if they were referenced by
> __builtin_object_size in the program source, enabling the
> -Wformat-length checker to detect more buffer overflows.
>
> Martin
>
> PS The function makes use of the init_function_sizes API that is
> otherwise unused outside the tree-object-size pass to initialize
> the internal structures, but then calls fini_object_sizes to
> release them before returning.  That seems wasteful because
> the size of the same object or one related to it might need
> to computed again in the context of the same function.  I
> experimented with allocating and releasing the structures only
> when current_function_decl changes but that led to crashes.
> I suspect I'm missing something about the management of memory
> allocated for these structures.  Does anyone have any suggestions
> how to make this work?  (Do I perhaps need to allocate them using
> a special allocator so they don't get garbage collected?)

PING 2 [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245)

[Martin Sebor]

Ping.  Still looking for a review of the patch below:

On 11/16/2016 10:33 AM, Martin Sebor wrote:
> I'm looking for a review of the patch below:
>
>   https://gcc.gnu.org/ml/gcc-patches/2016-11/msg00779.html
>
> Thanks
>
> On 11/08/2016 05:09 PM, Martin Sebor wrote:
>> The -Wformat-length checker relies on the compute_builtin_object_size
>> function to determine the size of the buffer it checks for overflow.
>> The function returns either a size computed by the tree-object-size
>> pass for objects referenced by the __builtin_object_size intrinsic
>> (if it's used in the program) or it tries to compute it for a small
>> subset of expressions otherwise.  This subset doesn't include objects
>> allocated by either malloc or alloca, and so for those the function
>> returns "unknown" or (size_t)-1 in the case of -Wformat-length.  As
>> a consequence, -Wformat-length is unable to detect overflows
>> involving such objects.
>>
>> The attached patch adds a new function, compute_object_size, that
>> uses the existing algorithms to compute and return the sizes of
>> allocated objects as well, as if they were referenced by
>> __builtin_object_size in the program source, enabling the
>> -Wformat-length checker to detect more buffer overflows.
>>
>> Martin
>>
>> PS The function makes use of the init_function_sizes API that is
>> otherwise unused outside the tree-object-size pass to initialize
>> the internal structures, but then calls fini_object_sizes to
>> release them before returning.  That seems wasteful because
>> the size of the same object or one related to it might need
>> to computed again in the context of the same function.  I
>> experimented with allocating and releasing the structures only
>> when current_function_decl changes but that led to crashes.
>> I suspect I'm missing something about the management of memory
>> allocated for these structures.  Does anyone have any suggestions
>> how to make this work?  (Do I perhaps need to allocate them using
>> a special allocator so they don't get garbage collected?)
>

Re: [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245)

[Jeff Law]

On 11/08/2016 05:09 PM, Martin Sebor wrote:
> The -Wformat-length checker relies on the compute_builtin_object_size
> function to determine the size of the buffer it checks for overflow.
> The function returns either a size computed by the tree-object-size
> pass for objects referenced by the __builtin_object_size intrinsic
> (if it's used in the program) or it tries to compute it for a small
> subset of expressions otherwise.  This subset doesn't include objects
> allocated by either malloc or alloca, and so for those the function
> returns "unknown" or (size_t)-1 in the case of -Wformat-length.  As
> a consequence, -Wformat-length is unable to detect overflows
> involving such objects.
>
> The attached patch adds a new function, compute_object_size, that
> uses the existing algorithms to compute and return the sizes of
> allocated objects as well, as if they were referenced by
> __builtin_object_size in the program source, enabling the
> -Wformat-length checker to detect more buffer overflows.
>
> Martin
>
> PS The function makes use of the init_function_sizes API that is
> otherwise unused outside the tree-object-size pass to initialize
> the internal structures, but then calls fini_object_sizes to
> release them before returning.  That seems wasteful because
> the size of the same object or one related to it might need
> to computed again in the context of the same function.  I
> experimented with allocating and releasing the structures only
> when current_function_decl changes but that led to crashes.
> I suspect I'm missing something about the management of memory
> allocated for these structures.  Does anyone have any suggestions
> how to make this work?  (Do I perhaps need to allocate them using
> a special allocator so they don't get garbage collected?)
>
> gcc-78245.diff
>
>
> PR middle-end/78245 - missing -Wformat-length on an overflow of a dynamically allocated buffer
>
> gcc/testsuite/ChangeLog:
>
> 	PR middle-end/78245
> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: Add tests.
>
> gcc/ChangeLog:
>
> 	PR middle-end/78245
> 	* gimple-ssa-sprintf.c (get_destination_size): Call compute_object_size.
> 	* tree-object-size.c (addr_object_size): Adjust.
> 	(pass_through_call): Adjust.
> 	(compute_object_size, internal_object_size): New functions.
> 	(compute_builtin_object_size): Call internal_object_size.
> 	(pass_object_sizes::execute): Adjust.
> 	* tree-object-size.h (compute_object_size): Declare.
Sorry.  Just not getting to many of the pre-stage1 close patches as fast 
as I'd like.

My only real concern here is that if we call compute_builtin_object_size 
without having initialized the passes, then we initialize, compute, then 
finalize.  Subsequent calls will go through the same process -- the key 
being each one re-computes the internal state which might get expensive.

Wouldn't it just make more sense to pull up the init/fini calls, either 
explicitly (which likely means externalizing the init/fini routines) or 
by wrapping all this stuff in a class and instantiating a suitable object?

I think the answer to your memory management question is that internal 
state is likely not marked as a GC root and thus if you get a GC call 
pieces of internal state are not seen as reachable, but you still can 
reference them.  ie, you end up with dangling pointers.

Usually all you'd have to do is mark them so that gengtype will see 
them.  Bitmaps, trees, rtl, are all good examples.  So marking the 
bitmap would look like:

static GTY (()) bitmap computed[4];

Or something like that.

You might try --enable-checking=yes,extra,gc,gcac

That will be slow, but is often helpful for tracking down cases where 
someone has an object expected to be live across passes, but it isn't 
reachable because someone failed to register a GC root.

Jeff

Re: [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245)

[Martin Sebor]

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

> My only real concern here is that if we call compute_builtin_object_size
> without having initialized the passes, then we initialize, compute, then
> finalize.  Subsequent calls will go through the same process -- the key
> being each one re-computes the internal state which might get expensive.
>
> Wouldn't it just make more sense to pull up the init/fini calls, either
> explicitly (which likely means externalizing the init/fini routines) or
> by wrapping all this stuff in a class and instantiating a suitable object?
>
> I think the answer to your memory management question is that internal
> state is likely not marked as a GC root and thus if you get a GC call
> pieces of internal state are not seen as reachable, but you still can
> reference them.  ie, you end up with dangling pointers.
>
> Usually all you'd have to do is mark them so that gengtype will see
> them.  Bitmaps, trees, rtl, are all good examples.  So marking the
> bitmap would look like:
>
> static GTY (()) bitmap computed[4];
>
> Or something like that.
>
> You might try --enable-checking=yes,extra,gc,gcac
>
> That will be slow, but is often helpful for tracking down cases where
> someone has an object expected to be live across passes, but it isn't
> reachable because someone failed to register a GC root.

Thanks.  Attached is an updated patch that avoids flushing the computed
data until the current function changes, and tells the garbage collector
not to collect it.  I haven't finished bootstrapping and regtesting it
yet but running it through Valgrind doesn't show any errors.  Please
let me know if this is what you had in mind.

Thanks
Martin

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

PR middle-end/78245 - missing -Wformat-length on an overflow of a dynamically allocated buffer

gcc/testsuite/ChangeLog:

	PR middle-end/78245
	* gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: Add tests.

gcc/ChangeLog:

	PR middle-end/78245
	* gimple-ssa-sprintf.c (object_sizes, computed): Declared GTY.
	(get_destination_size): Call compute_object_size.
	* tree-object-size.c (addr_object_size): Adjust.
	(pass_through_call): Adjust.
	(compute_object_size, internal_object_size): New functions.
	(compute_builtin_object_size): Call internal_object_size.
	(init_object_sizes): Initialize computed bitmap so the garbage
	collector knows about it.
	(fini_object_sizes): Clear the computed bitmap when non-null.
	(pass_object_sizes::execute): Adjust.
	* tree-object-size.h (compute_object_size): Declare.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index ead8b0e..ea56570 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -2468,7 +2468,7 @@ get_destination_size (tree dest)
      object (the function fails without optimization in this type).  */
   int ost = optimize > 0;
   unsigned HOST_WIDE_INT size;
-  if (compute_builtin_object_size (dest, ost, &size))
+  if (compute_object_size (dest, ost, &size))
     return size;
 
   return HOST_WIDE_INT_M1U;
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
index 8d97fa8..9874332 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
@@ -1,5 +1,10 @@
 /* { dg-do compile } */
-/* { dg-options "-std=c99 -O2 -Wformat -Wformat-length=1 -ftrack-macro-expansion=0" } */
+/* { dg-options "-O2 -Wformat -Wformat-length=1 -ftrack-macro-expansion=0" } */
+/* Verify that all sprintf built-ins detect overflow involving directives
+   with non-constant arguments known to be constrained by some range of
+   values, and even when writing into dynamically allocated buffers.
+   -O2 (-ftree-vrp) is necessary for the tests involving ranges to pass,
+   otherwise -O1 is sufficient.  */
 
 #ifndef LINE
 #  define LINE 0
@@ -7,18 +12,26 @@
 
 #define bos(x) __builtin_object_size (x, 0)
 
-#define T(bufsize, fmt, ...)						\
-    do {								\
-      if (!LINE || __LINE__ == LINE)					\
-	{								\
-	  char *d = (char *)__builtin_malloc (bufsize);			\
-	  __builtin___sprintf_chk (d, 0, bos (d), fmt, __VA_ARGS__);	\
-	  sink (d);							\
-	}								\
-    } while (0)
+/* Defined (and redefined) to the allocation function to use, either
+   malloc, or alloca, or a VLA.  */
+#define ALLOC(p, n)   (p) = __builtin_malloc (n)
 
-void
-sink (void*);
+/* Defined (and redefined) to the sprintf function to exercise.  */
+#define TEST_SPRINTF(d, maxsize, objsize, fmt, ...)		\
+  __builtin___sprintf_chk (d, 0, objsize, fmt, __VA_ARGS__)
+
+#define T(bufsize, fmt, ...)				\
+  do {							\
+    if (!LINE || __LINE__ == LINE)			\
+      {							\
+	char *d;					\
+	ALLOC (d, bufsize);				\
+	TEST_SPRINTF (d, 0, bos (d), fmt, __VA_ARGS__);	\
+	sink (d);					\
+      }							\
+  } while (0)
+
+void sink (void*);
 
 /* Identity function to verify that the checker figures out the value
    of the operand even when it's not constant (i.e., makes use of
@@ -232,3 +245,88 @@ void test_sprintf_chk_range_sshort (signed short *a, signed short *b)
   T ( 4, "%i",  Ra (998,  999));
   T ( 4, "%i",  Ra (999, 1000)); /* { dg-warning "may write a terminating nul past the end of the destination" } */
 }
+
+/* Exercise ordinary sprintf with malloc.  */
+#undef TEST_SPRINTF
+#define TEST_SPRINTF(d, maxsize, objsize, fmt, ...)	\
+  __builtin_sprintf (d, fmt, __VA_ARGS__)
+
+void test_sprintf_malloc (const char *s, const char *t)
+{
+#define x x ()
+
+  T (1, "%-s", x ? "" : "1");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : "");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : "1");        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : s);        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : t);
+
+  T (2, "%-s", x ? "" : "1");
+  T (2, "%-s", x ? "" : s);
+  T (2, "%-s", x ? "1" : "");
+  T (2, "%-s", x ? s : "");
+  T (2, "%-s", x ? "1" : "2");
+  T (2, "%-s", x ? "" : "12");      /* { dg-warning "nul past the end" } */
+  T (2, "%-s", x ? "12" : "");      /* { dg-warning "nul past the end" } */
+
+  T (2, "%-s", x ? "" : "123");     /* { dg-warning "into a region" } */
+  T (2, "%-s", x ? "123" : "");     /* { dg-warning "into a region" } */
+
+#undef x
+}
+
+/* Exercise ordinary sprintf with alloca.  */
+#undef ALLOC
+#define ALLOC(p, n) (p) = __builtin_alloca (n)
+
+void test_sprintf_alloca (const char *s, const char *t)
+{
+#define x x ()
+
+  T (1, "%-s", x ? "" : "1");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : "");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : "1");        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : s);        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : t);
+
+  T (2, "%-s", x ? "" : "1");
+  T (2, "%-s", x ? "" : s);
+  T (2, "%-s", x ? "1" : "");
+  T (2, "%-s", x ? s : "");
+  T (2, "%-s", x ? "1" : "2");
+  T (2, "%-s", x ? "" : "12");      /* { dg-warning "nul past the end" } */
+  T (2, "%-s", x ? "12" : "");      /* { dg-warning "nul past the end" } */
+
+  T (2, "%-s", x ? "" : "123");     /* { dg-warning "into a region" } */
+  T (2, "%-s", x ? "123" : "");     /* { dg-warning "into a region" } */
+
+#undef x
+}
+
+/* Exercise ordinary sprintf with a VLA.  */
+#undef ALLOC
+#define ALLOC(p, n) char vla [i (n)]; (p) = vla
+
+void test_sprintf_vla (const char *s, const char *t)
+{
+#define x x ()
+
+  T (1, "%-s", x ? "" : "1");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : "");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : "1");        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : s);        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : t);
+
+  T (2, "%-s", x ? "" : "1");
+  T (2, "%-s", x ? "" : s);
+  T (2, "%-s", x ? "1" : "");
+  T (2, "%-s", x ? s : "");
+  T (2, "%-s", x ? "1" : "2");
+  T (2, "%-s", x ? "" : "12");      /* { dg-warning "nul past the end" } */
+  T (2, "%-s", x ? "12" : "");      /* { dg-warning "nul past the end" } */
+
+  T (2, "%-s", x ? "" : "123");     /* { dg-warning "into a region" } */
+  T (2, "%-s", x ? "123" : "");     /* { dg-warning "into a region" } */
+
+#undef x
+}
diff --git a/gcc/tree-object-size.c b/gcc/tree-object-size.c
index 1317ad7..39c32e3 100644
--- a/gcc/tree-object-size.c
+++ b/gcc/tree-object-size.c
@@ -50,6 +50,8 @@ static const unsigned HOST_WIDE_INT unknown[4] = {
   0
 };
 
+static bool internal_object_size (tree, int, unsigned HOST_WIDE_INT *);
+static void fini_object_sizes (void);
 static tree compute_object_offset (const_tree, const_tree);
 static bool addr_object_size (struct object_size_info *,
 			      const_tree, int, unsigned HOST_WIDE_INT *);
@@ -72,10 +74,10 @@ static void check_for_plus_in_loops_1 (struct object_size_info *, tree,
    the subobject (innermost array or field with address taken).
    object_sizes[2] is lower bound for number of bytes till the end of
    the object and object_sizes[3] lower bound for subobject.  */
-static vec<unsigned HOST_WIDE_INT> object_sizes[4];
+static GTY (()) vec<unsigned HOST_WIDE_INT> object_sizes[4];
 
 /* Bitmaps what object sizes have been computed already.  */
-static bitmap computed[4];
+static GTY (()) bitmap computed[4];
 
 /* Maximum value of offset we consider to be addition.  */
 static unsigned HOST_WIDE_INT offset_limit;
@@ -187,7 +189,7 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
       if (!osi || (object_size_type & 1) != 0
 	  || TREE_CODE (TREE_OPERAND (pt_var, 0)) != SSA_NAME)
 	{
-	  compute_builtin_object_size (TREE_OPERAND (pt_var, 0),
+	  internal_object_size (TREE_OPERAND (pt_var, 0),
 				       object_size_type & ~1, &sz);
 	}
       else
@@ -491,14 +493,14 @@ pass_through_call (const gcall *call)
 }
 
 
-/* Compute __builtin_object_size value for PTR and set *PSIZE to
-   the resulting value.  OBJECT_SIZE_TYPE is the second argument
-   to __builtin_object_size.  Return true on success and false
-   when the object size could not be determined.  */
+/* Compute the size of the object pointed to by PTR and set *PSIZE
+   to the resulting value.  OBJECT_SIZE_TYPE is the second argument
+   to __builtin_object_size.  Return true on success and false when
+   the object size could not be determined.  */
 
 bool
-compute_builtin_object_size (tree ptr, int object_size_type,
-			     unsigned HOST_WIDE_INT *psize)
+internal_object_size (tree ptr, int object_size_type,
+		      unsigned HOST_WIDE_INT *psize)
 {
   gcc_assert (object_size_type >= 0 && object_size_type <= 3);
 
@@ -534,7 +536,7 @@ compute_builtin_object_size (tree ptr, int object_size_type,
 	      ptr = gimple_assign_rhs1 (def);
 
 	      if (cst_and_fits_in_hwi (offset)
-		  && compute_builtin_object_size (ptr, object_size_type, psize))
+		  && internal_object_size (ptr, object_size_type, psize))
 		{
 		  /* Return zero when the offset is out of bounds.  */
 		  unsigned HOST_WIDE_INT off = tree_to_shwi (offset);
@@ -664,6 +666,45 @@ compute_builtin_object_size (tree ptr, int object_size_type,
   return *psize != unknown[object_size_type];
 }
 
+/* Compute __builtin_object_size value for PTR and set *PSIZE to
+   the resulting value.  OBJECT_SIZE_TYPE is the second argument
+   to __builtin_object_size.  Return true on success and false
+   when the object size could not be determined.  */
+
+bool
+compute_builtin_object_size (tree ptr, int object_size_type,
+			     unsigned HOST_WIDE_INT *psize)
+{
+  return internal_object_size (ptr, object_size_type, psize);
+}
+
+
+/* Like compute_builtin_object_size but intended to be called
+   without a corresponding __builtin_object_size in the program.  */
+
+bool
+compute_object_size (tree ptr, int object_size_type,
+		     unsigned HOST_WIDE_INT *psize)
+{
+  static unsigned lastfunchash;
+  unsigned curfunchash
+    = IDENTIFIER_HASH_VALUE (DECL_NAME (current_function_decl));
+
+  /* Initialize the internal data structures for each new function
+     and keep the computed data around for any subsequent calls to
+     compute_object_size.  */
+  if (curfunchash != lastfunchash)
+    {
+      lastfunchash = curfunchash;
+      fini_object_sizes ();
+      init_object_sizes ();
+    }
+
+  bool retval = internal_object_size (ptr, object_size_type, psize);
+
+  return retval;
+}
+
 /* Compute object_sizes for PTR, defined to VALUE, which is not an SSA_NAME.  */
 
 static void
@@ -1221,7 +1262,7 @@ init_object_sizes (void)
   for (object_size_type = 0; object_size_type <= 3; object_size_type++)
     {
       object_sizes[object_size_type].safe_grow (num_ssa_names);
-      computed[object_size_type] = BITMAP_ALLOC (NULL);
+      computed[object_size_type] = BITMAP_GGC_ALLOC ();
     }
 
   init_offset_limit ();
@@ -1238,7 +1279,11 @@ fini_object_sizes (void)
   for (object_size_type = 0; object_size_type <= 3; object_size_type++)
     {
       object_sizes[object_size_type].release ();
-      BITMAP_FREE (computed[object_size_type]);
+      if (computed[object_size_type])
+	{
+	  bitmap_clear (computed[object_size_type]);
+	  computed[object_size_type] = NULL;
+	}
     }
 }
 
@@ -1325,7 +1370,7 @@ pass_object_sizes::execute (function *fun)
 		    {
 		      tree type = TREE_TYPE (lhs);
 		      unsigned HOST_WIDE_INT bytes;
-		      if (compute_builtin_object_size (ptr, object_size_type,
+		      if (internal_object_size (ptr, object_size_type,
 						       &bytes)
 			  && wi::fits_to_tree_p (bytes, type))
 			{
diff --git a/gcc/tree-object-size.h b/gcc/tree-object-size.h
index 38c3e07..60256e6 100644
--- a/gcc/tree-object-size.h
+++ b/gcc/tree-object-size.h
@@ -21,6 +21,7 @@ along with GCC; see the file COPYING3.  If not see
 #define GCC_TREE_OBJECT_SIZE_H
 
 extern void init_object_sizes (void);
+extern bool compute_object_size (tree, int, unsigned HOST_WIDE_INT *);
 extern bool compute_builtin_object_size (tree, int, unsigned HOST_WIDE_INT *);
 
 #endif  // GCC_TREE_OBJECT_SIZE_H

Re: [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245)

[Jeff Law]

On 11/23/2016 11:26 AM, Martin Sebor wrote:
>> My only real concern here is that if we call compute_builtin_object_size
>> without having initialized the passes, then we initialize, compute, then
>> finalize.  Subsequent calls will go through the same process -- the key
>> being each one re-computes the internal state which might get expensive.
>>
>> Wouldn't it just make more sense to pull up the init/fini calls, either
>> explicitly (which likely means externalizing the init/fini routines) or
>> by wrapping all this stuff in a class and instantiating a suitable
>> object?
>>
>> I think the answer to your memory management question is that internal
>> state is likely not marked as a GC root and thus if you get a GC call
>> pieces of internal state are not seen as reachable, but you still can
>> reference them.  ie, you end up with dangling pointers.
>>
>> Usually all you'd have to do is mark them so that gengtype will see
>> them.  Bitmaps, trees, rtl, are all good examples.  So marking the
>> bitmap would look like:
>>
>> static GTY (()) bitmap computed[4];
>>
>> Or something like that.
>>
>> You might try --enable-checking=yes,extra,gc,gcac
>>
>> That will be slow, but is often helpful for tracking down cases where
>> someone has an object expected to be live across passes, but it isn't
>> reachable because someone failed to register a GC root.
>
> Thanks.  Attached is an updated patch that avoids flushing the computed
> data until the current function changes, and tells the garbage collector
> not to collect it.  I haven't finished bootstrapping and regtesting it
> yet but running it through Valgrind doesn't show any errors.  Please
> let me know if this is what you had in mind.
>
> Thanks
> Martin
>
> gcc-78245.diff
>
>
> PR middle-end/78245 - missing -Wformat-length on an overflow of a dynamically allocated buffer
>
> gcc/testsuite/ChangeLog:
>
> 	PR middle-end/78245
> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: Add tests.
>
> gcc/ChangeLog:
>
> 	PR middle-end/78245
> 	* gimple-ssa-sprintf.c (object_sizes, computed): Declared GTY.
> 	(get_destination_size): Call compute_object_size.
> 	* tree-object-size.c (addr_object_size): Adjust.
> 	(pass_through_call): Adjust.
> 	(compute_object_size, internal_object_size): New functions.
> 	(compute_builtin_object_size): Call internal_object_size.
> 	(init_object_sizes): Initialize computed bitmap so the garbage
> 	collector knows about it.
> 	(fini_object_sizes): Clear the computed bitmap when non-null.
> 	(pass_object_sizes::execute): Adjust.
> 	* tree-object-size.h (compute_object_size): Declare.
>
> diff --git a/gcc/tree-object-size.c b/gcc/tree-object-size.c
> index 1317ad7..39c32e3 100644
> --- a/gcc/tree-object-size.c
> +++ b/gcc/tree-object-size.c
> @@ -72,10 +74,10 @@ static void check_for_plus_in_loops_1 (struct object_size_info *, tree,
>     the subobject (innermost array or field with address taken).
>     object_sizes[2] is lower bound for number of bytes till the end of
>     the object and object_sizes[3] lower bound for subobject.  */
> -static vec<unsigned HOST_WIDE_INT> object_sizes[4];
> +static GTY (()) vec<unsigned HOST_WIDE_INT> object_sizes[4];
I don't think this needs a GTY marker.
>
>  /* Bitmaps what object sizes have been computed already.  */
> -static bitmap computed[4];
> +static GTY (()) bitmap computed[4];
This is the one you probably needed :-)

>
> +/* Like compute_builtin_object_size but intended to be called
> +   without a corresponding __builtin_object_size in the program.  */
> +
> +bool
> +compute_object_size (tree ptr, int object_size_type,
> +		     unsigned HOST_WIDE_INT *psize)
> +{
> +  static unsigned lastfunchash;
> +  unsigned curfunchash
> +    = IDENTIFIER_HASH_VALUE (DECL_NAME (current_function_decl));
> +
> +  /* Initialize the internal data structures for each new function
> +     and keep the computed data around for any subsequent calls to
> +     compute_object_size.  */
> +  if (curfunchash != lastfunchash)
My worry here would be a hash collision.  Then we'd be using object 
sizes from the wrong function.


Isn't the goal here to be able to get format-length warnings when there 
aren't explicit calls to _b_o_s in the IL?   Can't you initialize the 
object-size framework at the start of your pass and tear it down when 
your pass is complete?  You could do that by exporting the init/fini 
routines and calling them directly, or by wrapping that in a class and 
instantiating the class when you need it.

That would avoid having to worry about the GC system entirely since you 
wouldn't have stuff living across passes.

Jeff

Re: [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245)

[Martin Sebor]

On 11/23/2016 12:10 PM, Jeff Law wrote:
> On 11/23/2016 11:26 AM, Martin Sebor wrote:
>>> My only real concern here is that if we call compute_builtin_object_size
>>> without having initialized the passes, then we initialize, compute, then
>>> finalize.  Subsequent calls will go through the same process -- the key
>>> being each one re-computes the internal state which might get expensive.
>>>
>>> Wouldn't it just make more sense to pull up the init/fini calls, either
>>> explicitly (which likely means externalizing the init/fini routines) or
>>> by wrapping all this stuff in a class and instantiating a suitable
>>> object?
>>>
>>> I think the answer to your memory management question is that internal
>>> state is likely not marked as a GC root and thus if you get a GC call
>>> pieces of internal state are not seen as reachable, but you still can
>>> reference them.  ie, you end up with dangling pointers.
>>>
>>> Usually all you'd have to do is mark them so that gengtype will see
>>> them.  Bitmaps, trees, rtl, are all good examples.  So marking the
>>> bitmap would look like:
>>>
>>> static GTY (()) bitmap computed[4];
>>>
>>> Or something like that.
>>>
>>> You might try --enable-checking=yes,extra,gc,gcac
>>>
>>> That will be slow, but is often helpful for tracking down cases where
>>> someone has an object expected to be live across passes, but it isn't
>>> reachable because someone failed to register a GC root.
>>
>> Thanks.  Attached is an updated patch that avoids flushing the computed
>> data until the current function changes, and tells the garbage collector
>> not to collect it.  I haven't finished bootstrapping and regtesting it
>> yet but running it through Valgrind doesn't show any errors.  Please
>> let me know if this is what you had in mind.
>>
>> Thanks
>> Martin
>>
>> gcc-78245.diff
>>
>>
>> PR middle-end/78245 - missing -Wformat-length on an overflow of a
>> dynamically allocated buffer
>>
>> gcc/testsuite/ChangeLog:
>>
>>     PR middle-end/78245
>>     * gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: Add tests.
>>
>> gcc/ChangeLog:
>>
>>     PR middle-end/78245
>>     * gimple-ssa-sprintf.c (object_sizes, computed): Declared GTY.
>>     (get_destination_size): Call compute_object_size.
>>     * tree-object-size.c (addr_object_size): Adjust.
>>     (pass_through_call): Adjust.
>>     (compute_object_size, internal_object_size): New functions.
>>     (compute_builtin_object_size): Call internal_object_size.
>>     (init_object_sizes): Initialize computed bitmap so the garbage
>>     collector knows about it.
>>     (fini_object_sizes): Clear the computed bitmap when non-null.
>>     (pass_object_sizes::execute): Adjust.
>>     * tree-object-size.h (compute_object_size): Declare.
>>
>> diff --git a/gcc/tree-object-size.c b/gcc/tree-object-size.c
>> index 1317ad7..39c32e3 100644
>> --- a/gcc/tree-object-size.c
>> +++ b/gcc/tree-object-size.c
>> @@ -72,10 +74,10 @@ static void check_for_plus_in_loops_1 (struct
>> object_size_info *, tree,
>>     the subobject (innermost array or field with address taken).
>>     object_sizes[2] is lower bound for number of bytes till the end of
>>     the object and object_sizes[3] lower bound for subobject.  */
>> -static vec<unsigned HOST_WIDE_INT> object_sizes[4];
>> +static GTY (()) vec<unsigned HOST_WIDE_INT> object_sizes[4];
> I don't think this needs a GTY marker.
>>
>>  /* Bitmaps what object sizes have been computed already.  */
>> -static bitmap computed[4];
>> +static GTY (()) bitmap computed[4];
> This is the one you probably needed :-)
>
>>
>> +/* Like compute_builtin_object_size but intended to be called
>> +   without a corresponding __builtin_object_size in the program.  */
>> +
>> +bool
>> +compute_object_size (tree ptr, int object_size_type,
>> +             unsigned HOST_WIDE_INT *psize)
>> +{
>> +  static unsigned lastfunchash;
>> +  unsigned curfunchash
>> +    = IDENTIFIER_HASH_VALUE (DECL_NAME (current_function_decl));
>> +
>> +  /* Initialize the internal data structures for each new function
>> +     and keep the computed data around for any subsequent calls to
>> +     compute_object_size.  */
>> +  if (curfunchash != lastfunchash)
> My worry here would be a hash collision.  Then we'd be using object
> sizes from the wrong function.

Ah, right, that might explain the ICE I just noticed during Ada
bootstrap.  Is there some other way to uniquely identify a function?
A DECL_UID maybe?

>
> Isn't the goal here to be able to get format-length warnings when there
> aren't explicit calls to _b_o_s in the IL?   Can't you initialize the
> object-size framework at the start of your pass and tear it down when
> your pass is complete?  You could do that by exporting the init/fini
> routines and calling them directly, or by wrapping that in a class and
> instantiating the class when you need it.
>
> That would avoid having to worry about the GC system entirely since you
> wouldn't have stuff living across passes.

Yes, that is the immediate goal of this patch.  Beyond it, though,
I would like to call this function from anywhere, including during
expansion (as is done in my patch for bug 53562 and related).

Martin

https://gcc.gnu.org/ml/gcc-patches/2016-11/msg00896.html

Re: [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245)

[Jeff Law]

On 11/23/2016 12:32 PM, Martin Sebor wrote:

>> My worry here would be a hash collision.  Then we'd be using object
>> sizes from the wrong function.
>
> Ah, right, that might explain the ICE I just noticed during Ada
> bootstrap.  Is there some other way to uniquely identify a function?
> A DECL_UID maybe?
DECL_UID would be your best bet.  But ISTM that trying to avoid 
invocations by reusing data from prior passes is likely to be more 
fragile than recomputing on a per-pass basis -- as long as the number of 
times we need this stuff is small (as I suspect it is).

>
>>
>> Isn't the goal here to be able to get format-length warnings when there
>> aren't explicit calls to _b_o_s in the IL?   Can't you initialize the
>> object-size framework at the start of your pass and tear it down when
>> your pass is complete?  You could do that by exporting the init/fini
>> routines and calling them directly, or by wrapping that in a class and
>> instantiating the class when you need it.
>>
>> That would avoid having to worry about the GC system entirely since you
>> wouldn't have stuff living across passes.
>
> Yes, that is the immediate goal of this patch.  Beyond it, though,
> I would like to call this function from anywhere, including during
> expansion (as is done in my patch for bug 53562 and related).
But why not detect the builtins during your pass and check there.  ie, I 
don't see why we necessarily need to have checking and expansion 
intertwined together.  Maybe I'm missing something.  Is there something 
that makes it inherently easier or better to implement checking during 
builtin expansion?

Jeff

Re: [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245)

[Martin Sebor]

On 11/23/2016 12:47 PM, Jeff Law wrote:
> On 11/23/2016 12:32 PM, Martin Sebor wrote:
>
>>> My worry here would be a hash collision.  Then we'd be using object
>>> sizes from the wrong function.
>>
>> Ah, right, that might explain the ICE I just noticed during Ada
>> bootstrap.  Is there some other way to uniquely identify a function?
>> A DECL_UID maybe?
> DECL_UID would be your best bet.  But ISTM that trying to avoid
> invocations by reusing data from prior passes is likely to be more
> fragile than recomputing on a per-pass basis -- as long as the number of
> times we need this stuff is small (as I suspect it is).
>
>>
>>>
>>> Isn't the goal here to be able to get format-length warnings when there
>>> aren't explicit calls to _b_o_s in the IL?   Can't you initialize the
>>> object-size framework at the start of your pass and tear it down when
>>> your pass is complete?  You could do that by exporting the init/fini
>>> routines and calling them directly, or by wrapping that in a class and
>>> instantiating the class when you need it.
>>>
>>> That would avoid having to worry about the GC system entirely since you
>>> wouldn't have stuff living across passes.
>>
>> Yes, that is the immediate goal of this patch.  Beyond it, though,
>> I would like to call this function from anywhere, including during
>> expansion (as is done in my patch for bug 53562 and related).
> But why not detect the builtins during your pass and check there.  ie, I
> don't see why we necessarily need to have checking and expansion
> intertwined together.  Maybe I'm missing something.  Is there something
> that makes it inherently easier or better to implement checking during
> builtin expansion?

I hadn't thought of extending the gimple-ssa-sprintf pass to all
the memxxx and strxxx builtins.  The _chk functions are already
being handled in builtins.c so calling compute_builtin_object_size
for the non-checking ones there and detecting overflow in those
was an easy and, I had hoped, non-controversial enhancement to make.
In a discussion of bug 77784 (handled in the patch for bug 53562)
Jakub also expressed a preference for some of the diagnostics
staying in builtins.c.

I also suspect that the answer to your question is yes.  Range
information is pretty bad in the gimple-ssa-sprintf pass (it looks
like it runs after EVRP but before VRP).  Maybe the pass should run
after VRP?

That said, I defer to you on how to proceed here.  I'm prepared
to do the work(*) but I do worry about jeopardizing the chances
of this patch and the others making it into 7.0.

Martin

PS If I understand what you are suggesting this would mean
extending the gimple-ssa-sprintf pass to the memxxx and strxxx
functions and running the pass later, after VRP.

Re: [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245)

[Jeff Law]

On 11/23/2016 01:09 PM, Martin Sebor wrote:
>
> I hadn't thought of extending the gimple-ssa-sprintf pass to all
> the memxxx and strxxx builtins.  The _chk functions are already
> being handled in builtins.c so calling compute_builtin_object_size
> for the non-checking ones there and detecting overflow in those
> was an easy and, I had hoped, non-controversial enhancement to make.
> In a discussion of bug 77784 (handled in the patch for bug 53562)
> Jakub also expressed a preference for some of the diagnostics
> staying in builtins.c.
I'm just trying to understand how the pieces fit together.  I wasn't 
aware of Jakub's desire to keep them in builtins.c.

>
> I also suspect that the answer to your question is yes.  Range
> information is pretty bad in the gimple-ssa-sprintf pass (it looks
> like it runs after EVRP but before VRP).  Maybe the pass should run
> after VRP?
Let's investigate this separately rather than draw in additional 
potential issues.  But I do think this is worth investigation.

>
> That said, I defer to you on how to proceed here.  I'm prepared
> to do the work(*) but I do worry about jeopardizing the chances
> of this patch and the others making it into 7.0.
So would it make sense to just init/fini the b_o_s framework in your 
pass and for builtin expansion?

> PS If I understand what you are suggesting this would mean
> extending the gimple-ssa-sprintf pass to the memxxx and strxxx
> functions and running the pass later, after VRP.
That was my original thought, but I'm certainly not deeply vested in it 
-- it was primarily to avoid initializing b_o_s an extra time.

Jeff

Re: [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245)

[Martin Sebor]

On 11/23/2016 01:30 PM, Jeff Law wrote:
> On 11/23/2016 01:09 PM, Martin Sebor wrote:
>>
>> I hadn't thought of extending the gimple-ssa-sprintf pass to all
>> the memxxx and strxxx builtins.  The _chk functions are already
>> being handled in builtins.c so calling compute_builtin_object_size
>> for the non-checking ones there and detecting overflow in those
>> was an easy and, I had hoped, non-controversial enhancement to make.
>> In a discussion of bug 77784 (handled in the patch for bug 53562)
>> Jakub also expressed a preference for some of the diagnostics
>> staying in builtins.c.
> I'm just trying to understand how the pieces fit together.  I wasn't
> aware of Jakub's desire to keep them in builtins.c.

After thinking about it a bit it does seem that having all the size
and buffer overflow checking (though not necessarily BOS itself) in
the same place or pass would make sense.

>> I also suspect that the answer to your question is yes.  Range
>> information is pretty bad in the gimple-ssa-sprintf pass (it looks
>> like it runs after EVRP but before VRP).  Maybe the pass should run
>> after VRP?
> Let's investigate this separately rather than draw in additional
> potential issues.  But I do think this is worth investigation.

Sounds good.

>
>>
>> That said, I defer to you on how to proceed here.  I'm prepared
>> to do the work(*) but I do worry about jeopardizing the chances
>> of this patch and the others making it into 7.0.
> So would it make sense to just init/fini the b_o_s framework in your
> pass and for builtin expansion?

I think that should work for the sprintf checking.  Let me test it.
We can deal with the memxxx and strxxx patch (53562) independently
if you prefer.

Thanks
Martin

Re: [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245)

[Martin Sebor]

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

>>> That said, I defer to you on how to proceed here.  I'm prepared
>>> to do the work(*) but I do worry about jeopardizing the chances
>>> of this patch and the others making it into 7.0.
>> So would it make sense to just init/fini the b_o_s framework in your
>> pass and for builtin expansion?
>
> I think that should work for the sprintf checking.  Let me test it.
> We can deal with the memxxx and strxxx patch (53562) independently
> if you prefer.

Attached is a modified patch that calls {init,fini}_object_sizes()
from the gimple-ssa-sprintf pass instead.

While this works fine, I do like the approach of making the calls
in a single function better because it makes for a more robust API.
Decoupling the init/fini calls from the compute_object_size()
function that depends on them having been made makes the API easier
to accidentally misuse by calling one while forgetting to call one
or both of the other two.

Martin


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

PR middle-end/78245 - missing -Wformat-length on an overflow of a dynamically allocated buffer

gcc/testsuite/ChangeLog:

	PR middle-end/78245
	* gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: Add tests.

gcc/ChangeLog:

	PR middle-end/78245
	* gimple-ssa-sprintf.c (get_destination_size): Call
	compute_object_size.
	* tree-object-size.c (addr_object_size): Adjust.
	(pass_through_call): Adjust.
	(internal_object_size): New function.
	(compute_builtin_object_size): Call internal_object_size.
	(pass_object_sizes::execute): Adjust.
	* tree-object-size.h (fini_object_sizes): Declare.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index ead8b0e..34b3723 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -2466,6 +2466,9 @@ get_destination_size (tree dest)
      a member array as opposed to the whole enclosing object), otherwise
      use type-zero object size to determine the size of the enclosing
      object (the function fails without optimization in this type).  */
+
+  init_object_sizes ();
+
   int ost = optimize > 0;
   unsigned HOST_WIDE_INT size;
   if (compute_builtin_object_size (dest, ost, &size))
@@ -2800,6 +2803,8 @@ pass_sprintf_length::execute (function *fun)
 	}
     }
 
+  fini_object_sizes ();
+
   return 0;
 }
 
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
index 8d97fa8..9874332 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
@@ -1,5 +1,10 @@
 /* { dg-do compile } */
-/* { dg-options "-std=c99 -O2 -Wformat -Wformat-length=1 -ftrack-macro-expansion=0" } */
+/* { dg-options "-O2 -Wformat -Wformat-length=1 -ftrack-macro-expansion=0" } */
+/* Verify that all sprintf built-ins detect overflow involving directives
+   with non-constant arguments known to be constrained by some range of
+   values, and even when writing into dynamically allocated buffers.
+   -O2 (-ftree-vrp) is necessary for the tests involving ranges to pass,
+   otherwise -O1 is sufficient.  */
 
 #ifndef LINE
 #  define LINE 0
@@ -7,18 +12,26 @@
 
 #define bos(x) __builtin_object_size (x, 0)
 
-#define T(bufsize, fmt, ...)						\
-    do {								\
-      if (!LINE || __LINE__ == LINE)					\
-	{								\
-	  char *d = (char *)__builtin_malloc (bufsize);			\
-	  __builtin___sprintf_chk (d, 0, bos (d), fmt, __VA_ARGS__);	\
-	  sink (d);							\
-	}								\
-    } while (0)
+/* Defined (and redefined) to the allocation function to use, either
+   malloc, or alloca, or a VLA.  */
+#define ALLOC(p, n)   (p) = __builtin_malloc (n)
 
-void
-sink (void*);
+/* Defined (and redefined) to the sprintf function to exercise.  */
+#define TEST_SPRINTF(d, maxsize, objsize, fmt, ...)		\
+  __builtin___sprintf_chk (d, 0, objsize, fmt, __VA_ARGS__)
+
+#define T(bufsize, fmt, ...)				\
+  do {							\
+    if (!LINE || __LINE__ == LINE)			\
+      {							\
+	char *d;					\
+	ALLOC (d, bufsize);				\
+	TEST_SPRINTF (d, 0, bos (d), fmt, __VA_ARGS__);	\
+	sink (d);					\
+      }							\
+  } while (0)
+
+void sink (void*);
 
 /* Identity function to verify that the checker figures out the value
    of the operand even when it's not constant (i.e., makes use of
@@ -232,3 +245,88 @@ void test_sprintf_chk_range_sshort (signed short *a, signed short *b)
   T ( 4, "%i",  Ra (998,  999));
   T ( 4, "%i",  Ra (999, 1000)); /* { dg-warning "may write a terminating nul past the end of the destination" } */
 }
+
+/* Exercise ordinary sprintf with malloc.  */
+#undef TEST_SPRINTF
+#define TEST_SPRINTF(d, maxsize, objsize, fmt, ...)	\
+  __builtin_sprintf (d, fmt, __VA_ARGS__)
+
+void test_sprintf_malloc (const char *s, const char *t)
+{
+#define x x ()
+
+  T (1, "%-s", x ? "" : "1");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : "");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : "1");        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : s);        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : t);
+
+  T (2, "%-s", x ? "" : "1");
+  T (2, "%-s", x ? "" : s);
+  T (2, "%-s", x ? "1" : "");
+  T (2, "%-s", x ? s : "");
+  T (2, "%-s", x ? "1" : "2");
+  T (2, "%-s", x ? "" : "12");      /* { dg-warning "nul past the end" } */
+  T (2, "%-s", x ? "12" : "");      /* { dg-warning "nul past the end" } */
+
+  T (2, "%-s", x ? "" : "123");     /* { dg-warning "into a region" } */
+  T (2, "%-s", x ? "123" : "");     /* { dg-warning "into a region" } */
+
+#undef x
+}
+
+/* Exercise ordinary sprintf with alloca.  */
+#undef ALLOC
+#define ALLOC(p, n) (p) = __builtin_alloca (n)
+
+void test_sprintf_alloca (const char *s, const char *t)
+{
+#define x x ()
+
+  T (1, "%-s", x ? "" : "1");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : "");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : "1");        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : s);        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : t);
+
+  T (2, "%-s", x ? "" : "1");
+  T (2, "%-s", x ? "" : s);
+  T (2, "%-s", x ? "1" : "");
+  T (2, "%-s", x ? s : "");
+  T (2, "%-s", x ? "1" : "2");
+  T (2, "%-s", x ? "" : "12");      /* { dg-warning "nul past the end" } */
+  T (2, "%-s", x ? "12" : "");      /* { dg-warning "nul past the end" } */
+
+  T (2, "%-s", x ? "" : "123");     /* { dg-warning "into a region" } */
+  T (2, "%-s", x ? "123" : "");     /* { dg-warning "into a region" } */
+
+#undef x
+}
+
+/* Exercise ordinary sprintf with a VLA.  */
+#undef ALLOC
+#define ALLOC(p, n) char vla [i (n)]; (p) = vla
+
+void test_sprintf_vla (const char *s, const char *t)
+{
+#define x x ()
+
+  T (1, "%-s", x ? "" : "1");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : "");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : "1");        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : s);        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : t);
+
+  T (2, "%-s", x ? "" : "1");
+  T (2, "%-s", x ? "" : s);
+  T (2, "%-s", x ? "1" : "");
+  T (2, "%-s", x ? s : "");
+  T (2, "%-s", x ? "1" : "2");
+  T (2, "%-s", x ? "" : "12");      /* { dg-warning "nul past the end" } */
+  T (2, "%-s", x ? "12" : "");      /* { dg-warning "nul past the end" } */
+
+  T (2, "%-s", x ? "" : "123");     /* { dg-warning "into a region" } */
+  T (2, "%-s", x ? "123" : "");     /* { dg-warning "into a region" } */
+
+#undef x
+}
diff --git a/gcc/tree-object-size.c b/gcc/tree-object-size.c
index 1317ad7..3a2d54d 100644
--- a/gcc/tree-object-size.c
+++ b/gcc/tree-object-size.c
@@ -50,6 +50,7 @@ static const unsigned HOST_WIDE_INT unknown[4] = {
   0
 };
 
+static bool internal_object_size (tree, int, unsigned HOST_WIDE_INT *);
 static tree compute_object_offset (const_tree, const_tree);
 static bool addr_object_size (struct object_size_info *,
 			      const_tree, int, unsigned HOST_WIDE_INT *);
@@ -187,7 +188,7 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
       if (!osi || (object_size_type & 1) != 0
 	  || TREE_CODE (TREE_OPERAND (pt_var, 0)) != SSA_NAME)
 	{
-	  compute_builtin_object_size (TREE_OPERAND (pt_var, 0),
+	  internal_object_size (TREE_OPERAND (pt_var, 0),
 				       object_size_type & ~1, &sz);
 	}
       else
@@ -491,14 +492,14 @@ pass_through_call (const gcall *call)
 }
 
 
-/* Compute __builtin_object_size value for PTR and set *PSIZE to
-   the resulting value.  OBJECT_SIZE_TYPE is the second argument
-   to __builtin_object_size.  Return true on success and false
-   when the object size could not be determined.  */
+/* Compute the size of the object pointed to by PTR and set *PSIZE
+   to the resulting value.  OBJECT_SIZE_TYPE is the second argument
+   to __builtin_object_size.  Return true on success and false when
+   the object size could not be determined.  */
 
 bool
-compute_builtin_object_size (tree ptr, int object_size_type,
-			     unsigned HOST_WIDE_INT *psize)
+internal_object_size (tree ptr, int object_size_type,
+		      unsigned HOST_WIDE_INT *psize)
 {
   gcc_assert (object_size_type >= 0 && object_size_type <= 3);
 
@@ -534,7 +535,7 @@ compute_builtin_object_size (tree ptr, int object_size_type,
 	      ptr = gimple_assign_rhs1 (def);
 
 	      if (cst_and_fits_in_hwi (offset)
-		  && compute_builtin_object_size (ptr, object_size_type, psize))
+		  && internal_object_size (ptr, object_size_type, psize))
 		{
 		  /* Return zero when the offset is out of bounds.  */
 		  unsigned HOST_WIDE_INT off = tree_to_shwi (offset);
@@ -664,6 +665,18 @@ compute_builtin_object_size (tree ptr, int object_size_type,
   return *psize != unknown[object_size_type];
 }
 
+/* Compute __builtin_object_size value for PTR and set *PSIZE to
+   the resulting value.  OBJECT_SIZE_TYPE is the second argument
+   to __builtin_object_size.  Return true on success and false
+   when the object size could not be determined.  */
+
+bool
+compute_builtin_object_size (tree ptr, int object_size_type,
+			     unsigned HOST_WIDE_INT *psize)
+{
+  return internal_object_size (ptr, object_size_type, psize);
+}
+
 /* Compute object_sizes for PTR, defined to VALUE, which is not an SSA_NAME.  */
 
 static void
@@ -1230,7 +1243,7 @@ init_object_sizes (void)
 
 /* Destroy data structures after the object size computation.  */
 
-static void
+void
 fini_object_sizes (void)
 {
   int object_size_type;
@@ -1325,7 +1338,7 @@ pass_object_sizes::execute (function *fun)
 		    {
 		      tree type = TREE_TYPE (lhs);
 		      unsigned HOST_WIDE_INT bytes;
-		      if (compute_builtin_object_size (ptr, object_size_type,
+		      if (internal_object_size (ptr, object_size_type,
 						       &bytes)
 			  && wi::fits_to_tree_p (bytes, type))
 			{
diff --git a/gcc/tree-object-size.h b/gcc/tree-object-size.h
index 38c3e07..b656339 100644
--- a/gcc/tree-object-size.h
+++ b/gcc/tree-object-size.h
@@ -21,6 +21,7 @@ along with GCC; see the file COPYING3.  If not see
 #define GCC_TREE_OBJECT_SIZE_H
 
 extern void init_object_sizes (void);
+extern void fini_object_sizes (void);
 extern bool compute_builtin_object_size (tree, int, unsigned HOST_WIDE_INT *);
 
 #endif  // GCC_TREE_OBJECT_SIZE_H

Re: [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245)

[Jeff Law]

On 11/29/2016 08:22 PM, Martin Sebor wrote:
>>>> That said, I defer to you on how to proceed here.  I'm prepared
>>>> to do the work(*) but I do worry about jeopardizing the chances
>>>> of this patch and the others making it into 7.0.
>>> So would it make sense to just init/fini the b_o_s framework in your
>>> pass and for builtin expansion?
>>
>> I think that should work for the sprintf checking.  Let me test it.
>> We can deal with the memxxx and strxxx patch (53562) independently
>> if you prefer.
>
> Attached is a modified patch that calls {init,fini}_object_sizes()
> from the gimple-ssa-sprintf pass instead.
>
> While this works fine, I do like the approach of making the calls
> in a single function better because it makes for a more robust API.
> Decoupling the init/fini calls from the compute_object_size()
> function that depends on them having been made makes the API easier
> to accidentally misuse by calling one while forgetting to call one
> or both of the other two.
It's not ideal, nor is the prospect of caching and potentially not 
invaliding properly.

I've started tackling these kinds problems by wrapping everything into a 
class with suitable ctors/dtors and methods.  With everything locked 
down inside a class, the only way to access the subsystem is by 
instantiating a suitable object (which obviously gives us control over 
init/fini).  The problem then boils down to not having that instantiated 
object live across passes, which usually isn't a problem in GCC :-)

I suggested it as a possibility, but wasn't going to demand it without 
knowing much more about the code in tree-object-size and how well it 
could be encapsulated.

ANyway, I'll take another look at the patch.  My recollection was that 
the only issue at hand was the init/fini/caching aspects.


jeff

Re: [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245)

[Jeff Law]

On 11/29/2016 08:22 PM, Martin Sebor wrote:
>>>> That said, I defer to you on how to proceed here.  I'm prepared
>>>> to do the work(*) but I do worry about jeopardizing the chances
>>>> of this patch and the others making it into 7.0.
>>> So would it make sense to just init/fini the b_o_s framework in your
>>> pass and for builtin expansion?
>>
>> I think that should work for the sprintf checking.  Let me test it.
>> We can deal with the memxxx and strxxx patch (53562) independently
>> if you prefer.
>
> Attached is a modified patch that calls {init,fini}_object_sizes()
> from the gimple-ssa-sprintf pass instead.
>
> While this works fine, I do like the approach of making the calls
> in a single function better because it makes for a more robust API.
> Decoupling the init/fini calls from the compute_object_size()
> function that depends on them having been made makes the API easier
> to accidentally misuse by calling one while forgetting to call one
> or both of the other two.
>
> Martin
>
>
> gcc-78245.diff
>
>
> PR middle-end/78245 - missing -Wformat-length on an overflow of a dynamically allocated buffer
>
> gcc/testsuite/ChangeLog:
>
> 	PR middle-end/78245
> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: Add tests.
>
> gcc/ChangeLog:
>
> 	PR middle-end/78245
> 	* gimple-ssa-sprintf.c (get_destination_size): Call
> 	compute_object_size.
> 	* tree-object-size.c (addr_object_size): Adjust.
> 	(pass_through_call): Adjust.
> 	(internal_object_size): New function.
> 	(compute_builtin_object_size): Call internal_object_size.
> 	(pass_object_sizes::execute): Adjust.
> 	* tree-object-size.h (fini_object_sizes): Declare.
>
> @@ -664,6 +665,18 @@ compute_builtin_object_size (tree ptr, int object_size_type,
>    return *psize != unknown[object_size_type];
>  }
>
> +/* Compute __builtin_object_size value for PTR and set *PSIZE to
> +   the resulting value.  OBJECT_SIZE_TYPE is the second argument
> +   to __builtin_object_size.  Return true on success and false
> +   when the object size could not be determined.  */
> +
> +bool
> +compute_builtin_object_size (tree ptr, int object_size_type,
> +			     unsigned HOST_WIDE_INT *psize)
> +{
> +  return internal_object_size (ptr, object_size_type, psize);
> +}
Is this wrapper still necessary now that we've pulled the init/fini 
routines out?  Seems like it shouldn't be.


I think that's the only issue left.  If the wrapper is needed, then the 
patch is fine.  If the wrapper isn't, then a patch with the wrapper 
removed is pre-approved after the usual testing.

jeff