Re: [PATCH 6/7] sim/ppc: Add ATTRIBUTE_PRINTF

[Andrew Burgess <aburgess@redhat.com>]

Tsukasa OI <research_trasio@irq.a4lg.com> writes:

> Clang generates a warning if the format string of a printf-like function is
> not a literal ("-Wformat-nonliteral").  On the default configuration, it
> causes a build failure (unless "--disable-werror" is specified).
>
> To avoid warnings on the printf-like wrapper, it requires proper
> __attribute__((format)) and we have ATTRIBUTE_PRINTF macro for this reason.
>
> This commit adds ATTRIBUTE_PRINTF to the printf-like functions.
>
> sim/ChangeLog:
>
> 	* ppc/main.c (error): Add ATTRIBUTE_PRINTF.
> 	* ppc/misc.c (error, dumpf): Likewise.
> 	* ppc/sim_calls.c (error): Likewise.
> ---
>  sim/ppc/main.c      | 2 +-
>  sim/ppc/misc.c      | 4 ++--
>  sim/ppc/sim_calls.c | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/sim/ppc/main.c b/sim/ppc/main.c
> index 83b629ec14a..4a88166106f 100644
> --- a/sim/ppc/main.c
> +++ b/sim/ppc/main.c
> @@ -68,7 +68,7 @@ sim_io_printf_filtered(const char *msg, ...)
>    va_end(ap);
>  }
>  
> -void
> +void ATTRIBUTE_PRINTF(1, 2)
>  error (const char *msg, ...)

I notice in this patch, and the previous one, you've added
ATTRIBUTE_PRINTF to both the declaration, and the definition of some
functions.

Is this required?  I thought we only needed the attribute on the
declaration.

In this case this difference is even more pronounced as you've added the
ATTRIBUTE_PRINTF, but the declaration also has ATTRIBUTE_NORETURN, which
you haven't added to the definition.

My preference would be to only have the attributes on the declaration if
that is sufficient.  Could you test that change and see if your build
issues are still resolved.

Thanks,
Andrew


>  {
>    va_list ap;
> diff --git a/sim/ppc/misc.c b/sim/ppc/misc.c
> index 8f2581e3ef3..71cda9fa298 100644
> --- a/sim/ppc/misc.c
> +++ b/sim/ppc/misc.c
> @@ -28,7 +28,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  
> -void
> +void ATTRIBUTE_PRINTF(1, 2)
>  error (const char *msg, ...)
>  {
>    va_list ap;
> @@ -48,7 +48,7 @@ zalloc(long size)
>    return memory;
>  }
>  
> -void
> +void ATTRIBUTE_PRINTF(2, 3)
>  dumpf (int indent, const char *msg, ...)
>  {
>    va_list ap;
> diff --git a/sim/ppc/sim_calls.c b/sim/ppc/sim_calls.c
> index fbc327c94e0..b0ed3d4c3cc 100644
> --- a/sim/ppc/sim_calls.c
> +++ b/sim/ppc/sim_calls.c
> @@ -388,7 +388,7 @@ sim_io_error (SIM_DESC sd, const char *fmt, ...)
>  
>  /****/
>  
> -void ATTRIBUTE_NORETURN
> +void ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF(1, 2)
>  error (const char *msg, ...)
>  {
>    va_list ap;
> -- 
> 2.34.1