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

[Markus Armbruster]

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(-)
>>>>>>>
>>>>>>> diff --git a/tests/qtest/arm-cpu-features.c 
>>>>>>> b/tests/qtest/arm-cpu-features.c
>>>>>>> index d20094d5a7..bc681a95d5 100644
>>>>>>> --- a/tests/qtest/arm-cpu-features.c
>>>>>>> +++ b/tests/qtest/arm-cpu-features.c
>>>>>>> @@ -536,7 +536,7 @@ static void 
>>>>>>> test_query_cpu_model_expansion_kvm(const void *data)
>>>>>>>          if (kvm_supports_sve) {
>>>>>>>              g_assert(vls != 0);
>>>>>>>              max_vq = 64 - __builtin_clzll(vls);
>>>>>>> -            sprintf(max_name, "sve%d", max_vq * 128);
>>>>>>> +            sprintf(max_name, "sve%u", max_vq * 128);
>>>>>>>
>>>>>>>              /* Enabling a supported length is of course fine. */
>>>>>>>              assert_sve_vls(qts, "host", vls, "{ %s: true }", max_name);
>>>>>>> @@ -556,7 +556,7 @@ static void 
>>>>>>> test_query_cpu_model_expansion_kvm(const void *data)
>>>>>>>                   * unless all larger, supported vector lengths are also
>>>>>>>                   * disabled.
>>>>>>>                   */
>>>>>>> -                sprintf(name, "sve%d", vq * 128);
>>>>>>> +                sprintf(name, "sve%u", vq * 128);
>>>>>>>                  error = g_strdup_printf("cannot disable %s", name);
>>>>>>>                  assert_error(qts, "host", error,
>>>>>>>                               "{ %s: true, %s: false }",
>>>>>>> @@ -569,7 +569,7 @@ static void 
>>>>>>> test_query_cpu_model_expansion_kvm(const void *data)
>>>>>>>               * we need at least one vector length enabled.
>>>>>>>               */
>>>>>>>              vq = __builtin_ffsll(vls);
>>>>>>> -            sprintf(name, "sve%d", vq * 128);
>>>>>>> +            sprintf(name, "sve%u", vq * 128);
>>>>>>>              error = g_strdup_printf("cannot disable %s", name);
>>>>>>>              assert_error(qts, "host", error, "{ %s: false }", name);
>>>>>>>              g_free(error);
>>>>>>> @@ -581,7 +581,7 @@ static void 
>>>>>>> test_query_cpu_model_expansion_kvm(const void *data)
>>>>>>>                  }
>>>>>>>              }
>>>>>>>              if (vq <= SVE_MAX_VQ) {
>>>>>>> -                sprintf(name, "sve%d", vq * 128);
>>>>>>> +                sprintf(name, "sve%u", vq * 128);
>>>>>>>                  error = g_strdup_printf("cannot enable %s", name);
>>>>>>>                  assert_error(qts, "host", error, "{ %s: true }", name);
>>>>>>>                  g_free(error);
>>>>>>>
>>>>>>
>>>>>> 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.

[...]