Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments

Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments

[Richard Biener]

On Mon, Dec 4, 2017 at 4:34 PM, Qing Zhao <qing.zhao@oracle.com> wrote:
> Hi,
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79538 <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79538>
> missing -Wformat-overflow with %s and non-member array arguments
>
> -Wformat-overflow uses the routine "get_range_strlen" to decide the
> maximum string length, however, currently "get_range_strlen" misses
> the handling of non-member arrays.
>
> Adding the handling of non-member array resolves the issue.
> Adding test case pr79538.c into gcc.dg.
>
> During gcc bootstrap, 2 source files (c-family/c-cppbuiltin.c,
> fortran/class.c) were detected new warnings by -Wformat-overflow
> due to the new handling of non-member array in "get_range_strlen".
> in order to avoid these new warnings and continue with bootstrap,
> updating these 2 files to avoid the warnings.
>
> in c-family/c-cppbuiltin.c, the warning is following:
>
> ../../latest_gcc_2/gcc/c-family/c-cppbuiltin.c:1627:15: note:
> ‘sprintf’ output 2 or more bytes (assuming 257) into a destination
> of size 256
>       sprintf (buf1, "%s=%s", macro, buf2);
>       ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> in the above, buf1 and buf2 are declared as:
> char buf1[256], buf2[256];
>
> i.e, buf1 and buf2 have same size. adjusting the size of buf1 and
> buf2 resolves the warning.
>
> fortran/class.c has the similar issue as above. Instead of adjusting
> size of the buffers, replacing sprintf with xasprintf.
>
> bootstraped and tested on both X86 and aarch64. no regression.
>
> Okay for trunk?
>
> thanks.
>
> Qing
>
> ===========================================================
>
> gcc/ChangeLog
>
> 2017-11-30  Qing Zhao  <qing.zhao@oracle.com <mailto:qing.zhao@oracle.com>>
>
>       PR middle_end/79538
>       * gimple-fold.c (get_range_strlen): Add the handling of non-member array.
>
> gcc/fortran/ChangeLog
>
> 2017-11-30  Qing Zhao  <qing.zhao@oracle.com <mailto:qing.zhao@oracle.com>>
>
>        PR middle_end/79538
>        * class.c (gfc_build_class_symbol): Replace call to
>        sprintf with xasprintf to avoid format-overflow warning.
>        (generate_finalization_wrapper): Likewise.
>        (gfc_find_derived_vtab): Likewise.
>        (find_intrinsic_vtab): Likewise.
>
>
> gcc/c-family/ChangeLog
>
> 2017-11-30  Qing Zhao  <qing.zhao@oracle.com <mailto:qing.zhao@oracle.com>>
>
>       PR middle_end/79538
>         * c-cppbuiltin.c (builtin_define_with_hex_fp_value):
>       Adjust the size of buf1 and buf2, add a new buf to avoid
>       format-overflow warning.
>
> gcc/testsuite/ChangeLog
>
> 2017-11-30  Qing Zhao  <qing.zhao@oracle.com <mailto:qing.zhao@oracle.com>>
>
>       PR middle_end/79538
>       * gcc.dg/pr79538.c: New test.
>
> ---
> gcc/c-family/c-cppbuiltin.c    | 10 ++++-----
> gcc/fortran/class.c            | 49 ++++++++++++++++++++++++------------------
> gcc/gimple-fold.c              | 13 +++++++++++
> gcc/testsuite/gcc.dg/pr79538.c | 23 ++++++++++++++++++++
> 4 files changed, 69 insertions(+), 26 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/pr79538.c
>
> diff --git a/gcc/c-family/c-cppbuiltin.c b/gcc/c-family/c-cppbuiltin.c
> index 2ac9616..9e33aed 100644
> --- a/gcc/c-family/c-cppbuiltin.c
> +++ b/gcc/c-family/c-cppbuiltin.c
> @@ -1613,7 +1613,7 @@ builtin_define_with_hex_fp_value (const char *macro,
>                                   const char *fp_cast)
> {
>   REAL_VALUE_TYPE real;
> -  char dec_str[64], buf1[256], buf2[256];
> +  char dec_str[64], buf[256], buf1[128], buf2[64];
>
>   /* This is very expensive, so if possible expand them lazily.  */
>   if (lazy_hex_fp_value_count < 12
> @@ -1656,11 +1656,11 @@ builtin_define_with_hex_fp_value (const char *macro,
>
>   /* Assemble the macro in the following fashion
>      macro = fp_cast [dec_str fp_suffix] */
> -  sprintf (buf1, "%s%s", dec_str, fp_suffix);
> -  sprintf (buf2, fp_cast, buf1);
> -  sprintf (buf1, "%s=%s", macro, buf2);
> +  sprintf (buf2, "%s%s", dec_str, fp_suffix);
> +  sprintf (buf1, fp_cast, buf2);
> +  sprintf (buf, "%s=%s", macro, buf1);
>
> -  cpp_define (parse_in, buf1);
> +  cpp_define (parse_in, buf);
> }

This change looks ok.

> /* Return a string constant for the suffix for a value of type TYPE
> diff --git a/gcc/fortran/class.c b/gcc/fortran/class.c
> index ebbd41b..a08fb8d 100644
> --- a/gcc/fortran/class.c
> +++ b/gcc/fortran/class.c
> @@ -602,7 +602,8 @@ bool
> gfc_build_class_symbol (gfc_typespec *ts, symbol_attribute *attr,
>                         gfc_array_spec **as)
> {
> -  char name[GFC_MAX_SYMBOL_LEN+1], tname[GFC_MAX_SYMBOL_LEN+1];
> +  char tname[GFC_MAX_SYMBOL_LEN+1];
> +  char *name;
>   gfc_symbol *fclass;
>   gfc_symbol *vtab;
>   gfc_component *c;
> @@ -633,17 +634,17 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_attribute *attr,
>   rank = !(*as) || (*as)->rank == -1 ? GFC_MAX_DIMENSIONS : (*as)->rank;
>   get_unique_hashed_string (tname, ts->u.derived);
>   if ((*as) && attr->allocatable)
> -    sprintf (name, "__class_%s_%d_%da", tname, rank, (*as)->corank);
> +    name = xasprintf ("__class_%s_%d_%da", tname, rank, (*as)->corank);
>   else if ((*as) && attr->pointer)
> -    sprintf (name, "__class_%s_%d_%dp", tname, rank, (*as)->corank);
> +    name = xasprintf ("__class_%s_%d_%dp", tname, rank, (*as)->corank);
>   else if ((*as))
> -    sprintf (name, "__class_%s_%d_%dt", tname, rank, (*as)->corank);
> +    name = xasprintf ("__class_%s_%d_%dt", tname, rank, (*as)->corank);
>   else if (attr->pointer)
> -    sprintf (name, "__class_%s_p", tname);
> +    name = xasprintf ("__class_%s_p", tname);
>   else if (attr->allocatable)
> -    sprintf (name, "__class_%s_a", tname);
> +    name = xasprintf ("__class_%s_a", tname);
>   else
> -    sprintf (name, "__class_%s_t", tname);
> +    name = xasprintf ("__class_%s_t", tname);
>
>   if (ts->u.derived->attr.unlimited_polymorphic)
>     {
> @@ -738,6 +739,7 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_attribute *attr,
>   ts->u.derived = fclass;
>   attr->allocatable = attr->pointer = attr->dimension = attr->codimension = 0;
>   (*as) = NULL;
> +  free (name);
>   return true;
> }
>
> @@ -1527,7 +1529,7 @@ generate_finalization_wrapper (gfc_symbol *derived, gfc_namespace *ns,
>   gfc_component *comp;
>   gfc_namespace *sub_ns;
>   gfc_code *last_code, *block;
> -  char name[GFC_MAX_SYMBOL_LEN+1];
> +  char *name;
>   bool finalizable_comp = false;
>   bool expr_null_wrapper = false;
>   gfc_expr *ancestor_wrapper = NULL, *rank;
> @@ -1606,7 +1608,7 @@ generate_finalization_wrapper (gfc_symbol *derived, gfc_namespace *ns,
>   sub_ns->resolved = 1;
>
>   /* Set up the procedure symbol.  */
> -  sprintf (name, "__final_%s", tname);
> +  name = xasprintf ("__final_%s", tname);
>   gfc_get_symbol (name, sub_ns, &final);
>   sub_ns->proc_name = final;
>   final->attr.flavor = FL_PROCEDURE;
> @@ -2172,6 +2174,7 @@ generate_finalization_wrapper (gfc_symbol *derived, gfc_namespace *ns,
>   gfc_free_expr (rank);
>   vtab_final->initializer = gfc_lval_expr_from_sym (final);
>   vtab_final->ts.interface = final;
> +  free (name);
> }
>
>
> @@ -2239,10 +2242,11 @@ gfc_find_derived_vtab (gfc_symbol *derived)
>
>   if (ns)
>     {
> -      char name[GFC_MAX_SYMBOL_LEN+1], tname[GFC_MAX_SYMBOL_LEN+1];
> +      char tname[GFC_MAX_SYMBOL_LEN+1];
> +      char *name;
>
>       get_unique_hashed_string (tname, derived);
> -      sprintf (name, "__vtab_%s", tname);
> +      name = xasprintf ("__vtab_%s", tname);
>
>       /* Look for the vtab symbol in various namespaces.  */
>       if (gsym && gsym->ns)
> @@ -2270,7 +2274,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
>           vtab->attr.vtab = 1;
>           vtab->attr.access = ACCESS_PUBLIC;
>           gfc_set_sym_referenced (vtab);
> -         sprintf (name, "__vtype_%s", tname);
> +         name = xasprintf ("__vtype_%s", tname);
>
>           gfc_find_symbol (name, ns, 0, &vtype);
>           if (vtype == NULL)
> @@ -2373,7 +2377,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
>               else
>                 {
>                   /* Construct default initialization variable.  */
> -                 sprintf (name, "__def_init_%s", tname);
> +                 name = xasprintf ("__def_init_%s", tname);
>                   gfc_get_symbol (name, ns, &def_init);
>                   def_init->attr.target = 1;
>                   def_init->attr.artificial = 1;
> @@ -2406,7 +2410,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
>                   ns->contained = sub_ns;
>                   sub_ns->resolved = 1;
>                   /* Set up procedure symbol.  */
> -                 sprintf (name, "__copy_%s", tname);
> +                 name = xasprintf ("__copy_%s", tname);
>                   gfc_get_symbol (name, sub_ns, &copy);
>                   sub_ns->proc_name = copy;
>                   copy->attr.flavor = FL_PROCEDURE;
> @@ -2483,7 +2487,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
>                   ns->contained = sub_ns;
>                   sub_ns->resolved = 1;
>                   /* Set up procedure symbol.  */
> -                 sprintf (name, "__deallocate_%s", tname);
> +                 name = xasprintf ("__deallocate_%s", tname);
>                   gfc_get_symbol (name, sub_ns, &dealloc);
>                   sub_ns->proc_name = dealloc;
>                   dealloc->attr.flavor = FL_PROCEDURE;
> @@ -2532,6 +2536,7 @@ have_vtype:
>           vtab->ts.u.derived = vtype;
>           vtab->value = gfc_default_initializer (&vtab->ts);
>         }
> +      free (name);
>     }
>
>   found_sym = vtab;
> @@ -2623,13 +2628,14 @@ find_intrinsic_vtab (gfc_typespec *ts)
>
>   if (ns)
>     {
> -      char name[GFC_MAX_SYMBOL_LEN+1], tname[GFC_MAX_SYMBOL_LEN+1];
> -
> +      char tname[GFC_MAX_SYMBOL_LEN+1];
> +      char *name;
> +
>       /* Encode all types as TYPENAME_KIND_ including especially character
>          arrays, whose length is now consistently stored in the _len component
>          of the class-variable.  */
>       sprintf (tname, "%s_%d_", gfc_basic_typename (ts->type), ts->kind);
> -      sprintf (name, "__vtab_%s", tname);
> +      name = xasprintf ("__vtab_%s", tname);
>
>       /* Look for the vtab symbol in the top-level namespace only.  */
>       gfc_find_symbol (name, ns, 0, &vtab);
> @@ -2646,7 +2652,7 @@ find_intrinsic_vtab (gfc_typespec *ts)
>           vtab->attr.vtab = 1;
>           vtab->attr.access = ACCESS_PUBLIC;
>           gfc_set_sym_referenced (vtab);
> -         sprintf (name, "__vtype_%s", tname);
> +         name = xasprintf ("__vtype_%s", tname);
>
>           gfc_find_symbol (name, ns, 0, &vtype);
>           if (vtype == NULL)
> @@ -2722,12 +2728,12 @@ find_intrinsic_vtab (gfc_typespec *ts)
>               c->tb->ppc = 1;
>
>               if (ts->type != BT_CHARACTER)
> -               sprintf (name, "__copy_%s", tname);
> +               name = xasprintf ("__copy_%s", tname);
>               else
>                 {
>                   /* __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
for fortran patches (done).

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

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

Richard.

> +              /* Set the minimum size to zero since the string in
> +                 the array could have zero length.  */
> +              *minlen = ssize_int (0);
> +            }
>         }
>
>       if (!val)
> diff --git a/gcc/testsuite/gcc.dg/pr79538.c b/gcc/testsuite/gcc.dg/pr79538.c
> new file mode 100644
> index 0000000..c5a631e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr79538.c
> @@ -0,0 +1,23 @@
> +/* PR middle-end/79538 - missing -Wformat-overflow with %s and non-member array arguments
> +   { dg-do compile }
> +   { dg-options "-O2 -Wformat-overflow" } */
> +
> +char a3[3];
> +char a4[4];
> +char d[3];
> +
> +void g (int i)
> +{
> +  const char *s = i < 0 ? a3 : a4;
> +  __builtin_sprintf (d, "%s", s);      /* { dg-warning ".__builtin_sprintf. may write a terminating nul past the end of the destination" } */
> +  return;
> +}
> +
> +void f ()
> +{
> +  char des[3];
> +  char src[] = "abcd";
> +  __builtin_sprintf (des, "%s", src); /* { dg-warning "directive writing up to 4 bytes into a region of size 3" } */
> +  return;
> +}
> +
> --
> 1.9.1

Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments

[Qing Zhao]

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:

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

Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments

[Richard Biener]

On December 12, 2017 5:13:19 PM GMT+01:00, Qing Zhao <qing.zhao@oracle.com> 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));

