| Message ID | 5FA28117.3020802@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qtest: Fix bad printf format specifiers | expand |
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. Thanks, Thomas
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. Looking forward to your reply. Thanks, Alex
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. But if there is ever a compiler where the size of int is different, you'll get a compiler warning here again. So if we now fix this up, then let's do it really right and use PRIu32, please. Thomas
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.
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.
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. > > 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. Can we add a checkpatch rule to avoid using 'PRI[dux]32' format, so it is clear for everyone? > > 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. > >
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 11/6/20 7:33 AM, Markus Armbruster wrote: [...] >> In other words "%" PRIu32 is just a less legible alias for "%u" in all >> cases that matter. > > Can we add a checkpatch rule to avoid using 'PRI[dux]32' format, > so it is clear for everyone? I guess we could, but *I* can't: -ENOTIME, sorry.
On 05/11/2020 09.19, Markus Armbruster wrote: > 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. Ouch. Ok. Then let's go with this v1 patch instead of v2. Thomas
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? 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? >> In other words "%" PRIu32 is just a less legible alias for "%u" in all >> cases that matter. > > Can we add a checkpatch rule to avoid using 'PRI[dux]32' format, > so it is clear for everyone? I don't think that this is a good idea. Using PRI*32 is IMHO perfectly fine if you really have a variable of the type [u]int32_t ... it's just not if you do some arithmetics with it first. 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. >> >> > >
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. [...]
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? Thanks, Alex
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.
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
Thomas Huth <thuth@redhat.com> writes: > 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/ Awesome :) If the system typedefs uint32_t to unsigned long, the integer promotions do not apply, even when unsigned long is just as wide as unsigned int. My take on that is "isn't there enough suffering in the world?" Since I don't have such a toolchain handy, I faked it (see appended patch). The fakery is not expected compile (it triggers compile-time assertions like the ones in cpu.h, for instance). It *is* expected to ferret out use of %u with uint32_t and similar. And it does: I get format ‘%x’ expects argument of type ‘unsigned int’, but argument 6 has type ‘uint32_t’ {aka ‘const long unsigned int’} [-Wformat=] and the like almost 15,000 times. And that's with --target-list=x86_64-softmmu. A full compile can only have more. I think we have better things to do than "fixing" these. diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index f9ec8c84e9..70123f70c1 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -50,6 +50,37 @@ #define __STDC_FORMAT_MACROS #endif +#define int32_t xxxint32_t +#define uint32_t xxxuint32_t +#include <stdint.h> +#undef int32_t +#undef uint32_t +typedef long int32_t; +typedef unsigned long uint32_t; +#include <inttypes.h> +#undef PRId32 +#undef PRIi32 +#undef PRIo32 +#undef PRIu32 +#undef PRIx32 +#undef PRIX32 +#undef SCNd32 +#undef SCNi32 +#undef SCNo32 +#undef SCNu32 +#undef SCNx32 +#define PRId32 "ld" +#define PRIi32 "li" +#define PRIo32 "lo" +#define PRIu32 "lu" +#define PRIx32 "lx" +#define PRIX32 "lX" +#define SCNd32 "ld" +#define SCNi32 "li" +#define SCNo32 "lo" +#define SCNu32 "lu" +#define SCNx32 "lx" + /* The following block of code temporarily renames the daemon() function so the * compiler does not see the warning associated with it in stdlib.h on OSX */
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);
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(-)