build: Work around SIZE_MAX bug in OSX headers

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(+)

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.