diff mbox series

Fix Werror=format-diag with --disable-nls.

Message ID 9532d734-eac1-eecb-6dbe-cd0a7fc55b36@suse.cz
State New
Headers
Series Fix Werror=format-diag with --disable-nls. |

Commit Message

Martin Liška Jan. 20, 2022, 9:43 a.m. UTC 
  The patch disables "-Wformat-diag" for dump_aggr_type.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

	PR c++/104134

gcc/cp/ChangeLog:

	* error.cc (dump_aggr_type): Partially disable the warning.
---
  gcc/cp/error.cc | 9 +++++++++
  1 file changed, 9 insertions(+)

Comments

Jakub Jelinek Jan. 20, 2022, 10:17 a.m. UTC | #1 
On Thu, Jan 20, 2022 at 10:43:55AM +0100, Martin Liška wrote:
> The patch disables "-Wformat-diag" for dump_aggr_type.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?
> Thanks,
> Martin
> 
> 	PR c++/104134
> 
> gcc/cp/ChangeLog:
> 
> 	* error.cc (dump_aggr_type): Partially disable the warning.
> ---
>  gcc/cp/error.cc | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc
> index 1ab0c25a477..c031c75cc5e 100644
> --- a/gcc/cp/error.cc
> +++ b/gcc/cp/error.cc
> @@ -768,6 +768,11 @@ class_key_or_enum_as_string (tree t)
>      return "struct";
>  }
> +#if __GNUC__ >= 10
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wformat-diag"
> +#endif
> +
>  /* Print out a class declaration T under the control of FLAGS,
>     in the form `class foo'.  */
> @@ -851,6 +856,10 @@ dump_aggr_type (cxx_pretty_printer *pp, tree t, int flags)
>  			 flags & ~TFF_TEMPLATE_HEADER);
>  }
> +#if __GNUC__ >= 10
> +#pragma GCC diagnostic pop
> +#endif
> +

Please add an empty line above #if lines.
Also, it would be nice to use the same style of these at least in the
same file.  The others are:

/* Disable warnings about missing quoting in GCC diagnostics for
   the pp_verbatim calls.  Their format strings deliberately don't
   follow GCC diagnostic conventions.  */
#if __GNUC__ >= 10
#  pragma GCC diagnostic push
#  pragma GCC diagnostic ignored "-Wformat-diag"
#endif

and

#if __GNUC__ >= 10
#  pragma GCC diagnostic pop
#endif

The 2 spaces between # and pragma look just weird, so either
use in all the 4 spaces 1 space between # and pragma, or 0 spaces.
And also the copy the comment from above the other diagnostic push,
perhaps with small tweak (pp_verbatim -> pp_printf)?

Otherwise LGTM.

	Jakub
Jakub Jelinek Jan. 20, 2022, 10:28 a.m. UTC | #2 
On Thu, Jan 20, 2022 at 11:17:28AM +0100, Jakub Jelinek via Gcc-patches wrote:
> > --- a/gcc/cp/error.cc
> > +++ b/gcc/cp/error.cc
> > @@ -768,6 +768,11 @@ class_key_or_enum_as_string (tree t)
> >      return "struct";
> >  }
> > +#if __GNUC__ >= 10
> > +#pragma GCC diagnostic push
> > +#pragma GCC diagnostic ignored "-Wformat-diag"
> > +#endif
> > +
> >  /* Print out a class declaration T under the control of FLAGS,
> >     in the form `class foo'.  */
> > @@ -851,6 +856,10 @@ dump_aggr_type (cxx_pretty_printer *pp, tree t, int flags)
> >  			 flags & ~TFF_TEMPLATE_HEADER);
> >  }
> > +#if __GNUC__ >= 10
> > +#pragma GCC diagnostic pop
> > +#endif

Oh, and one more thing, but this time not about this source file but about
the warning.  Does it handle the gettext case?
I think -Wformat generally does, gettext has format_arg attribute.
If the warning handles
  pp_printf ("<unnamed %s>", str);
and
  pp_printf (cond ? "<unnamed %s>" : "<unnamed %s>", str);
