From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 47051 invoked by alias); 12 Dec 2017 08:20:24 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 47024 invoked by uid 89); 12 Dec 2017 08:20:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.7 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-Spam-User: qpsmtpd, 2 recipients X-HELO: mail-wm0-f42.google.com Received: from mail-wm0-f42.google.com (HELO mail-wm0-f42.google.com) (74.125.82.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 12 Dec 2017 08:20:21 +0000 Received: by mail-wm0-f42.google.com with SMTP id r78so19082712wme.5; Tue, 12 Dec 2017 00:20:20 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=xZGnDyQimcuuR+cI4QMj8mr3Mz1QxRIZS3CpblM6lNM=; b=kuSeZ+SHfwBDMth4/tZiuyS/hH5kurIYFthm2GDVaSHk+/HRAE7pRDQuvZCbHEK4Mi 5w0asonoWucs8UfM+DczCKdI19dzMAl+7fMeX3bfouBH97s0TatEB52QQD1/rznZv9Yc a3ZcL93lfFI/iznJAguTeAMTO6Y1OJJM0fwGcnldl2FW39fGF9FZAzJ3KMzUKopQCPxv ezgTbQ83M5jgg5zpAIRoxMVNlhZtVaf+keIyMpLopfN/1r8Kb6zoMdPoxkMsGmqI6vQr 8SM8g1ABHvgXn+6H8lnTxOq2aXUapY4ekd2Va8mXlmcxYJ6v8Kicfdo3+i49IFpjE0VK mxDQ== X-Gm-Message-State: AKGB3mL6+quf9EKeTB1HcjfskdAOWAF3ySySaqeLZahsHAC+RaxZMDHc A1Ax9AbERdSNFvKmz1ejLY68XEzUsyddde7gyzs= X-Google-Smtp-Source: ACJfBouhbufnjx+fASbrPTJy/ACrdNbr5d4c8B4Uw+kbBOXQq66pvyOP6bDiMI3mRDNu92H4ipRfnH/VCxbHojL32cw= X-Received: by 10.80.195.12 with SMTP id a12mr1783546edb.142.1513066818605; Tue, 12 Dec 2017 00:20:18 -0800 (PST) MIME-Version: 1.0 Received: by 10.80.167.196 with HTTP; Tue, 12 Dec 2017 00:20:18 -0800 (PST) In-Reply-To: References: From: Richard Biener Date: Tue, 12 Dec 2017 08:20:00 -0000 Message-ID: Subject: Re: [PATCH][Middle-end]79538 missing -Wformat-overflow with %s and non-member array arguments To: Qing Zhao , "fortran@gcc.gnu.org" Cc: gcc-patches , jeff Law , Richard Biener , Thomas Koenig Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2017-12/txt/msg00706.txt.bz2 On Mon, Dec 4, 2017 at 4:34 PM, Qing Zhao wrote: > Hi, > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D79538 > 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: > =E2=80=98sprintf=E2=80=99 output 2 or more bytes (assuming 257) into a de= stination > of size 256 > sprintf (buf1, "%s=3D%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 > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > gcc/ChangeLog > > 2017-11-30 Qing Zhao > > > 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 > > > 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 > > > 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 > > > 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 *mac= ro, > > /* Assemble the macro in the following fashion > macro =3D fp_cast [dec_str fp_suffix] */ > - sprintf (buf1, "%s%s", dec_str, fp_suffix); > - sprintf (buf2, fp_cast, buf1); > - sprintf (buf1, "%s=3D%s", macro, buf2); > + sprintf (buf2, "%s%s", dec_str, fp_suffix); > + sprintf (buf1, fp_cast, buf2); > + sprintf (buf, "%s=3D%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_at= tribute *attr, > rank =3D !(*as) || (*as)->rank =3D=3D -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 =3D 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 =3D xasprintf ("__class_%s_%d_%dp", tname, rank, (*as)->corank); > else if ((*as)) > - sprintf (name, "__class_%s_%d_%dt", tname, rank, (*as)->corank); > + name =3D xasprintf ("__class_%s_%d_%dt", tname, rank, (*as)->corank); > else if (attr->pointer) > - sprintf (name, "__class_%s_p", tname); > + name =3D xasprintf ("__class_%s_p", tname); > else if (attr->allocatable) > - sprintf (name, "__class_%s_a", tname); > + name =3D xasprintf ("__class_%s_a", tname); > else > - sprintf (name, "__class_%s_t", tname); > + name =3D xasprintf ("__class_%s_t", tname); > > if (ts->u.derived->attr.unlimited_polymorphic) > { > @@ -738,6 +739,7 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_attr= ibute *attr, > ts->u.derived =3D fclass; > attr->allocatable =3D attr->pointer =3D attr->dimension =3D attr->codim= ension =3D 0; > (*as) =3D 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 =3D false; > bool expr_null_wrapper =3D false; > gfc_expr *ancestor_wrapper =3D NULL, *rank; > @@ -1606,7 +1608,7 @@ generate_finalization_wrapper (gfc_symbol *derived,= gfc_namespace *ns, > sub_ns->resolved =3D 1; > > /* Set up the procedure symbol. */ > - sprintf (name, "__final_%s", tname); > + name =3D xasprintf ("__final_%s", tname); > gfc_get_symbol (name, sub_ns, &final); > sub_ns->proc_name =3D final; > final->attr.flavor =3D FL_PROCEDURE; > @@ -2172,6 +2174,7 @@ generate_finalization_wrapper (gfc_symbol *derived,= gfc_namespace *ns, > gfc_free_expr (rank); > vtab_final->initializer =3D gfc_lval_expr_from_sym (final); > vtab_final->ts.interface =3D 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 =3D 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 =3D 1; > vtab->attr.access =3D ACCESS_PUBLIC; > gfc_set_sym_referenced (vtab); > - sprintf (name, "__vtype_%s", tname); > + name =3D xasprintf ("__vtype_%s", tname); > > gfc_find_symbol (name, ns, 0, &vtype); > if (vtype =3D=3D NULL) > @@ -2373,7 +2377,7 @@ gfc_find_derived_vtab (gfc_symbol *derived) > else > { > /* Construct default initialization variable. */ > - sprintf (name, "__def_init_%s", tname); > + name =3D xasprintf ("__def_init_%s", tname); > gfc_get_symbol (name, ns, &def_init); > def_init->attr.target =3D 1; > def_init->attr.artificial =3D 1; > @@ -2406,7 +2410,7 @@ gfc_find_derived_vtab (gfc_symbol *derived) > ns->contained =3D sub_ns; > sub_ns->resolved =3D 1; > /* Set up procedure symbol. */ > - sprintf (name, "__copy_%s", tname); > + name =3D xasprintf ("__copy_%s", tname); > gfc_get_symbol (name, sub_ns, ©); > sub_ns->proc_name =3D copy; > copy->attr.flavor =3D FL_PROCEDURE; > @@ -2483,7 +2487,7 @@ gfc_find_derived_vtab (gfc_symbol *derived) > ns->contained =3D sub_ns; > sub_ns->resolved =3D 1; > /* Set up procedure symbol. */ > - sprintf (name, "__deallocate_%s", tname); > + name =3D xasprintf ("__deallocate_%s", tname); > gfc_get_symbol (name, sub_ns, &dealloc); > sub_ns->proc_name =3D dealloc; > dealloc->attr.flavor =3D FL_PROCEDURE; > @@ -2532,6 +2536,7 @@ have_vtype: > vtab->ts.u.derived =3D vtype; > vtab->value =3D gfc_default_initializer (&vtab->ts); > } > + free (name); > } > > found_sym =3D 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 comp= onent > of the class-variable. */ > sprintf (tname, "%s_%d_", gfc_basic_typename (ts->type), ts->kind); > - sprintf (name, "__vtab_%s", tname); > + name =3D 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 =3D 1; > vtab->attr.access =3D ACCESS_PUBLIC; > gfc_set_sym_referenced (vtab); > - sprintf (name, "__vtype_%s", tname); > + name =3D xasprintf ("__vtype_%s", tname); > > gfc_find_symbol (name, ns, 0, &vtype); > if (vtype =3D=3D NULL) > @@ -2722,12 +2728,12 @@ find_intrinsic_vtab (gfc_typespec *ts) > c->tb->ppc =3D 1; > > if (ts->type !=3D BT_CHARACTER) > - sprintf (name, "__copy_%s", tname); > + name =3D 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 =3D xasprintf ("__copy_character_%d", ts->kind); > contained =3D ns->contained; > for (; contained; contained =3D contained->sibling) > if (contained->proc_name > @@ -2796,6 +2802,7 @@ find_intrinsic_vtab (gfc_typespec *ts) > vtab->ts.u.derived =3D vtype; > vtab->value =3D 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 =3D 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 =3D ssize_int (0); > } > + > + if (VAR_P (arg) > + && TREE_CODE (TREE_TYPE (arg)) =3D=3D ARRAY_TYPE) > + { > + val =3D 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) !=3D INTEGER_CST here. > + return false; > + val =3D fold_build2 (MINUS_EXPR, TREE_TYPE (val), val, > + integer_one_node); val =3D 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 va= riant 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 =3D ssize_int (0); > + } > } > > if (!val) > diff --git a/gcc/testsuite/gcc.dg/pr79538.c b/gcc/testsuite/gcc.dg/pr7953= 8.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-membe= r 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 =3D i < 0 ? a3 : a4; > + __builtin_sprintf (d, "%s", s); /* { dg-warning ".__builtin_sprin= tf. may write a terminating nul past the end of the destination" } */ > + return; > +} > + > +void f () > +{ > + char des[3]; > + char src[] =3D "abcd"; > + __builtin_sprintf (des, "%s", src); /* { dg-warning "directive writing= up to 4 bytes into a region of size 3" } */ > + return; > +} > + > -- > 1.9.1