format string is not a string literal

format string is not a string literal

[Jack Howarth]

   Building gdb 7.9 on x86_64-apple-darwin14 produces a number of
warnings from -Wformat-nonliteral...

darwin-nat.c:184:23: warning: format string is not a string literal
[-Wformat-nonliteral]
  vprintf_unfiltered (fmt, ap);
                      ^~~

remote.c:6986:37: warning: format string is not a string literal
[-Wformat-nonliteral]
  if (vsnprintf (rs->buf, max_size, format, ap) >= max_size)
                                    ^~~~~~
/usr/include/secure/_stdio.h:75:63: note: expanded from macro 'vsnprintf'
  __builtin___vsnprintf_chk (str, len, 0, __darwin_obsz(str), format, ap)
                                                              ^
ctf.c:109:39: warning: format string is not a string literal
[-Wformat-nonliteral]
  if (vfprintf (handler->metadata_fd, format, args) < 0)
                                      ^~~~~~

./guile/scm-string.c:70:25: warning: format string is not a string
literal [-Wformat-nonliteral]
  string = xstrvprintf (format, args);
                        ^~~~~~

./guile/scm-utils.c:86:25: warning: format string is not a string
literal [-Wformat-nonliteral]
  string = xstrvprintf (format, args);
                        ^~~~~~

auto-load.c:486:40: warning: format string is not a string literal
[-Wformat-nonliteral]
      vfprintf_unfiltered (gdb_stdlog, debug_fmt, debug_args);
                                       ^~~~~~~~~

complaints.c:188:10: warning: format string is not a string literal
[-Wformat-nonliteral]
                       complaint->fmt, args);
                       ^~~~~~~~~~~~~~
complaints.c:195:12: warning: format string is not a string literal
[-Wformat-nonliteral]
        vwarning (complaint->fmt, args);
                  ^~~~~~~~~~~~~~
complaints.c:200:23: warning: format string is not a string literal
[-Wformat-nonliteral]
          msg = xstrvprintf (complaint->fmt, args);
                             ^~~~~~~~~~~~~~

./compile/compile-object-load.c:235:22: warning: format string is not
a string literal [-Wformat-nonliteral]
  str = xstrvprintf (fmt, ap);
                     ^~~
./compile/compile-loc2c.c:453:30: warning: format string is not a
string literal [-Wformat-nonliteral]
  vfprintf_filtered (stream, format, args);
                             ^~~~~~
./compile/compile-loc2c.c:470:30: warning: format string is not a
string literal [-Wformat-nonliteral]
  vfprintf_filtered (stream, format, args);
                             ^~~~~~
./compile/compile-loc2c.c:485:30: warning: format string is not a
string literal [-Wformat-nonliteral]
  vfprintf_filtered (stream, format, args);
                             ^~~~~~

Re: format string is not a string literal

[Andrew Pinski]

On Tue, Feb 24, 2015 at 9:13 AM, Jack Howarth
<howarth.mailing.lists@gmail.com> wrote:
>    Building gdb 7.9 on x86_64-apple-darwin14 produces a number of
> warnings from -Wformat-nonliteral...
>
> darwin-nat.c:184:23: warning: format string is not a string literal
> [-Wformat-nonliteral]
>   vprintf_unfiltered (fmt, ap);
>                       ^~~

Looks like a clang bug because vprintf takes a format and an va_arg
type which means there is no way to tell what the format is going to
look into.
Looks like all are the same clang bug.
Please file a bug with Apple and/or LLVM about this bring broken.

>
> remote.c:6986:37: warning: format string is not a string literal
> [-Wformat-nonliteral]
>   if (vsnprintf (rs->buf, max_size, format, ap) >= max_size)
>                                     ^~~~~~
> /usr/include/secure/_stdio.h:75:63: note: expanded from macro 'vsnprintf'
>   __builtin___vsnprintf_chk (str, len, 0, __darwin_obsz(str), format, ap)
>                                                               ^


See how they even warn about their own header too.


Thanks,
Andrew

> ctf.c:109:39: warning: format string is not a string literal
> [-Wformat-nonliteral]
>   if (vfprintf (handler->metadata_fd, format, args) < 0)
>                                       ^~~~~~
>
> ./guile/scm-string.c:70:25: warning: format string is not a string
> literal [-Wformat-nonliteral]
>   string = xstrvprintf (format, args);
>                         ^~~~~~
>
> ./guile/scm-utils.c:86:25: warning: format string is not a string
> literal [-Wformat-nonliteral]
>   string = xstrvprintf (format, args);
>                         ^~~~~~
>
> auto-load.c:486:40: warning: format string is not a string literal
> [-Wformat-nonliteral]
>       vfprintf_unfiltered (gdb_stdlog, debug_fmt, debug_args);
>                                        ^~~~~~~~~
>
> complaints.c:188:10: warning: format string is not a string literal
> [-Wformat-nonliteral]
>                        complaint->fmt, args);
>                        ^~~~~~~~~~~~~~
> complaints.c:195:12: warning: format string is not a string literal
> [-Wformat-nonliteral]
>         vwarning (complaint->fmt, args);
>                   ^~~~~~~~~~~~~~
> complaints.c:200:23: warning: format string is not a string literal
> [-Wformat-nonliteral]
>           msg = xstrvprintf (complaint->fmt, args);
>                              ^~~~~~~~~~~~~~
>
> ./compile/compile-object-load.c:235:22: warning: format string is not
> a string literal [-Wformat-nonliteral]
>   str = xstrvprintf (fmt, ap);
>                      ^~~
> ./compile/compile-loc2c.c:453:30: warning: format string is not a
> string literal [-Wformat-nonliteral]
>   vfprintf_filtered (stream, format, args);
>                              ^~~~~~
> ./compile/compile-loc2c.c:470:30: warning: format string is not a
> string literal [-Wformat-nonliteral]
>   vfprintf_filtered (stream, format, args);
>                              ^~~~~~
> ./compile/compile-loc2c.c:485:30: warning: format string is not a
> string literal [-Wformat-nonliteral]
>   vfprintf_filtered (stream, format, args);
>                              ^~~~~~

Re: format string is not a string literal

[Andrew Pinski]

On Tue, Feb 24, 2015 at 9:13 AM, Jack Howarth
<howarth.mailing.lists@gmail.com> wrote:
>    Building gdb 7.9 on x86_64-apple-darwin14 produces a number of
> warnings from -Wformat-nonliteral...
>
> darwin-nat.c:184:23: warning: format string is not a string literal
> [-Wformat-nonliteral]
>   vprintf_unfiltered (fmt, ap);
>                       ^~~

Looks like a clang bug because vprintf takes a format and an va_arg
type which means there is no way to tell what the format is going to
look into.
Looks like all are the same clang bug.
Please file a bug with Apple and/or LLVM about this bring broken.

>
> remote.c:6986:37: warning: format string is not a string literal
> [-Wformat-nonliteral]
>   if (vsnprintf (rs->buf, max_size, format, ap) >= max_size)
>                                     ^~~~~~
> /usr/include/secure/_stdio.h:75:63: note: expanded from macro 'vsnprintf'
>   __builtin___vsnprintf_chk (str, len, 0, __darwin_obsz(str), format, ap)
>                                                               ^


