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

  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?

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

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

---
  

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
>
  

On 06 Oct 2022 14:39, Tsukasa OI via Gdb-patches wrote:
> 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.

adding ATTRIBUTE_PRINTF doesn't suppress warnings, it fixes them
-mike
  

On 2022/10/23 21:22, Mike Frysinger wrote:
> On 06 Oct 2022 14:39, Tsukasa OI via Gdb-patches wrote:
>> 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.
> 
> adding ATTRIBUTE_PRINTF doesn't suppress warnings, it fixes them
> -mike

Correct.  I'm focusing on fixing "build failures caused by Clang
warnings" and I think I wrote commit messages without considering why
warnings are gone.

Thanks for pointing this out.
Tsukasa