Re: [PATCH] qtest: Fix bad printf format specifiers

[Markus Armbruster]

Alex Chen <alex.chen@huawei.com> writes:

> On 2020/11/9 15:57, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> On 06/11/2020 15.18, Philippe Mathieu-Daudé wrote:
>>>> On 11/6/20 7:33 AM, Markus Armbruster wrote:
>>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>>
>>>>>> On 05/11/2020 06.14, AlexChen wrote:
>>>>>>> On 2020/11/4 18:44, Thomas Huth wrote:
>>>>>>>> On 04/11/2020 11.23, AlexChen wrote:
>>>>>>>>> We should use printf format specifier "%u" instead of "%d" for
>>>>>>>>> argument of type "unsigned int".
>>>>>>>>>
>>>>>>>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>>>>>>>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
>>>>>>>>> ---
>>>>>>>>>  tests/qtest/arm-cpu-features.c | 8 ++++----
>>>>>>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>>>>
> [...]
>>>>>>>>>
>>>>>>>>
>>>>>>>> max_vq and vq are both "uint32_t" and not "unsigned int" ... so if you 
>>>>>>>> want
>>>>>>>> to fix this really really correctly, please use PRIu32 from inttypes.h 
>>>>>>>> instead.
>>>>>>>>
>>>>>>>
>>>>>>> Hi Thomas,
>>>>>>> Thanks for your review.
>>>>>>> According to the definition of the macro PRIu32(# define PRIu32         
>>>>>>> "u"),
>>>>>>> using PRIu32 works the same as using %u to print, and using PRIu32 to 
>>>>>>> print
>>>>>>> is relatively rare in QEMU(%u 720, PRIu32 only 120). Can we continue to 
>>>>>>> use %u to
>>>>>>> print max_vq and vq in this patch.
>>>>>>> Of course, this is just my small small suggestion. If you think it is 
>>>>>>> better to use
>>>>>>> PRIu32 for printing, I will send patch V2.
>>>>>>
>>>>>> Well, %u happens to work since "int" is 32-bit with all current compilers
>>>>>> that we support.
>>>>>
>>>>> Yes, it works.
>>>>>
>>>>>>                  But if there is ever a compiler where the size of int is
>>>>>> different, you'll get a compiler warning here again.
>>>>>
>>>>> No, we won't.
>>>>>
>>>>> If we ever use a compiler where int is narrower than 32 bits, then the
>>>>> type of the argument is actually uint32_t[1].  We can forget about this
>>>>> case, because "int narrower than 32 bits" is not going to fly with our
>>>>> code base.
>>>
>>> Agreed.
>>>
>>>>> If we ever use a compiler where int is wider than 32 bits, then the type
>>>>> of the argument is *not* uint32_t[2].  PRIu32 will work anyway, because
>>>>> it will actually retrieve an unsigned int argument, *not* an uint32_t
>>>>> argument[3].
>>>
>>> I can hardly believe that this can be true. Sure, it's true for such cases
>>> like this one here, where you multiply with an "int". But if you just try to
>>> print a plain uint32_t variable?
>> 
>> Default argument promotions (§6.5.2.2 Function calls) still apply: "the
>> integer promotions are performed on each argument, and arguments that
>> have type float are promoted to double."
>> 
>>> I've seen compiler warning in cases one tries to print a 16-bit (i.e. short)
>>> variable in the past if you use %d instead of the proper PRId16 (or %hd)
>>> format specifier - maybe not on x86, but certainly on other architectures.
>>> If you're statement was right, that should not have happened, should it?
>> 
>> §7.19.6.1 "The fprintf function" on length modifier 'h':
>> 
>>     Specifies that a following d, i, o, u, x, or X conversion specifier
>>     applies to a short int or unsigned short int argument (the argument
>>     will have been promoted according to the integer promotions, but its
>>     value shall be converted to short int or unsigned short int before
>>     printing)
>> 
>> Integer promotions preserve value including sign.  So, printing a short
>> value with %hd first promotes it to int, then converts it back to short.
>> Neither conversion has an effect.
>> 
>> However, printing an int with %hd has: it converts int to short.
>> Implementation-defined behavior when the value doesn't fit.
>> 
>> Length modifier 'h' is pretty pointless with printf().  So would be a
>> warning to nudge people towards its use.
>> 
>> In fact, GNU libc's PRIu32 does not use it.  inttypes.h:
>> 
>>     /* Unsigned integers.  */
>>     # define PRIu8           "u"
>>     # define PRIu16          "u"
>>     # define PRIu32          "u"
>>     # define PRIu64          __PRI64_PREFIX "u"
>> 
>> where __PRI64_PREFIX is "l" or "ll" depending on system-dependent
>> __WORDSIZE.
>> 
>> In short:
>> 
>>>>> In other words "%" PRIu32 is just a less legible alias for "%u" in all
>>>>> cases that matter.
>> 
>
> Hi Markus,
>
> Thanks for your reply, I have learned a lot.
> May I understand it as follows:
> %u is used when there are parameters obtained by arithmetic operation;
> otherwise, PRIu32 is used to print uint32_t type parameters?

No.  Use "%u" unless you need portability to machines where unsigned is
narrower than 32 bits (we don't).

On machines where unsigned int is at least 32 bit wide, "%" PRIu32
is the same as "%u".  It's not wrong, just illegible.