See how they even warn about their own header too.


Thanks,
Andrew

> ctf.c:109:39: warning: format string is not a string literal
> [-Wformat-nonliteral]
>   if (vfprintf (handler->metadata_fd, format, args) < 0)
>                                       ^~~~~~
>
> ./guile/scm-string.c:70:25: warning: format string is not a string
> literal [-Wformat-nonliteral]
>   string = xstrvprintf (format, args);
>                         ^~~~~~
>
> ./guile/scm-utils.c:86:25: warning: format string is not a string
> literal [-Wformat-nonliteral]
>   string = xstrvprintf (format, args);
>                         ^~~~~~
>
> auto-load.c:486:40: warning: format string is not a string literal
> [-Wformat-nonliteral]
>       vfprintf_unfiltered (gdb_stdlog, debug_fmt, debug_args);
>                                        ^~~~~~~~~
>
> complaints.c:188:10: warning: format string is not a string literal
> [-Wformat-nonliteral]
>                        complaint->fmt, args);
>                        ^~~~~~~~~~~~~~
> complaints.c:195:12: warning: format string is not a string literal
> [-Wformat-nonliteral]
>         vwarning (complaint->fmt, args);
>                   ^~~~~~~~~~~~~~
> complaints.c:200:23: warning: format string is not a string literal
> [-Wformat-nonliteral]
>           msg = xstrvprintf (complaint->fmt, args);
>                              ^~~~~~~~~~~~~~
>
> ./compile/compile-object-load.c:235:22: warning: format string is not
> a string literal [-Wformat-nonliteral]
>   str = xstrvprintf (fmt, ap);
>                      ^~~
> ./compile/compile-loc2c.c:453:30: warning: format string is not a
> string literal [-Wformat-nonliteral]
>   vfprintf_filtered (stream, format, args);
>                              ^~~~~~
> ./compile/compile-loc2c.c:470:30: warning: format string is not a
> string literal [-Wformat-nonliteral]
>   vfprintf_filtered (stream, format, args);
>                              ^~~~~~
> ./compile/compile-loc2c.c:485:30: warning: format string is not a
> string literal [-Wformat-nonliteral]
>   vfprintf_filtered (stream, format, args);
>                              ^~~~~~

Re: format string is not a string literal

[Andrew Pinski]

On Tue, Feb 24, 2015 at 9:13 AM, Jack Howarth
<howarth.mailing.lists@gmail.com> wrote:
>    Building gdb 7.9 on x86_64-apple-darwin14 produces a number of
> warnings from -Wformat-nonliteral...
>
> darwin-nat.c:184:23: warning: format string is not a string literal
> [-Wformat-nonliteral]
>   vprintf_unfiltered (fmt, ap);
>                       ^~~

Looks like a clang bug because vprintf takes a format and an va_arg
type which means there is no way to tell what the format is going to
look into.
Looks like all are the same clang bug.
Please file a bug with Apple and/or LLVM about this bring broken.

>
> remote.c:6986:37: warning: format string is not a string literal
> [-Wformat-nonliteral]
>   if (vsnprintf (rs->buf, max_size, format, ap) >= max_size)
>                                     ^~~~~~
> /usr/include/secure/_stdio.h:75:63: note: expanded from macro 'vsnprintf'
>   __builtin___vsnprintf_chk (str, len, 0, __darwin_obsz(str), format, ap)
>                                                               ^


See how they even warn about their own header too.


Thanks,
Andrew

> ctf.c:109:39: warning: format string is not a string literal
> [-Wformat-nonliteral]
>   if (vfprintf (handler->metadata_fd, format, args) < 0)
>                                       ^~~~~~
>
> ./guile/scm-string.c:70:25: warning: format string is not a string
> literal [-Wformat-nonliteral]
>   string = xstrvprintf (format, args);
>                         ^~~~~~
>
> ./guile/scm-utils.c:86:25: warning: format string is not a string
> literal [-Wformat-nonliteral]
>   string = xstrvprintf (format, args);
>                         ^~~~~~
>
> auto-load.c:486:40: warning: format string is not a string literal
> [-Wformat-nonliteral]
>       vfprintf_unfiltered (gdb_stdlog, debug_fmt, debug_args);
>                                        ^~~~~~~~~
>
> complaints.c:188:10: warning: format string is not a string literal
> [-Wformat-nonliteral]
>                        complaint->fmt, args);
>                        ^~~~~~~~~~~~~~
> complaints.c:195:12: warning: format string is not a string literal
> [-Wformat-nonliteral]
>         vwarning (complaint->fmt, args);
>                   ^~~~~~~~~~~~~~
> complaints.c:200:23: warning: format string is not a string literal
> [-Wformat-nonliteral]
>           msg = xstrvprintf (complaint->fmt, args);
>                              ^~~~~~~~~~~~~~
>
> ./compile/compile-object-load.c:235:22: warning: format string is not
> a string literal [-Wformat-nonliteral]
>   str = xstrvprintf (fmt, ap);
>                      ^~~
> ./compile/compile-loc2c.c:453:30: warning: format string is not a
> string literal [-Wformat-nonliteral]
>   vfprintf_filtered (stream, format, args);
>                              ^~~~~~
> ./compile/compile-loc2c.c:470:30: warning: format string is not a
> string literal [-Wformat-nonliteral]
>   vfprintf_filtered (stream, format, args);
>                              ^~~~~~
> ./compile/compile-loc2c.c:485:30: warning: format string is not a
> string literal [-Wformat-nonliteral]
>   vfprintf_filtered (stream, format, args);
>                              ^~~~~~

Re: format string is not a string literal

[Paul_Koning]

> On Feb 24, 2015, at 12:13 PM, Jack Howarth <howarth.mailing.lists@gmail.com> wrote:
> 
>   Building gdb 7.9 on x86_64-apple-darwin14 produces a number of
> warnings from -Wformat-nonliteral...

Sounds like it’s time to turn off that warning, since the code is legit.

	paul

Re: format string is not a string literal

[Jack Howarth]

Paul,
    The clang developers disagree...

http://llvm.org/bugs/show_bug.cgi?id=22701#c3

        Jack

On Tue, Feb 24, 2015 at 1:09 PM,  <Paul_Koning@dell.com> wrote:
>
>> On Feb 24, 2015, at 12:13 PM, Jack Howarth <howarth.mailing.lists@gmail.com> wrote:
>>
>>   Building gdb 7.9 on x86_64-apple-darwin14 produces a number of
>> warnings from -Wformat-nonliteral...
>
> Sounds like it’s time to turn off that warning, since the code is legit.
>
>         paul

Re: format string is not a string literal

[Paul_Koning]

> On Feb 25, 2015, at 6:21 PM, Jack Howarth <howarth.mailing.lists@gmail.com> wrote:
> 
> Paul,
>    The clang developers disagree...
> 
> http://llvm.org/bugs/show_bug.cgi?id=22701#c3
> 
>        Jack
> 
> On Tue, Feb 24, 2015 at 1:09 PM,  <Paul_Koning@dell.com> wrote:
>> 
>>> On Feb 24, 2015, at 12:13 PM, Jack Howarth <howarth.mailing.lists@gmail.com> wrote:
>>> 
>>>  Building gdb 7.9 on x86_64-apple-darwin14 produces a number of
>>> warnings from -Wformat-nonliteral...
>> 
>> Sounds like it’s time to turn off that warning, since the code is legit.
>> 
>>        paul

