diff mbox

[pushed] microblaze-tdep: Add ATTRIBUTE_PRINTF to microblaze_debug

Message ID 1505995878-10687-1-git-send-email-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi Sept. 21, 2017, 12:11 p.m. UTC 
  I am getting this warning with clang:

/home/emaisin/src/binutils-gdb/gdb/microblaze-tdep.c:94:28: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
       vprintf_unfiltered (fmt, args);
                           ^~~

Adding ATTRIBUTE_PRINTF to microblaze_debug gets rid of it.  Strangely,
gcc doesn't warn about non-literal format strings when calling vprintf
(or a vprintf-style function, like vprintf_unfiltered).  I filed this
gcc bug:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82206

gdb/ChangeLog:

	* microblaze-tdep.c (microblaze_debug): Add ATTRIBUTE_PRINTF.
---
 gdb/ChangeLog         | 4 ++++
 gdb/microblaze-tdep.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Michael Eager Sept. 25, 2017, 2:14 p.m. UTC | #1 
On 09/21/2017 05:11 AM, Simon Marchi wrote:
> I am getting this warning with clang:
> 
> /home/emaisin/src/binutils-gdb/gdb/microblaze-tdep.c:94:28: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
>         vprintf_unfiltered (fmt, args);
>                             ^~~
> 
> Adding ATTRIBUTE_PRINTF to microblaze_debug gets rid of it. Strangely, > gcc doesn't warn about non-literal format strings when calling vprintf
> (or a vprintf-style function, like vprintf_unfiltered).  I filed this
> gcc bug:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82206

I'll admit to not being familiar with the nuances of __attribute__ ((format ...)),
or why adding this to the function declaration of microblaze_debug() would suppress
a diagnostic on the call to vprintf_unfiltered(), but the better fix seems to me to
be to turn off the obviously inappropriate -Wformat-nonliteral option in microblaze_debug()
using
#pragma GCC diagnostic ignored "-Wformat-nonliteral"

See https://stackoverflow.com/questions/12171132/avoid-warning-in-wrapper-around-printf
Pedro Alves Sept. 25, 2017, 4:06 p.m. UTC | #2 
On 09/25/2017 03:14 PM, Michael Eager wrote:
> On 09/21/2017 05:11 AM, Simon Marchi wrote:
>> I am getting this warning with clang:
>>
>> /home/emaisin/src/binutils-gdb/gdb/microblaze-tdep.c:94:28: error:
>> format string is not a string literal [-Werror,-Wformat-nonliteral]
>>         vprintf_unfiltered (fmt, args);
>>                             ^~~
>>
>> Adding ATTRIBUTE_PRINTF to microblaze_debug gets rid of it. Strangely,
>> > gcc doesn't warn about non-literal format strings when calling vprintf
>> (or a vprintf-style function, like vprintf_unfiltered).  I filed this
>> gcc bug:
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82206
> 
> I'll admit to not being familiar with the nuances of __attribute__
> ((format ...)),
> or why adding this to the function declaration of microblaze_debug()
> would suppress
> a diagnostic on the call to vprintf_unfiltered(), 

Because with that the compiler can assume that the 'fmt' argument
as passed down to microblaze_debug is a string literal.  I.e.,
the compiler can tell that the argument to vprintf_unfiltered is
itself transitively a string literal.

> but the better fix
> seems to me to
> be to turn off the obviously inappropriate -Wformat-nonliteral option in
> microblaze_debug() using
> #pragma GCC diagnostic ignored "-Wformat-nonliteral"

That doesn't make sense to me.

Thanks,
Pedro Alves
Michael Eager Sept. 25, 2017, 4:43 p.m. UTC | #3 
On 09/25/2017 09:06 AM, Pedro Alves wrote:
>> but the better fix
>> seems to me to
>> be to turn off the obviously inappropriate -Wformat-nonliteral option in
>> microblaze_debug() using
>> #pragma GCC diagnostic ignored "-Wformat-nonliteral"
> That doesn't make sense to me.

The argument passed to vprintf_unformatted in microblaze_debug is not a string
literal.  The diagnostic message is correct.  If you don't want the diagnostic,
turning it off seems correct.

This is somewhat moot.  All uses of microblaze_debug use literal string formats,
so adding ATTRIBUTE_PRINTF is OK.
Pedro Alves Sept. 25, 2017, 5:09 p.m. UTC | #4 
On 09/25/2017 05:43 PM, Michael Eager wrote:
> On 09/25/2017 09:06 AM, Pedro Alves wrote:
>>> but the better fix
>>> seems to me to
>>> be to turn off the obviously inappropriate -Wformat-nonliteral option in
>>> microblaze_debug() using
>>> #pragma GCC diagnostic ignored "-Wformat-nonliteral"
>> That doesn't make sense to me.
> 
> The argument passed to vprintf_unformatted in microblaze_debug is not a
> string
> literal.  The diagnostic message is correct.  If you don't want the
> diagnostic,
> turning it off seems correct.

The diagnostic hasn't gone away.  Instead, a warning is now emitted one
level up, at the microblaze_debug call sites, iff someone passes a
non-literal string as format string by mistake.  That makes total
sense, because microblaze_debug is just a wrapper
around *printf_unfiltered.

> This is somewhat moot.  All uses of microblaze_debug use literal string
> formats,
> so adding ATTRIBUTE_PRINTF is OK.

And if someone passes a non-literal format, we'll get a warning then.
If we suppressed the warning inside microblaze_debug as you were
suggesting, then we'd risk passing a non-literal as format string to
vprintf_unfiltered.  That's the whole point.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index adafa4c..88e7a6e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@ 
+2017-09-21  Simon Marchi  <simon.marchi@ericsson.com>
+
+	* microblaze-tdep.c (microblaze_debug): Add ATTRIBUTE_PRINTF.
+
 2017-09-21  Yao Qi  <yao.qi@linaro.org>
 
 	* configure.tgt (aarch64*-*-freebsd*): Add fbsd-tdep.o solib-svr4.o
diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
index 7547581..caa7d2f 100644
--- a/gdb/microblaze-tdep.c
+++ b/gdb/microblaze-tdep.c
@@ -82,7 +82,7 @@  static const char *microblaze_register_names[] =
 
 static unsigned int microblaze_debug_flag = 0;
 
-static void
+static void ATTRIBUTE_PRINTF (1, 2)
 microblaze_debug (const char *fmt, ...)
 { 
   if (microblaze_debug_flag)