Hi Eric, On 06/29/2018 09:19 AM, Eric Blake wrote: > On 06/28/2018 05:53 PM, Philippe Mathieu-Daudé wrote: > >>>>> +#ifndef QEMU_UNITS_H >>>>> +#define QEMU_UNITS_H >>>>> + >>>>> +#define KiB (INT64_C(1) << 10) >>>>> +#define MiB (INT64_C(1) << 20) >>>>> +#define GiB (INT64_C(1) << 30) >>>>> +#define TiB (INT64_C(1) << 40) >>>>> +#define PiB (INT64_C(1) << 50) >>>>> +#define EiB (INT64_C(1) << 60) >>>> Shouldn't above use UINT64_C() >>> >>> Since the decision of signed vs. unsigned was intentional based on >>> review on earlier versions, it may be worth a comment in this file that >>> these constants are intentionally signed (in usage patterns, these tend >>> to be multiplied by another value; and while it is easy to go to >>> unsigned by doing '1U * KiB', you can't go in the opposite direction if >>> you want a signed number for '1 * KiB' unless KiB is signed). >> >> OK. > > Actually, '1U * KiB' still ends up signed. Why? Because as written, KiB > is a 64-bit quantity, but 1U is 32-bit; type promotion says that since a > 64-bit int can represent all 32-bit unsigned values, the result of the > expression is still signed 64-bit.
Are you suggesting this? #define KiB (INT32_C(1) << 10) #define MiB (INT32_C(1) << 20) #define GiB (INT32_C(1) << 30) #define TiB (INT64_C(1) << 40) #define PiB (INT64_C(1) << 50) #define EiB (INT64_C(1) << 60) Now than I reread what Richard reviewed, I guess understand he suggested the same change: http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg03284.html > To get unsigned KiB, you either have to use '1ULL * KiB', or KiB should > be changed to be (INT32_C(1) << 10) (a 32-bit constant, rather than a > 64-bit one). > >> >> I'll also change this tests using your suggestion: >> >> diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c >> @@ -709,8 +709,7 @@ static void test_opts_parse_size(void) >> false, &error_abort); >> - g_assert_cmphex(qemu_opt_get_size(opts, "size2", 0), >> - ==, 16777215 * T_BYTE); >> + g_assert_cmphex(qemu_opt_get_size(opts, "size2", 0), ==, 16777215U >> * TiB); >> >> to avoid this error on 32-bit archs: >> >> source/qemu/tests/test-qemu-opts.c: In function 'test_opts_parse_size': >> source/qemu/tests/test-qemu-opts.c:713:71: error: integer overflow in >> expression [-Werror=overflow] >> g_assert_cmphex(qemu_opt_get_size(opts, "size2", 0), ==, 16777215 * >> TiB); >> ^ > > And this compile error is proof of the confusion when we have a signed > integer overflow (why it only happens on 32-bit arch and not also on > 64-bit arch is subtle - it's because TiB is 'long long' on 32-bit, but > merely 'long' on 64-bit, which results in a different type after type > promotion - although I still find it odd that the 64-bit compiler isn't > warning). >