I don't see this requested change. 

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

Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments

[Martin Sebor]

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

Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments

[Qing Zhao]

> On Dec 12, 2017, at 10:50 AM, Richard Biener <rguenther@suse.de> wrote:
>>> 
>>>> +                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));
> 
> I don't see this requested change. 

>> +              val = fold_build2 (MINUS_EXPR, TREE_TYPE (val), val,
>> +				 build_int_cst (TREE_TYPE (val), 1));

for my original code:

>>>> +              val = fold_build2 (MINUS_EXPR, TREE_TYPE (val), val,
>>>> +                                 integer_one_node);

I thought that your original major concern was:

“ you pass a possibly bogus type of 1 to fold_build2 above” 

so I use “build_int_cst (TREE_TYPE (val), 1)" to replace the original “integer_one_node”
to make the type of the 1 exactly the same as “val”. 

do I miss anything here?

I don’t quite understand why I have to use:

 val = wide_int_to_tree (TREE_TYPE (val), wi::minus (wi::to_wide(val), 1));

to replace the original fold_build2 (MINUS_EXPR, TREE_TYPE (val), val, integer_one_node)? 

thanks.

Qing

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

Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments

[Qing Zhao]

Hi, Martin,

thanks for the suggestion, this might be a good enhancement for get_range_strlen for a future work. 

