Re: format string is not a string literal

Jack Howarth
2015-02-26T16:48:31Z

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