Re: [PATCH] c-format: Add -Wformat-same-precision option [PR80060]

[Martin Sebor <msebor@gmail.com>]

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?

Some more comments on the documentation text are below.

> 
> Signed-off-by: Daniil Stas <daniil.stas@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

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

Martin