I didn’t say it’s a bug, Andrew did.  But I agree with him.

My comment (“the code is legit”) simply meant that GDB uses variable formats for obvious valid reasons (so the format can vary, being user-supplied).  Given that it’s intentional, the warning is not wanted.

But that point is really applicable to printf, not vprintf.  Andrew’s point is that checking formats for vprintf is not possible because you can’t know the argument list; only in printf do you see the arguments so you can match the types.  So the bug is that format checking and complaining for non-literal formats should not be enabled at all for vprintf.  That may be a header issue rather than a compiler issue, but either way, it’s not the right thing to do.

	paul

Re: format string is not a string literal

[Simon Marchi]

> I didn’t say it’s a bug, Andrew did.  But I agree with him.
>
> My comment (“the code is legit”) simply meant that GDB uses variable formats for obvious valid reasons (so the format can vary, being user-supplied).  Given that it’s intentional, the warning is not wanted.
>
> But that point is really applicable to printf, not vprintf.  Andrew’s point is that checking formats for vprintf is not possible because you can’t know the argument list; only in printf do you see the arguments so you can match the types.  So the bug is that format checking and complaining for non-literal formats should not be enabled at all for vprintf.  That may be a header issue rather than a compiler issue, but either way, it’s not the right thing to do.
>
>         paul

I think the warning is relevant. If you instruct the compiler that
inferior_debug takes a format string and format arguments (using a
format attribute, as mentioned by Richard in the bug report), then it
can check if the callers are doing something wrong.

In the case of inferior_debug, the attribute should be
    __attribute__((format (printf, 2, 3)))

By adding the attribute, you get nice warnings of this kind:

test.c: In function ‘main’:
test.c:17:2: warning: too many arguments for format [-Wformat-extra-args]
  inferior_debug (1, "pouet %d", 2, "hello");

If the function is vprintf-style, it's similar but the last argument
should be 0. It will push the argument check a level higher, where
eventually they are explicitely defined printf-style. The doc is
somewhere here [2] in the middle.

The warning also has some value because it will tell you if the string
originally comes from a non-literal, which should be avoided [1].

[1] http://en.wikipedia.org/wiki/Uncontrolled_format_string
[2] https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html

Simon

Re: format string is not a string literal

[pinskia]

On Feb 25, 2015, at 4:05 PM, Simon Marchi <simon.marchi@polymtl.ca> wrote:

>> I didn’t say it’s a bug, Andrew did.  But I agree with him.
>> 
>> My comment (“the code is legit”) simply meant that GDB uses variable formats for obvious valid reasons (so the format can vary, being user-supplied).  Given that it’s intentional, the warning is not wanted.
>> 
>> But that point is really applicable to printf, not vprintf.  Andrew’s point is that checking formats for vprintf is not possible because you can’t know the argument list; only in printf do you see the arguments so you can match the types.  So the bug is that format checking and complaining for non-literal formats should not be enabled at all for vprintf.  That may be a header issue rather than a compiler issue, but either way, it’s not the right thing to do.
>> 
>>        paul
> 
> I think the warning is relevant. If you instruct the compiler that
> inferior_debug takes a format string and format arguments (using a
> format attribute, as mentioned by Richard in the bug report), then it
> can check if the callers are doing something wrong.
> 
> In the case of inferior_debug, the attribute should be
>    __attribute__((format (printf, 2, 3)))
> 
> By adding the attribute, you get nice warnings of this kind:
> 
> test.c: In function ‘main’:
> test.c:17:2: warning: too many arguments for format [-Wformat-extra-args]
>  inferior_debug (1, "pouet %d", 2, "hello");
> 
> If the function is vprintf-style, it's similar but the last argument
> should be 0. It will push the argument check a level higher, where
> eventually they are explicitely defined printf-style. The doc is
> somewhere here [2] in the middle.

Then clang's warning should suggest putting the format attribute on that function rather than giving out a warning that seems like it is a bogus one. 

Gcc does that iirc why not clang. 

Thanks,
Andrew


> 
> The warning also has some value because it will tell you if the string
> originally comes from a non-literal, which should be avoided [1].
> 
> [1] http://en.wikipedia.org/wiki/Uncontrolled_format_string
> [2] https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
> 
> Simon

Re: format string is not a string literal

[Jack Howarth]

Andrew,
     See the additional comments from the llvm.org clang developers at...

http://llvm.org/bugs/show_bug.cgi?id=22701#c5

         Jack

On Wed, Feb 25, 2015 at 7:12 PM,  <pinskia@gmail.com> wrote:
>
>
>
>
> On Feb 25, 2015, at 4:05 PM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>
>>> I didn’t say it’s a bug, Andrew did.  But I agree with him.
>>>
>>> My comment (“the code is legit”) simply meant that GDB uses variable formats for obvious valid reasons (so the format can vary, being user-supplied).  Given that it’s intentional, the warning is not wanted.
>>>
>>> But that point is really applicable to printf, not vprintf.  Andrew’s point is that checking formats for vprintf is not possible because you can’t know the argument list; only in printf do you see the arguments so you can match the types.  So the bug is that format checking and complaining for non-literal formats should not be enabled at all for vprintf.  That may be a header issue rather than a compiler issue, but either way, it’s not the right thing to do.
>>>
>>>        paul
>>
>> I think the warning is relevant. If you instruct the compiler that
>> inferior_debug takes a format string and format arguments (using a
>> format attribute, as mentioned by Richard in the bug report), then it
>> can check if the callers are doing something wrong.
>>
>> In the case of inferior_debug, the attribute should be
>>    __attribute__((format (printf, 2, 3)))
>>
>> By adding the attribute, you get nice warnings of this kind:
>>
>> test.c: In function ‘main’:
>> test.c:17:2: warning: too many arguments for format [-Wformat-extra-args]
>>  inferior_debug (1, "pouet %d", 2, "hello");
>>
>> If the function is vprintf-style, it's similar but the last argument
>> should be 0. It will push the argument check a level higher, where
>> eventually they are explicitely defined printf-style. The doc is
>> somewhere here [2] in the middle.
>
> Then clang's warning should suggest putting the format attribute on that function rather than giving out a warning that seems like it is a bogus one.
>
> Gcc does that iirc why not clang.
>
> Thanks,
> Andrew
>
>
>>
>> The warning also has some value because it will tell you if the string
>> originally comes from a non-literal, which should be avoided [1].
>>
>> [1] http://en.wikipedia.org/wiki/Uncontrolled_format_string
>> [2] https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
>>
>> Simon

Re: format string is not a string literal

[pinskia]

> On Feb 25, 2015, at 4:41 PM, Jack Howarth <howarth.mailing.lists@gmail.com> wrote:
> 
> Andrew,
>     See the additional comments from the llvm.org clang developers at...
> 
> http://llvm.org/bugs/show_bug.cgi?id=22701#c5

Then put this warning under a different flag. Anyways clang is broken and gdb should not change due to a broken compiler. 

