[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:58, Tim Deegan wrote:
At 09:41 +0100 on 23 Jun (1498210893), Julien Grall wrote:
Hi Jan,

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 ...

I don't personally try every single compiler every time I am writing a
patch... This is too complex given that different stakeholders (Linaro,
Debian, Ubuntu,...) provide various binaries with their own patches on top.

I asked you because I was wondering what is happening on x86 (I don't
have 4.9 x86 in hand) and to rule out a bug in the compiler provided by
Linaro.


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).
For 4.9.3 I've also specifically taken care to try not only the
x86 compiler, but also the arm32 and arm64 ones. So I'm afraid
I lack enough detail to understand what the issue is and what a
solution may look like.

I don't have much except the following error:

/tmp/test.c:6:1: error: initializer element is not constant
  mfn_t v = (const mfn_t){~0UL};
  ^

If it works for you on 4.9, then it might be a bug in the GCC provided
by Linaro and will report it.

This fails for me on x86 gcc 4.9.4, using -xc -std=gnu99.  The
complaint is valid, as Jan pointed out: the literal is a static object
and so not a valid initializer.  GCC also complains about the
'debug' version for the same reason. :(

Using a plain initializer works OK for both debug and non-debug:

  mfn_t v = {~0UL};

but I haven't checked whether other compilers like that as well.

This seems to be a bug in GCC up to 5.0:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64856

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®.