[committed] Don't suppress bogus usage of macros from system headers in -Wformat (PR c/78304)

c-lex.c: lex_string uses cpp_get_token rather than
cpp_get_token_with_location, and hence the C family of frontends
record the physical locations of tokens in string concatenations, rather
than the virtual locations, discarding any macro expansion information.

The resulting *tree* node from the concatenation does use virtual
locations (if appropriate).

Hence if we have a macro expansion within a string literal, the macro
expansion information is only recorded for the initial string token, and
not for any followup string tokens concatenated with it.

Hence any usage of macros defined in system headers in the concatenated
string literal (apart from as the first token) will erroneously be
treated as being within the system header by the substring location
code, and hence we fail to emit a warning for this bogus code:

void test (const char *msg)
{
  printf ("size: %" PRIu32 "\n", msg);
}

format_warning_va determines whether or not the pertinent substring
(here "%u") is spelled fully within the spelling of the whole format
string, and if it is (case 1), it uses just the pertinent substring as
the location of the diagnostic.

In the above case, we have the '%' within the format string, but the
'u' is within the macro definition; the location of 'u' is used for the
caret location, and this is the location used for determining if we're
inside a system header, and so we erroneously bail out without printing
 the warning.

This patch fixes things by tightening up the logic for case 1 of
format_warning_va to detect non-trivial situations like this, and to fall
back on cases 2 and 3, so that the location of the first string token of
the format string will be used as the primary location of the diagnostics
(and hence won't be in a system header); the location of the macro
definition may be printed as a supplementary note (for case 2).

For the test case above, this gives us back the missing warning
(hitting case 2, and hence printing the macro defn):

pr78304.c: In function ‘test’:
pr78304.c:9:11: warning: format ‘%u’ expects argument of type
‘unsigned int’, but argument 2 has type ‘size_t {aka long unsigned int}’ [-Wformat=]
   printf ("size: %" PRIu32 "\n", size);
           ^~~~~~~~~
In file included from pr78304.c:4:0:
/usr/include/inttypes.h:104:19: note: format string is defined here
 # define PRIu32  "u"

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu;
adds 34 PASS results to gcc.sum.

Committed to trunk as r244453.

gcc/ChangeLog:
	PR c/78304
	* substring-locations.c (format_warning_va): Strengthen case 1 so
	that both endpoints of the substring must be within the format
	range for just the substring to be printed.

gcc/testsuite/ChangeLog:
	PR c/78304
	* gcc.dg/format/diagnostic-ranges.c (test_macro): Undef INT_FMT.
	(test_macro_2): New test.
	(test_macro_3): New test.
	(test_macro_4): New test.
	(test_non_contiguous_strings): Convert line number to line offset.
	* gcc.dg/format/pr78304-2.c: New test case.
	* gcc.dg/format/pr78304.c: New test case.
---
 gcc/substring-locations.c                       |  2 +
 gcc/testsuite/gcc.dg/format/diagnostic-ranges.c | 53 ++++++++++++++++++++++++-
 gcc/testsuite/gcc.dg/format/pr78304-2.c         | 11 +++++
 gcc/testsuite/gcc.dg/format/pr78304.c           | 10 +++++
 4 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/format/pr78304-2.c
 create mode 100644 gcc/testsuite/gcc.dg/format/pr78304.c

Hi David,

On 13 January 2017 at 21:04, David Malcolm <dmalcolm@redhat.com> wrote:
> c-lex.c: lex_string uses cpp_get_token rather than
> cpp_get_token_with_location, and hence the C family of frontends
> record the physical locations of tokens in string concatenations, rather
> than the virtual locations, discarding any macro expansion information.
>
> The resulting *tree* node from the concatenation does use virtual
> locations (if appropriate).
>
> Hence if we have a macro expansion within a string literal, the macro
> expansion information is only recorded for the initial string token, and
> not for any followup string tokens concatenated with it.
>
> Hence any usage of macros defined in system headers in the concatenated
> string literal (apart from as the first token) will erroneously be
> treated as being within the system header by the substring location
> code, and hence we fail to emit a warning for this bogus code:
>
> void test (const char *msg)
> {
>   printf ("size: %" PRIu32 "\n", msg);
> }
>
> format_warning_va determines whether or not the pertinent substring
> (here "%u") is spelled fully within the spelling of the whole format
> string, and if it is (case 1), it uses just the pertinent substring as
> the location of the diagnostic.
>
> In the above case, we have the '%' within the format string, but the
> 'u' is within the macro definition; the location of 'u' is used for the
> caret location, and this is the location used for determining if we're
> inside a system header, and so we erroneously bail out without printing
>  the warning.
>
> This patch fixes things by tightening up the logic for case 1 of
> format_warning_va to detect non-trivial situations like this, and to fall
> back on cases 2 and 3, so that the location of the first string token of
> the format string will be used as the primary location of the diagnostics
> (and hence won't be in a system header); the location of the macro
> definition may be printed as a supplementary note (for case 2).
>
> For the test case above, this gives us back the missing warning
> (hitting case 2, and hence printing the macro defn):
>
> pr78304.c: In function ‘test’:
> pr78304.c:9:11: warning: format ‘%u’ expects argument of type
> ‘unsigned int’, but argument 2 has type ‘size_t {aka long unsigned int}’ [-Wformat=]
>    printf ("size: %" PRIu32 "\n", size);
>            ^~~~~~~~~
> In file included from pr78304.c:4:0:
> /usr/include/inttypes.h:104:19: note: format string is defined here
>  # define PRIu32  "u"
>
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu;
> adds 34 PASS results to gcc.sum.
>
These 2 tests fail on arm:

  gcc.dg/format/pr78304.c     (test for warnings, line 9)
  gcc.dg/format/pr78304.c   -DWIDE   (test for warnings, line 9)

Christophe



> Committed to trunk as r244453.
>
> gcc/ChangeLog:
>         PR c/78304
>         * substring-locations.c (format_warning_va): Strengthen case 1 so
>         that both endpoints of the substring must be within the format
>         range for just the substring to be printed.
>
> gcc/testsuite/ChangeLog:
>         PR c/78304
>         * gcc.dg/format/diagnostic-ranges.c (test_macro): Undef INT_FMT.
>         (test_macro_2): New test.
>         (test_macro_3): New test.
>         (test_macro_4): New test.
>         (test_non_contiguous_strings): Convert line number to line offset.
>         * gcc.dg/format/pr78304-2.c: New test case.
>         * gcc.dg/format/pr78304.c: New test case.
> ---
>  gcc/substring-locations.c                       |  2 +
>  gcc/testsuite/gcc.dg/format/diagnostic-ranges.c | 53 ++++++++++++++++++++++++-
>  gcc/testsuite/gcc.dg/format/pr78304-2.c         | 11 +++++
>  gcc/testsuite/gcc.dg/format/pr78304.c           | 10 +++++
>  4 files changed, 75 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.dg/format/pr78304-2.c
>  create mode 100644 gcc/testsuite/gcc.dg/format/pr78304.c
>
> diff --git a/gcc/substring-locations.c b/gcc/substring-locations.c
> index 8b41f2b..e2d8dd7 100644
> --- a/gcc/substring-locations.c
> +++ b/gcc/substring-locations.c
> @@ -118,6 +118,8 @@ format_warning_va (const substring_loc &fmt_loc,
>    else
>      {
>        if (fmt_substring_range.m_start >= fmt_loc_range.m_start
> +         && fmt_substring_range.m_start <= fmt_loc_range.m_finish
> +         && fmt_substring_range.m_finish >= fmt_loc_range.m_start
>           && fmt_substring_range.m_finish <= fmt_loc_range.m_finish)
>         /* Case 1.  */
>         {
> diff --git a/gcc/testsuite/gcc.dg/format/diagnostic-ranges.c b/gcc/testsuite/gcc.dg/format/diagnostic-ranges.c
> index e5e6ade..cab30f2 100644
> --- a/gcc/testsuite/gcc.dg/format/diagnostic-ranges.c
> +++ b/gcc/testsuite/gcc.dg/format/diagnostic-ranges.c
> @@ -254,12 +254,63 @@ void test_macro (const char *msg)
>                    ~^
>                    %s
>     { dg-end-multiline-output "" } */
> +#undef INT_FMT
> +}
> +
> +void test_macro_2 (const char *msg)
> +{
> +#define PRIu32 "u" /* { dg-message "17: format string is defined here" } */
> +  printf("hello %" PRIu32 " world", msg);  /* { dg-warning "10: format '%u' expects argument of type 'unsigned int', but argument 2 has type 'const char \\*' " } */
> +/* { dg-begin-multiline-output "" }
> +   printf("hello %" PRIu32 " world", msg);
> +          ^~~~~~~~~
> +   { dg-end-multiline-output "" } */
> +/* { dg-begin-multiline-output "" }
> + #define PRIu32 "u"
> +                 ^
> +   { dg-end-multiline-output "" } */
> +#undef PRIu32
> +}
> +
> +void test_macro_3 (const char *msg)
> +{
> +#define FMT_STRING "hello %i world" /* { dg-warning "20: format '%i' expects argument of type 'int', but argument 2 has type 'const char \\*' " } */
> +  printf(FMT_STRING, msg);  /* { dg-message "10: in expansion of macro 'FMT_STRING" } */
> +/* { dg-begin-multiline-output "" }
> + #define FMT_STRING "hello %i world"
> +                    ^
> +   { dg-end-multiline-output "" } */
> +/* { dg-begin-multiline-output "" }
> +   printf(FMT_STRING, msg);
> +          ^~~~~~~~~~
> +   { dg-end-multiline-output "" } */
> +#undef FMT_STRING
> +}
> +
> +void test_macro_4 (const char *msg)
> +{
> +#define FMT_STRING "hello %i world" /* { dg-warning "20: format '%i' expects argument of type 'int', but argument 2 has type 'const char \\*' " } */
> +  printf(FMT_STRING "\n", msg);  /* { dg-message "10: in expansion of macro 'FMT_STRING" } */
> +/* { dg-begin-multiline-output "" }
> + #define FMT_STRING "hello %i world"
> +                    ^
> +   { dg-end-multiline-output "" } */
> +/* { dg-begin-multiline-output "" }
> +   printf(FMT_STRING "\n", msg);
> +          ^~~~~~~~~~
> +   { dg-end-multiline-output "" } */
> +/* { dg-begin-multiline-output "" }
> + #define FMT_STRING "hello %i world"
> +                           ~^
> +                           %s
> +   { dg-end-multiline-output "" } */
> +#undef FMT_STRING
>  }
>
>  void test_non_contiguous_strings (void)
>  {
>    __builtin_printf(" %" "d ", 0.5); /* { dg-warning "20: format .%d. expects argument of type .int., but argument 2 has type .double." } */
> -                                    /* { dg-message "26: format string is defined here" "" { target *-*-* } 261 } */
> +                                    /* { dg-message "26: format string is defined here" "" { target *-*-* } .-1 } */
>    /* { dg-begin-multiline-output "" }
>     __builtin_printf(" %" "d ", 0.5);
>                      ^~~~
> diff --git a/gcc/testsuite/gcc.dg/format/pr78304-2.c b/gcc/testsuite/gcc.dg/format/pr78304-2.c
> new file mode 100644
> index 0000000..5ee6d65
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/format/pr78304-2.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -Wall -Wextra" } */
> +
> +extern int printf (const char *, ...);
> +
> +# define PRIu32                "u"
> +
> +void test (long size)
> +{
> +  printf ("size: %" PRIu32 "\n", size); /* { dg-warning "expects argument of type" } */
> +}
> diff --git a/gcc/testsuite/gcc.dg/format/pr78304.c b/gcc/testsuite/gcc.dg/format/pr78304.c
> new file mode 100644
> index 0000000..d0a96f6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/format/pr78304.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile { target inttypes_types } } */
> +/* { dg-options "-O2 -Wall -Wextra" } */
> +
> +#include <inttypes.h>
> +#include <stdio.h>
> +
> +void test (size_t size)
> +{
> +  printf ("size: %" PRIu32 "\n", size); /* { dg-warning "expects argument of type" } */
> +}
> --
> 1.8.5.3
>

Hi Christophe,

>> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu;
>> adds 34 PASS results to gcc.sum.
>>
> These 2 tests fail on arm:
>
>   gcc.dg/format/pr78304.c     (test for warnings, line 9)
>   gcc.dg/format/pr78304.c   -DWIDE   (test for warnings, line 9)

also on sparc-sun-solaris2.12 and i386-pc-solaris2.12, 32-bit only.

	Rainer