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

[Thomas Huth <thuth@redhat.com>]

On 09/11/2020 13.50, Markus Armbruster wrote:
> 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.

Just FYI, there are also apparently toolchains where uint32_t is defined as
unsigned long:

https://patchwork.kernel.org/project/kvm/patch/20201105135936.55088-1-alexandru.elisei@arm.com/

 Thomas