| Message ID | 3096076082478dafe78553ab5cbd8b572904cbc4.1394794127.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Mar 14, 2014 at 06:50:37AM -0400, Jeff Cody wrote: > On 32-bit hosts, some compilers will warn on too large integer constants > for constants that are 64-bit in length. Explicitly put a 'ULL' suffix > on those defines. > > Reported-by: Alexander Graf <agraf@suse.de> > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/vhdx.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
On Fri, Mar 14, 2014 at 06:50:37AM -0400, Jeff Cody wrote: > On 32-bit hosts, some compilers will warn on too large integer constants > for constants that are 64-bit in length. Explicitly put a 'ULL' suffix > on those defines. > -#define VHDX_FILE_SIGNATURE 0x656C696678646876 /* "vhdxfile" in ASCII */ > +#define VHDX_FILE_SIGNATURE 0x656C696678646876ULL /* "vhdxfile" in ASCII */ I think it's better to use this C99-defined feature (from <stdint.h>): #define VHDX_FILE_SIGNATURE UINT64_C(0x656C696678646876) Rich.
On 14 March 2014 15:36, Richard W.M. Jones <rjones@redhat.com> wrote: > On Fri, Mar 14, 2014 at 06:50:37AM -0400, Jeff Cody wrote: >> On 32-bit hosts, some compilers will warn on too large integer constants >> for constants that are 64-bit in length. Explicitly put a 'ULL' suffix >> on those defines. >> -#define VHDX_FILE_SIGNATURE 0x656C696678646876 /* "vhdxfile" in ASCII */ >> +#define VHDX_FILE_SIGNATURE 0x656C696678646876ULL /* "vhdxfile" in ASCII */ > > I think it's better to use this C99-defined feature (from <stdint.h>): > > #define VHDX_FILE_SIGNATURE UINT64_C(0x656C696678646876) Why? It's longer and we barely use it anywhere else in the codebase, whereas we use the ULL suffix all over the place... thanks -- PMM
On Fri, Mar 14, 2014 at 03:38:55PM +0000, Peter Maydell wrote: > On 14 March 2014 15:36, Richard W.M. Jones <rjones@redhat.com> wrote: > > On Fri, Mar 14, 2014 at 06:50:37AM -0400, Jeff Cody wrote: > >> On 32-bit hosts, some compilers will warn on too large integer constants > >> for constants that are 64-bit in length. Explicitly put a 'ULL' suffix > >> on those defines. > >> -#define VHDX_FILE_SIGNATURE 0x656C696678646876 /* "vhdxfile" in ASCII */ > >> +#define VHDX_FILE_SIGNATURE 0x656C696678646876ULL /* "vhdxfile" in ASCII */ > > > > I think it's better to use this C99-defined feature (from <stdint.h>): > > > > #define VHDX_FILE_SIGNATURE UINT64_C(0x656C696678646876) > > Why? It's longer and we barely use it anywhere else > in the codebase, whereas we use the ULL suffix all > over the place... It's permitted for unsigned long long to be longer than 64 bits. The UINT64_C() macro will always return a 64 bit int constant. Yup, it's not likely and for this particular macro it wouldn't matter. (And I'm not going to get into language-lawyering ...) Rich.
On 03/14/14 16:36, Richard W.M. Jones wrote: > On Fri, Mar 14, 2014 at 06:50:37AM -0400, Jeff Cody wrote: >> On 32-bit hosts, some compilers will warn on too large integer constants >> for constants that are 64-bit in length. Explicitly put a 'ULL' suffix >> on those defines. >> -#define VHDX_FILE_SIGNATURE 0x656C696678646876 /* "vhdxfile" in ASCII */ >> +#define VHDX_FILE_SIGNATURE 0x656C696678646876ULL /* "vhdxfile" in ASCII */ > > I think it's better to use this C99-defined feature (from <stdint.h>): > > #define VHDX_FILE_SIGNATURE UINT64_C(0x656C696678646876) Here's some pointing to the standard(s)... "Unsigned long long" is a gnu-ism for C89. It's a standard part of C99. Last time I checked, qemu used the gnu89 dialect on all build hosts except SunOS. So, when you use an integer constant like 0x656C696678646876, that constant is undefined in C89. Namely, - 6.1.3.2 "integer constants" (in C89) describes the "ladder" only up to "unsigned long int" (unsuffixed hexadecimal), - ULONG_MAX need not be higher than (2^32-1) (5.2.4.2.1 "sizes of integral types <limits.h>"), which in fact happens to be the case in an -m32 build, - 6.1.3.2 "integer constants" (in C89) doesn't say anything about "extended integer types" (C99 does mention those), - in gnu89 dialect, gcc decides not to pick "long long unsigned" on its own for this constant (IOW gcc doesn't elect to "fill in" this undefined behavior for you even in gnu89 dialect). So, in gnu89 you really have no other choice than saying "long long unsigned" yourself (in this instance with the ULL suffix). In C99 this is not an issue, because the ladder goes up to "long long unsigned" (see 6.4.4.1 Integer constants), and that type must be able to represent (2^64-1) minimally (5.2.4.2.1 Sizes of integer types <limits.h>). The "problem" with the proposed UINT64_C() is that it's C99 only. If we used C99 rather than gnu89, there would be no need for using UINT64_C(), the constant would simply work. (UINT64_C() might be part of gnu89 too, but then the ULL suffix, which is also gnu89, is just shorter.) In one sentence, the 0x656C696678646876 constant falls in the gnu89 \ c89 relative complement set for -m32, and gcc simply opts against automatically picking the "unsigned long long" type (which otherwise belongs to the same relative complement set of language features too), and leaves the behavior undefined. (That's my thinking at least.) Laszlo
On 03/14/14 16:57, Richard W.M. Jones wrote: > On Fri, Mar 14, 2014 at 03:38:55PM +0000, Peter Maydell wrote: >> On 14 March 2014 15:36, Richard W.M. Jones <rjones@redhat.com> wrote: >>> On Fri, Mar 14, 2014 at 06:50:37AM -0400, Jeff Cody wrote: >>>> On 32-bit hosts, some compilers will warn on too large integer constants >>>> for constants that are 64-bit in length. Explicitly put a 'ULL' suffix >>>> on those defines. >>>> -#define VHDX_FILE_SIGNATURE 0x656C696678646876 /* "vhdxfile" in ASCII */ >>>> +#define VHDX_FILE_SIGNATURE 0x656C696678646876ULL /* "vhdxfile" in ASCII */ >>> >>> I think it's better to use this C99-defined feature (from <stdint.h>): >>> >>> #define VHDX_FILE_SIGNATURE UINT64_C(0x656C696678646876) >> >> Why? It's longer and we barely use it anywhere else >> in the codebase, whereas we use the ULL suffix all >> over the place... > > It's permitted for unsigned long long to be longer than 64 bits. Yes. > The > UINT64_C() macro will always return a 64 bit int constant. That's incorrect, for two reasons: (a) uint64_t (exact-width integer types in general) are optional in C99. See 7.18.1.1 "Exact-width integer types" p3: These types are optional. However, if an implementation provides integer types with widths of 8, 16, 32, or 64 bits, it shall define the corresponding typedef names. I general we can't state that the "UINT64_C() macro will always return a 64 bit int constant", because the C implementation might not even support such a type. (b) UINT64_C() is for "uint_least64_t" (7.18.4.1 Macros for minimum-width integer constants). "uint_least64_t" is a required type (7.18.1.2 Minimum-width integer types). In practice I'd say it doesn't matter which one we use: - ULL suffix is gnu89, - UINT64_C() macro is gnu89, - "unsigned long long" could be wider in general than 64 bits, - "uint_least64_t" too could be wider in general than 64 bits, - for us both results in uint64_t exactly. So the above is a tie, but the ULL suffix is just nicer. (IMHO :)) Laszlo
On 14 March 2014 16:17, Laszlo Ersek <lersek@redhat.com> wrote: > "Unsigned long long" is a gnu-ism for C89. It's a standard part of C99. > Last time I checked, qemu used the gnu89 dialect on all build hosts > except SunOS. HACKING says we use C99... -- PMM
On 03/14/14 17:26, Peter Maydell wrote: > On 14 March 2014 16:17, Laszlo Ersek <lersek@redhat.com> wrote: >> "Unsigned long long" is a gnu-ism for C89. It's a standard part of C99. >> Last time I checked, qemu used the gnu89 dialect on all build hosts >> except SunOS. > > HACKING says we use C99... HACKING lies then :) grep for "std=c99" or "std=gnu99". You will find no hits for the former, and one hit for the latter, when the build host is SunOS. (Or just grep for '-std='.) In gcc-4.8.2, -std still defaults to gnu89. Laszlo
On 03/14/14 17:35, Laszlo Ersek wrote: > On 03/14/14 17:26, Peter Maydell wrote: >> On 14 March 2014 16:17, Laszlo Ersek <lersek@redhat.com> wrote: >>> "Unsigned long long" is a gnu-ism for C89. It's a standard part of C99. >>> Last time I checked, qemu used the gnu89 dialect on all build hosts >>> except SunOS. >> >> HACKING says we use C99... > > HACKING lies then :) Apologies, I didn't impy "lie" as in "lie". I meant "mistake". Sorry! Laszlo
On Fri, Mar 14, 2014 at 05:26:06PM +0100, Laszlo Ersek wrote: > (b) UINT64_C() is for "uint_least64_t" (7.18.4.1 Macros for > minimum-width integer constants). "uint_least64_t" is a required type > (7.18.1.2 Minimum-width integer types). > > In practice I'd say it doesn't matter which one we use: > - ULL suffix is gnu89, > - UINT64_C() macro is gnu89, > - "unsigned long long" could be wider in general than 64 bits, > - "uint_least64_t" too could be wider in general than 64 bits, > - for us both results in uint64_t exactly. > > So the above is a tie, but the ULL suffix is just nicer. (IMHO :)) Interesting discussion here: https://stackoverflow.com/questions/16360828/what-is-the-purpose-of-macros-for-minimum-width-integer-constants suggesting that these macros aren't well-specified. Ho hum. Rich.
On 14 March 2014 16:35, Laszlo Ersek <lersek@redhat.com> wrote: > On 03/14/14 17:26, Peter Maydell wrote: >> On 14 March 2014 16:17, Laszlo Ersek <lersek@redhat.com> wrote: >>> "Unsigned long long" is a gnu-ism for C89. It's a standard part of C99. >>> Last time I checked, qemu used the gnu89 dialect on all build hosts >>> except SunOS. >> >> HACKING says we use C99... > > HACKING lies then :) > > grep for "std=c99" or "std=gnu99". You will find no hits for the former, > and one hit for the latter, when the build host is SunOS. (Or just grep > for '-std='.) > > In gcc-4.8.2, -std still defaults to gnu89. HACKING says what we intend. If we need to pass an argument to gcc to get it to accept C99 constructs we should fix configure. -- PMM
On 03/14/14 17:51, Richard W.M. Jones wrote: > On Fri, Mar 14, 2014 at 05:26:06PM +0100, Laszlo Ersek wrote: >> (b) UINT64_C() is for "uint_least64_t" (7.18.4.1 Macros for >> minimum-width integer constants). "uint_least64_t" is a required type >> (7.18.1.2 Minimum-width integer types). >> >> In practice I'd say it doesn't matter which one we use: >> - ULL suffix is gnu89, >> - UINT64_C() macro is gnu89, >> - "unsigned long long" could be wider in general than 64 bits, >> - "uint_least64_t" too could be wider in general than 64 bits, >> - for us both results in uint64_t exactly. >> >> So the above is a tie, but the ULL suffix is just nicer. (IMHO :)) > > Interesting discussion here: > > https://stackoverflow.com/questions/16360828/what-is-the-purpose-of-macros-for-minimum-width-integer-constants > > suggesting that these macros aren't well-specified. Ho hum. I think they are well specified. UINT64_C(x) is the same as ((uint_least64_t)(x)), only the macro replacement text in the latter uses the appropriate suffix rather than a cast. It produces one integer constant. (Specific syntax in 6.4.4.1 Integer constants.) (Note that when I say "the same", the domain of "x" is restricted to "a decimal, octal, or hexadecimal constant [...] with a value that does not exceed the limits for the corresponding type.) *Why* someone would want to use an integer constant with type "uint_least64_t" is a separate matter. One example follows -- assume all of the below: - suppose you write portable C99 source code, - hence you can't take uint64_t for granted, - you want a constant that's otherwise small enough to be represented as "int", - but you want that constant to trigger the "usual arithmetic conversions" (see 6.3.1.8) to evaluate expressions that the constant participates in in at least 64 bits, - you want the narrowest type that allows you to do this. Consider #define MY_CONSTANT UINT64_C(123) /* ... */ { int i = ...; ... (MY_CONSTANT * i) ... } If the mathematical result of the multiplication (ie. the math product) fits in 64 value bits, then the result of the above C multiplication will be able to *represent* it. If you just write #define MY_CONSTANT2 123 /* ... */ { int i = ...; ... (MY_CONSTANT * i) ... } then this may not be the case. You ensure this "safer" representation (without explicit casts in the client expressions) by encoding the wider type in the constant itself. You could use the explicit cast there: #define MY_CONSTANT ((uint_least64_t)123) but UINT64_C() promises not to produce a cast, just an integer constant. You might care about that for whatever reason. (You could of course use #define MY_CONSTANT 123ULL too, but that wouldn't select a minimal type for your purpose, portably speaking (when you don't know much about your target platform).) I think the mechanics of UINT64_C() are well specified, just the purpose is obscure :) 7.18.4 "Macros for integer constants" itself states (about all the macros together): The following function-like macros [...] expand to integer constants suitable for initializing objects that have integer types corresponding to types defined in <stdint.h>. [...] This question could be asked in the comp.lang.c Usenet group (and then obsessed over for a few days! :)) Thanks Laszlo
On 03/14/14 18:08, Peter Maydell wrote: > On 14 March 2014 16:35, Laszlo Ersek <lersek@redhat.com> wrote: >> On 03/14/14 17:26, Peter Maydell wrote: >>> On 14 March 2014 16:17, Laszlo Ersek <lersek@redhat.com> wrote: >>>> "Unsigned long long" is a gnu-ism for C89. It's a standard part of C99. >>>> Last time I checked, qemu used the gnu89 dialect on all build hosts >>>> except SunOS. >>> >>> HACKING says we use C99... >> >> HACKING lies then :) >> >> grep for "std=c99" or "std=gnu99". You will find no hits for the former, >> and one hit for the latter, when the build host is SunOS. (Or just grep >> for '-std='.) >> >> In gcc-4.8.2, -std still defaults to gnu89. > > HACKING says what we intend. If we need to pass an argument > to gcc to get it to accept C99 constructs we should fix > configure. I agree 100%. However, it wouldn't be an immediate, transparent change. For example, out-of-range left-shifting for a signed int is explicitly undefined behavior in C99 (6.5.7p4) -- equally for shifting left a negative value -- and the argument has been made before that C89 does *not* say this. (Actually I think that it's undefined just the same in C89 -- undefined by omission. See 3.16, "... by the omission of any explicit definition of behavior...") IOW I welcome your proposal, but such a step will make patches like your own [Qemu-devel] [PATCH 00/12] Avoid shifting left into sign bit very necessary, not just a convenience to sanitize warnings that only clang emits today. In any case, I'd propose gnu99 rather than c99, because c99 might not be a superset of gnu89. In some aspects it would be a step forward, but in others it could be a step back. Gnu99 only goes forward. (Gnu99 is promised as the next gcc default.) Personal note: if qemu moves to c99 or gnu99, as per -std=, I want a personal license to use constants like 0u and 1u in the code wherever I want; no style complaints. Deal? :) Thanks Laszlo
On Fri, Mar 14, 2014 at 06:27:07PM +0100, Laszlo Ersek wrote: > *Why* someone would want to use an integer constant with type > "uint_least64_t" is a separate matter. One example follows -- assume all > of the below: > - suppose you write portable C99 source code, > - hence you can't take uint64_t for granted, > - you want a constant that's otherwise small enough to be represented as > "int", > - but you want that constant to trigger the "usual arithmetic > conversions" (see 6.3.1.8) to evaluate expressions that the constant > participates in in at least 64 bits, > - you want the narrowest type that allows you to do this. - You want the code to self-document its intentions. I don't think ULL does that because it requires people to know that ULL is at least 64 bits. Rich.
On 03/14/14 18:49, Richard W.M. Jones wrote: > On Fri, Mar 14, 2014 at 06:27:07PM +0100, Laszlo Ersek wrote: >> *Why* someone would want to use an integer constant with type >> "uint_least64_t" is a separate matter. One example follows -- assume all >> of the below: >> - suppose you write portable C99 source code, >> - hence you can't take uint64_t for granted, >> - you want a constant that's otherwise small enough to be represented as >> "int", >> - but you want that constant to trigger the "usual arithmetic >> conversions" (see 6.3.1.8) to evaluate expressions that the constant >> participates in in at least 64 bits, >> - you want the narrowest type that allows you to do this. > > - You want the code to self-document its intentions. I do. > I don't think ULL does that because it requires people to know that > ULL is at least 64 bits. Oh. I see what you mean. This never crossed my mind (= people missing the fact that unsigned long long can represent at least up to 2^64-1). Laszlo
On 14 March 2014 17:42, Laszlo Ersek <lersek@redhat.com> wrote: > However, it wouldn't be an immediate, transparent change. For example, > out-of-range left-shifting for a signed int is explicitly undefined > behavior in C99 (6.5.7p4) -- equally for shifting left a negative value > -- and the argument has been made before that C89 does *not* say this. Does gcc *actually* change its behaviour in this area depending on the stanadrd specified? thanks -- PMM
On 03/14/14 19:22, Peter Maydell wrote: > On 14 March 2014 17:42, Laszlo Ersek <lersek@redhat.com> wrote: >> However, it wouldn't be an immediate, transparent change. For example, >> out-of-range left-shifting for a signed int is explicitly undefined >> behavior in C99 (6.5.7p4) -- equally for shifting left a negative value >> -- and the argument has been made before that C89 does *not* say this. > > Does gcc *actually* change its behaviour in this area depending > on the stanadrd specified? There are at least two aspects to this question. - The first aspect is: assume that there is a silent change between C89 and C99, and gcc does implement both versions of the standard correctly. Then the silent change will affect the qemu code base. One step in the direction of auditing this is downloading <http://www.open-std.org/jtc1/sc22/wg14/www/C99RationaleV5.10.pdf>, and searching it for the string QUIET CHANGE. One good example is for 6.4.4.1 "Integer constants": QUIET CHANGE IN C99 Unsuffixed integer constants may have different types in C99 than in C89. Such constants greater than LONG_MAX are of type unsigned long in C89, but are of type long long in C99 (if long long has more range than long). I have no clue what gnu89 does. - The 2nd aspect is level of C99 support in gcc. We could be using a non-C89 language feature that has worked well in gnu89 dialect since forever. The same language feature could be broken in C99 mode on an old gcc version. Quickly skimming <http://gcc.gnu.org/c99status.html>, there are C99-related fixes that are as recent as * extended identifiers: GCC 4.1 -- we shouldn't be using those, * integer promotion rules: GCC 4.0 -- no idea about the specifics, but this is very important, * inline functions: GCC 4.3 -- "Inline function support present since at least GCC 1.21, but with major differences from C99 semantics until 4.3." For example, RHEL-5 ships gcc-4.1.2-55.el5 (and I gather that people still build upstream qemu on RHEL-5). Using a c99 dialect, inline functions could work differently between gcc-4.1 and say gcc-4.8, while using a gnu89 dialect, there's probably no difference for inline functions between gcc-4.1 and gcc-4.8. I think we should ask gcc people... and go forward to gnu99, and fix resultant bugs gradually. Laszlo
On 03/14/14 19:49, Laszlo Ersek wrote: > One good example is for 6.4.4.1 "Integer constants": > > QUIET CHANGE IN C99 > > Unsuffixed integer constants may have different types in C99 than > in C89. Such constants greater than LONG_MAX are of type unsigned > long in C89, but are of type long long in C99 (if long long has > more range than long). > > I have no clue what gnu89 does. x.c: #include <stdio.h> int main(void) { fprintf(stdout, "%u\n", (unsigned)sizeof 2147483648); return 0; } The following script: for I in c89 gnu89 c99 gnu99; do echo "==== $I ====" gcc -m32 -o x -std=$I x.c ./x done outputs: ==== c89 ==== x.c: In function 'main': x.c:6:3: warning: this decimal constant is unsigned only in ISO C90 [enabled by default] fprintf(stdout, "%u\n", (unsigned)sizeof 2147483648); ^ 4 ==== gnu89 ==== x.c: In function 'main': x.c:6:3: warning: this decimal constant is unsigned only in ISO C90 [enabled by default] fprintf(stdout, "%u\n", (unsigned)sizeof 2147483648); ^ 4 ==== c99 ==== 8 ==== gnu99 ==== 8 Laszlo
diff --git a/block/vhdx.h b/block/vhdx.h index 2acd7c2..8103d4c 100644 --- a/block/vhdx.h +++ b/block/vhdx.h @@ -61,7 +61,7 @@ /* These structures are ones that are defined in the VHDX specification * document */ -#define VHDX_FILE_SIGNATURE 0x656C696678646876 /* "vhdxfile" in ASCII */ +#define VHDX_FILE_SIGNATURE 0x656C696678646876ULL /* "vhdxfile" in ASCII */ typedef struct VHDXFileIdentifier { uint64_t signature; /* "vhdxfile" in ASCII */ uint16_t creator[256]; /* optional; utf-16 string to identify @@ -238,7 +238,7 @@ typedef struct QEMU_PACKED VHDXLogDataSector { /* upper 44 bits are the file offset in 1MB units lower 3 bits are the state other bits are reserved */ #define VHDX_BAT_STATE_BIT_MASK 0x07 -#define VHDX_BAT_FILE_OFF_MASK 0xFFFFFFFFFFF00000 /* upper 44 bits */ +#define VHDX_BAT_FILE_OFF_MASK 0xFFFFFFFFFFF00000ULL /* upper 44 bits */ typedef uint64_t VHDXBatEntry; /* ---- METADATA REGION STRUCTURES ---- */ @@ -247,7 +247,7 @@ typedef uint64_t VHDXBatEntry; #define VHDX_METADATA_MAX_ENTRIES 2047 /* not including the header */ #define VHDX_METADATA_TABLE_MAX_SIZE \ (VHDX_METADATA_ENTRY_SIZE * (VHDX_METADATA_MAX_ENTRIES+1)) -#define VHDX_METADATA_SIGNATURE 0x617461646174656D /* "metadata" in ASCII */ +#define VHDX_METADATA_SIGNATURE 0x617461646174656DULL /* "metadata" in ASCII */ typedef struct QEMU_PACKED VHDXMetadataTableHeader { uint64_t signature; /* "metadata" in ASCII */ uint16_t reserved;
On 32-bit hosts, some compilers will warn on too large integer constants for constants that are 64-bit in length. Explicitly put a 'ULL' suffix on those defines. Reported-by: Alexander Graf <agraf@suse.de> Signed-off-by: Jeff Cody <jcody@redhat.com> --- block/vhdx.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)