Thanks,
Andrew


> 
>         Jack
> 
>> On Wed, Feb 25, 2015 at 7:12 PM,  <pinskia@gmail.com> wrote:
>> 
>> 
>> 
>> 
>> On Feb 25, 2015, at 4:05 PM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>> 
>>>> I didn’t say it’s a bug, Andrew did.  But I agree with him.
>>>> 
>>>> My comment (“the code is legit”) simply meant that GDB uses variable formats for obvious valid reasons (so the format can vary, being user-supplied).  Given that it’s intentional, the warning is not wanted.
>>>> 
>>>> But that point is really applicable to printf, not vprintf.  Andrew’s point is that checking formats for vprintf is not possible because you can’t know the argument list; only in printf do you see the arguments so you can match the types.  So the bug is that format checking and complaining for non-literal formats should not be enabled at all for vprintf.  That may be a header issue rather than a compiler issue, but either way, it’s not the right thing to do.
>>>> 
>>>>       paul
>>> 
>>> I think the warning is relevant. If you instruct the compiler that
>>> inferior_debug takes a format string and format arguments (using a
>>> format attribute, as mentioned by Richard in the bug report), then it
>>> can check if the callers are doing something wrong.
>>> 
>>> In the case of inferior_debug, the attribute should be
>>>   __attribute__((format (printf, 2, 3)))
>>> 
>>> By adding the attribute, you get nice warnings of this kind:
>>> 
>>> test.c: In function ‘main’:
>>> test.c:17:2: warning: too many arguments for format [-Wformat-extra-args]
>>> inferior_debug (1, "pouet %d", 2, "hello");
>>> 
>>> If the function is vprintf-style, it's similar but the last argument
>>> should be 0. It will push the argument check a level higher, where
>>> eventually they are explicitely defined printf-style. The doc is
>>> somewhere here [2] in the middle.
>> 
>> Then clang's warning should suggest putting the format attribute on that function rather than giving out a warning that seems like it is a bogus one.
>> 
>> Gcc does that iirc why not clang.
>> 
>> Thanks,
>> Andrew
>> 
>> 
>>> 
>>> The warning also has some value because it will tell you if the string
>>> originally comes from a non-literal, which should be avoided [1].
>>> 
>>> [1] http://en.wikipedia.org/wiki/Uncontrolled_format_string
>>> [2] https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
>>> 
>>> Simon

Re: format string is not a string literal

[Simon Marchi]

>> On Feb 25, 2015, at 4:41 PM, Jack Howarth <howarth.mailing.lists@gmail.com> wrote:
>>
>> Andrew,
>>     See the additional comments from the llvm.org clang developers at...
>>
>> http://llvm.org/bugs/show_bug.cgi?id=22701#c5
>
> Then put this warning under a different flag. Anyways clang is broken and gdb should not change due to a broken compiler.
>
> Thanks,
> Andrew

I would not say that clang is broken or that the warning is unclear.
The flag is -Wformat-nonliteral and the warning message is "format
string is not a string  literal". I don't know how you can be more
clear and direct about the fact that the format string is not a string
literal. Looking at the code, we observe that clang is right (the
format string is indeed not a string literal). With the proper
function attributes, the compiler could do better checks and prevent
programming mistakes. Since clang pointed out this low-hanging fruit
improvement to gdb's code, why shouldn't we take advantage of it?

Anyway, Jack, you are welcome to submit a patch for this if you feel
like it and/or file a bug if it is not already done.

Simon

Re: format string is not a string literal

[Andrew Pinski]

On Wed, Feb 25, 2015 at 6:30 PM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>>> On Feb 25, 2015, at 4:41 PM, Jack Howarth <howarth.mailing.lists@gmail.com> wrote:
>>>
>>> Andrew,
>>>     See the additional comments from the llvm.org clang developers at...
>>>
>>> http://llvm.org/bugs/show_bug.cgi?id=22701#c5
>>
>> Then put this warning under a different flag. Anyways clang is broken and gdb should not change due to a broken compiler.
>>
>> Thanks,
>> Andrew
>
> I would not say that clang is broken or that the warning is unclear.
> The flag is -Wformat-nonliteral and the warning message is "format
> string is not a string  literal". I don't know how you can be more
> clear and direct about the fact that the format string is not a string
> literal. Looking at the code, we observe that clang is right (the
> format string is indeed not a string literal). With the proper
> function attributes, the compiler could do better checks and prevent
> programming mistakes. Since clang pointed out this low-hanging fruit
> improvement to gdb's code, why shouldn't we take advantage of it?
>
> Anyway, Jack, you are welcome to submit a patch for this if you feel
> like it and/or file a bug if it is not already done.


Except GCC documents this options differently than clang implements:
-Wformat-nonliteralIf -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 va_list.


Notice the last part of the documentation.

So this again is clang implements a GCC option but does not follow the
documentation of GCC.
How does clang document this option?

Thanks,
Andrew

>
> Simon

Re: format string is not a string literal

[Andrew Pinski]

On Wed, Feb 25, 2015 at 6:35 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Wed, Feb 25, 2015 at 6:30 PM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>>>> On Feb 25, 2015, at 4:41 PM, Jack Howarth <howarth.mailing.lists@gmail.com> wrote:
>>>>
>>>> Andrew,
>>>>     See the additional comments from the llvm.org clang developers at...
>>>>
>>>> http://llvm.org/bugs/show_bug.cgi?id=22701#c5
>>>
>>> Then put this warning under a different flag. Anyways clang is broken and gdb should not change due to a broken compiler.
>>>
>>> Thanks,
>>> Andrew
>>
>> I would not say that clang is broken or that the warning is unclear.
>> The flag is -Wformat-nonliteral and the warning message is "format
>> string is not a string  literal". I don't know how you can be more
>> clear and direct about the fact that the format string is not a string
>> literal. Looking at the code, we observe that clang is right (the
>> format string is indeed not a string literal). With the proper
>> function attributes, the compiler could do better checks and prevent
>> programming mistakes. Since clang pointed out this low-hanging fruit
>> improvement to gdb's code, why shouldn't we take advantage of it?
>>
>> Anyway, Jack, you are welcome to submit a patch for this if you feel
>> like it and/or file a bug if it is not already done.
>
>
> Except GCC documents this options differently than clang implements:
> -Wformat-nonliteralIf -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 va_list.
>
>
> Notice the last part of the documentation.
>
> So this again is clang implements a GCC option but does not follow the
> documentation of GCC.
> How does clang document this option?
>

GCC has always documented this option this way since at least 3.1.1:
https://gcc.gnu.org/onlinedocs/gcc-3.1.1/gcc/Warning-Options.html#Warning%20Options

And there is no user documentation for clang for this option so clang is broken.

Thanks,
Andrew



> Thanks,
> Andrew
>
>>
>> Simon

Re: format string is not a string literal

[Jack Howarth]

