[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 01/16] xen/mm: Don't use _{g, m}fn for defining INVALID_{G, M}FN





On 23/06/17 09:30, Jan Beulich wrote:
On 22.06.17 at 20:31, <julien.grall@xxxxxxx> wrote:
Hi,

On 20/06/17 11:32, Jan Beulich wrote:
On 20.06.17 at 12:06, <tim@xxxxxxx> wrote:
At 03:36 -0600 on 20 Jun (1497929778), Jan Beulich wrote:
On 20.06.17 at 11:14, <tim@xxxxxxx> wrote:
At 01:32 -0600 on 20 Jun (1497922345), Jan Beulich wrote:
On 19.06.17 at 18:57, <julien.grall@xxxxxxx> wrote:
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -56,7 +56,7 @@

 TYPE_SAFE(unsigned long, mfn);
 #define PRI_mfn          "05lx"
-#define INVALID_MFN      _mfn(~0UL)
+#define INVALID_MFN      (mfn_t){ ~0UL }

While I don't expect anyone to wish to use a suffix expression on
this constant, for maximum compatibility this should still be fully
parenthesized, I think. Of course this should be easy enough to
do while committing.

Are you able to assure us that clang supports this gcc extension
(compound literal for non-compound types)

AIUI this is a C99 feature, not a GCCism.

Most parts of it yes (it is a gcc extension in C89 mode only), but the
specific use here isn't afaict: Compound literals outside of functions
are static objects, and hence couldn't be used as initializers of other
objects.

Ah, I see.  So would it be better to use

  #define INVALID_MFN ((const mfn_t) { ~0UL })

?

While I think we should indeed consider adding the const, the above
still is a static object, and hence still not suitable as an initializer as
per C99 or C11. But as long as gcc and clang permit it, we're fine.

Actually this solutions breaks on GCC 4.9 provided by Linaro ([1]
4.9-2016-02 and 4.9-2017.01).

This small reproducer does not compile with -std=gnu99 (used by Xen) but
compile with this option. Jan, have you tried 4.9 with this patch?

That's sort of an odd question - you've sent the patch, so I would
have expected you to have made sure it doesn't break (and
while it was me to add the const, this was discussed, and you don't
make clear whether that's the issue). In any event, I've tried ...

typedef struct
{
     unsigned long i;
} mfn_t;

mfn_t v = (const mfn_t){~0UL};

... this now with 7.1.0, 6.3.0, 5.4.0, 5.2.0, and 4.9.3, and all
of them compile this without errors or warnings (at -Wall -W).

Actually did you build with -std=gnu99? I just tried 4.9.3 for x86 and
also 4.8 for ARM64 on Ubuntu Trusty. Both are broken.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.