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

Tsukasa OI via Gdb-patches
2022-10-06T05:33:09Z

On 2022/10/05 19:57, Andrew Burgess wrote:
> 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.

Yes, declaration is sufficient. Because recent "build for Clang"
patches are collection of many attempts so I think I mixed it somewhere.
In the next version, I'll append this attribute to declarations, not
definitions.

Thanks,
Tsukasa

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