my understanding is,  the current get_range_strlen does not use value range info yet, and also does not handle VLA. 
we can improve it from both aspects in a later work.

Qing

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

Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments

[Martin Sebor]

On 12/12/2017 10:56 AM, Qing Zhao wrote:
> Hi, Martin,
>
> thanks for the suggestion, this might be a good enhancement for
> get_range_strlen for a future work.
>
> my understanding is,  the current get_range_strlen does not use value
> range info yet, and also does not handle VLA.
> we can improve it from both aspects in a later work.

I agree (that's also what I meant).  Your patch is a valuable
improvement in and of itself.

Martin

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

Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments

[Qing Zhao]

Hi,

I updated gimple-fold.c as you suggested, bootstrapped and re-tested on both x86 and aarch64. no any issue.

====
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index 353a46e..eb6a87a 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 = wide_int_to_tree (TREE_TYPE (val), 
+				      wi::sub(wi::to_wide (val), 1));
+	      /* Set the minimum size to zero since the string in
+		 the array could have zero length.  */
+	      *minlen = ssize_int (0);
+	    }
 	}
====

I plan to commit the change very soon. 
let me know if you have further comment.

thanks.

Qing

==========

the updated full patch is as following:

gcc/ChangeLog

2017-12-13  Qing Zhao  <qing.zhao@oracle.com >

     PR middle_end/79538
     * gimple-fold.c (get_range_strlen): Add the handling of non-member array.

gcc/fortran/ChangeLog

2017-12-13  Qing Zhao  <qing.zhao@oracle.com >

      PR middle_end/79538
      * class.c (gfc_build_class_symbol): Replace call to
      sprintf with xasprintf to avoid format-overflow warning.
      (generate_finalization_wrapper): Likewise.
      (gfc_find_derived_vtab): Likewise.
      (find_intrinsic_vtab): Likewise.


gcc/c-family/ChangeLog

