Re: format string is not a string literal

[Jack Howarth <howarth dot mailing dot lists at gmail dot com>]

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