On Wed, Feb 25, 2015 at 9:38 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Wed, Feb 25, 2015 at 6:35 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>> On Wed, Feb 25, 2015 at 6:30 PM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>>>>> On Feb 25, 2015, at 4:41 PM, Jack Howarth <howarth.mailing.lists@gmail.com> wrote:
>>>>>
>>>>> Andrew,
>>>>>     See the additional comments from the llvm.org clang developers at...
>>>>>
>>>>> http://llvm.org/bugs/show_bug.cgi?id=22701#c5
>>>>
>>>> Then put this warning under a different flag. Anyways clang is broken and gdb should not change due to a broken compiler.
>>>>
>>>> Thanks,
>>>> Andrew
>>>
>>> I would not say that clang is broken or that the warning is unclear.
>>> The flag is -Wformat-nonliteral and the warning message is "format
>>> string is not a string  literal". I don't know how you can be more
>>> clear and direct about the fact that the format string is not a string
>>> literal. Looking at the code, we observe that clang is right (the
>>> format string is indeed not a string literal). With the proper
>>> function attributes, the compiler could do better checks and prevent
>>> programming mistakes. Since clang pointed out this low-hanging fruit
>>> improvement to gdb's code, why shouldn't we take advantage of it?
>>>
>>> Anyway, Jack, you are welcome to submit a patch for this if you feel
>>> like it and/or file a bug if it is not already done.
>>
>>
>> Except GCC documents this options differently than clang implements:
>> -Wformat-nonliteralIf -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 va_list.
>>
>>
>> Notice the last part of the documentation.
>>
>> So this again is clang implements a GCC option but does not follow the
>> documentation of GCC.
>> How does clang document this option?
>>
>
> GCC has always documented this option this way since at least 3.1.1:
> https://gcc.gnu.org/onlinedocs/gcc-3.1.1/gcc/Warning-Options.html#Warning%20Options
>
> And there is no user documentation for clang for this option so clang is broken.

http://clang.llvm.org/docs/AttributeReference.html#format-gnu-format

>
> Thanks,
> Andrew
>
>
>
>> Thanks,
>> Andrew
>>
>>>
>>> Simon

Re: format string is not a string literal

[Andrew Pinski]

On Thu, Feb 26, 2015 at 12:39 AM, Jack Howarth
<howarth.mailing.lists@gmail.com> wrote:
> On Wed, Feb 25, 2015 at 9:38 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>> On Wed, Feb 25, 2015 at 6:35 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>>> On Wed, Feb 25, 2015 at 6:30 PM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>>>>>> On Feb 25, 2015, at 4:41 PM, Jack Howarth <howarth.mailing.lists@gmail.com> wrote:
>>>>>>
>>>>>> Andrew,
>>>>>>     See the additional comments from the llvm.org clang developers at...
>>>>>>
>>>>>> http://llvm.org/bugs/show_bug.cgi?id=22701#c5
>>>>>
>>>>> Then put this warning under a different flag. Anyways clang is broken and gdb should not change due to a broken compiler.
>>>>>
>>>>> Thanks,
>>>>> Andrew
>>>>
>>>> I would not say that clang is broken or that the warning is unclear.
>>>> The flag is -Wformat-nonliteral and the warning message is "format
>>>> string is not a string  literal". I don't know how you can be more
>>>> clear and direct about the fact that the format string is not a string
>>>> literal. Looking at the code, we observe that clang is right (the
>>>> format string is indeed not a string literal). With the proper
>>>> function attributes, the compiler could do better checks and prevent
>>>> programming mistakes. Since clang pointed out this low-hanging fruit
>>>> improvement to gdb's code, why shouldn't we take advantage of it?
>>>>
>>>> Anyway, Jack, you are welcome to submit a patch for this if you feel
>>>> like it and/or file a bug if it is not already done.
>>>
>>>
>>> Except GCC documents this options differently than clang implements:
>>> -Wformat-nonliteralIf -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 va_list.
>>>
>>>
>>> Notice the last part of the documentation.
>>>
>>> So this again is clang implements a GCC option but does not follow the
>>> documentation of GCC.
>>> How does clang document this option?
>>>
>>
>> GCC has always documented this option this way since at least 3.1.1:
>> https://gcc.gnu.org/onlinedocs/gcc-3.1.1/gcc/Warning-Options.html#Warning%20Options
>>
>> And there is no user documentation for clang for this option so clang is broken.
>
> http://clang.llvm.org/docs/AttributeReference.html#format-gnu-format

Who would have thought to look into the documentation part of the
attribute for option documentation :).
Anyways this is an documented incompatibility which means gdb should
just test if it is being compiled with clang, not add the option.
Or better yet people should push back to clang that they need to
separate out the option or have an option to be GCC compatible.

Thanks,
Andrew

>
>>
>> Thanks,
>> Andrew
>>
>>
>>
>>> Thanks,
>>> Andrew
>>>
>>>>
>>>> Simon

Re: format string is not a string literal

[Pedro Alves]

On 02/26/2015 12:41 AM, Jack Howarth wrote:

>>> I think the warning is relevant. If you instruct the compiler that
>>> inferior_debug takes a format string and format arguments (using a
>>> format attribute, as mentioned by Richard in the bug report), then it
>>> can check if the callers are doing something wrong.
>>>
>>> In the case of inferior_debug, the attribute should be
>>>    __attribute__((format (printf, 2, 3)))
>>>
>>> By adding the attribute, you get nice warnings of this kind:
>>>
>>> test.c: In function ‘main’:
>>> test.c:17:2: warning: too many arguments for format [-Wformat-extra-args]
>>>  inferior_debug (1, "pouet %d", 2, "hello");
>>>

Agreed.  Jack, can you try this patch please?  It applies cleanly
against both mainline and the 7.9 branch.  I think it fixes all the
warnings you reported.

--------
From bd87d33c7b2236857f2c32ce5da85aae474ce7bd Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 26 Feb 2015 09:47:21 +0000
Subject: [PATCH] Add ATTRIBUTE_PRINTF attributes, and fix fallout

gdb/ChangeLog:
2015-02-26  Pedro Alves  <palves@redhat.com>

	* auto-load.h (file_is_auto_load_safe): Add ATTRIBUTE_PRINTF.
	* compile/compile-loc2c.c (pushf, unary, binary): Add
	ATTRIBUTE_PRINTF.
	(do_compile_dwarf_expr_to_c): Pass string literal as format string
	to pushf.
	(BINARY): Pass string literal as format string to 'binary'.
	* compile/compile-object-load.c (link_callbacks_einfo): Add
	ATTRIBUTE_PRINTF.
	* complaints.c (vcomplaint): Pass argument FMT directly to
	printf-like functions instead of complaint->fmt.
	* darwin-nat.c (inferior_debug): Add ATTRIBUTE_PRINTF.
---
 gdb/auto-load.h                   |  3 ++-
 gdb/compile/compile-loc2c.c       | 19 ++++++++++++++-----
 gdb/compile/compile-object-load.c |  3 +++
 gdb/complaints.c                  | 13 ++++++++-----
 gdb/darwin-nat.c                  |  3 +++
 gdb/guile/guile-internal.h        |  3 ++-
 gdb/remote.c                      |  3 +++
 7 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/gdb/auto-load.h b/gdb/auto-load.h