2017-12-13  Qing Zhao  <qing.zhao@oracle.com >

     PR middle_end/79538 
	* c-cppbuiltin.c (builtin_define_with_hex_fp_value):
     Adjust the size of buf1 and buf2, add a new buf to avoid
     format-overflow warning.

gcc/testsuite/ChangeLog

2017-12-13  Qing Zhao  <qing.zhao@oracle.com >

     PR middle_end/79538
     * gcc.dg/pr79538.c: New test.

---
 gcc/c-family/c-cppbuiltin.c    | 10 ++++-----
 gcc/fortran/class.c            | 49 ++++++++++++++++++++++++------------------
 gcc/gimple-fold.c              | 13 +++++++++++
 gcc/testsuite/gcc.dg/pr79538.c | 23 ++++++++++++++++++++
 4 files changed, 69 insertions(+), 26 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr79538.c

diff --git a/gcc/c-family/c-cppbuiltin.c b/gcc/c-family/c-cppbuiltin.c
index 2ac9616..9e33aed 100644
--- a/gcc/c-family/c-cppbuiltin.c
+++ b/gcc/c-family/c-cppbuiltin.c
@@ -1613,7 +1613,7 @@ builtin_define_with_hex_fp_value (const char *macro,
 				  const char *fp_cast)
 {
   REAL_VALUE_TYPE real;
-  char dec_str[64], buf1[256], buf2[256];
+  char dec_str[64], buf[256], buf1[128], buf2[64];
 
   /* This is very expensive, so if possible expand them lazily.  */
   if (lazy_hex_fp_value_count < 12
@@ -1656,11 +1656,11 @@ builtin_define_with_hex_fp_value (const char *macro,
 
   /* Assemble the macro in the following fashion
      macro = fp_cast [dec_str fp_suffix] */
-  sprintf (buf1, "%s%s", dec_str, fp_suffix);
-  sprintf (buf2, fp_cast, buf1);
-  sprintf (buf1, "%s=%s", macro, buf2);
+  sprintf (buf2, "%s%s", dec_str, fp_suffix);
+  sprintf (buf1, fp_cast, buf2);
+  sprintf (buf, "%s=%s", macro, buf1);
 
-  cpp_define (parse_in, buf1);
+  cpp_define (parse_in, buf);
 }
 
 /* Return a string constant for the suffix for a value of type TYPE
diff --git a/gcc/fortran/class.c b/gcc/fortran/class.c
index ebbd41b..a08fb8d 100644
--- a/gcc/fortran/class.c
+++ b/gcc/fortran/class.c
@@ -602,7 +602,8 @@ bool
 gfc_build_class_symbol (gfc_typespec *ts, symbol_attribute *attr,
 			gfc_array_spec **as)
 {
-  char name[GFC_MAX_SYMBOL_LEN+1], tname[GFC_MAX_SYMBOL_LEN+1];
+  char tname[GFC_MAX_SYMBOL_LEN+1];
+  char *name;
   gfc_symbol *fclass;
   gfc_symbol *vtab;
   gfc_component *c;
@@ -633,17 +634,17 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_attribute *attr,
   rank = !(*as) || (*as)->rank == -1 ? GFC_MAX_DIMENSIONS : (*as)->rank;
   get_unique_hashed_string (tname, ts->u.derived);
   if ((*as) && attr->allocatable)
-    sprintf (name, "__class_%s_%d_%da", tname, rank, (*as)->corank);
+    name = xasprintf ("__class_%s_%d_%da", tname, rank, (*as)->corank);
   else if ((*as) && attr->pointer)
-    sprintf (name, "__class_%s_%d_%dp", tname, rank, (*as)->corank);
+    name = xasprintf ("__class_%s_%d_%dp", tname, rank, (*as)->corank);
   else if ((*as))
-    sprintf (name, "__class_%s_%d_%dt", tname, rank, (*as)->corank);
+    name = xasprintf ("__class_%s_%d_%dt", tname, rank, (*as)->corank);
   else if (attr->pointer)
-    sprintf (name, "__class_%s_p", tname);
+    name = xasprintf ("__class_%s_p", tname);
   else if (attr->allocatable)
-    sprintf (name, "__class_%s_a", tname);
+    name = xasprintf ("__class_%s_a", tname);
   else
-    sprintf (name, "__class_%s_t", tname);
+    name = xasprintf ("__class_%s_t", tname);
 
   if (ts->u.derived->attr.unlimited_polymorphic)
     {
@@ -738,6 +739,7 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_attribute *attr,
   ts->u.derived = fclass;
   attr->allocatable = attr->pointer = attr->dimension = attr->codimension = 0;
   (*as) = NULL;
+  free (name);
   return true;
 }
 
@@ -1527,7 +1529,7 @@ generate_finalization_wrapper (gfc_symbol *derived, gfc_namespace *ns,
   gfc_component *comp;
   gfc_namespace *sub_ns;
   gfc_code *last_code, *block;
-  char name[GFC_MAX_SYMBOL_LEN+1];
+  char *name;
   bool finalizable_comp = false;
   bool expr_null_wrapper = false;
   gfc_expr *ancestor_wrapper = NULL, *rank;
@@ -1606,7 +1608,7 @@ generate_finalization_wrapper (gfc_symbol *derived, gfc_namespace *ns,
   sub_ns->resolved = 1;
 
   /* Set up the procedure symbol.  */
-  sprintf (name, "__final_%s", tname);
+  name = xasprintf ("__final_%s", tname);
   gfc_get_symbol (name, sub_ns, &final);
   sub_ns->proc_name = final;
   final->attr.flavor = FL_PROCEDURE;
@@ -2172,6 +2174,7 @@ generate_finalization_wrapper (gfc_symbol *derived, gfc_namespace *ns,
   gfc_free_expr (rank);
   vtab_final->initializer = gfc_lval_expr_from_sym (final);
   vtab_final->ts.interface = final;
+  free (name);
 }
 
 
@@ -2239,10 +2242,11 @@ gfc_find_derived_vtab (gfc_symbol *derived)
 
   if (ns)
     {
-      char name[GFC_MAX_SYMBOL_LEN+1], tname[GFC_MAX_SYMBOL_LEN+1];
+      char tname[GFC_MAX_SYMBOL_LEN+1];
+      char *name;
 
       get_unique_hashed_string (tname, derived);
-      sprintf (name, "__vtab_%s", tname);
+      name = xasprintf ("__vtab_%s", tname);
 
       /* Look for the vtab symbol in various namespaces.  */
       if (gsym && gsym->ns)
@@ -2270,7 +2274,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
 	  vtab->attr.vtab = 1;
 	  vtab->attr.access = ACCESS_PUBLIC;
 	  gfc_set_sym_referenced (vtab);
-	  sprintf (name, "__vtype_%s", tname);
+	  name = xasprintf ("__vtype_%s", tname);
 
 	  gfc_find_symbol (name, ns, 0, &vtype);
 	  if (vtype == NULL)
@@ -2373,7 +2377,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
 	      else
 		{
 		  /* Construct default initialization variable.  */
-		  sprintf (name, "__def_init_%s", tname);
+		  name = xasprintf ("__def_init_%s", tname);
 		  gfc_get_symbol (name, ns, &def_init);
 		  def_init->attr.target = 1;
 		  def_init->attr.artificial = 1;
@@ -2406,7 +2410,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
 		  ns->contained = sub_ns;
 		  sub_ns->resolved = 1;
 		  /* Set up procedure symbol.  */
-		  sprintf (name, "__copy_%s", tname);
+		  name = xasprintf ("__copy_%s", tname);
 		  gfc_get_symbol (name, sub_ns, &copy);
 		  sub_ns->proc_name = copy;
 		  copy->attr.flavor = FL_PROCEDURE;
@@ -2483,7 +2487,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
 		  ns->contained = sub_ns;
 		  sub_ns->resolved = 1;
 		  /* Set up procedure symbol.  */
-		  sprintf (name, "__deallocate_%s", tname);
+		  name = xasprintf ("__deallocate_%s", tname);
 		  gfc_get_symbol (name, sub_ns, &dealloc);
 		  sub_ns->proc_name = dealloc;
 		  dealloc->attr.flavor = FL_PROCEDURE;
@@ -2532,6 +2536,7 @@ have_vtype:
 	  vtab->ts.u.derived = vtype;
 	  vtab->value = gfc_default_initializer (&vtab->ts);
 	}
+      free (name);
     }
 
   found_sym = vtab;
@@ -2623,13 +2628,14 @@ find_intrinsic_vtab (gfc_typespec *ts)
 
   if (ns)
     {
-      char name[GFC_MAX_SYMBOL_LEN+1], tname[GFC_MAX_SYMBOL_LEN+1];
-
+      char tname[GFC_MAX_SYMBOL_LEN+1];
+      char *name;
+      
       /* Encode all types as TYPENAME_KIND_ including especially character
 	 arrays, whose length is now consistently stored in the _len component
 	 of the class-variable.  */
       sprintf (tname, "%s_%d_", gfc_basic_typename (ts->type), ts->kind);
-      sprintf (name, "__vtab_%s", tname);
+      name = xasprintf ("__vtab_%s", tname);
 
       /* Look for the vtab symbol in the top-level namespace only.  */
       gfc_find_symbol (name, ns, 0, &vtab);
@@ -2646,7 +2652,7 @@ find_intrinsic_vtab (gfc_typespec *ts)
 	  vtab->attr.vtab = 1;
 	  vtab->attr.access = ACCESS_PUBLIC;
 	  gfc_set_sym_referenced (vtab);
-	  sprintf (name, "__vtype_%s", tname);
+	  name = xasprintf ("__vtype_%s", tname);
 
 	  gfc_find_symbol (name, ns, 0, &vtype);
 	  if (vtype == NULL)
@@ -2722,12 +2728,12 @@ find_intrinsic_vtab (gfc_typespec *ts)
 	      c->tb->ppc = 1;
 
 	      if (ts->type != BT_CHARACTER)
-		sprintf (name, "__copy_%s", tname);
+		name = xasprintf ("__copy_%s", tname);
 	      else
 		{
 		  /* __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);
     }
 
   found_sym = vtab;
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index 353a46e..eb6a87a 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 = wide_int_to_tree (TREE_TYPE (val), 
+				      wi::sub(wi::to_wide (val), 1));
+	      /* Set the minimum size to zero since the string in
+		 the array could have zero length.  */
+	      *minlen = ssize_int (0);
+	    }
 	}
 
       if (!val)
diff --git a/gcc/testsuite/gcc.dg/pr79538.c b/gcc/testsuite/gcc.dg/pr79538.c
new file mode 100644
index 0000000..c5a631e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr79538.c
@@ -0,0 +1,23 @@
+/* PR middle-end/79538 - missing -Wformat-overflow with %s and non-member array arguments
+   { dg-do compile }
+   { dg-options "-O2 -Wformat-overflow" } */
+
+char a3[3];
+char a4[4];
+char d[3];
+
+void g (int i)
+{
+  const char *s = i < 0 ? a3 : a4;
+  __builtin_sprintf (d, "%s", s);      /* { dg-warning ".__builtin_sprintf. may write a terminating nul past the end of the destination" } */
+  return;
+}
+
+void f ()
+{
+  char des[3];
+  char src[] = "abcd";
+  __builtin_sprintf (des, "%s", src); /* { dg-warning "directive writing up to 4 bytes into a region of size 3" } */
+  return;
+}
+
-- 
1.9.1


> On Dec 12, 2017, at 10:50 AM, Richard Biener <rguenther@suse.de> wrote:
>>> 
>> 
>> 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));
> 
> I don't see this requested change. 
> 

Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments

