From: Marek Polacek <polacek@redhat.com>
To: Frolov Daniil <frolov.da@phystech.edu>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: -Wformat-overflow handling for %b and %B directives in C2X standard
Date: Fri, 1 Apr 2022 16:15:34 -0400 [thread overview]
Message-ID: <YkddZh/bgSxSUWI9@redhat.com> (raw)
In-Reply-To: <CAE_xJnacKik3kevkU1iQ3wEv1BR5GZXxox2ZzxqBrviN=4iWTA@mail.gmail.com>
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 <frolov.da@phystech.edu>
> 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
next prev parent reply other threads:[~2022-04-01 20:15 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-01 19:19 Frolov Daniil
2022-04-01 20:15 ` Marek Polacek [this message]
2022-04-06 21:10 ` Frolov Daniil
2022-04-11 21:56 ` Marek Polacek
2022-08-15 16:42 ` Frolov Daniil
2022-08-30 14:56 ` Marek Polacek
2022-09-01 9:41 ` Даниил Александрович Фролов
2022-11-28 17:36 ` Jeff Law
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YkddZh/bgSxSUWI9@redhat.com \
--to=polacek@redhat.com \
--cc=frolov.da@phystech.edu \
--cc=gcc-patches@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).