index 1d10e91..d241946 100644
--- a/gdb/auto-load.h
+++ b/gdb/auto-load.h
@@ -45,7 +45,8 @@ extern struct cmd_list_element **auto_load_show_cmdlist_get (void);
 extern struct cmd_list_element **auto_load_info_cmdlist_get (void);
 
 extern int file_is_auto_load_safe (const char *filename,
-				   const char *debug_fmt, ...);
+				   const char *debug_fmt, ...)
+  ATTRIBUTE_PRINTF (2, 3);
 
 extern int auto_load_gdb_scripts_enabled
   (const struct extension_language_defn *extlang);
diff --git a/gdb/compile/compile-loc2c.c b/gdb/compile/compile-loc2c.c
index 3f43e58..6a3615d 100644
--- a/gdb/compile/compile-loc2c.c
+++ b/gdb/compile/compile-loc2c.c
@@ -443,6 +443,9 @@ push (int indent, struct ui_file *stream, ULONGEST l)
 /* Emit code to push an arbitrary expression.  This works like
    printf.  */
 
+static void pushf (int indent, struct ui_file *stream, const char *format, ...)
+  ATTRIBUTE_PRINTF (3, 4);
+
 static void
 pushf (int indent, struct ui_file *stream, const char *format, ...)
 {
@@ -460,6 +463,9 @@ pushf (int indent, struct ui_file *stream, const char *format, ...)
 /* Emit code for a unary expression -- one which operates in-place on
    the top-of-stack.  This works like printf.  */
 
+static void unary (int indent, struct ui_file *stream, const char *format, ...)
+  ATTRIBUTE_PRINTF (3, 4);
+
 static void
 unary (int indent, struct ui_file *stream, const char *format, ...)
 {
@@ -474,6 +480,8 @@ unary (int indent, struct ui_file *stream, const char *format, ...)
 
 /* Emit code for a unary expression -- one which uses the top two
    stack items, popping the topmost one.  This works like printf.  */
+static void binary (int indent, struct ui_file *stream, const char *format, ...)
+  ATTRIBUTE_PRINTF (3, 4);
 
 static void
 binary (int indent, struct ui_file *stream, const char *format, ...)
@@ -651,7 +659,7 @@ do_compile_dwarf_expr_to_c (int indent, struct ui_file *stream,
   fprintfi_filtered (indent, stream, "int __gdb_tos = -1;\n");
 
   if (initial != NULL)
-    pushf (indent, stream, core_addr_to_string (*initial));
+    pushf (indent, stream, "%s", core_addr_to_string (*initial));
 
   while (op_ptr < op_end)
     {
@@ -911,7 +919,8 @@ do_compile_dwarf_expr_to_c (int indent, struct ui_file *stream,
 
 	case DW_OP_pick:
 	  offset = *op_ptr++;
-	  pushf (indent, stream, "__gdb_stack[__gdb_tos - %d]", offset);
+	  pushf (indent, stream, "__gdb_stack[__gdb_tos - %s]",
+		 plongest (offset));
 	  break;
 
 	case DW_OP_swap:
@@ -1000,8 +1009,8 @@ do_compile_dwarf_expr_to_c (int indent, struct ui_file *stream,
 	  break;
 
 #define BINARY(OP)							\
-	  binary (indent, stream, ("__gdb_stack[__gdb_tos-1] " #OP	\
-				   " __gdb_stack[__gdb_tos]"));	\
+	  binary (indent, stream, "%s", "__gdb_stack[__gdb_tos-1] " #OP \
+				   " __gdb_stack[__gdb_tos]");	\
 	  break
 
 	case DW_OP_and:
@@ -1076,7 +1085,7 @@ do_compile_dwarf_expr_to_c (int indent, struct ui_file *stream,
 					    addr_size,
 					    cfa_start, cfa_end,
 					    &text_offset, per_cu);
-		pushf (indent, stream, cfa_name);
+		pushf (indent, stream, "%s", cfa_name);
 	      }
 	  }
 
diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
index 5903f18..7d2b683 100644
--- a/gdb/compile/compile-object-load.c
+++ b/gdb/compile/compile-object-load.c
@@ -224,6 +224,9 @@ link_callbacks_unattached_reloc (struct bfd_link_info *link_info,
 
 /* Helper for link_callbacks callbacks vector.  */
 
+static void link_callbacks_einfo (const char *fmt, ...)
+  ATTRIBUTE_PRINTF (1, 2);
+
 static void
 link_callbacks_einfo (const char *fmt, ...)
 {
diff --git a/gdb/complaints.c b/gdb/complaints.c
index 7e52656..ab36d9b 100644
--- a/gdb/complaints.c
+++ b/gdb/complaints.c
@@ -183,21 +183,24 @@ vcomplaint (struct complaints **c, const char *file,
   else
     series = complaints->series;
 
+  /* Use FMT from here on instead of complaint->fmt, to avoid "format
+     string is not a string literal" warnings.  */
+  gdb_assert (complaint->fmt == fmt);
+
   if (complaint->file != NULL)
-    internal_vwarning (complaint->file, complaint->line, 
-		       complaint->fmt, args);
+    internal_vwarning (complaint->file, complaint->line, fmt, args);
   else if (deprecated_warning_hook)
-    (*deprecated_warning_hook) (complaint->fmt, args);
+    (*deprecated_warning_hook) (fmt, args);
   else
     {
       if (complaints->explanation == NULL)
 	/* A [v]warning() call always appends a newline.  */
-	vwarning (complaint->fmt, args);
+	vwarning (fmt, args);
       else
 	{
 	  char *msg;
 	  struct cleanup *cleanups;
-	  msg = xstrvprintf (complaint->fmt, args);
+	  msg = xstrvprintf (fmt, args);
 	  cleanups = make_cleanup (xfree, msg);
 	  wrap_here ("");
 	  if (series != SUBSEQUENT_MESSAGE)
diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index f9481c7..dfce179 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -171,6 +171,9 @@ __attribute__ ((section ("__TEXT,__info_plist"),used)) =
   "</dict>\n"
   "</plist>\n";
 
+static void inferior_debug (int level, const char *fmt, ...)
+  ATTRIBUTE_PRINTF (2, 3);
+
 static void
 inferior_debug (int level, const char *fmt, ...)
 {
diff --git a/gdb/guile/guile-internal.h b/gdb/guile/guile-internal.h
index 968b4d3..86fc2f6 100644
--- a/gdb/guile/guile-internal.h
+++ b/gdb/guile/guile-internal.h
@@ -484,7 +484,8 @@ extern char *gdbscm_scm_to_c_string (SCM string);
 
 extern SCM gdbscm_scm_from_c_string (const char *string);
 
-extern SCM gdbscm_scm_from_printf (const char *format, ...);
+extern SCM gdbscm_scm_from_printf (const char *format, ...)
+    ATTRIBUTE_PRINTF (1, 2);
 
 extern char *gdbscm_scm_to_string (SCM string, size_t *lenp,
 				   const char *charset,
diff --git a/gdb/remote.c b/gdb/remote.c
index e971a29..0b0770f 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6973,6 +6973,9 @@ remote_read_bytes (struct target_ops *ops, CORE_ADDR memaddr,
    FORMAT and the remaining arguments, then gets the reply.  Returns
    whether the packet was a success, a failure, or unknown.  */
 
+static enum packet_result remote_send_printf (const char *format, ...)
+  ATTRIBUTE_PRINTF (1, 2);
+
 static enum packet_result
 remote_send_printf (const char *format, ...)
 {
-- 
1.9.3

Re: format string is not a string literal

[Jack Howarth]

Pedro,
      It is almost complete except for...

ctf.c:109:39: warning: format string is not a string literal
[-Wformat-nonliteral]
  if (vfprintf (handler->metadata_fd, format, args) < 0)
                                      ^~~~~~

and

./guile/scm-utils.c:86:25: warning: format string is not a string
literal [-Wformat-nonliteral]
  string = xstrvprintf (format, args);
                        ^~~~~~

            Jack

On Thu, Feb 26, 2015 at 4:52 AM, Pedro Alves <palves@redhat.com> wrote:
> On 02/26/2015 12:41 AM, Jack Howarth wrote:
>
>>>> I think the warning is relevant. If you instruct the compiler that
>>>> inferior_debug takes a format string and format arguments (using a
>>>> format attribute, as mentioned by Richard in the bug report), then it
>>>> can check if the callers are doing something wrong.
>>>>
>>>> In the case of inferior_debug, the attribute should be
>>>>    __attribute__((format (printf, 2, 3)))
>>>>
>>>> By adding the attribute, you get nice warnings of this kind:
>>>>
>>>> test.c: In function ‘main’:
>>>> test.c:17:2: warning: too many arguments for format [-Wformat-extra-args]
>>>>  inferior_debug (1, "pouet %d", 2, "hello");
>>>>
>
> Agreed.  Jack, can you try this patch please?  It applies cleanly
> against both mainline and the 7.9 branch.  I think it fixes all the
> warnings you reported.
>
> --------
> From bd87d33c7b2236857f2c32ce5da85aae474ce7bd Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 26 Feb 2015 09:47:21 +0000
> Subject: [PATCH] Add ATTRIBUTE_PRINTF attributes, and fix fallout
>
> gdb/ChangeLog:
> 2015-02-26  Pedro Alves  <palves@redhat.com>
>
>         * auto-load.h (file_is_auto_load_safe): Add ATTRIBUTE_PRINTF.
>         * compile/compile-loc2c.c (pushf, unary, binary): Add
>         ATTRIBUTE_PRINTF.
>         (do_compile_dwarf_expr_to_c): Pass string literal as format string
>         to pushf.
>         (BINARY): Pass string literal as format string to 'binary'.
>         * compile/compile-object-load.c (link_callbacks_einfo): Add
>         ATTRIBUTE_PRINTF.
>         * complaints.c (vcomplaint): Pass argument FMT directly to
>         printf-like functions instead of complaint->fmt.
>         * darwin-nat.c (inferior_debug): Add ATTRIBUTE_PRINTF.
> ---
>  gdb/auto-load.h                   |  3 ++-
>  gdb/compile/compile-loc2c.c       | 19 ++++++++++++++-----
>  gdb/compile/compile-object-load.c |  3 +++
>  gdb/complaints.c                  | 13 ++++++++-----
>  gdb/darwin-nat.c                  |  3 +++
>  gdb/guile/guile-internal.h        |  3 ++-
>  gdb/remote.c                      |  3 +++
>  7 files changed, 35 insertions(+), 12 deletions(-)
>
> diff --git a/gdb/auto-load.h b/gdb/auto-load.h
> index 1d10e91..d241946 100644
> --- a/gdb/auto-load.h
> +++ b/gdb/auto-load.h
> @@ -45,7 +45,8 @@ extern struct cmd_list_element **auto_load_show_cmdlist_get (void);
>  extern struct cmd_list_element **auto_load_info_cmdlist_get (void);
>
>  extern int file_is_auto_load_safe (const char *filename,
> -                                  const char *debug_fmt, ...);
> +                                  const char *debug_fmt, ...)
> +  ATTRIBUTE_PRINTF (2, 3);
>
>  extern int auto_load_gdb_scripts_enabled
>    (const struct extension_language_defn *extlang);
> diff --git a/gdb/compile/compile-loc2c.c b/gdb/compile/compile-loc2c.c
> index 3f43e58..6a3615d 100644
> --- a/gdb/compile/compile-loc2c.c
> +++ b/gdb/compile/compile-loc2c.c
> @@ -443,6 +443,9 @@ push (int indent, struct ui_file *stream, ULONGEST l)
>  /* Emit code to push an arbitrary expression.  This works like
>     printf.  */
>
> +static void pushf (int indent, struct ui_file *stream, const char *format, ...)
> +  ATTRIBUTE_PRINTF (3, 4);
> +
>  static void
>  pushf (int indent, struct ui_file *stream, const char *format, ...)
>  {
> @@ -460,6 +463,9 @@ pushf (int indent, struct ui_file *stream, const char *format, ...)
>  /* Emit code for a unary expression -- one which operates in-place on
>     the top-of-stack.  This works like printf.  */
>
> +static void unary (int indent, struct ui_file *stream, const char *format, ...)
> +  ATTRIBUTE_PRINTF (3, 4);
> +
>  static void
>  unary (int indent, struct ui_file *stream, const char *format, ...)
>  {
> @@ -474,6 +480,8 @@ unary (int indent, struct ui_file *stream, const char *format, ...)
>
>  /* Emit code for a unary expression -- one which uses the top two
>     stack items, popping the topmost one.  This works like printf.  */
> +static void binary (int indent, struct ui_file *stream, const char *format, ...)
> +  ATTRIBUTE_PRINTF (3, 4);
>
>  static void
>  binary (int indent, struct ui_file *stream, const char *format, ...)
> @@ -651,7 +659,7 @@ do_compile_dwarf_expr_to_c (int indent, struct ui_file *stream,
>    fprintfi_filtered (indent, stream, "int __gdb_tos = -1;\n");
>
>    if (initial != NULL)
> -    pushf (indent, stream, core_addr_to_string (*initial));
> +    pushf (indent, stream, "%s", core_addr_to_string (*initial));
>
>    while (op_ptr < op_end)
>      {
> @@ -911,7 +919,8 @@ do_compile_dwarf_expr_to_c (int indent, struct ui_file *stream,
>
>         case DW_OP_pick:
>           offset = *op_ptr++;
> -         pushf (indent, stream, "__gdb_stack[__gdb_tos - %d]", offset);
> +         pushf (indent, stream, "__gdb_stack[__gdb_tos - %s]",
> +                plongest (offset));
>           break;
>
>         case DW_OP_swap:
> @@ -1000,8 +1009,8 @@ do_compile_dwarf_expr_to_c (int indent, struct ui_file *stream,
>           break;
>
>  #define BINARY(OP)                                                     \
> -         binary (indent, stream, ("__gdb_stack[__gdb_tos-1] " #OP      \
> -                                  " __gdb_stack[__gdb_tos]")); \
> +         binary (indent, stream, "%s", "__gdb_stack[__gdb_tos-1] " #OP \
> +                                  " __gdb_stack[__gdb_tos]");  \
>           break
>
>         case DW_OP_and:
> @@ -1076,7 +1085,7 @@ do_compile_dwarf_expr_to_c (int indent, struct ui_file *stream,
>                                             addr_size,
>                                             cfa_start, cfa_end,
>                                             &text_offset, per_cu);
> -               pushf (indent, stream, cfa_name);
> +               pushf (indent, stream, "%s", cfa_name);
>               }
>           }
>
> diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
> index 5903f18..7d2b683 100644
> --- a/gdb/compile/compile-object-load.c
> +++ b/gdb/compile/compile-object-load.c
> @@ -224,6 +224,9 @@ link_callbacks_unattached_reloc (struct bfd_link_info *link_info,
>
>  /* Helper for link_callbacks callbacks vector.  */
>
> +static void link_callbacks_einfo (const char *fmt, ...)
> +  ATTRIBUTE_PRINTF (1, 2);
> +
>  static void
>  link_callbacks_einfo (const char *fmt, ...)
>  {
> diff --git a/gdb/complaints.c b/gdb/complaints.c
> index 7e52656..ab36d9b 100644
> --- a/gdb/complaints.c
> +++ b/gdb/complaints.c
> @@ -183,21 +183,24 @@ vcomplaint (struct complaints **c, const char *file,
>    else
>      series = complaints->series;
>
> +  /* Use FMT from here on instead of complaint->fmt, to avoid "format
> +     string is not a string literal" warnings.  */
> +  gdb_assert (complaint->fmt == fmt);
> +
>    if (complaint->file != NULL)
> -    internal_vwarning (complaint->file, complaint->line,
> -                      complaint->fmt, args);
> +    internal_vwarning (complaint->file, complaint->line, fmt, args);
>    else if (deprecated_warning_hook)
> -    (*deprecated_warning_hook) (complaint->fmt, args);
> +    (*deprecated_warning_hook) (fmt, args);
>    else
>      {
>        if (complaints->explanation == NULL)
>         /* A [v]warning() call always appends a newline.  */
> -       vwarning (complaint->fmt, args);
> +       vwarning (fmt, args);
>        else
>         {
>           char *msg;
>           struct cleanup *cleanups;
> -         msg = xstrvprintf (complaint->fmt, args);
> +         msg = xstrvprintf (fmt, args);
>           cleanups = make_cleanup (xfree, msg);
>           wrap_here ("");
>           if (series != SUBSEQUENT_MESSAGE)
> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
> index f9481c7..dfce179 100644
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -171,6 +171,9 @@ __attribute__ ((section ("__TEXT,__info_plist"),used)) =
>    "</dict>\n"
>    "</plist>\n";
>
> +static void inferior_debug (int level, const char *fmt, ...)
> +  ATTRIBUTE_PRINTF (2, 3);
> +
>  static void
>  inferior_debug (int level, const char *fmt, ...)
>  {
> diff --git a/gdb/guile/guile-internal.h b/gdb/guile/guile-internal.h
> index 968b4d3..86fc2f6 100644
> --- a/gdb/guile/guile-internal.h
> +++ b/gdb/guile/guile-internal.h
> @@ -484,7 +484,8 @@ extern char *gdbscm_scm_to_c_string (SCM string);
>
>  extern SCM gdbscm_scm_from_c_string (const char *string);
>
> -extern SCM gdbscm_scm_from_printf (const char *format, ...);
> +extern SCM gdbscm_scm_from_printf (const char *format, ...)
> +    ATTRIBUTE_PRINTF (1, 2);
>
>  extern char *gdbscm_scm_to_string (SCM string, size_t *lenp,
>                                    const char *charset,
> diff --git a/gdb/remote.c b/gdb/remote.c
> index e971a29..0b0770f 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -6973,6 +6973,9 @@ remote_read_bytes (struct target_ops *ops, CORE_ADDR memaddr,
>     FORMAT and the remaining arguments, then gets the reply.  Returns
>     whether the packet was a success, a failure, or unknown.  */
>
> +static enum packet_result remote_send_printf (const char *format, ...)
> +  ATTRIBUTE_PRINTF (1, 2);
> +
>  static enum packet_result
>  remote_send_printf (const char *format, ...)
>  {
> --
> 1.9.3
>
>

Re: format string is not a string literal

[Pedro Alves]

On 02/26/2015 04:48 PM, Jack Howarth wrote:
> Pedro,
>       It is almost complete except for...
> 
> ctf.c:109:39: warning: format string is not a string literal
> [-Wformat-nonliteral]
>   if (vfprintf (handler->metadata_fd, format, args) < 0)
>                                       ^~~~~~
> 
> and
> 
> ./guile/scm-utils.c:86:25: warning: format string is not a string
> literal [-Wformat-nonliteral]
>   string = xstrvprintf (format, args);
>                         ^~~~~~
> 

Thanks.  I've fixed those and pushed it in:

https://sourceware.org/ml/gdb-patches/2015-02/msg00780.html

Paul, I've improved the comment in vcomplain to clarify
the solution.  Thanks for poking about that.

Thanks,
Pedro Alves

Re: format string is not a string literal

[Paul Smith]

On Thu, 2015-02-26 at 18:34 +0000, Pedro Alves wrote:
> Paul, I've improved the comment in vcomplain to clarify
> the solution.  Thanks for poking about that.

Aha thanks; your comment is a little more clear than your email reply; I
was wondering how the compiler could know that the caller's format
string was literal...but from your comment it's clear it just assumes it
if we add proper attributes to vcomplaint().  Spiffy!

Cheers!

Re: format string is not a string literal

[Paul Smith]

On Thu, 2015-02-26 at 09:52 +0000, Pedro Alves wrote:
> +  /* Use FMT from here on instead of complaint->fmt, to avoid "format
> +     string is not a string literal" warnings.  */
> +  gdb_assert (complaint->fmt == fmt);
> +
>    if (complaint->file != NULL)
> -    internal_vwarning (complaint->file, complaint->line, 
> -                      complaint->fmt, args);
> +    internal_vwarning (complaint->file, complaint->line, fmt, args);

Can someone quickly explain how using fmt instead of complaint->fmt here
removes the warning?  fmt is not a "string literal" either...?

Just curious what the trick is...

Re: format string is not a string literal

[Pedro Alves]

On 02/26/2015 05:44 PM, Paul Smith wrote:
> On Thu, 2015-02-26 at 09:52 +0000, Pedro Alves wrote:
>> +  /* Use FMT from here on instead of complaint->fmt, to avoid "format
>> +     string is not a string literal" warnings.  */
>> +  gdb_assert (complaint->fmt == fmt);
>> +
>>    if (complaint->file != NULL)
>> -    internal_vwarning (complaint->file, complaint->line, 
>> -                      complaint->fmt, args);
>> +    internal_vwarning (complaint->file, complaint->line, fmt, args);
> 
> Can someone quickly explain how using fmt instead of complaint->fmt here
> removes the warning?  fmt is not a "string literal" either...?
> 
> Just curious what the trick is...

Simon explained it, actually:

> If the function is vprintf-style, it's similar but the last argument
> should be 0. It will push the argument check a level higher, where
> eventually they are explicitely defined printf-style. The doc is
> somewhere here [2] in the middle.

That code is in this function:

> static void ATTRIBUTE_PRINTF (4, 0)
vcomplaint (struct complaints **c, const char *file,
	    int line, const char *fmt,
	    va_list args)
{

So here the compiler will check FMT in the caller.

Thanks,
Pedro Alves