[Richard Biener]

On Wed, 13 Dec 2017, Qing Zhao wrote:

> Hi,
> 
> I updated gimple-fold.c as you suggested, bootstrapped and re-tested on both x86 and aarch64. no any issue.
> 
> ====
> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
> index 353a46e..eb6a87a 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 = wide_int_to_tree (TREE_TYPE (val), 
> +				      wi::sub(wi::to_wide (val), 1));
> +	      /* Set the minimum size to zero since the string in
> +		 the array could have zero length.  */
> +	      *minlen = ssize_int (0);
> +	    }
>  	}
> ====
> 
> I plan to commit the change very soon. 
> let me know if you have further comment.

Looks good to me.

Richard.

> thanks.
> 
> Qing
> 
> ==========
> 
> the updated full patch is as following:
> 
> gcc/ChangeLog
> 
> 2017-12-13  Qing Zhao  <qing.zhao@oracle.com >
> 
>      PR middle_end/79538
>      * gimple-fold.c (get_range_strlen): Add the handling of non-member array.
> 
> gcc/fortran/ChangeLog
> 
> 2017-12-13  Qing Zhao  <qing.zhao@oracle.com >
> 
>       PR middle_end/79538
>       * class.c (gfc_build_class_symbol): Replace call to
>       sprintf with xasprintf to avoid format-overflow warning.
>       (generate_finalization_wrapper): Likewise.
>       (gfc_find_derived_vtab): Likewise.
>       (find_intrinsic_vtab): Likewise.
> 
> 
> gcc/c-family/ChangeLog
> 
> 2017-12-13  Qing Zhao  <qing.zhao@oracle.com >
> 
>      PR middle_end/79538 
> 	* c-cppbuiltin.c (builtin_define_with_hex_fp_value):
>      Adjust the size of buf1 and buf2, add a new buf to avoid
>      format-overflow warning.
> 
> gcc/testsuite/ChangeLog
> 
> 2017-12-13  Qing Zhao  <qing.zhao@oracle.com >
> 
>      PR middle_end/79538
>      * gcc.dg/pr79538.c: New test.
> 
> ---
>  gcc/c-family/c-cppbuiltin.c    | 10 ++++-----
>  gcc/fortran/class.c            | 49 ++++++++++++++++++++++++------------------
>  gcc/gimple-fold.c              | 13 +++++++++++
>  gcc/testsuite/gcc.dg/pr79538.c | 23 ++++++++++++++++++++
>  4 files changed, 69 insertions(+), 26 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/pr79538.c
> 
> diff --git a/gcc/c-family/c-cppbuiltin.c b/gcc/c-family/c-cppbuiltin.c
> index 2ac9616..9e33aed 100644
> --- a/gcc/c-family/c-cppbuiltin.c
> +++ b/gcc/c-family/c-cppbuiltin.c
> @@ -1613,7 +1613,7 @@ builtin_define_with_hex_fp_value (const char *macro,
>  				  const char *fp_cast)
>  {
>    REAL_VALUE_TYPE real;
> -  char dec_str[64], buf1[256], buf2[256];
> +  char dec_str[64], buf[256], buf1[128], buf2[64];
>  
>    /* This is very expensive, so if possible expand them lazily.  */
>    if (lazy_hex_fp_value_count < 12
> @@ -1656,11 +1656,11 @@ builtin_define_with_hex_fp_value (const char *macro,
>  
>    /* Assemble the macro in the following fashion
>       macro = fp_cast [dec_str fp_suffix] */
> -  sprintf (buf1, "%s%s", dec_str, fp_suffix);
> -  sprintf (buf2, fp_cast, buf1);
> -  sprintf (buf1, "%s=%s", macro, buf2);
> +  sprintf (buf2, "%s%s", dec_str, fp_suffix);
> +  sprintf (buf1, fp_cast, buf2);
> +  sprintf (buf, "%s=%s", macro, buf1);
>  
> -  cpp_define (parse_in, buf1);
> +  cpp_define (parse_in, buf);
>  }
>  
>  /* Return a string constant for the suffix for a value of type TYPE
> diff --git a/gcc/fortran/class.c b/gcc/fortran/class.c
> index ebbd41b..a08fb8d 100644
> --- a/gcc/fortran/class.c
> +++ b/gcc/fortran/class.c
> @@ -602,7 +602,8 @@ bool
>  gfc_build_class_symbol (gfc_typespec *ts, symbol_attribute *attr,
>  			gfc_array_spec **as)
>  {
> -  char name[GFC_MAX_SYMBOL_LEN+1], tname[GFC_MAX_SYMBOL_LEN+1];
> +  char tname[GFC_MAX_SYMBOL_LEN+1];
> +  char *name;
>    gfc_symbol *fclass;
>    gfc_symbol *vtab;
>    gfc_component *c;
> @@ -633,17 +634,17 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_attribute *attr,
>    rank = !(*as) || (*as)->rank == -1 ? GFC_MAX_DIMENSIONS : (*as)->rank;
>    get_unique_hashed_string (tname, ts->u.derived);
>    if ((*as) && attr->allocatable)
> -    sprintf (name, "__class_%s_%d_%da", tname, rank, (*as)->corank);
> +    name = xasprintf ("__class_%s_%d_%da", tname, rank, (*as)->corank);
>    else if ((*as) && attr->pointer)
> -    sprintf (name, "__class_%s_%d_%dp", tname, rank, (*as)->corank);
> +    name = xasprintf ("__class_%s_%d_%dp", tname, rank, (*as)->corank);
>    else if ((*as))
> -    sprintf (name, "__class_%s_%d_%dt", tname, rank, (*as)->corank);
> +    name = xasprintf ("__class_%s_%d_%dt", tname, rank, (*as)->corank);
>    else if (attr->pointer)
> -    sprintf (name, "__class_%s_p", tname);
> +    name = xasprintf ("__class_%s_p", tname);
>    else if (attr->allocatable)
> -    sprintf (name, "__class_%s_a", tname);
> +    name = xasprintf ("__class_%s_a", tname);
>    else
> -    sprintf (name, "__class_%s_t", tname);
> +    name = xasprintf ("__class_%s_t", tname);
>  
>    if (ts->u.derived->attr.unlimited_polymorphic)
>      {
> @@ -738,6 +739,7 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_attribute *attr,
>    ts->u.derived = fclass;
>    attr->allocatable = attr->pointer = attr->dimension = attr->codimension = 0;
>    (*as) = NULL;
> +  free (name);
>    return true;
>  }
>  
> @@ -1527,7 +1529,7 @@ generate_finalization_wrapper (gfc_symbol *derived, gfc_namespace *ns,
>    gfc_component *comp;
>    gfc_namespace *sub_ns;
>    gfc_code *last_code, *block;
> -  char name[GFC_MAX_SYMBOL_LEN+1];
> +  char *name;
>    bool finalizable_comp = false;
>    bool expr_null_wrapper = false;
>    gfc_expr *ancestor_wrapper = NULL, *rank;
> @@ -1606,7 +1608,7 @@ generate_finalization_wrapper (gfc_symbol *derived, gfc_namespace *ns,
>    sub_ns->resolved = 1;
>  
>    /* Set up the procedure symbol.  */
> -  sprintf (name, "__final_%s", tname);
> +  name = xasprintf ("__final_%s", tname);
>    gfc_get_symbol (name, sub_ns, &final);
>    sub_ns->proc_name = final;
>    final->attr.flavor = FL_PROCEDURE;
> @@ -2172,6 +2174,7 @@ generate_finalization_wrapper (gfc_symbol *derived, gfc_namespace *ns,
>    gfc_free_expr (rank);
>    vtab_final->initializer = gfc_lval_expr_from_sym (final);
>    vtab_final->ts.interface = final;
> +  free (name);
>  }
>  
>  
> @@ -2239,10 +2242,11 @@ gfc_find_derived_vtab (gfc_symbol *derived)
>  
>    if (ns)
>      {
> -      char name[GFC_MAX_SYMBOL_LEN+1], tname[GFC_MAX_SYMBOL_LEN+1];
> +      char tname[GFC_MAX_SYMBOL_LEN+1];
> +      char *name;
>  
>        get_unique_hashed_string (tname, derived);
> -      sprintf (name, "__vtab_%s", tname);
> +      name = xasprintf ("__vtab_%s", tname);
>  
>        /* Look for the vtab symbol in various namespaces.  */
>        if (gsym && gsym->ns)
> @@ -2270,7 +2274,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
>  	  vtab->attr.vtab = 1;
>  	  vtab->attr.access = ACCESS_PUBLIC;
>  	  gfc_set_sym_referenced (vtab);
> -	  sprintf (name, "__vtype_%s", tname);
> +	  name = xasprintf ("__vtype_%s", tname);
>  
>  	  gfc_find_symbol (name, ns, 0, &vtype);
>  	  if (vtype == NULL)
> @@ -2373,7 +2377,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
>  	      else
>  		{
>  		  /* Construct default initialization variable.  */
> -		  sprintf (name, "__def_init_%s", tname);
> +		  name = xasprintf ("__def_init_%s", tname);
>  		  gfc_get_symbol (name, ns, &def_init);
>  		  def_init->attr.target = 1;
>  		  def_init->attr.artificial = 1;
> @@ -2406,7 +2410,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
>  		  ns->contained = sub_ns;
>  		  sub_ns->resolved = 1;
>  		  /* Set up procedure symbol.  */
> -		  sprintf (name, "__copy_%s", tname);
> +		  name = xasprintf ("__copy_%s", tname);
>  		  gfc_get_symbol (name, sub_ns, &copy);
>  		  sub_ns->proc_name = copy;
>  		  copy->attr.flavor = FL_PROCEDURE;
> @@ -2483,7 +2487,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
>  		  ns->contained = sub_ns;
>  		  sub_ns->resolved = 1;
>  		  /* Set up procedure symbol.  */
> -		  sprintf (name, "__deallocate_%s", tname);
> +		  name = xasprintf ("__deallocate_%s", tname);
>  		  gfc_get_symbol (name, sub_ns, &dealloc);
>  		  sub_ns->proc_name = dealloc;
>  		  dealloc->attr.flavor = FL_PROCEDURE;
> @@ -2532,6 +2536,7 @@ have_vtype:
>  	  vtab->ts.u.derived = vtype;
>  	  vtab->value = gfc_default_initializer (&vtab->ts);
>  	}
> +      free (name);
>      }
>  
>    found_sym = vtab;
> @@ -2623,13 +2628,14 @@ find_intrinsic_vtab (gfc_typespec *ts)
>  
>    if (ns)
>      {
> -      char name[GFC_MAX_SYMBOL_LEN+1], tname[GFC_MAX_SYMBOL_LEN+1];
> -
> +      char tname[GFC_MAX_SYMBOL_LEN+1];
> +      char *name;
> +      
>        /* Encode all types as TYPENAME_KIND_ including especially character
>  	 arrays, whose length is now consistently stored in the _len component
>  	 of the class-variable.  */
>        sprintf (tname, "%s_%d_", gfc_basic_typename (ts->type), ts->kind);
> -      sprintf (name, "__vtab_%s", tname);
> +      name = xasprintf ("__vtab_%s", tname);
>  
>        /* Look for the vtab symbol in the top-level namespace only.  */
>        gfc_find_symbol (name, ns, 0, &vtab);
> @@ -2646,7 +2652,7 @@ find_intrinsic_vtab (gfc_typespec *ts)
>  	  vtab->attr.vtab = 1;
>  	  vtab->attr.access = ACCESS_PUBLIC;
>  	  gfc_set_sym_referenced (vtab);
> -	  sprintf (name, "__vtype_%s", tname);
> +	  name = xasprintf ("__vtype_%s", tname);
>  
>  	  gfc_find_symbol (name, ns, 0, &vtype);
>  	  if (vtype == NULL)
> @@ -2722,12 +2728,12 @@ find_intrinsic_vtab (gfc_typespec *ts)
>  	      c->tb->ppc = 1;
>  
>  	      if (ts->type != BT_CHARACTER)
> -		sprintf (name, "__copy_%s", tname);
> +		name = xasprintf ("__copy_%s", tname);
>  	      else
>  		{
>  		  /* __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);
>      }
>  
>    found_sym = vtab;
> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
> index 353a46e..eb6a87a 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 = wide_int_to_tree (TREE_TYPE (val), 
> +				      wi::sub(wi::to_wide (val), 1));
> +	      /* Set the minimum size to zero since the string in
> +		 the array could have zero length.  */
> +	      *minlen = ssize_int (0);
> +	    }
>  	}
>  
>        if (!val)
> diff --git a/gcc/testsuite/gcc.dg/pr79538.c b/gcc/testsuite/gcc.dg/pr79538.c
> new file mode 100644
> index 0000000..c5a631e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr79538.c
> @@ -0,0 +1,23 @@
> +/* PR middle-end/79538 - missing -Wformat-overflow with %s and non-member array arguments
> +   { dg-do compile }
> +   { dg-options "-O2 -Wformat-overflow" } */
> +
> +char a3[3];
> +char a4[4];
> +char d[3];
> +
> +void g (int i)
> +{
> +  const char *s = i < 0 ? a3 : a4;
> +  __builtin_sprintf (d, "%s", s);      /* { dg-warning ".__builtin_sprintf. may write a terminating nul past the end of the destination" } */
> +  return;
> +}
> +
> +void f ()
> +{
> +  char des[3];
> +  char src[] = "abcd";
> +  __builtin_sprintf (des, "%s", src); /* { dg-warning "directive writing up to 4 bytes into a region of size 3" } */
> +  return;
> +}
> +
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments

[Qing Zhao]

> On Dec 14, 2017, at 2:05 AM, Richard Biener <rguenther@suse.de> wrote:
> 
> On Wed, 13 Dec 2017, Qing Zhao wrote:
> 
>> Hi,
>> 
>> I updated gimple-fold.c as you suggested, bootstrapped and re-tested on both x86 and aarch64. no any issue.
>> 
>> ====
>> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
>> index 353a46e..eb6a87a 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 = wide_int_to_tree (TREE_TYPE (val), 
>> +				      wi::sub(wi::to_wide (val), 1));
>> +	      /* Set the minimum size to zero since the string in
>> +		 the array could have zero length.  */
>> +	      *minlen = ssize_int (0);
>> +	    }
>> 	}
>> ====
>> 
>> I plan to commit the change very soon. 
>> let me know if you have further comment.
> 
> Looks good to me.