and
  pp_printf (cond ? "<unnamed %s>" : "something %s", str);
and
  pp_printf (gettext ("<unnamed %s>"), str);
then maybe it should also handle
  pp_printf (cond ? gettext ("<unnamed %s>") : "<unnamed %s>, str);
and
  pp_printf (cond ? gettext ("<unnamed %s>") : "something %s, str);
too?

	Jakub
Martin Sebor Jan. 20, 2022, 4:33 p.m. UTC | #3 
On 1/20/22 03:28, Jakub Jelinek wrote:
> On Thu, Jan 20, 2022 at 11:17:28AM +0100, Jakub Jelinek via Gcc-patches wrote:
>>> --- a/gcc/cp/error.cc
>>> +++ b/gcc/cp/error.cc
>>> @@ -768,6 +768,11 @@ class_key_or_enum_as_string (tree t)
>>>       return "struct";
>>>   }
>>> +#if __GNUC__ >= 10
>>> +#pragma GCC diagnostic push
>>> +#pragma GCC diagnostic ignored "-Wformat-diag"
>>> +#endif
>>> +
>>>   /* Print out a class declaration T under the control of FLAGS,
>>>      in the form `class foo'.  */
>>> @@ -851,6 +856,10 @@ dump_aggr_type (cxx_pretty_printer *pp, tree t, int flags)
>>>   			 flags & ~TFF_TEMPLATE_HEADER);
>>>   }
>>> +#if __GNUC__ >= 10
>>> +#pragma GCC diagnostic pop
>>> +#endif
> 
> Oh, and one more thing, but this time not about this source file but about
> the warning.  Does it handle the gettext case?
> I think -Wformat generally does, gettext has format_arg attribute.
> If the warning handles
>    pp_printf ("<unnamed %s>", str);
> and
>    pp_printf (cond ? "<unnamed %s>" : "<unnamed %s>", str);
> and
>    pp_printf (cond ? "<unnamed %s>" : "something %s", str);
> and
>    pp_printf (gettext ("<unnamed %s>"), str);
> then maybe it should also handle
>    pp_printf (cond ? gettext ("<unnamed %s>") : "<unnamed %s>, str);
> and
>    pp_printf (cond ? gettext ("<unnamed %s>") : "something %s, str);
> too?

-Wformat-diag is part of -Wformat so they both should handle the same
things.  Do you see a difference between what they handle?

Martin

> 
> 	Jakub
>
Jakub Jelinek Jan. 20, 2022, 4:43 p.m. UTC | #4 
On Thu, Jan 20, 2022 at 09:33:30AM -0700, Martin Sebor wrote:
> > Oh, and one more thing, but this time not about this source file but about
> > the warning.  Does it handle the gettext case?
> > I think -Wformat generally does, gettext has format_arg attribute.
> > If the warning handles
> >    pp_printf ("<unnamed %s>", str);
> > and
> >    pp_printf (cond ? "<unnamed %s>" : "<unnamed %s>", str);
> > and
> >    pp_printf (cond ? "<unnamed %s>" : "something %s", str);
> > and
> >    pp_printf (gettext ("<unnamed %s>"), str);
> > then maybe it should also handle
> >    pp_printf (cond ? gettext ("<unnamed %s>") : "<unnamed %s>, str);
> > and
> >    pp_printf (cond ? gettext ("<unnamed %s>") : "something %s, str);
> > too?
> 
> -Wformat-diag is part of -Wformat so they both should handle the same
> things.  Do you see a difference between what they handle?

With normal -Wformat I see all expected warnings in:
char *foo (const char *) __attribute__((format_arg(1)));
void bar (const char *, ...) __attribute__((format(printf, 1, 2)));

void
baz (int x)
{
  bar ("%ld", x);
  bar (x ? "%ld" : "%ld", x);
  bar (x ? "%ld" : "%lld", x);
  bar (foo ("%ld"), x);
  bar (x ? foo ("%ld") : "%ld", x);
  bar (x ? foo ("%ld") : "%lld", x);
  bar (foo (x ? "%ld" : "%ld"), x);
  bar (foo (x ? "%ld" : "%lld"), x);
}
(on all bar calls, on those with different strings or one in foo and other
not 2).
From the fact that -Wformat-diag didn't warn on the
pp_printf (cond ? gettext ("<unnamed %s>") : "<unnamed %s>, str);
case I assume -Wformat-diag doesn't handle this.

	Jakub
Martin Sebor Jan. 20, 2022, 4:56 p.m. UTC | #5 
On 1/20/22 09:43, Jakub Jelinek wrote:
> On Thu, Jan 20, 2022 at 09:33:30AM -0700, Martin Sebor wrote:
>>> Oh, and one more thing, but this time not about this source file but about
>>> the warning.  Does it handle the gettext case?
>>> I think -Wformat generally does, gettext has format_arg attribute.
>>> If the warning handles
>>>     pp_printf ("<unnamed %s>", str);
>>> and
>>>     pp_printf (cond ? "<unnamed %s>" : "<unnamed %s>", str);
>>> and
>>>     pp_printf (cond ? "<unnamed %s>" : "something %s", str);
>>> and
>>>     pp_printf (gettext ("<unnamed %s>"), str);
>>> then maybe it should also handle
>>>     pp_printf (cond ? gettext ("<unnamed %s>") : "<unnamed %s>, str);
>>> and
>>>     pp_printf (cond ? gettext ("<unnamed %s>") : "something %s, str);
>>> too?
>>
>> -Wformat-diag is part of -Wformat so they both should handle the same
>> things.  Do you see a difference between what they handle?
> 
> With normal -Wformat I see all expected warnings in:
> char *foo (const char *) __attribute__((format_arg(1)));
> void bar (const char *, ...) __attribute__((format(printf, 1, 2)));

-Wformat-diag is internal to GCC and needs one of the GCC-internal
attributes to enable, like __gcc_cxxdiag__, for example like this:

   __attribute__ ((format (__gcc_cxxdiag__, 1, 2)))
   void bar (const char *, ...);

With that it triggers in all the same instances as -Wformat below
(as near I can tell for a modified test case).

Martin

> 
> void
> baz (int x)
> {
>    bar ("%ld", x);
>    bar (x ? "%ld" : "%ld", x);
>    bar (x ? "%ld" : "%lld", x);
>    bar (foo ("%ld"), x);
>    bar (x ? foo ("%ld") : "%ld", x);
>    bar (x ? foo ("%ld") : "%lld", x);
>    bar (foo (x ? "%ld" : "%ld"), x);
>    bar (foo (x ? "%ld" : "%lld"), x);
> }
> (on all bar calls, on those with different strings or one in foo and other
> not 2).
>  From the fact that -Wformat-diag didn't warn on the
> pp_printf (cond ? gettext ("<unnamed %s>") : "<unnamed %s>, str);
> case I assume -Wformat-diag doesn't handle this.
> 
> 	Jakub
>
Jakub Jelinek Jan. 20, 2022, 5:03 p.m. UTC | #6 
On Thu, Jan 20, 2022 at 09:56:59AM -0700, Martin Sebor wrote:
> > With normal -Wformat I see all expected warnings in:
> > char *foo (const char *) __attribute__((format_arg(1)));
> > void bar (const char *, ...) __attribute__((format(printf, 1, 2)));
> 
> -Wformat-diag is internal to GCC and needs one of the GCC-internal
> attributes to enable, like __gcc_cxxdiag__, for example like this:
> 
>   __attribute__ ((format (__gcc_cxxdiag__, 1, 2)))
>   void bar (const char *, ...);
> 
> With that it triggers in all the same instances as -Wformat below
> (as near I can tell for a modified test case).

Glad to hear that, but then I don't understand why we didn't warn on
cp/error.cc before Martin L.'s change when --disable-nls wasn't used.

	Jakub
Martin Sebor Jan. 20, 2022, 5:52 p.m. UTC | #7 
On 1/20/22 10:03, Jakub Jelinek wrote:
> On Thu, Jan 20, 2022 at 09:56:59AM -0700, Martin Sebor wrote:
>>> With normal -Wformat I see all expected warnings in:
>>> char *foo (const char *) __attribute__((format_arg(1)));
>>> void bar (const char *, ...) __attribute__((format(printf, 1, 2)));
>>
>> -Wformat-diag is internal to GCC and needs one of the GCC-internal
>> attributes to enable, like __gcc_cxxdiag__, for example like this:
>>
>>    __attribute__ ((format (__gcc_cxxdiag__, 1, 2)))
>>    void bar (const char *, ...);
>>
>> With that it triggers in all the same instances as -Wformat below
>> (as near I can tell for a modified test case).
> 
> Glad to hear that, but then I don't understand why we didn't warn on
> cp/error.cc before Martin L.'s change when --disable-nls wasn't used.

Good question!  There does seem to be some strange interplay between
parentheses and -Wformat for __gcc_cdiag__ functions in the C++ front
end:

__attribute__ ((format (__gcc_cxxdiag__, 1, 2)))
void bar (const char *, ...);

void
baz (int x)
{
   bar (x ? "<%s" : "%i", x);           // -Wformat-diag
   bar ((x ? "<%s" : "%i"), x);         // silence
   bar ((x ? ("<%s") : ("%i")), x);     // silence
}

The C front end warns on all three calls.

With attribute printf both the C and C++ front ends issue a -Wformat
for all three calls as expected (passing an int to %s).

Martin
Jakub Jelinek Jan. 20, 2022, 6:18 p.m. UTC | #8 
On Thu, Jan 20, 2022 at 10:52:10AM -0700, Martin Sebor wrote:
> On 1/20/22 10:03, Jakub Jelinek wrote:
> > On Thu, Jan 20, 2022 at 09:56:59AM -0700, Martin Sebor wrote:
> > > > With normal -Wformat I see all expected warnings in:
> > > > char *foo (const char *) __attribute__((format_arg(1)));
> > > > void bar (const char *, ...) __attribute__((format(printf, 1, 2)));
> > > 
> > > -Wformat-diag is internal to GCC and needs one of the GCC-internal
> > > attributes to enable, like __gcc_cxxdiag__, for example like this:
> > > 
> > >    __attribute__ ((format (__gcc_cxxdiag__, 1, 2)))
> > >    void bar (const char *, ...);
> > > 
> > > With that it triggers in all the same instances as -Wformat below
> > > (as near I can tell for a modified test case).
> > 
> > Glad to hear that, but then I don't understand why we didn't warn on
> > cp/error.cc before Martin L.'s change when --disable-nls wasn't used.
> 
> Good question!  There does seem to be some strange interplay between
> parentheses and -Wformat for __gcc_cdiag__ functions in the C++ front
> end:
> 
> __attribute__ ((format (__gcc_cxxdiag__, 1, 2)))
> void bar (const char *, ...);
> 
> void
> baz (int x)
> {
>   bar (x ? "<%s" : "%i", x);           // -Wformat-diag
>   bar ((x ? "<%s" : "%i"), x);         // silence
>   bar ((x ? ("<%s") : ("%i")), x);     // silence
> }
> 
> The C front end warns on all three calls.
> 
> With attribute printf both the C and C++ front ends issue a -Wformat
> for all three calls as expected (passing an int to %s).

Filed PR104148 now.

	Jakub
diff mbox series

Patch

diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc
index 1ab0c25a477..c031c75cc5e 100644
--- a/gcc/cp/error.cc
+++ b/gcc/cp/error.cc
@@ -768,6 +768,11 @@  class_key_or_enum_as_string (tree t)
      return "struct";
  }
  
+#if __GNUC__ >= 10
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wformat-diag"
+#endif
+
  /* Print out a class declaration T under the control of FLAGS,
     in the form `class foo'.  */
  
@@ -851,6 +856,10 @@  dump_aggr_type (cxx_pretty_printer *pp, tree t, int flags)
  			 flags & ~TFF_TEMPLATE_HEADER);
  }
  
+#if __GNUC__ >= 10
+#pragma GCC diagnostic pop
+#endif
+
  /* Dump into the obstack the initial part of the output for a given type.
     This is necessary when dealing with things like functions returning
     functions.  Examples: