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

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> 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

Not quite.  They are, but the product isn't.  Assuming it is is actually
a common misconception of how C works.

C99 § 6.3.1.8 "Usual arithmetic conversions" applies.  Short summary:
first, both operands of * undergo integer promotion (§ 6.3.1.1 Boolean,
characters, and integers), then we find a "common" integer type, convert
the operands to it, and multiply in that type.

128 is int (§ 6.4.4.1 Integer constants).  Integer promotion does
nothing.

@vq is uint32_t per its declaration.  If int can represent any uint32_t
value, it promotes to int; else if unsigned int can represent, it
promotes to unsigned int; else it stays the same.

In QEMU practice, "stays the same" is impossible, because unsigned int
narrower than 32 bits is.  "Promotes to int" is unlikely, because int
wider than 32 bits is.

So, the "common" type is almost certainly unsigned int for us, but we
may want to do the right thing for the unlikely case of signed int.

It is uint32_t only when it's unsigned int, and the system makes
uint32_t an alias for unsigned int, say with typedef unsigned uint32_t.

> to fix this really really correctly, please use PRIu32 from inttypes.h 
> instead.

I wouldn't.

The PRI macros are required for integer types wider than signed /
unsigned int.

Narrower types promote to int or unsigned.

For equally wide types, it doesn't matter.