| Message ID | 1468336912-20396-1-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/12/2016 09:21 AM, Eric Blake wrote: > C99 requires SIZE_MAX to be declared with the same type as the > integral promotion of size_t, but OSX mistakenly defines it as > an unsigned long long expression even though size_t is only > unsigned long. Rather than futzing around with whether size_t > is 32- or 64-bits wide, let the compiler get the right type > for us by virtue of integer promotion. > > See also https://patchwork.ozlabs.org/patch/542327/ for an > instance where the wrong type trips us up if we don't fix it > for good in osdep.h. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > I can't test this on OSX, but I _did_ test that if I remove the > '#ifdef __APPLE__' conditional (and just blindly do the redefine), > things still compile on Linux (which means I got the type > computation correct). > > This is my response to > https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg02276.html > > +/* Mac OSX has a <stdint.h> bug that incorrectly defines SIZE_MAX with > + * the wrong type */ > +#ifdef __APPLE__ > +#undef SIZE_MAX > +#define SIZE_MAX ((sizeof(char)) * -1) I guess I over-parenthesized that, but I'm not sure if that alone is worth a respin. This violates POSIX, which requires that: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdint.h.html#tag_13_48 "Each instance of these macros shall be replaced by a constant expression suitable for use in #if preprocessing directives, and this expression shall have the same type as would an expression that is an object of the corresponding type converted according to the integer promotions." That is, it is valid C to write '#if SIZE_MAX == 0xffffffff', while my replacement fails that test: foo.c:6:26: error: missing binary operator before token "(" #define SIZE_MAX ((sizeof(char)) * -1) ^ foo.c:7:5: note: in expansion of macro ‘SIZE_MAX’ On the other hand, we don't have anything in current qemu sources that uses SIZE_MAX inside #if. I'd rather not worry about #if directives on a respin (other than to enhance the commit message to document it as an intentional violation), unless someone argues otherwise, because writing an expression that is a preprocessor constant requires configure-time probing to determine the type of ssize_t, which is a more complex task than the simpler compile-time probing I did here. But I'll wait for a review before deciding if a v2 is even needed (even if only to drop a redundant ()). Oh, and by complete coincidence, I learned via a libvirt build failure today that glibc has a similar bug to the OSX bug addressed here, but in <limits.h> for SSIZE_MAX, and only on 32-bit platforms: https://sourceware.org/bugzilla/show_bug.cgi?id=13575
On 12 July 2016 at 19:23, Eric Blake <eblake@redhat.com> wrote: > This violates POSIX, which requires that: > http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdint.h.html#tag_13_48 > "Each instance of these macros shall be replaced by a constant > expression suitable for use in #if preprocessing directives, and this > expression shall have the same type as would an expression that is an > object of the corresponding type converted according to the integer > promotions." > > That is, it is valid C to write '#if SIZE_MAX == 0xffffffff', while my > replacement fails that test: > > foo.c:6:26: error: missing binary operator before token "(" > #define SIZE_MAX ((sizeof(char)) * -1) > ^ > foo.c:7:5: note: in expansion of macro ‘SIZE_MAX’ I tested this patch with a compile on OSX, and it does compile without warnings or errors. (NB: haven't tested that it fixes the warning that was being complained about in the other patchset.) I don't have a very strong opinion about whether it's the best fix, but a couple of thoughts: * my inclination is to prefer not to override system headers except where we've checked and know they're broken (ie in a future world where Apple get their headers right I'd rather we automatically ended up using their version rather than ours) * we don't have any #if ...SIZE_MAX, but we do have some for other kinds of _MAX constant. thanks -- PMM
On 07/12/2016 01:35 PM, Peter Maydell wrote: > I tested this patch with a compile on OSX, and it does compile > without warnings or errors. (NB: haven't tested that it > fixes the warning that was being complained about in the > other patchset.) Ultimately, the combination of this patch plus the block patchset in question is where we prove if it makes a positive difference, so if you do have a chance to test it on OSX, that would be nice. And that's true whether we keep this version of the patch (with #if __APPLE__) or do a second version based on a witness set at configure time, because the interaction you're testing is if the replacement #define SIZE_MAX silences the warning about printf(%zd). > > I don't have a very strong opinion about whether it's the > best fix, but a couple of thoughts: > * my inclination is to prefer not to override system > headers except where we've checked and know they're broken > (ie in a future world where Apple get their headers right > I'd rather we automatically ended up using their version > rather than ours) Indeed, a configure-based solution would avoid checkpatch.pl griping about adding an #if __APPLE__. And since glibc has the same bug with SSIZE_MAX, a configure-based solution would be easier to copy-and-paste if we needed to work around that too (then again, we don't have any use of SSIZE_MAX at the moment). > * we don't have any #if ...SIZE_MAX, but we do have some > for other kinds of _MAX constant. I'll wait a bit longer to see if any other opinions surface, but sounds like I'll probably be doing a v2 and trying for a configure solution.
Peter Maydell <peter.maydell@linaro.org> writes: > On 12 July 2016 at 19:23, Eric Blake <eblake@redhat.com> wrote: >> This violates POSIX, which requires that: >> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdint.h.html#tag_13_48 >> "Each instance of these macros shall be replaced by a constant >> expression suitable for use in #if preprocessing directives, and this >> expression shall have the same type as would an expression that is an >> object of the corresponding type converted according to the integer >> promotions." >> >> That is, it is valid C to write '#if SIZE_MAX == 0xffffffff', while my >> replacement fails that test: >> >> foo.c:6:26: error: missing binary operator before token "(" >> #define SIZE_MAX ((sizeof(char)) * -1) >> ^ >> foo.c:7:5: note: in expansion of macro ‘SIZE_MAX’ > > I tested this patch with a compile on OSX, and it does compile > without warnings or errors. (NB: haven't tested that it > fixes the warning that was being complained about in the > other patchset.) > > I don't have a very strong opinion about whether it's the > best fix, but a couple of thoughts: > * my inclination is to prefer not to override system > headers except where we've checked and know they're broken > (ie in a future world where Apple get their headers right > I'd rather we automatically ended up using their version > rather than ours) That's a good point. > * we don't have any #if ...SIZE_MAX, but we do have some > for other kinds of _MAX constant.
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index 7361006..9b4b2d3 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -141,6 +141,13 @@ extern int daemon(int, int); # error Unknown pointer size #endif +/* Mac OSX has a <stdint.h> bug that incorrectly defines SIZE_MAX with + * the wrong type */ +#ifdef __APPLE__ +#undef SIZE_MAX +#define SIZE_MAX ((sizeof(char)) * -1) +#endif + #ifndef MIN #define MIN(a, b) (((a) < (b)) ? (a) : (b)) #endif
C99 requires SIZE_MAX to be declared with the same type as the integral promotion of size_t, but OSX mistakenly defines it as an unsigned long long expression even though size_t is only unsigned long. Rather than futzing around with whether size_t is 32- or 64-bits wide, let the compiler get the right type for us by virtue of integer promotion. See also https://patchwork.ozlabs.org/patch/542327/ for an instance where the wrong type trips us up if we don't fix it for good in osdep.h. Signed-off-by: Eric Blake <eblake@redhat.com> --- I can't test this on OSX, but I _did_ test that if I remove the '#ifdef __APPLE__' conditional (and just blindly do the redefine), things still compile on Linux (which means I got the type computation correct). This is my response to https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg02276.html include/qemu/osdep.h | 7 +++++++ 1 file changed, 7 insertions(+)