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

[Tsukasa OI <research_trasio@irq.a4lg.com>]

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
>