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