| Message ID | 20170530140103.GI32274@thinpad.lan.raisama.net |
|---|---|
| State | New |
| Headers | show |
On 05/30/2017 09:01 AM, Eduardo Habkost wrote: >> The OSX compiler is pickier about format strings than gcc, >> and neither "MAX_NODES" nor "MAX(anything)" are uint16_t type. > > src and dst are both uint16_t, so MAX(src, dst) is also uint16_t, > isn't it? It looks like MAX_NODES is the problem. No. MAX() invokes arithmetic (both ?: and > operators), and arithmetic involves default promotion (anything shorter than int becomes 'int'). > +++ b/numa.c > @@ -232,7 +232,7 @@ static void parse_numa_distance(NumaDistOptions *dist, Error **errp) > if (src >= MAX_NODES || dst >= MAX_NODES) { > error_setg(errp, > "Invalid node %" PRIu16 > - ", max possible could be %" PRIu16, > + ", max possible could be %d", Don't you want %u to match PRIu16, rather than %d?
On 30 May 2017 at 15:01, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, May 30, 2017 at 11:45:55AM +0100, Peter Maydell wrote: >> Hi all; I'm afraid this patch breaks compilation on OSX: > > It looks like this wasn't detected by travis-ci because the build > is not using -Werror: > https://travis-ci.org/ehabkost/qemu/jobs/236485778#L3881 Hmm. It would be nice to fix that. configure doesn't enable -Werror by default on OSX because it doesn't handle some of the pragmas to disable warnings that we use, but on the typical OSX build the warnings that we use the pragmas to suppress don't get generated anyway, so -Werror works in practice. It needs configure '--extra-cflags=-Werror' to turn on. (It might also need --disable-vnc-sasl depending on OSX version since Apple helpfully added a pile of deprecation warnings to their version of the sasl headers without suggesting what to use instead.) thanks -- PMM
On Tue, May 30, 2017 at 06:08:04PM +0100, Peter Maydell wrote: > On 30 May 2017 at 15:01, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Tue, May 30, 2017 at 11:45:55AM +0100, Peter Maydell wrote: > >> Hi all; I'm afraid this patch breaks compilation on OSX: > > > > It looks like this wasn't detected by travis-ci because the build > > is not using -Werror: > > https://travis-ci.org/ehabkost/qemu/jobs/236485778#L3881 > > Hmm. It would be nice to fix that. configure doesn't enable -Werror > by default on OSX because it doesn't handle some of the pragmas > to disable warnings that we use, but on the typical OSX build > the warnings that we use the pragmas to suppress don't get > generated anyway, so -Werror works in practice. It needs > configure '--extra-cflags=-Werror' to turn on. > (It might also need --disable-vnc-sasl depending on OSX version > since Apple helpfully added a pile of deprecation warnings to > their version of the sasl headers without suggesting what to > use instead.) The same warnings hit libvirt on OS-X too and some pragmas were sufficient to shut it up _Pragma ("GCC diagnostic push") _Pragma ("GCC diagnostic ignored \"-Wdeprecated-declarations\"") .... sasl code... _Pragma ("GCC diagnostic pop") Regards, Daniel
On Tue, May 30, 2017 at 10:28:21AM -0500, Eric Blake wrote: > On 05/30/2017 09:01 AM, Eduardo Habkost wrote: > > >> The OSX compiler is pickier about format strings than gcc, > >> and neither "MAX_NODES" nor "MAX(anything)" are uint16_t type. > > > > src and dst are both uint16_t, so MAX(src, dst) is also uint16_t, > > isn't it? It looks like MAX_NODES is the problem. > > No. MAX() invokes arithmetic (both ?: and > operators), and arithmetic > involves default promotion (anything shorter than int becomes 'int'). Oh, I didn't know that. > > > +++ b/numa.c > > @@ -232,7 +232,7 @@ static void parse_numa_distance(NumaDistOptions *dist, Error **errp) > > if (src >= MAX_NODES || dst >= MAX_NODES) { > > error_setg(errp, > > "Invalid node %" PRIu16 > > - ", max possible could be %" PRIu16, > > + ", max possible could be %d", > > Don't you want %u to match PRIu16, rather than %d? Can't this (using %u with an int argument) make more pedantic compilers complain?
On 05/30/2017 01:10 PM, Eduardo Habkost wrote: > On Tue, May 30, 2017 at 10:28:21AM -0500, Eric Blake wrote: >> On 05/30/2017 09:01 AM, Eduardo Habkost wrote: >> >>>> The OSX compiler is pickier about format strings than gcc, >>>> and neither "MAX_NODES" nor "MAX(anything)" are uint16_t type. >>> >>> src and dst are both uint16_t, so MAX(src, dst) is also uint16_t, >>> isn't it? It looks like MAX_NODES is the problem. >> >> No. MAX() invokes arithmetic (both ?: and > operators), and arithmetic >> involves default promotion (anything shorter than int becomes 'int'). > > Oh, I didn't know that. And the fact that var-arg passing REQUIRES promotion means you CAN'T tell the difference from a true uint16_t argument and an int argument which resulted from arithmetic promotion. Which is why Paolo has argued that the MacOS compiler warning is bogus because it is overly picky. But we obviously are in no position to get it changed. > >> >>> +++ b/numa.c >>> @@ -232,7 +232,7 @@ static void parse_numa_distance(NumaDistOptions *dist, Error **errp) >>> if (src >= MAX_NODES || dst >= MAX_NODES) { >>> error_setg(errp, >>> "Invalid node %" PRIu16 >>> - ", max possible could be %" PRIu16, >>> + ", max possible could be %d", >> >> Don't you want %u to match PRIu16, rather than %d? > > Can't this (using %u with an int argument) make more pedantic > compilers complain? Yes, gcc's -Wformat-signedness would (probably) flag it. But we don't use that. At any rate, ALL uint16_t values are formatted identically for both %u and %d, so as long as we have a formula for keeping picky compilers silent, I don't care beyond that point.
diff --git a/numa.c b/numa.c index ca731455e9..df62b55a00 100644 --- a/numa.c +++ b/numa.c @@ -232,7 +232,7 @@ static void parse_numa_distance(NumaDistOptions *dist, Error **errp) if (src >= MAX_NODES || dst >= MAX_NODES) { error_setg(errp, "Invalid node %" PRIu16 - ", max possible could be %" PRIu16, + ", max possible could be %d", MAX(src, dst), MAX_NODES); return; }