This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments
On 12/12/2017 09:13 AM, Qing Zhao wrote:
Hi, Richard,
thanks a lot for your review.
{
/* __copy is always the same for characters.
Check to see if copy function already exists. */
- sprintf (name, "__copy_character_%d", ts->kind);
+ name = xasprintf ("__copy_character_%d", ts->kind);
contained = ns->contained;
for (; contained; contained = contained->sibling)
if (contained->proc_name
@@ -2796,6 +2802,7 @@ find_intrinsic_vtab (gfc_typespec *ts)
vtab->ts.u.derived = vtype;
vtab->value = gfc_default_initializer (&vtab->ts);
}
+ free (name);
It looks like this might be in a performance critical lookup path
where we'd really
like to avoid allocating/freeing memory. Why's GFC_MAX_SYMBOL_LEN+1 not
enough? Leaving for fortran folks to comment - note you should CC
fortran@gcc.gnu.org <mailto:fortran@gcc.gnu.org>
for fortran patches (done).
I have sent this patch to fortran@gcc.gnu.org <mailto:fortran@gcc.gnu.org> per Thomas’s suggestion last week, and got approval by fortran team on last Friday:
https://gcc.gnu.org/ml/fortran/2017-12/msg00027.html
}
found_sym = vtab;
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index 353a46e..fef1969 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -1323,6 +1323,19 @@ get_range_strlen (tree arg, tree length[2], bitmap *visited, int type,
the array could have zero length. */
*minlen = ssize_int (0);
}
+
+ if (VAR_P (arg)
+ && TREE_CODE (TREE_TYPE (arg)) == ARRAY_TYPE)
+ {
+ val = TYPE_SIZE_UNIT (TREE_TYPE (arg));
+ if (!val || integer_zerop (val))
val might be non-constant so you also need to check TREE_CODE (val) !=
INTEGER_CST here.
Okay.
+ return false;
+ val = fold_build2 (MINUS_EXPR, TREE_TYPE (val), val,
+ integer_one_node);
val = wide_int_to_tree (TREE_TYPE (val), wi::minus (wi::to_wide (val), 1));
you pass a possibly bogus type of 1 to fold_build2 above so the wide-int variant
is prefered.
The gimple-fold.c changes are ok with that change.
Per your comments, the updated gimple-fold.c is like the following:
FWIW, I suspect Richard is thinking of VLAs with the INTEGER_CST
comment above. This is the case we discussed in private. It is
handled either way but the handling could be improved by determining
the size of the VLA from the first __builtin_alloca_with_align
argument and using it or its range as the minimum and maximum.
With that, the range of string lengths that fit in vla in
the following could be determined and the sprintf call could
be diagnosed:
char d[30];
void f (unsigned n)
{
if (n < 32)
{
char vla[n];
__builtin_sprintf (d, "%s", vla);
}
}
I think this would be a nice enhancement to add on top of yours,
not just for VLAs but for dynamically allocated arrays as well,
and not just for the sprintf pass but also for the strlen pass
to optimize cases like:
void f (unsigned n)
{
if (n < 32)
{
char vla[n];
fgets (vla, n, stdin);
unsigned len = strlen (vla);
if (len >= n) // cannot hold
abort (); // can be eliminated
}
}
That said, at some point, it might make more sense to change
those passes to start tracking these things as they traverse
the CFG rather than having get_range_strlen() do the work.
Martin
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index 353a46e..0500fba 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -1323,6 +1323,19 @@ get_range_strlen (tree arg, tree length[2], bitmap *visited, int type,
the array could have zero length. */
*minlen = ssize_int (0);
}
+
+ if (VAR_P (arg)
+ && TREE_CODE (TREE_TYPE (arg)) == ARRAY_TYPE)
+ {
+ val = TYPE_SIZE_UNIT (TREE_TYPE (arg));
+ if (!val || TREE_CODE (val) != INTEGER_CST || integer_zerop (val))
+ return false;
+ val = fold_build2 (MINUS_EXPR, TREE_TYPE (val), val,
+ build_int_cst (TREE_TYPE (val), 1));
+ /* Set the minimum size to zero since the string in
+ the array could have zero length. */
+ *minlen = ssize_int (0);
+ }
}
if (!val)
let me know any further issue with the above.
thanks a lot.
Qing