Hi, Martin
On Thu, 30 Sep 2021 09:02:28 -0600
Martin Sebor <mse...@gmail.com> wrote:
> On 9/26/21 3:52 PM, Daniil Stas via Gcc-patches wrote:
> > This option is enabled by default when -Wformat option is enabled. A
> > user can specify -Wno-format-same-precision to disable emitting
> > warnings about an argument passed to printf-like function having a
> > different type from the one specified in the format string if the
> > types precisions are the same.
>
> Having an option to control this -Wformat aspect seems useful so
> just a few comments mostly on the wording/naming choices.
>
> Coming up with good names is tricky but I wonder if we can find
> one that's clearer than "-Wformat-same-precision". Precision can
> mean a few different things in this context: in the representation
> of integers it refers to the number of value bits. In that of
> floating types, it refers to the number of significand bits. And
> in printf directives, it refers to what comes after the optional
> period and what controls the minimum number of digits to format
> (or maximum number of characters in a string). So "same precision"
> seems rather vague (and the proposed manual entry doesn't make it
> clear).
>
> IIUC, the option is specifically for directives that take integer
> arguments and controls whether using an argument of an incompatible
> integer type to a conversion specifier like i or x is diagnosed when
> the argument has the same precision as the expected type.
>
> With that in mind, would mentioning the word integer (or just int
> for short) be an improvement? E.g., -Wformat-int-precision?
>
Yes, I like -Wformat-int-precision name too.
> Some more comments on the documentation text are below.
>
> >
> > Signed-off-by: Daniil Stas <daniil.s...@posteo.net>
> >
> > gcc/c-family/ChangeLog:
> >
> > * c-format.c (check_format_types): Don't emit warnings about
> > type differences with the format string if
> > -Wno-format-same-precision is specified and the types have
> > the same precision.
> > * c.opt: Add -Wformat-same-precision option.
> >
> > gcc/ChangeLog:
> >
> > * doc/invoke.texi: Add -Wformat-same-precision option
> > description.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * c-c++-common/Wformat-same-precision-1.c: New test.
> > * c-c++-common/Wformat-same-precision-2.c: New test.
> > ---
> > gcc/c-family/c-format.c | 2 +-
> > gcc/c-family/c.opt | 5 +++++
> > gcc/doc/invoke.texi | 8 +++++++-
> > gcc/testsuite/c-c++-common/Wformat-same-precision-1.c | 7 +++++++
> > gcc/testsuite/c-c++-common/Wformat-same-precision-2.c | 7 +++++++
> > 5 files changed, 27 insertions(+), 2 deletions(-)
> > create mode 100644
> > gcc/testsuite/c-c++-common/Wformat-same-precision-1.c create mode
> > 100644 gcc/testsuite/c-c++-common/Wformat-same-precision-2.c
> >
> > diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c
> > index b4cb765a9d3..07cdcefbef8 100644
> > --- a/gcc/c-family/c-format.c
> > +++ b/gcc/c-family/c-format.c
> > @@ -4243,7 +4243,7 @@ check_format_types (const substring_loc
> > &fmt_loc, && (!pedantic || i < 2)
> > && char_type_flag)
> > continue;
> > - if (types->scalar_identity_flag
> > + if ((types->scalar_identity_flag ||
> > !warn_format_same_precision) && (TREE_CODE (cur_type) == TREE_CODE
> > (wanted_type) || (INTEGRAL_TYPE_P (cur_type)
> > && INTEGRAL_TYPE_P (wanted_type)))
> > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> > index 9c151d19870..e7af7365c91 100644
> > --- a/gcc/c-family/c.opt
> > +++ b/gcc/c-family/c.opt
> > @@ -656,6 +656,11 @@ C ObjC C++ LTO ObjC++ Warning
> > Alias(Wformat-overflow=, 1, 0) IntegerRange(0, 2) Warn about
> > function calls with format strings that write past the end of the
> > destination region. Same as -Wformat-overflow=1.
> > +Wformat-same-precision
> > +C ObjC C++ ObjC++ Var(warn_format_same_precision) Warning
> > LangEnabledBy(C ObjC C++ ObjC++,Wformat=,warn_format >= 1, 0) +Warn
> > about type differences with the format string even if the types
> > +precision is the same.
>
> The grammar doesn't seem quite right here (I recommend to adjust
> the text as well along similar lines as the manual, except more
> brief as is customary here).
>
>
> > +
> > Wformat-security
> > C ObjC C++ ObjC++ Var(warn_format_security) Warning
> > LangEnabledBy(C ObjC C++ ObjC++,Wformat=, warn_format >= 2, 0) Warn
> > about possible security problems with format functions. diff --git
> > a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index
> > ba98eab68a5..8833f257d75 100644 --- a/gcc/doc/invoke.texi
> > +++ b/gcc/doc/invoke.texi
> > @@ -347,7 +347,7 @@ Objective-C and Objective-C++ Dialects}.
> > -Werror -Werror=* -Wexpansion-to-defined -Wfatal-errors @gol
> > -Wfloat-conversion -Wfloat-equal -Wformat -Wformat=2 @gol
> > -Wno-format-contains-nul -Wno-format-extra-args @gol
> > --Wformat-nonliteral -Wformat-overflow=@var{n} @gol
> > +-Wformat-nonliteral -Wformat-overflow=@var{n}
> > -Wformat-same-precision @gol -Wformat-security -Wformat-signedness
> > -Wformat-truncation=@var{n} @gol -Wformat-y2k -Wframe-address @gol
> > -Wframe-larger-than=@var{byte-size} -Wno-free-nonheap-object @gol
> > @@ -6054,6 +6054,12 @@ If @option{-Wformat} is specified, also warn
> > if the format string is not a string literal and so cannot be
> > checked, unless the format function takes its format arguments as a
> > @code{va_list}.
> > +@item -Wformat-same-precision
> > +@opindex Wformat-same-precision
> > +@opindex Wno-format-same-precision
> > +If @option{-Wformat} is specified, warn about type differences
> > with the format +string even if the types precision is the same.
> > +
>
> As I mentioned above, the description seems rather vague. How about
> this instead:
>
> Warn when passing an argument of an incompatible integer type to
> a d, i, o, u, x, or X conversion specifier even when it has
> the same precision as the expected type. For example, on targets
> where int64_t is a typedef for long, the warning is issued for
> the printf call below even when both long and long long have
> the same size and precision.
>
> @smallexample
> extern int64_t n;
> printf ("%lli", n);
> @end smallexample
>
Ok, I will reword the documentation.
> > @item -Wformat-security
> > @opindex Wformat-security
> > @opindex Wno-format-security
> > diff --git a/gcc/testsuite/c-c++-common/Wformat-same-precision-1.c
> > b/gcc/testsuite/c-c++-common/Wformat-same-precision-1.c new file
> > mode 100644 index 00000000000..fbc11e4200a
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/Wformat-same-precision-1.c
> > @@ -0,0 +1,7 @@
> > +/* { dg-do compile { target lp64 } } */
> > +/* { dg-options "-Wformat" } */
> > +
> > +void test ()
> > +{
> > + __builtin_printf ("%lu\n", (long long) 1); /* { dg-warning
> > "expects argument of type" } */ +}
> > diff --git a/gcc/testsuite/c-c++-common/Wformat-same-precision-2.c
> > b/gcc/testsuite/c-c++-common/Wformat-same-precision-2.c new file
> > mode 100644 index 00000000000..17e643e0441
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/Wformat-same-precision-2.c
> > @@ -0,0 +1,7 @@
> > +/* { dg-do compile { target lp64 } } */
> > +/* { dg-options "-Wformat -Wno-format-same-precision" } */
> > +
> > +void test ()
> > +{
> > + __builtin_printf ("%lu\n", (long long) 1); /* { ! dg-warning
> > "expects argument of type" } */
>
> I have never seen this syntax used in a dg-warning directive. Does
> it really work? (Normally, we'd use dg-bogus to verify that a message
> is not issued on a line.)
>
> Incidentally, by using #pragma GCC diagnostic to toggle the option
> both of these tests can be in the same file.
This test case passes with -Wno-format-same-precision option and fails
without it. But yes, maybe it just works by an accident (I could not
find the documentation how to make the inverse of dg-warning and tried
to guess the syntax a little). I will rework the tests too.
Thanks for your suggestions.
Best regards,
Daniil