thanks a lot for your review.

committed the patch as revision 255654
https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=255654 <https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=255654>

PR 79538 was filed against GCC7.0, So, I assume that this patch need to be backported to GCC7 branch.
I will do the backporting to GCC7 later this week if there is no objection. 

Qing
> Richard.
> 

Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments

[Jeff Law]

On 12/14/2017 12:22 PM, Qing Zhao wrote:
> 
>> On Dec 14, 2017, at 2:05 AM, Richard Biener <rguenther@suse.de
>> <mailto:rguenther@suse.de>> wrote:
>>
>> On Wed, 13 Dec 2017, Qing Zhao wrote:
>>
>>> Hi,
>>>
>>> I updated gimple-fold.c as you suggested, bootstrapped and re-tested
>>> on both x86 and aarch64. no any issue.
>>>
>>> ====
>>> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
>>> index 353a46e..eb6a87a 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 = wide_int_to_tree (TREE_TYPE (val), 
>>> +      wi::sub(wi::to_wide (val), 1));
>>> +      /* Set the minimum size to zero since the string in
>>> + the array could have zero length.  */
>>> +      *minlen = ssize_int (0);
>>> +    }
>>> }
>>> ====
>>>
>>> I plan to commit the change very soon. 
>>> let me know if you have further comment.
>>
>> Looks good to me.
> 
> thanks a lot for your review.
> 
> committed the patch as revision 255654
> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=255654
> 
> PR 79538 was filed against GCC7.0, So, I assume that this patch need to
> be backported to GCC7 branch.
> I will do the backporting to GCC7 later this week if there is no objection. 
We don't try to backport all fixes to the release branches -- we tend to
focus more on regressions that apply to those releases and codegen
correctness issues.

