[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] qtest: Fix bad printf format specifiers
From: |
Markus Armbruster |
Subject: |
Re: [PATCH] qtest: Fix bad printf format specifiers |
Date: |
Fri, 06 Nov 2020 07:33:55 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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.
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].
In other words "%" PRIu32 is just a less legible alias for "%u" in all
cases that matter.
And that's why I would simply use "%u".
> So if we now fix this
> up, then let's do it really right and use PRIu32, please.
>
> Thomas
[1] Because promotion does nothing either argument, and the usual
arithmetic conversions convert to uint32_t. See my first reply.
[2] Because uint32_t gets promoted to unsigned int. See my first reply.
[3] Because variable arguments undergo default argument promotion (§
6.5.2.2 Function calls), which promotes uint32_t to unsigned int.
- [PATCH] qtest: Fix bad printf format specifiers, AlexChen, 2020/11/04
- Re: [PATCH] qtest: Fix bad printf format specifiers, Thomas Huth, 2020/11/04
- Re: [PATCH] qtest: Fix bad printf format specifiers, AlexChen, 2020/11/05
- Re: [PATCH] qtest: Fix bad printf format specifiers, Thomas Huth, 2020/11/05
- Re: [PATCH] qtest: Fix bad printf format specifiers,
Markus Armbruster <=
- Re: [PATCH] qtest: Fix bad printf format specifiers, Philippe Mathieu-Daudé, 2020/11/06
- Re: [PATCH] qtest: Fix bad printf format specifiers, Markus Armbruster, 2020/11/06
- Re: [PATCH] qtest: Fix bad printf format specifiers, Thomas Huth, 2020/11/08
- Re: [PATCH] qtest: Fix bad printf format specifiers, Markus Armbruster, 2020/11/09
- Re: [PATCH] qtest: Fix bad printf format specifiers, Alex Chen, 2020/11/09
- Re: [PATCH] qtest: Fix bad printf format specifiers, Markus Armbruster, 2020/11/09
- Re: [PATCH] qtest: Fix bad printf format specifiers, Thomas Huth, 2020/11/10
- Re: [PATCH] qtest: Fix bad printf format specifiers, Markus Armbruster, 2020/11/11
Re: [PATCH] qtest: Fix bad printf format specifiers, Markus Armbruster, 2020/11/05