Re: [PATCH 4/4] sim: Suppress non-literal printf warning

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

On 2022/10/05 20:45, Andrew Burgess wrote:
> Tsukasa OI <research_trasio@irq.a4lg.com> writes:
> 
>> Clang generates a warning if the format parameter of a printf-like function
>> is not a literal.  However, on hw_vabort, it's unavoidable to use non-
>> literal as a format string (unless we make huge redesign).
>>
>> We have "include/diagnostics.h" to suppress certain warnings only when
>> necessary.  Because DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL can suppress
>> warnings when the format parameter of a printf-like function is not a
>> literal, this commit adds this (only where necessary) to suppress this
>> error with "-Werror", the default configuration.
>>
>> sim/ChangeLog:
>>
>> 	* common/sim-hw.c (hw_vabort): Suppress non-literal printf warning
>> 	by using DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL.
>> ---
>>  sim/common/sim-hw.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/sim/common/sim-hw.c b/sim/common/sim-hw.c
>> index cece5638bc9..36f355d2262 100644
>> --- a/sim/common/sim-hw.c
>> +++ b/sim/common/sim-hw.c
>> @@ -425,10 +425,13 @@ hw_vabort (struct hw *me,
>>    strcat (msg, ": ");
>>    strcat (msg, fmt);
>>    /* report the problem */
>> +  DIAGNOSTIC_PUSH
>> +  DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
>>    sim_engine_vabort (hw_system (me),
>>  		     STATE_HW (hw_system (me))->cpu,
>>  		     STATE_HW (hw_system (me))->cia,
>>  		     msg, ap);
>> +  DIAGNOSTIC_POP
> 
> Rather than disabling diagnostics, I'd like to propose the patch below
> which expands FMT and AP within sim-hw.c, then passes the expanded
> string through to sim_engine_abort.  What do you think of this?

Ah, It took a while to understand but makes sense to me.

I just needed to add ATTRIBUTE_PRINTF (2, 0) to suppress "-Werror
-Wformat-nonliteral" but I prefer to use your patch instead.

> 
> My motivation is to avoid disabling diagnostics as much as possible.

I support your opinion as possible.  I sometimes disable some warnings
intentionally but it's because I thought disabling the warning is the
only viable solution.  In this case, it wasn't.

Thanks,
Tsukasa

> 
> As far as I can tell the host_callback_struct::evprintf_filtered
> callback is just the standard printf API, so using vsnprintf should
> expand everything correctly.
> 
> Thanks,
> Andrew
> 
> ---
> 
> diff --git a/sim/common/sim-hw.c b/sim/common/sim-hw.c
> index cece5638bc9..7bfe91e4ae2 100644
> --- a/sim/common/sim-hw.c
> +++ b/sim/common/sim-hw.c
> @@ -408,8 +408,11 @@ hw_vabort (struct hw *me,
>  	   const char *fmt,
>  	   va_list ap)
>  {
> +  int len;
>    const char *name;
>    char *msg;
> +  va_list cpy;
> +
>    /* find an identity */
>    if (me != NULL && hw_path (me) != NULL && hw_path (me) [0] != '\0')
>      name = hw_path (me);
> @@ -419,16 +422,19 @@ hw_vabort (struct hw *me,
>      name = hw_family (me);
>    else
>      name = "device";
> -  /* construct an updated format string */
> -  msg = alloca (strlen (name) + strlen (": ") + strlen (fmt) + 1);
> -  strcpy (msg, name);
> -  strcat (msg, ": ");
> -  strcat (msg, fmt);
> +
> +  /* Expand FMT and AP into MSG buffer.  */
> +  va_copy (cpy, ap);
> +  len = vsnprintf (NULL, 0, fmt, cpy) + 1;
> +  va_end (cpy);
> +  msg = alloca (len);
> +  vsnprintf (msg, len, fmt, ap);
> +
>    /* report the problem */
> -  sim_engine_vabort (hw_system (me),
> -		     STATE_HW (hw_system (me))->cpu,
> -		     STATE_HW (hw_system (me))->cia,
> -		     msg, ap);
> +  sim_engine_abort (hw_system (me),
> +		    STATE_HW (hw_system (me))->cpu,
> +		    STATE_HW (hw_system (me))->cia,
> +		    "%s: %s", name, msg);
>  }
>  
>  void
>