I'd think a missed warning isn't that important to backport.

Jeff

Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments

[Qing Zhao]

> On Dec 14, 2017, at 1:36 PM, Jeff Law <law@redhat.com> wrote:
> 
> On 12/14/2017 12:22 PM, Qing Zhao wrote:
>> 
>>> On Dec 14, 2017, at 2:05 AM, Richard Biener <rguenther@suse.de
>>> <mailto:rguenther@suse.de>> wrote:
>>> 
>>> On Wed, 13 Dec 2017, Qing Zhao wrote:
>>> 
>>>> Hi,
>>>> 
>>>> I updated gimple-fold.c as you suggested, bootstrapped and re-tested
>>>> on both x86 and aarch64. no any issue.
>>>> 
>>>> ====
>>>> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
>>>> index 353a46e..eb6a87a 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 = wide_int_to_tree (TREE_TYPE (val), 
>>>> +      wi::sub(wi::to_wide (val), 1));
>>>> +      /* Set the minimum size to zero since the string in
>>>> + the array could have zero length.  */
>>>> +      *minlen = ssize_int (0);
>>>> +    }
>>>> }
>>>> ====
>>>> 
>>>> I plan to commit the change very soon. 
>>>> let me know if you have further comment.
>>> 
>>> Looks good to me.
>> 
>> thanks a lot for your review.
>> 
>> committed the patch as revision 255654
>> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=255654
>> 
>> PR 79538 was filed against GCC7.0, So, I assume that this patch need to
>> be backported to GCC7 branch.
>> I will do the backporting to GCC7 later this week if there is no objection. 
> We don't try to backport all fixes to the release branches -- we tend to
> focus more on regressions that apply to those releases and codegen
> correctness issues.
> 
> I'd think a missed warning isn't that important to backport.

Okay. I see. 

then I will close PR79538 as fixed.

thanks.

Qing
> 
> Jeff