From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 3F3083858D1E for ; Mon, 11 Apr 2022 21:56:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3F3083858D1E Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-157-zjFWtWmtNfuFXwjCm4-hew-1; Mon, 11 Apr 2022 17:56:32 -0400 X-MC-Unique: zjFWtWmtNfuFXwjCm4-hew-1 Received: by mail-qv1-f72.google.com with SMTP id eq9-20020ad45969000000b0044444f71bb4so3525721qvb.3 for ; Mon, 11 Apr 2022 14:56:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=LMmpX/iX1mPHjLnrTp1Qqnc359eeF4e/uHaJN/PkMFU=; b=OQP5sHoTaOtjCfeh/M7K3B67X0MrSLFqf+DktBiHUQjIq5coIxD7hnKyseSypB5UCL s9AhEjam3EwQhuXf+ocg4VCcvPIg9mlrcgKFjQK/2cj/8kEmIe3SltqqnX30V5eXmbrW 9q8Xr+OY7mbOj70BsHwvP8v57Rer7GwsBxxFReH+M+0oiIL1wIKE5cHsZ/ynHjGIaWHm nXBuDpFw4+qViLajF6pgmsMqjnn2SHf2qifVzyX3W8Ys7wYEEsntqGnp5CjqsdXhewHU pk50e5GBWkJrCHgkf0moCmh76lLf6CbD7jrBbNRrJP/bfwSwNknaU44OdXhfdz5+3c6g QsRw== X-Gm-Message-State: AOAM533cDpR1Xnr4/AMEED2VKx1oQ3F3c40P4/1d2g2XhBCnUkB8otRQ jAYt0I7Cy8KC70BAjfl/GUOeicmbr2ZPwiufOmCMYs6EyFVoUomICzFp0x8QUYt6dftEVNxeGF4 nHbwth/5bkbCCghOgeg== X-Received: by 2002:a05:620a:2787:b0:69c:2fb0:4abf with SMTP id g7-20020a05620a278700b0069c2fb04abfmr1068859qkp.551.1649714191852; Mon, 11 Apr 2022 14:56:31 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzHIEA5HfIdrMsFT/Opf0w3ZRXp1lq1xhzZOwT//NnhquPCg21MyKa8Q7z47v/B77D21yeJNQ== X-Received: by 2002:a05:620a:2787:b0:69c:2fb0:4abf with SMTP id g7-20020a05620a278700b0069c2fb04abfmr1068847qkp.551.1649714191537; Mon, 11 Apr 2022 14:56:31 -0700 (PDT) Received: from redhat.com ([2601:184:4780:4310::d1bb]) by smtp.gmail.com with ESMTPSA id c20-20020a05622a025400b002e1dd71e797sm26023500qtx.15.2022.04.11.14.56.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 11 Apr 2022 14:56:30 -0700 (PDT) Date: Mon, 11 Apr 2022 17:56:28 -0400 From: Marek Polacek To: Frolov Daniil Cc: gcc-patches@gcc.gnu.org Subject: Re: -Wformat-overflow handling for %b and %B directives in C2X standard Message-ID: References: MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/2.1.5 (2021-12-30) X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Apr 2022 21:56:36 -0000 On Thu, Apr 07, 2022 at 02:10:48AM +0500, Frolov Daniil wrote: > Hello! Thanks for your feedback. I've tried to take into account your > comments. New patch applied to the letter. Thanks. > The only thing I have not removed is the check_std_c2x () function. From my > point of view -Wformat-overflow shouldn't be thrown if the standard < C2X. > So it's protection for false triggering. Sorry but I still think that is the wrong behavior. If you want to warn about C2X constructs in pre-C2X modes, use -Wpedantic. But if you want to use %b/%B as an extension in older dialects, that's OK too, so I don't know why users would want -Wformat-overflow disabled in that case. But perhaps other people disagree with me. > сб, 2 апр. 2022 г. в 01:15, Marek Polacek : > > > On Sat, Apr 02, 2022 at 12:19:47AM +0500, Frolov Daniil via Gcc-patches > > wrote: > > > Hello, I've noticed that -Wformat-overflow doesn't handle %b and %B > > > directives in the sprintf function. I've added a relevant issue in > > bugzilla > > > (bug #105129). > > > I attach a patch with a possible solution to the letter. > > > > Thanks for the patch. Support for C2X %b, %B formats is relatively new > > (Oct 2021) so it looks like gimple-ssa-sprintf.cc hasn't caught up. > > > > This is not a regression, so should probably wait till GCC 13. Anyway... > > > > > From 2051344e9500651f6e94c44cbc7820715382b957 Mon Sep 17 00:00:00 2001 > > > From: Frolov Daniil > > > Date: Fri, 1 Apr 2022 00:47:03 +0500 > > > Subject: [PATCH] Support %b, %B for -Wformat-overflow (sprintf, snprintf) > > > > > > testsuite: add tests to check -Wformat-overflow on %b. > > > Wformat-overflow1.c is compiled using -std=c2x so warning has to > > > be throwed > > > > > > Wformat-overflow2.c doesn't throw warnings cause c2x std isn't > > > used > > > > > > gcc/ChangeLog: > > > > > > * gimple-ssa-sprintf.cc > > > (check_std_c2x): New function > > > (fmtresult::type_max_digits): add base == 2 handling > > > (tree_digits): add handle for base == 2 > > > (format_integer): now handle %b and %B using base = 2 > > > (parse_directive): add cases to handle %b and %B directives > > > (compute_format_length): add handling for base = 2 > > > > The descriptions should start with a capital letter and end with a period, > > like "Handle base == 2." > > > > > gcc/testsuite/ChangeLog: > > > > > > * gcc.dg/Wformat-overflow1.c: New test. (using -std=c2x) > > > * gcc.dg/Wformat-overflow2.c: New test. (-std=c11 no warning) > > > > You can just say "New test." > > > > > --- > > > gcc/gimple-ssa-sprintf.cc | 42 ++++++++++++++++++++---- > > > gcc/testsuite/gcc.dg/Wformat-overflow1.c | 28 ++++++++++++++++ > > > gcc/testsuite/gcc.dg/Wformat-overflow2.c | 16 +++++++++ > > > 3 files changed, 79 insertions(+), 7 deletions(-) > > > create mode 100644 gcc/testsuite/gcc.dg/Wformat-overflow1.c > > > create mode 100644 gcc/testsuite/gcc.dg/Wformat-overflow2.c > > > > > > diff --git a/gcc/gimple-ssa-sprintf.cc b/gcc/gimple-ssa-sprintf.cc > > > index c93f12f90b5..7f68c2b6e51 100644 > > > --- a/gcc/gimple-ssa-sprintf.cc > > > +++ b/gcc/gimple-ssa-sprintf.cc > > > @@ -107,6 +107,15 @@ namespace { > > > > > > static int warn_level; > > > > > > +/* b_overflow_flag depends on the current standart when using gcc */ > > > > "standard" > > > > /* Comments should be formatted like this. */ > > > > > +static bool b_overflow_flag; > > > + > > > +/* check is current standart version equals C2X*/ > > > +static bool check_std_c2x () > > > +{ > > > + return !strcmp (lang_hooks.name, "GNU C2X"); > > > +} > > > > Is this really needed? ISTM that this new checking shouldn't depend on > > -std=c2x. If not using C2X, you only get a warning if -Wpedantic. So > > I think you should remove b_overflow_flag. > > > > > /* The minimum, maximum, likely, and unlikely maximum number of bytes > > > of output either a formatting function or an individual directive > > > can result in. */ > > > @@ -535,6 +544,8 @@ fmtresult::type_max_digits (tree type, int base) > > > unsigned prec = TYPE_PRECISION (type); > > > switch (base) > > > { > > > + case 2: > > > + return prec; > > > case 8: > > > return (prec + 2) / 3; > > > case 10: > > > @@ -857,11 +868,11 @@ tree_digits (tree x, int base, HOST_WIDE_INT prec, > > bool plus, bool prefix) > > > > > > /* Adjust a non-zero value for the base prefix, either hexadecimal, > > > or, unless precision has resulted in a leading zero, also octal. > > */ > > > - if (prefix && absval && (base == 16 || prec <= ndigs)) > > > + if (prefix && absval && (base == 2 || base == 16 || prec <= ndigs)) > > > { > > > if (base == 8) > > > res += 1; > > > - else if (base == 16) > > > + else if (base == 16 || base == 2) /*0x...(0X...) and > > 0b...(0B...)*/ > > > res += 2; > > > } > > > > > > @@ -1229,6 +1240,10 @@ format_integer (const directive &dir, tree arg, > > pointer_query &ptr_qry) > > > case 'u': > > > base = 10; > > > break; > > > + case 'b': > > > + case 'B': > > > + base = 2; > > > + break; > > > case 'o': > > > base = 8; > > > break; > > > @@ -1351,10 +1366,10 @@ format_integer (const directive &dir, tree arg, > > pointer_query &ptr_qry) > > > > > > /* Bump up the counters if WIDTH is greater than LEN. */ > > > res.adjust_for_width_or_precision (dir.width, dirtype, base, > > > - (sign | maybebase) + (base == > > 16)); > > > + (sign | maybebase) + (base == 2 > > || base == 16)); > > > /* Bump up the counters again if PRECision is greater still. */ > > > res.adjust_for_width_or_precision (dir.prec, dirtype, base, > > > - (sign | maybebase) + (base == > > 16)); > > > + (sign | maybebase) + (base == 2 > > || base == 16)); > > > > > > return res; > > > } > > > @@ -1503,7 +1518,7 @@ format_integer (const directive &dir, tree arg, > > pointer_query &ptr_qry) > > > if (res.range.min == 1) > > > res.range.likely += base == 8 ? 1 : 2; > > > else if (res.range.min == 2 > > > - && base == 16 > > > + && (base == 16 || base == 2) > > > && (dir.width[0] == 2 || dir.prec[0] == 2)) > > > ++res.range.likely; > > > } > > > @@ -1511,9 +1526,9 @@ format_integer (const directive &dir, tree arg, > > pointer_query &ptr_qry) > > > > > > res.range.unlikely = res.range.max; > > > res.adjust_for_width_or_precision (dir.width, dirtype, base, > > > - (sign | maybebase) + (base == 16)); > > > + (sign | maybebase) + (base == 2 || > > base == 16)); > > > res.adjust_for_width_or_precision (dir.prec, dirtype, base, > > > - (sign | maybebase) + (base == 16)); > > > + (sign | maybebase) + (base == 2 || > > base == 16)); > > > > > > return res; > > > } > > > @@ -3680,6 +3695,8 @@ parse_directive (call_info &info, > > > ++pf; > > > break; > > > } > > > + > > > + > > > > Drop this spurious change. > > > > > switch (target_to_host (*pf)) > > > { > > > @@ -3713,6 +3730,14 @@ parse_directive (call_info &info, > > > case 'X': > > > dir.fmtfunc = format_integer; > > > break; > > > + > > > + case 'b': > > > + case 'B': > > > + if (b_overflow_flag) { > > > + dir.fmtfunc = format_integer; > > > + break; > > > + } > > > + return 0; > > > > > > case 'p': > > > /* The %p output is implementation-defined. It's possible > > > @@ -4038,6 +4063,9 @@ compute_format_length (call_info &info, > > format_result *res, > > > > > > bool success = true; > > > > > > + /* Check for GNU C2X standart */ > > > + b_overflow_flag = check_std_c2x (); > > > + > > > for (const char *pf = info.fmtstr; ; ++dirno) > > > { > > > directive dir (&info, dirno); > > > diff --git a/gcc/testsuite/gcc.dg/Wformat-overflow1.c > > b/gcc/testsuite/gcc.dg/Wformat-overflow1.c > > > new file mode 100644 > > > index 00000000000..cf9766fae14 > > > --- /dev/null > > > +++ b/gcc/testsuite/gcc.dg/Wformat-overflow1.c > > > @@ -0,0 +1,28 @@ > > > +/* > > > + { dg-do compile } > > > + { dg-options "-Wformat-overflow -std=c2x" } > > > +*/ > > > + > > > +extern int sprintf (char* restrict, const char* restrict, ...); > > > + > > > +void test_warn () { > > > + > > > + int n = __INT_MAX__; > > > + char dst [5] = {0}; > > > + sprintf (dst, "%b", n); /* { dg-warning "-Wformat-overflow" } */ > > > + > > > + sprintf (dst, "%#b", n); /* { dg-warning "-Wformat-overflow" } */ > > > + > > > +} > > > + > > > +void test_no_warn () { > > > + > > > + char dst [5] = {0}; > > > + int n = 8; > > > + sprintf (dst, "%b", n); > > > + > > > + char another_dst [34] = {0}; > > > + n = __INT_MAX__; > > > + sprintf (another_dst, "%#b", n); > > > + > > > +} > > > diff --git a/gcc/testsuite/gcc.dg/Wformat-overflow2.c > > b/gcc/testsuite/gcc.dg/Wformat-overflow2.c > > > new file mode 100644 > > > index 00000000000..c6b1d9062a6 > > > --- /dev/null > > > +++ b/gcc/testsuite/gcc.dg/Wformat-overflow2.c > > > @@ -0,0 +1,16 @@ > > > +/* > > > + { dg-do compile } > > > + { dg-options "-Wformat-overflow -std=c11" } > > > +*/ > > > + > > > +extern int sprintf (char* restrict, const char* restrict, ...); > > > + > > > +void test_no_warn () { > > > + > > > + /*There is no reason to throw warning if std < c2x*/ > > > + > > > + char dst [5] = {0}; > > > + int n = 32; > > > + sprintf (dst, "%b", n); > > > + > > > +} > > > -- > > > 2.25.1 > > > > > > > > > Marek > > > > > From 7b14a2aa909194841fa916f2db5d8aa1e4a6367e Mon Sep 17 00:00:00 2001 > From: Frolov Daniil > Date: Thu, 7 Apr 2022 02:05:58 +0500 > Subject: [PATCH] Support %b, %B for -Wformat-overflow (sprintf, snprintf) > > gcc/ChangeLog: > > * gimple-ssa-sprintf.cc (check_std_c2x): Handle base == 2. > (fmtresult::type_max_digits): Handle base == 2. > (tree_digits): Handle base == 2. > (format_integer): Handle base == 2. > (parse_directive): Add cases for %b and %B directives. > (compute_format_length): Handle base == 2. > > gcc/testsuite/ChangeLog: > > * gcc.dg/Wformat-overflow1.c: New test. > * gcc.dg/Wformat-overflow2.c: New test. > --- > gcc/gimple-ssa-sprintf.cc | 40 +++++++++++++++++++----- > gcc/testsuite/gcc.dg/Wformat-overflow1.c | 28 +++++++++++++++++ > gcc/testsuite/gcc.dg/Wformat-overflow2.c | 16 ++++++++++ > 3 files changed, 77 insertions(+), 7 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/Wformat-overflow1.c > create mode 100644 gcc/testsuite/gcc.dg/Wformat-overflow2.c > > diff --git a/gcc/gimple-ssa-sprintf.cc b/gcc/gimple-ssa-sprintf.cc > index c93f12f90b5..6a14a004483 100644 > --- a/gcc/gimple-ssa-sprintf.cc > +++ b/gcc/gimple-ssa-sprintf.cc > @@ -107,6 +107,15 @@ namespace { > > static int warn_level; > > +/* The b_overflow_flag depends on the current standard when using gcc. */ > +static bool b_overflow_flag; > + > +/* Check is current standard version equals C2X. */ > +static bool check_std_c2x () > +{ > + return !strcmp (lang_hooks.name, "GNU C2X"); > +} > + > /* The minimum, maximum, likely, and unlikely maximum number of bytes > of output either a formatting function or an individual directive > can result in. */ > @@ -535,6 +544,8 @@ fmtresult::type_max_digits (tree type, int base) > unsigned prec = TYPE_PRECISION (type); > switch (base) > { > + case 2: > + return prec; > case 8: > return (prec + 2) / 3; > case 10: > @@ -857,11 +868,11 @@ tree_digits (tree x, int base, HOST_WIDE_INT prec, bool plus, bool prefix) > > /* Adjust a non-zero value for the base prefix, either hexadecimal, > or, unless precision has resulted in a leading zero, also octal. */ > - if (prefix && absval && (base == 16 || prec <= ndigs)) > + if (prefix && absval && (base == 2 || base == 16 || prec <= ndigs)) > { > if (base == 8) > res += 1; > - else if (base == 16) > + else if (base == 16 || base == 2) /* 0x...(0X...) and 0b...(0B...) */ > res += 2; > } > > @@ -1229,6 +1240,10 @@ format_integer (const directive &dir, tree arg, pointer_query &ptr_qry) > case 'u': > base = 10; > break; > + case 'b': > + case 'B': > + base = 2; > + break; > case 'o': > base = 8; > break; > @@ -1351,10 +1366,10 @@ format_integer (const directive &dir, tree arg, pointer_query &ptr_qry) > > /* Bump up the counters if WIDTH is greater than LEN. */ > res.adjust_for_width_or_precision (dir.width, dirtype, base, > - (sign | maybebase) + (base == 16)); > + (sign | maybebase) + (base == 2 || base == 16)); > /* Bump up the counters again if PRECision is greater still. */ > res.adjust_for_width_or_precision (dir.prec, dirtype, base, > - (sign | maybebase) + (base == 16)); > + (sign | maybebase) + (base == 2 || base == 16)); > > return res; > } > @@ -1503,7 +1518,7 @@ format_integer (const directive &dir, tree arg, pointer_query &ptr_qry) > if (res.range.min == 1) > res.range.likely += base == 8 ? 1 : 2; > else if (res.range.min == 2 > - && base == 16 > + && (base == 16 || base == 2) > && (dir.width[0] == 2 || dir.prec[0] == 2)) > ++res.range.likely; > } > @@ -1511,9 +1526,9 @@ format_integer (const directive &dir, tree arg, pointer_query &ptr_qry) > > res.range.unlikely = res.range.max; > res.adjust_for_width_or_precision (dir.width, dirtype, base, > - (sign | maybebase) + (base == 16)); > + (sign | maybebase) + (base == 2 || base == 16)); > res.adjust_for_width_or_precision (dir.prec, dirtype, base, > - (sign | maybebase) + (base == 16)); > + (sign | maybebase) + (base == 2 || base == 16)); > > return res; > } > @@ -3713,6 +3728,14 @@ parse_directive (call_info &info, > case 'X': > dir.fmtfunc = format_integer; > break; > + > + case 'b': > + case 'B': > + if (b_overflow_flag) { > + dir.fmtfunc = format_integer; > + break; > + } > + return 0; > > case 'p': > /* The %p output is implementation-defined. It's possible > @@ -4038,6 +4061,9 @@ compute_format_length (call_info &info, format_result *res, > > bool success = true; > > + /* Check for GNU C2X standard */ > + b_overflow_flag = check_std_c2x (); > + > for (const char *pf = info.fmtstr; ; ++dirno) > { > directive dir (&info, dirno); > diff --git a/gcc/testsuite/gcc.dg/Wformat-overflow1.c b/gcc/testsuite/gcc.dg/Wformat-overflow1.c > new file mode 100644 > index 00000000000..cf9766fae14 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/Wformat-overflow1.c > @@ -0,0 +1,28 @@ > +/* > + { dg-do compile } > + { dg-options "-Wformat-overflow -std=c2x" } > +*/ > + > +extern int sprintf (char* restrict, const char* restrict, ...); > + > +void test_warn () { > + > + int n = __INT_MAX__; > + char dst [5] = {0}; > + sprintf (dst, "%b", n); /* { dg-warning "-Wformat-overflow" } */ > + > + sprintf (dst, "%#b", n); /* { dg-warning "-Wformat-overflow" } */ > + > +} > + > +void test_no_warn () { > + > + char dst [5] = {0}; > + int n = 8; > + sprintf (dst, "%b", n); > + > + char another_dst [34] = {0}; > + n = __INT_MAX__; > + sprintf (another_dst, "%#b", n); > + > +} > diff --git a/gcc/testsuite/gcc.dg/Wformat-overflow2.c b/gcc/testsuite/gcc.dg/Wformat-overflow2.c > new file mode 100644 > index 00000000000..c6b1d9062a6 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/Wformat-overflow2.c > @@ -0,0 +1,16 @@ > +/* > + { dg-do compile } > + { dg-options "-Wformat-overflow -std=c11" } > +*/ > + > +extern int sprintf (char* restrict, const char* restrict, ...); > + > +void test_no_warn () { > + > + /*There is no reason to throw warning if std < c2x*/ > + > + char dst [5] = {0}; > + int n = 32; > + sprintf (dst, "%b", n); > + > +} > -- > 2.25.1 > Marek