* [PATCH 1/8] xen/lib: Fix ASSERT() to build with clang
2016-02-09 20:01 [PATCH 0/8] xen/x86: Fix build with Clang 3.5 Andrew Cooper
@ 2016-02-09 20:01 ` Andrew Cooper
2016-02-09 20:01 ` [PATCH 2/8] xen/misc: Remove or annotate possibly-unused functions Andrew Cooper
` (8 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Andrew Cooper @ 2016-02-09 20:01 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Tim Deegan, Ian Campbell, Jan Beulich
Clang warns about a semicolon immediately following an if() clause as a
possible mistake, and recommends putting the semicolon on a new line if it was
intentional. A newline is not an option here, so use a set of empty braces
instead.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
---
xen/include/xen/lib.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 4258912..76232b2 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -32,7 +32,7 @@
#define ASSERT_UNREACHABLE() assert_failed("unreachable")
#define debug_build() 1
#else
-#define ASSERT(p) do { if ( 0 && (p) ); } while (0)
+#define ASSERT(p) do { if ( 0 && (p) ){} } while (0)
#define ASSERT_UNREACHABLE() do { } while (0)
#define debug_build() 0
#endif
--
2.1.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 2/8] xen/misc: Remove or annotate possibly-unused functions
2016-02-09 20:01 [PATCH 0/8] xen/x86: Fix build with Clang 3.5 Andrew Cooper
2016-02-09 20:01 ` [PATCH 1/8] xen/lib: Fix ASSERT() to build with clang Andrew Cooper
@ 2016-02-09 20:01 ` Andrew Cooper
2016-02-10 10:42 ` Tim Deegan
2016-02-10 13:06 ` Jan Beulich
2016-02-09 20:01 ` [PATCH 3/8] xen/x86: Remove %z modifier from inline assembly Andrew Cooper
` (7 subsequent siblings)
9 siblings, 2 replies; 26+ messages in thread
From: Andrew Cooper @ 2016-02-09 20:01 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Tim Deegan, Ian Campbell, Jan Beulich
Clang notices more unused functions than GCC.
* sh_next_page() is only used at GUEST_PAGING_LEVELS=2, so remove it from the
other guest level translation units
* rcu_batch_after() is completely unused.
* Various of the COMPAT() generated functions are used only for their
BUILD_BUG_ON() properties. Annotate them all as __maybe_used.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
---
xen/arch/x86/mm/shadow/multi.c | 2 ++
xen/common/rcupdate.c | 6 ------
xen/include/xen/compat.h | 14 ++++++++------
3 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 4d0b317..aaf8db7 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -436,6 +436,7 @@ sh_cmpxchg_guest_entry(struct vcpu *v, intpte_t *p, intpte_t *old,
* space.)
*/
+#if GUEST_PAGING_LEVELS == 2
/* From one page of a multi-page shadow, find the next one */
static inline mfn_t sh_next_page(mfn_t smfn)
{
@@ -454,6 +455,7 @@ static inline mfn_t sh_next_page(mfn_t smfn)
ASSERT(!next->u.sh.head);
return page_to_mfn(next);
}
+#endif
static inline u32
guest_index(void *ptr)
diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
index f13b87b..8cc5a82 100644
--- a/xen/common/rcupdate.c
+++ b/xen/common/rcupdate.c
@@ -141,12 +141,6 @@ static inline int rcu_batch_before(long a, long b)
return (a - b) < 0;
}
-/* Is batch a after batch b ? */
-static inline int rcu_batch_after(long a, long b)
-{
- return (a - b) > 0;
-}
-
static void force_quiescent_state(struct rcu_data *rdp,
struct rcu_ctrlblk *rcp)
{
diff --git a/xen/include/xen/compat.h b/xen/include/xen/compat.h
index e5c23e2..3f4cef6 100644
--- a/xen/include/xen/compat.h
+++ b/xen/include/xen/compat.h
@@ -134,14 +134,16 @@
#define CHECK_NAME_(k, n, tag) __check ## tag ## k ## _ ## n
#define CHECK_TYPE(name) \
-static inline int CHECK_NAME(name, T)(xen_ ## name ## _t *x, \
- compat_ ## name ## _t *c) \
+static inline int __maybe_unused \
+CHECK_NAME(name, T)(xen_ ## name ## _t *x, \
+ compat_ ## name ## _t *c) \
{ \
return x == c; \
}
#define CHECK_TYPE_(k, n) \
-static inline int CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
- k compat_ ## n *c) \
+static inline int __maybe_unused \
+CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
+ k compat_ ## n *c) \
{ \
return x == c; \
}
@@ -154,14 +156,14 @@ static inline int CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
sizeof(k compat_ ## n)) * 2]
#define CHECK_FIELD_COMMON(name, t, f) \
-static inline int name(xen_ ## t ## _t *x, compat_ ## t ## _t *c) \
+static inline int __maybe_unused name(xen_ ## t ## _t *x, compat_ ## t ## _t *c) \
{ \
BUILD_BUG_ON(offsetof(xen_ ## t ## _t, f) != \
offsetof(compat_ ## t ## _t, f)); \
return &x->f == &c->f; \
}
#define CHECK_FIELD_COMMON_(k, name, n, f) \
-static inline int name(k xen_ ## n *x, k compat_ ## n *c) \
+static inline int __maybe_unused name(k xen_ ## n *x, k compat_ ## n *c) \
{ \
BUILD_BUG_ON(offsetof(k xen_ ## n, f) != \
offsetof(k compat_ ## n, f)); \
--
2.1.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 2/8] xen/misc: Remove or annotate possibly-unused functions
2016-02-09 20:01 ` [PATCH 2/8] xen/misc: Remove or annotate possibly-unused functions Andrew Cooper
@ 2016-02-10 10:42 ` Tim Deegan
2016-02-10 13:06 ` Jan Beulich
1 sibling, 0 replies; 26+ messages in thread
From: Tim Deegan @ 2016-02-10 10:42 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Ian Campbell, Jan Beulich, Xen-devel
At 20:01 +0000 on 09 Feb (1455048102), Andrew Cooper wrote:
> Clang notices more unused functions than GCC.
>
> * sh_next_page() is only used at GUEST_PAGING_LEVELS=2, so remove it from the
> other guest level translation units
> * rcu_batch_after() is completely unused.
> * Various of the COMPAT() generated functions are used only for their
> BUILD_BUG_ON() properties. Annotate them all as __maybe_used.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Shadow stuff Acked-by: Tim Deegan <tim@xen.org>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/8] xen/misc: Remove or annotate possibly-unused functions
2016-02-09 20:01 ` [PATCH 2/8] xen/misc: Remove or annotate possibly-unused functions Andrew Cooper
2016-02-10 10:42 ` Tim Deegan
@ 2016-02-10 13:06 ` Jan Beulich
2016-02-10 13:15 ` Andrew Cooper
1 sibling, 1 reply; 26+ messages in thread
From: Jan Beulich @ 2016-02-10 13:06 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Tim Deegan, Ian Campbell, Xen-devel
>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/common/rcupdate.c
> +++ b/xen/common/rcupdate.c
> @@ -141,12 +141,6 @@ static inline int rcu_batch_before(long a, long b)
> return (a - b) < 0;
> }
>
> -/* Is batch a after batch b ? */
> -static inline int rcu_batch_after(long a, long b)
> -{
> - return (a - b) > 0;
> -}
To me it is the nature of inline functions that they may or may not be
used, regardless of whether they live in a header file (where I would
have supposed Clang won't warn about, but the change below makes
me assume I'm wrong) or in a source file.
> --- a/xen/include/xen/compat.h
> +++ b/xen/include/xen/compat.h
> @@ -134,14 +134,16 @@
> #define CHECK_NAME_(k, n, tag) __check ## tag ## k ## _ ## n
>
> #define CHECK_TYPE(name) \
> -static inline int CHECK_NAME(name, T)(xen_ ## name ## _t *x, \
> - compat_ ## name ## _t *c) \
> +static inline int __maybe_unused \
> +CHECK_NAME(name, T)(xen_ ## name ## _t *x, \
> + compat_ ## name ## _t *c) \
> { \
> return x == c; \
> }
> #define CHECK_TYPE_(k, n) \
> -static inline int CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
> - k compat_ ## n *c) \
> +static inline int __maybe_unused \
> +CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
> + k compat_ ## n *c) \
> { \
> return x == c; \
> }
> @@ -154,14 +156,14 @@ static inline int CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
> sizeof(k compat_ ## n)) * 2]
>
> #define CHECK_FIELD_COMMON(name, t, f) \
> -static inline int name(xen_ ## t ## _t *x, compat_ ## t ## _t *c) \
> +static inline int __maybe_unused name(xen_ ## t ## _t *x, compat_ ## t ## _t *c) \
> { \
> BUILD_BUG_ON(offsetof(xen_ ## t ## _t, f) != \
> offsetof(compat_ ## t ## _t, f)); \
> return &x->f == &c->f; \
> }
> #define CHECK_FIELD_COMMON_(k, name, n, f) \
> -static inline int name(k xen_ ## n *x, k compat_ ## n *c) \
> +static inline int __maybe_unused name(k xen_ ## n *x, k compat_ ## n *c) \
> { \
> BUILD_BUG_ON(offsetof(k xen_ ## n, f) != \
> offsetof(k compat_ ## n, f)); \
So if these are all noticed to be unused, why would others in other
header files not be? I think there's at the very least some aspect
missing in the description, explaining what makes these stand out
from the others.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 2/8] xen/misc: Remove or annotate possibly-unused functions
2016-02-10 13:06 ` Jan Beulich
@ 2016-02-10 13:15 ` Andrew Cooper
0 siblings, 0 replies; 26+ messages in thread
From: Andrew Cooper @ 2016-02-10 13:15 UTC (permalink / raw)
To: Jan Beulich; +Cc: Tim Deegan, Ian Campbell, Xen-devel
On 10/02/16 13:06, Jan Beulich wrote:
>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/common/rcupdate.c
>> +++ b/xen/common/rcupdate.c
>> @@ -141,12 +141,6 @@ static inline int rcu_batch_before(long a, long b)
>> return (a - b) < 0;
>> }
>>
>> -/* Is batch a after batch b ? */
>> -static inline int rcu_batch_after(long a, long b)
>> -{
>> - return (a - b) > 0;
>> -}
> To me it is the nature of inline functions that they may or may not be
> used, regardless of whether they live in a header file (where I would
> have supposed Clang won't warn about, but the change below makes
> me assume I'm wrong) or in a source file.
>
>> --- a/xen/include/xen/compat.h
>> +++ b/xen/include/xen/compat.h
>> @@ -134,14 +134,16 @@
>> #define CHECK_NAME_(k, n, tag) __check ## tag ## k ## _ ## n
>>
>> #define CHECK_TYPE(name) \
>> -static inline int CHECK_NAME(name, T)(xen_ ## name ## _t *x, \
>> - compat_ ## name ## _t *c) \
>> +static inline int __maybe_unused \
>> +CHECK_NAME(name, T)(xen_ ## name ## _t *x, \
>> + compat_ ## name ## _t *c) \
>> { \
>> return x == c; \
>> }
>> #define CHECK_TYPE_(k, n) \
>> -static inline int CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
>> - k compat_ ## n *c) \
>> +static inline int __maybe_unused \
>> +CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
>> + k compat_ ## n *c) \
>> { \
>> return x == c; \
>> }
>> @@ -154,14 +156,14 @@ static inline int CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
>> sizeof(k compat_ ## n)) * 2]
>>
>> #define CHECK_FIELD_COMMON(name, t, f) \
>> -static inline int name(xen_ ## t ## _t *x, compat_ ## t ## _t *c) \
>> +static inline int __maybe_unused name(xen_ ## t ## _t *x, compat_ ## t ## _t *c) \
>> { \
>> BUILD_BUG_ON(offsetof(xen_ ## t ## _t, f) != \
>> offsetof(compat_ ## t ## _t, f)); \
>> return &x->f == &c->f; \
>> }
>> #define CHECK_FIELD_COMMON_(k, name, n, f) \
>> -static inline int name(k xen_ ## n *x, k compat_ ## n *c) \
>> +static inline int __maybe_unused name(k xen_ ## n *x, k compat_ ## n *c) \
>> { \
>> BUILD_BUG_ON(offsetof(k xen_ ## n, f) != \
>> offsetof(k compat_ ## n, f)); \
> So if these are all noticed to be unused, why would others in other
> header files not be?
Because they are instantiated in translation units, with no callers, by
code like common/trace.c:
#ifdef CONFIG_COMPAT
#include <compat/trace.h>
#define xen_t_buf t_buf
CHECK_t_buf;
#undef xen_t_buf
#else
#define compat_t_rec t_rec
#endif
This was the first example which blew up.
~Andrew
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/8] xen/x86: Remove %z modifier from inline assembly
2016-02-09 20:01 [PATCH 0/8] xen/x86: Fix build with Clang 3.5 Andrew Cooper
2016-02-09 20:01 ` [PATCH 1/8] xen/lib: Fix ASSERT() to build with clang Andrew Cooper
2016-02-09 20:01 ` [PATCH 2/8] xen/misc: Remove or annotate possibly-unused functions Andrew Cooper
@ 2016-02-09 20:01 ` Andrew Cooper
2016-02-10 13:10 ` Jan Beulich
2016-02-09 20:01 ` [PATCH 4/8] xen/x86: Fix section type mismatch in mm.c Andrew Cooper
` (6 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Andrew Cooper @ 2016-02-09 20:01 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
Clang doesn't support the %z modifier. Replace both uses with an explicit l
suffix, and cover the changes with BUILD_BUG_ON()s
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/xstate.c | 3 ++-
xen/include/asm-x86/spinlock.h | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index c5d17ff..4f2fb8e 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -353,11 +353,12 @@ void xrstor(struct vcpu *v, uint64_t mask)
{
switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
{
+ BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */
#define XRSTOR(pfx) \
alternative_io("1: .byte " pfx "0x0f,0xae,0x2f\n" \
"3:\n" \
" .section .fixup,\"ax\"\n" \
- "2: inc%z[faults] %[faults]\n" \
+ "2: incl %[faults]\n" \
" jmp 3b\n" \
" .previous\n" \
_ASM_EXTABLE(1b, 2b), \
diff --git a/xen/include/asm-x86/spinlock.h b/xen/include/asm-x86/spinlock.h
index 70a85af..be72c0f 100644
--- a/xen/include/asm-x86/spinlock.h
+++ b/xen/include/asm-x86/spinlock.h
@@ -2,7 +2,8 @@
#define __ASM_SPINLOCK_H
#define _raw_read_unlock(l) \
- asm volatile ( "lock; dec%z0 %0" : "+m" ((l)->lock) :: "memory" )
+ BUILD_BUG_ON(sizeof((l)->lock) != 4); /* Clang doesn't support %z in asm. */ \
+ asm volatile ( "lock; decl %0" : "+m" ((l)->lock) :: "memory" )
/*
* On x86 the only reordering is of reads with older writes. In the
--
2.1.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 4/8] xen/x86: Fix section type mismatch in mm.c
2016-02-09 20:01 [PATCH 0/8] xen/x86: Fix build with Clang 3.5 Andrew Cooper
` (2 preceding siblings ...)
2016-02-09 20:01 ` [PATCH 3/8] xen/x86: Remove %z modifier from inline assembly Andrew Cooper
@ 2016-02-09 20:01 ` Andrew Cooper
2016-02-10 10:01 ` George Dunlap
2016-02-09 20:01 ` [PATCH 5/8] xen/x86: Improve annotation of autogen_entrypoints[] Andrew Cooper
` (5 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Andrew Cooper @ 2016-02-09 20:01 UTC (permalink / raw)
To: Xen-devel; +Cc: George Dunlap, Andrew Cooper, Jan Beulich
Clang doesn't like mixing const and non-const data in the same section. Move
zero_page into .bss.page_aligned.const and wildcard .bss.page_aligned when
linking.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
xen/arch/x86/mm.c | 2 +-
xen/arch/x86/xen.lds.S | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d62d9ec..c214fc5 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -577,7 +577,7 @@ static inline void guest_get_eff_kern_l1e(struct vcpu *v, unsigned long addr,
TOGGLE_MODE();
}
-static const char __section(".bss.page_aligned") zero_page[PAGE_SIZE];
+static const char __section(".bss.page_aligned.const") zero_page[PAGE_SIZE];
static void invalidate_shadow_ldt(struct vcpu *v, int flush)
{
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index c1ce027..3b199ca 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -170,7 +170,7 @@ SECTIONS
__bss_start = .;
*(.bss.stack_aligned)
. = ALIGN(PAGE_SIZE);
- *(.bss.page_aligned)
+ *(.bss.page_aligned*)
*(.bss)
. = ALIGN(SMP_CACHE_BYTES);
__per_cpu_start = .;
--
2.1.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 4/8] xen/x86: Fix section type mismatch in mm.c
2016-02-09 20:01 ` [PATCH 4/8] xen/x86: Fix section type mismatch in mm.c Andrew Cooper
@ 2016-02-10 10:01 ` George Dunlap
0 siblings, 0 replies; 26+ messages in thread
From: George Dunlap @ 2016-02-10 10:01 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: George Dunlap, Jan Beulich
On 09/02/16 20:01, Andrew Cooper wrote:
> Clang doesn't like mixing const and non-const data in the same section. Move
> zero_page into .bss.page_aligned.const and wildcard .bss.page_aligned when
> linking.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> xen/arch/x86/mm.c | 2 +-
> xen/arch/x86/xen.lds.S | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index d62d9ec..c214fc5 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -577,7 +577,7 @@ static inline void guest_get_eff_kern_l1e(struct vcpu *v, unsigned long addr,
> TOGGLE_MODE();
> }
>
> -static const char __section(".bss.page_aligned") zero_page[PAGE_SIZE];
> +static const char __section(".bss.page_aligned.const") zero_page[PAGE_SIZE];
>
> static void invalidate_shadow_ldt(struct vcpu *v, int flush)
> {
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index c1ce027..3b199ca 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -170,7 +170,7 @@ SECTIONS
> __bss_start = .;
> *(.bss.stack_aligned)
> . = ALIGN(PAGE_SIZE);
> - *(.bss.page_aligned)
> + *(.bss.page_aligned*)
> *(.bss)
> . = ALIGN(SMP_CACHE_BYTES);
> __per_cpu_start = .;
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 5/8] xen/x86: Improve annotation of autogen_entrypoints[]
2016-02-09 20:01 [PATCH 0/8] xen/x86: Fix build with Clang 3.5 Andrew Cooper
` (3 preceding siblings ...)
2016-02-09 20:01 ` [PATCH 4/8] xen/x86: Fix section type mismatch in mm.c Andrew Cooper
@ 2016-02-09 20:01 ` Andrew Cooper
2016-02-09 20:01 ` [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays Andrew Cooper
` (4 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Andrew Cooper @ 2016-02-09 20:01 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
Clang complains that the __used attribute is not applicable to an extern. In
this case, the only relevent attribute is that the data is constant.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/traps.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e105b95..d19250a 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -3689,7 +3689,7 @@ void __init init_idt_traps(void)
this_cpu(compat_gdt_table) = boot_cpu_compat_gdt_table;
}
-extern void (*__initconst autogen_entrypoints[NR_VECTORS])(void);
+extern void (*const autogen_entrypoints[NR_VECTORS])(void);
void __init trap_init(void)
{
unsigned int vector;
--
2.1.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays
2016-02-09 20:01 [PATCH 0/8] xen/x86: Fix build with Clang 3.5 Andrew Cooper
` (4 preceding siblings ...)
2016-02-09 20:01 ` [PATCH 5/8] xen/x86: Improve annotation of autogen_entrypoints[] Andrew Cooper
@ 2016-02-09 20:01 ` Andrew Cooper
2016-02-10 10:11 ` George Dunlap
` (2 more replies)
2016-02-09 20:01 ` [PATCH 7/8] xen/x86: Fix get_cpu_info() when built with clang Andrew Cooper
` (3 subsequent siblings)
9 siblings, 3 replies; 26+ messages in thread
From: Andrew Cooper @ 2016-02-09 20:01 UTC (permalink / raw)
To: Xen-devel
Cc: George Dunlap, Andrew Cooper, Kevin Tian, Jun Nakajima, Jan Beulich
Clang objects to having multiple initialisers when creating an array.
As this warning is useful for spotting obscure bugs, disabling it is
unhelpful. Instead, fix our two deliberate usecases.
In the p2m-ept case, pull the array out into a helper function, so the helper
can guarentee to cover the NULL pointer case.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
xen/arch/x86/cpu/mtrr/generic.c | 3 +--
xen/arch/x86/mm/p2m-ept.c | 27 ++++++++++++++++-----------
2 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c
index ea0efe2..8839f8d 100644
--- a/xen/arch/x86/cpu/mtrr/generic.c
+++ b/xen/arch/x86/cpu/mtrr/generic.c
@@ -91,7 +91,6 @@ static const char *__init mtrr_attrib_to_str(mtrr_type x)
{
static const char __initconst strings[MTRR_NUM_TYPES][16] =
{
- [0 ... MTRR_NUM_TYPES - 1] = "?",
[MTRR_TYPE_UNCACHABLE] = "uncachable",
[MTRR_TYPE_WRCOMB] = "write-combining",
[MTRR_TYPE_WRTHROUGH] = "write-through",
@@ -99,7 +98,7 @@ static const char *__init mtrr_attrib_to_str(mtrr_type x)
[MTRR_TYPE_WRBACK] = "write-back",
};
- return x < MTRR_NUM_TYPES ? strings[x] : "?";
+ return x < MTRR_NUM_TYPES ? (strings[x] ?: "?") : "?";
}
static unsigned int __initdata last_fixed_start;
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index c094320..c5fe906 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m)
free_cpumask_var(ept->invalidate);
}
+static const char *memory_type_to_str(unsigned int x)
+{
+ static const char memory_types[8][2] = {
+ [MTRR_TYPE_UNCACHABLE] = "UC",
+ [MTRR_TYPE_WRCOMB] = "WC",
+ [MTRR_TYPE_WRTHROUGH] = "WT",
+ [MTRR_TYPE_WRPROT] = "WP",
+ [MTRR_TYPE_WRBACK] = "WB",
+ [MTRR_NUM_TYPES] = "??"
+ };
+
+ return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?";
+}
+
static void ept_dump_p2m_table(unsigned char key)
{
struct domain *d;
@@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key)
unsigned long record_counter = 0;
struct p2m_domain *p2m;
struct ept_data *ept;
- static const char memory_types[8][2] = {
- [0 ... 7] = "?",
- [MTRR_TYPE_UNCACHABLE] = "UC",
- [MTRR_TYPE_WRCOMB] = "WC",
- [MTRR_TYPE_WRTHROUGH] = "WT",
- [MTRR_TYPE_WRPROT] = "WP",
- [MTRR_TYPE_WRBACK] = "WB",
- [MTRR_NUM_TYPES] = "??"
- };
for_each_domain(d)
{
@@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key)
ept_entry->r ? 'r' : ' ',
ept_entry->w ? 'w' : ' ',
ept_entry->x ? 'x' : ' ',
- memory_types[ept_entry->emt][0],
- memory_types[ept_entry->emt][1]
+ memory_type_to_str(ept_entry->emt)[0],
+ memory_type_to_str(ept_entry->emt)[1]
?: ept_entry->emt + '0',
c ?: ept_entry->ipat ? '!' : ' ');
--
2.1.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays
2016-02-09 20:01 ` [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays Andrew Cooper
@ 2016-02-10 10:11 ` George Dunlap
2016-02-10 13:22 ` Jan Beulich
2016-02-16 7:06 ` Tian, Kevin
2 siblings, 0 replies; 26+ messages in thread
From: George Dunlap @ 2016-02-10 10:11 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: George Dunlap, Kevin Tian, Jun Nakajima, Jan Beulich
On 09/02/16 20:01, Andrew Cooper wrote:
> Clang objects to having multiple initialisers when creating an array.
>
> As this warning is useful for spotting obscure bugs, disabling it is
> unhelpful. Instead, fix our two deliberate usecases.
>
> In the p2m-ept case, pull the array out into a helper function, so the helper
> can guarentee to cover the NULL pointer case.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> ---
> xen/arch/x86/cpu/mtrr/generic.c | 3 +--
> xen/arch/x86/mm/p2m-ept.c | 27 ++++++++++++++++-----------
> 2 files changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c
> index ea0efe2..8839f8d 100644
> --- a/xen/arch/x86/cpu/mtrr/generic.c
> +++ b/xen/arch/x86/cpu/mtrr/generic.c
> @@ -91,7 +91,6 @@ static const char *__init mtrr_attrib_to_str(mtrr_type x)
> {
> static const char __initconst strings[MTRR_NUM_TYPES][16] =
> {
> - [0 ... MTRR_NUM_TYPES - 1] = "?",
> [MTRR_TYPE_UNCACHABLE] = "uncachable",
> [MTRR_TYPE_WRCOMB] = "write-combining",
> [MTRR_TYPE_WRTHROUGH] = "write-through",
> @@ -99,7 +98,7 @@ static const char *__init mtrr_attrib_to_str(mtrr_type x)
> [MTRR_TYPE_WRBACK] = "write-back",
> };
>
> - return x < MTRR_NUM_TYPES ? strings[x] : "?";
> + return x < MTRR_NUM_TYPES ? (strings[x] ?: "?") : "?";
> }
>
> static unsigned int __initdata last_fixed_start;
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index c094320..c5fe906 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m)
> free_cpumask_var(ept->invalidate);
> }
>
> +static const char *memory_type_to_str(unsigned int x)
> +{
> + static const char memory_types[8][2] = {
> + [MTRR_TYPE_UNCACHABLE] = "UC",
> + [MTRR_TYPE_WRCOMB] = "WC",
> + [MTRR_TYPE_WRTHROUGH] = "WT",
> + [MTRR_TYPE_WRPROT] = "WP",
> + [MTRR_TYPE_WRBACK] = "WB",
> + [MTRR_NUM_TYPES] = "??"
> + };
> +
> + return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?";
> +}
> +
> static void ept_dump_p2m_table(unsigned char key)
> {
> struct domain *d;
> @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key)
> unsigned long record_counter = 0;
> struct p2m_domain *p2m;
> struct ept_data *ept;
> - static const char memory_types[8][2] = {
> - [0 ... 7] = "?",
> - [MTRR_TYPE_UNCACHABLE] = "UC",
> - [MTRR_TYPE_WRCOMB] = "WC",
> - [MTRR_TYPE_WRTHROUGH] = "WT",
> - [MTRR_TYPE_WRPROT] = "WP",
> - [MTRR_TYPE_WRBACK] = "WB",
> - [MTRR_NUM_TYPES] = "??"
> - };
>
> for_each_domain(d)
> {
> @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key)
> ept_entry->r ? 'r' : ' ',
> ept_entry->w ? 'w' : ' ',
> ept_entry->x ? 'x' : ' ',
> - memory_types[ept_entry->emt][0],
> - memory_types[ept_entry->emt][1]
> + memory_type_to_str(ept_entry->emt)[0],
> + memory_type_to_str(ept_entry->emt)[1]
> ?: ept_entry->emt + '0',
> c ?: ept_entry->ipat ? '!' : ' ');
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays
2016-02-09 20:01 ` [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays Andrew Cooper
2016-02-10 10:11 ` George Dunlap
@ 2016-02-10 13:22 ` Jan Beulich
2016-02-10 13:50 ` Andrew Cooper
2016-02-16 7:06 ` Tian, Kevin
2 siblings, 1 reply; 26+ messages in thread
From: Jan Beulich @ 2016-02-10 13:22 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, Kevin Tian, JunNakajima, Xen-devel
>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
> Clang objects to having multiple initialisers when creating an array.
>
> As this warning is useful for spotting obscure bugs, disabling it is
> unhelpful. Instead, fix our two deliberate usecases.
Ugly again, but - well ...
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m)
> free_cpumask_var(ept->invalidate);
> }
>
> +static const char *memory_type_to_str(unsigned int x)
> +{
> + static const char memory_types[8][2] = {
> + [MTRR_TYPE_UNCACHABLE] = "UC",
> + [MTRR_TYPE_WRCOMB] = "WC",
> + [MTRR_TYPE_WRTHROUGH] = "WT",
> + [MTRR_TYPE_WRPROT] = "WP",
> + [MTRR_TYPE_WRBACK] = "WB",
> + [MTRR_NUM_TYPES] = "??"
> + };
> +
> + return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?";
I think this should really ASSERT() the first condition.
> @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key)
> unsigned long record_counter = 0;
> struct p2m_domain *p2m;
> struct ept_data *ept;
> - static const char memory_types[8][2] = {
> - [0 ... 7] = "?",
> - [MTRR_TYPE_UNCACHABLE] = "UC",
> - [MTRR_TYPE_WRCOMB] = "WC",
> - [MTRR_TYPE_WRTHROUGH] = "WT",
> - [MTRR_TYPE_WRPROT] = "WP",
> - [MTRR_TYPE_WRBACK] = "WB",
> - [MTRR_NUM_TYPES] = "??"
> - };
>
> for_each_domain(d)
> {
> @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key)
> ept_entry->r ? 'r' : ' ',
> ept_entry->w ? 'w' : ' ',
> ept_entry->x ? 'x' : ' ',
> - memory_types[ept_entry->emt][0],
> - memory_types[ept_entry->emt][1]
> + memory_type_to_str(ept_entry->emt)[0],
> + memory_type_to_str(ept_entry->emt)[1]
> ?: ept_entry->emt + '0',
> c ?: ept_entry->ipat ? '!' : ' ');
There's actually a bug here, which I think is worth fixing at once:
The default initializer was a string of length 1, resulting in a
premature NUL character to get placed into the fully expanded
string, causing - afaict - truncation of the intended message. I
therefore think the default string should be e.g. "? ".
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays
2016-02-10 13:22 ` Jan Beulich
@ 2016-02-10 13:50 ` Andrew Cooper
2016-02-10 14:03 ` Jan Beulich
0 siblings, 1 reply; 26+ messages in thread
From: Andrew Cooper @ 2016-02-10 13:50 UTC (permalink / raw)
To: Jan Beulich; +Cc: George Dunlap, Kevin Tian, JunNakajima, Xen-devel
On 10/02/16 13:22, Jan Beulich wrote:
>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
>> Clang objects to having multiple initialisers when creating an array.
>>
>> As this warning is useful for spotting obscure bugs, disabling it is
>> unhelpful. Instead, fix our two deliberate usecases.
> Ugly again, but - well ...
>
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m)
>> free_cpumask_var(ept->invalidate);
>> }
>>
>> +static const char *memory_type_to_str(unsigned int x)
>> +{
>> + static const char memory_types[8][2] = {
>> + [MTRR_TYPE_UNCACHABLE] = "UC",
>> + [MTRR_TYPE_WRCOMB] = "WC",
>> + [MTRR_TYPE_WRTHROUGH] = "WT",
>> + [MTRR_TYPE_WRPROT] = "WP",
>> + [MTRR_TYPE_WRBACK] = "WB",
>> + [MTRR_NUM_TYPES] = "??"
>> + };
>> +
>> + return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?";
> I think this should really ASSERT() the first condition.
>
>> @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key)
>> unsigned long record_counter = 0;
>> struct p2m_domain *p2m;
>> struct ept_data *ept;
>> - static const char memory_types[8][2] = {
>> - [0 ... 7] = "?",
>> - [MTRR_TYPE_UNCACHABLE] = "UC",
>> - [MTRR_TYPE_WRCOMB] = "WC",
>> - [MTRR_TYPE_WRTHROUGH] = "WT",
>> - [MTRR_TYPE_WRPROT] = "WP",
>> - [MTRR_TYPE_WRBACK] = "WB",
>> - [MTRR_NUM_TYPES] = "??"
>> - };
>>
>> for_each_domain(d)
>> {
>> @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key)
>> ept_entry->r ? 'r' : ' ',
>> ept_entry->w ? 'w' : ' ',
>> ept_entry->x ? 'x' : ' ',
>> - memory_types[ept_entry->emt][0],
>> - memory_types[ept_entry->emt][1]
>> + memory_type_to_str(ept_entry->emt)[0],
>> + memory_type_to_str(ept_entry->emt)[1]
>> ?: ept_entry->emt + '0',
>> c ?: ept_entry->ipat ? '!' : ' ');
> There's actually a bug here, which I think is worth fixing at once:
> The default initializer was a string of length 1, resulting in a
> premature NUL character to get placed into the fully expanded
> string, causing - afaict - truncation of the intended message. I
> therefore think the default string should be e.g. "? ".
The code is very opaque. However, that appears to be precisely how it
is intended to work. (Having said that - it is your code from c/s
90e9c95f).
The following line will only format the raw emt value as a number if
there is a NUL character returned from memory_type_to_str(). Putting a
space in instead would break this.
~Andrew
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays
2016-02-10 13:50 ` Andrew Cooper
@ 2016-02-10 14:03 ` Jan Beulich
2016-02-10 14:13 ` George Dunlap
0 siblings, 1 reply; 26+ messages in thread
From: Jan Beulich @ 2016-02-10 14:03 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, Kevin Tian, JunNakajima, Xen-devel
>>> On 10.02.16 at 14:50, <andrew.cooper3@citrix.com> wrote:
> On 10/02/16 13:22, Jan Beulich wrote:
>>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
>>> Clang objects to having multiple initialisers when creating an array.
>>>
>>> As this warning is useful for spotting obscure bugs, disabling it is
>>> unhelpful. Instead, fix our two deliberate usecases.
>> Ugly again, but - well ...
>>
>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>> @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m)
>>> free_cpumask_var(ept->invalidate);
>>> }
>>>
>>> +static const char *memory_type_to_str(unsigned int x)
>>> +{
>>> + static const char memory_types[8][2] = {
>>> + [MTRR_TYPE_UNCACHABLE] = "UC",
>>> + [MTRR_TYPE_WRCOMB] = "WC",
>>> + [MTRR_TYPE_WRTHROUGH] = "WT",
>>> + [MTRR_TYPE_WRPROT] = "WP",
>>> + [MTRR_TYPE_WRBACK] = "WB",
>>> + [MTRR_NUM_TYPES] = "??"
>>> + };
>>> +
>>> + return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?";
>> I think this should really ASSERT() the first condition.
>>
>>> @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key)
>>> unsigned long record_counter = 0;
>>> struct p2m_domain *p2m;
>>> struct ept_data *ept;
>>> - static const char memory_types[8][2] = {
>>> - [0 ... 7] = "?",
>>> - [MTRR_TYPE_UNCACHABLE] = "UC",
>>> - [MTRR_TYPE_WRCOMB] = "WC",
>>> - [MTRR_TYPE_WRTHROUGH] = "WT",
>>> - [MTRR_TYPE_WRPROT] = "WP",
>>> - [MTRR_TYPE_WRBACK] = "WB",
>>> - [MTRR_NUM_TYPES] = "??"
>>> - };
>>>
>>> for_each_domain(d)
>>> {
>>> @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key)
>>> ept_entry->r ? 'r' : ' ',
>>> ept_entry->w ? 'w' : ' ',
>>> ept_entry->x ? 'x' : ' ',
>>> - memory_types[ept_entry->emt][0],
>>> - memory_types[ept_entry->emt][1]
>>> + memory_type_to_str(ept_entry->emt)[0],
>>> + memory_type_to_str(ept_entry->emt)[1]
>>> ?: ept_entry->emt + '0',
>>> c ?: ept_entry->ipat ? '!' : ' ');
>> There's actually a bug here, which I think is worth fixing at once:
>> The default initializer was a string of length 1, resulting in a
>> premature NUL character to get placed into the fully expanded
>> string, causing - afaict - truncation of the intended message. I
>> therefore think the default string should be e.g. "? ".
>
> The code is very opaque. However, that appears to be precisely how it
> is intended to work. (Having said that - it is your code from c/s
> 90e9c95f).
I know.
> The following line will only format the raw emt value as a number if
> there is a NUL character returned from memory_type_to_str(). Putting a
> space in instead would break this.
Oh, right - this is the operand to a ?:, not by itself passed to
printk(). Line breaks like this (to aid people with old editors) are
really undesirable in places like this...
Sorry for the noise,
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays
2016-02-10 14:03 ` Jan Beulich
@ 2016-02-10 14:13 ` George Dunlap
0 siblings, 0 replies; 26+ messages in thread
From: George Dunlap @ 2016-02-10 14:13 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper
Cc: George Dunlap, Kevin Tian, JunNakajima, Xen-devel
On 10/02/16 14:03, Jan Beulich wrote:
>>>> On 10.02.16 at 14:50, <andrew.cooper3@citrix.com> wrote:
>> On 10/02/16 13:22, Jan Beulich wrote:
>>>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
>>>> Clang objects to having multiple initialisers when creating an array.
>>>>
>>>> As this warning is useful for spotting obscure bugs, disabling it is
>>>> unhelpful. Instead, fix our two deliberate usecases.
>>> Ugly again, but - well ...
>>>
>>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>>> @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m)
>>>> free_cpumask_var(ept->invalidate);
>>>> }
>>>>
>>>> +static const char *memory_type_to_str(unsigned int x)
>>>> +{
>>>> + static const char memory_types[8][2] = {
>>>> + [MTRR_TYPE_UNCACHABLE] = "UC",
>>>> + [MTRR_TYPE_WRCOMB] = "WC",
>>>> + [MTRR_TYPE_WRTHROUGH] = "WT",
>>>> + [MTRR_TYPE_WRPROT] = "WP",
>>>> + [MTRR_TYPE_WRBACK] = "WB",
>>>> + [MTRR_NUM_TYPES] = "??"
>>>> + };
>>>> +
>>>> + return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?";
>>> I think this should really ASSERT() the first condition.
>>>
>>>> @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key)
>>>> unsigned long record_counter = 0;
>>>> struct p2m_domain *p2m;
>>>> struct ept_data *ept;
>>>> - static const char memory_types[8][2] = {
>>>> - [0 ... 7] = "?",
>>>> - [MTRR_TYPE_UNCACHABLE] = "UC",
>>>> - [MTRR_TYPE_WRCOMB] = "WC",
>>>> - [MTRR_TYPE_WRTHROUGH] = "WT",
>>>> - [MTRR_TYPE_WRPROT] = "WP",
>>>> - [MTRR_TYPE_WRBACK] = "WB",
>>>> - [MTRR_NUM_TYPES] = "??"
>>>> - };
>>>>
>>>> for_each_domain(d)
>>>> {
>>>> @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key)
>>>> ept_entry->r ? 'r' : ' ',
>>>> ept_entry->w ? 'w' : ' ',
>>>> ept_entry->x ? 'x' : ' ',
>>>> - memory_types[ept_entry->emt][0],
>>>> - memory_types[ept_entry->emt][1]
>>>> + memory_type_to_str(ept_entry->emt)[0],
>>>> + memory_type_to_str(ept_entry->emt)[1]
>>>> ?: ept_entry->emt + '0',
>>>> c ?: ept_entry->ipat ? '!' : ' ');
>>> There's actually a bug here, which I think is worth fixing at once:
>>> The default initializer was a string of length 1, resulting in a
>>> premature NUL character to get placed into the fully expanded
>>> string, causing - afaict - truncation of the intended message. I
>>> therefore think the default string should be e.g. "? ".
>>
>> The code is very opaque. However, that appears to be precisely how it
>> is intended to work. (Having said that - it is your code from c/s
>> 90e9c95f).
>
> I know.
>
>> The following line will only format the raw emt value as a number if
>> there is a NUL character returned from memory_type_to_str(). Putting a
>> space in instead would break this.
>
> Oh, right - this is the operand to a ?:, not by itself passed to
> printk(). Line breaks like this (to aid people with old editors) are
> really undesirable in places like this...
Even more so over-clever undocumented code. If you're going to do
things like this, you need to leave a comment near the string definition
saying that the second byte being NULL is a flag for the printing
routine to print the number, so that people who come along later (maybe
even yourself, as in this case) know there's a dependency there.
-George
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays
2016-02-09 20:01 ` [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays Andrew Cooper
2016-02-10 10:11 ` George Dunlap
2016-02-10 13:22 ` Jan Beulich
@ 2016-02-16 7:06 ` Tian, Kevin
2 siblings, 0 replies; 26+ messages in thread
From: Tian, Kevin @ 2016-02-16 7:06 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: George Dunlap, Nakajima, Jun, Jan Beulich
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Wednesday, February 10, 2016 4:02 AM
>
> Clang objects to having multiple initialisers when creating an array.
>
> As this warning is useful for spotting obscure bugs, disabling it is
> unhelpful. Instead, fix our two deliberate usecases.
>
> In the p2m-ept case, pull the array out into a helper function, so the helper
> can guarentee to cover the NULL pointer case.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 7/8] xen/x86: Fix get_cpu_info() when built with clang
2016-02-09 20:01 [PATCH 0/8] xen/x86: Fix build with Clang 3.5 Andrew Cooper
` (5 preceding siblings ...)
2016-02-09 20:01 ` [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays Andrew Cooper
@ 2016-02-09 20:01 ` Andrew Cooper
2016-02-09 20:01 ` [PATCH 8/8] x86/efi: Generate uefi_call_wrapper() when compiling " Andrew Cooper
` (2 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Andrew Cooper @ 2016-02-09 20:01 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
Clang understands the GCCism in use here, but still complains that sp is
unintialised. In such cases, resort to the older version of this code, which
directly reads %rsp into the temporary variable.
Note that we still keep the GCCism in the default case, as it causes GCC to
create rather better assembly.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/include/asm-x86/current.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/xen/include/asm-x86/current.h b/xen/include/asm-x86/current.h
index f011d2d..15b99ff 100644
--- a/xen/include/asm-x86/current.h
+++ b/xen/include/asm-x86/current.h
@@ -47,7 +47,13 @@ struct cpu_info {
static inline struct cpu_info *get_cpu_info(void)
{
+#ifdef __clang__
+ /* Clang complains that sp in the else case is not initialised. */
+ unsigned long sp;
+ asm ( "mov %%rsp, %0" : "=r" (sp) );
+#else
register unsigned long sp asm("rsp");
+#endif
return (struct cpu_info *)((sp & ~(STACK_SIZE-1)) + STACK_SIZE) - 1;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 8/8] x86/efi: Generate uefi_call_wrapper() when compiling with clang
2016-02-09 20:01 [PATCH 0/8] xen/x86: Fix build with Clang 3.5 Andrew Cooper
` (6 preceding siblings ...)
2016-02-09 20:01 ` [PATCH 7/8] xen/x86: Fix get_cpu_info() when built with clang Andrew Cooper
@ 2016-02-09 20:01 ` Andrew Cooper
2016-02-10 13:31 ` Jan Beulich
2016-02-09 21:09 ` [PATCH 0/8] xen/x86: Fix build with Clang 3.5 Doug Goldstein
2016-02-10 9:28 ` Ian Campbell
9 siblings, 1 reply; 26+ messages in thread
From: Andrew Cooper @ 2016-02-09 20:01 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
What is the GCC version check supposed to be achieving here? GCC has
supported this syntax since 3.0
---
xen/include/asm-x86/x86_64/efibind.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/include/asm-x86/x86_64/efibind.h b/xen/include/asm-x86/x86_64/efibind.h
index 2db3568..af5f424 100644
--- a/xen/include/asm-x86/x86_64/efibind.h
+++ b/xen/include/asm-x86/x86_64/efibind.h
@@ -274,7 +274,7 @@ typedef uint64_t UINTN;
#endif
#endif
-#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
+#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) || __clang__
#define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__)
#else
/* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
--
2.1.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 8/8] x86/efi: Generate uefi_call_wrapper() when compiling with clang
2016-02-09 20:01 ` [PATCH 8/8] x86/efi: Generate uefi_call_wrapper() when compiling " Andrew Cooper
@ 2016-02-10 13:31 ` Jan Beulich
2016-02-10 13:41 ` Andrew Cooper
0 siblings, 1 reply; 26+ messages in thread
From: Jan Beulich @ 2016-02-10 13:31 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
>
> What is the GCC version check supposed to be achieving here? GCC has
> supported this syntax since 3.0
This is best answered by looking at where we've got these headers
from - the gnu-efi package. It has ...
> --- a/xen/include/asm-x86/x86_64/efibind.h
> +++ b/xen/include/asm-x86/x86_64/efibind.h
> @@ -274,7 +274,7 @@ typedef uint64_t UINTN;
> #endif
> #endif
>
> -#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
> +#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) || __clang__
> #define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__)
> #else
> /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
/* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
#if defined(HAVE_USE_MS_ABI)
#define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__)
#else
UINTN uefi_call_wrapper(void *func, unsigned long va_num, ...);
#endif
#define EFI_FUNCTION __attribute__((ms_abi))
I think this makes clear that the needed feature here is the
attribute, which certainly wasn't available in older gcc.
With that the question is whether the Clang case won't also need
a version check, since I can't imagine them having supported this
prior to gcc introducing it.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 8/8] x86/efi: Generate uefi_call_wrapper() when compiling with clang
2016-02-10 13:31 ` Jan Beulich
@ 2016-02-10 13:41 ` Andrew Cooper
2016-02-10 19:11 ` Andrew Cooper
0 siblings, 1 reply; 26+ messages in thread
From: Andrew Cooper @ 2016-02-10 13:41 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 10/02/16 13:31, Jan Beulich wrote:
>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>>
>> What is the GCC version check supposed to be achieving here? GCC has
>> supported this syntax since 3.0
> This is best answered by looking at where we've got these headers
> from - the gnu-efi package. It has ...
>
>> --- a/xen/include/asm-x86/x86_64/efibind.h
>> +++ b/xen/include/asm-x86/x86_64/efibind.h
>> @@ -274,7 +274,7 @@ typedef uint64_t UINTN;
>> #endif
>> #endif
>>
>> -#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
>> +#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) || __clang__
>> #define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__)
>> #else
>> /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
> /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
> #if defined(HAVE_USE_MS_ABI)
> #define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__)
> #else
> UINTN uefi_call_wrapper(void *func, unsigned long va_num, ...);
> #endif
> #define EFI_FUNCTION __attribute__((ms_abi))
>
> I think this makes clear that the needed feature here is the
> attribute, which certainly wasn't available in older gcc.
>
> With that the question is whether the Clang case won't also need
> a version check, since I can't imagine them having supported this
> prior to gcc introducing it.
Clang has an substantially more sane way than GCC of checking for
individual features. I will introduce and use the
__has_{attribute,feature}() infrastructure to tests like this.
Experimentally, Clang 3.5 does have ms_abi support
~Andrew
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 8/8] x86/efi: Generate uefi_call_wrapper() when compiling with clang
2016-02-10 13:41 ` Andrew Cooper
@ 2016-02-10 19:11 ` Andrew Cooper
2016-02-11 10:45 ` Jan Beulich
0 siblings, 1 reply; 26+ messages in thread
From: Andrew Cooper @ 2016-02-10 19:11 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 10/02/16 13:41, Andrew Cooper wrote:
> On 10/02/16 13:31, Jan Beulich wrote:
>>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>>
>>> What is the GCC version check supposed to be achieving here? GCC has
>>> supported this syntax since 3.0
>> This is best answered by looking at where we've got these headers
>> from - the gnu-efi package. It has ...
>>
>>> --- a/xen/include/asm-x86/x86_64/efibind.h
>>> +++ b/xen/include/asm-x86/x86_64/efibind.h
>>> @@ -274,7 +274,7 @@ typedef uint64_t UINTN;
>>> #endif
>>> #endif
>>>
>>> -#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
>>> +#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) || __clang__
>>> #define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__)
>>> #else
>>> /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
>> /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
>> #if defined(HAVE_USE_MS_ABI)
>> #define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__)
>> #else
>> UINTN uefi_call_wrapper(void *func, unsigned long va_num, ...);
>> #endif
>> #define EFI_FUNCTION __attribute__((ms_abi))
>>
>> I think this makes clear that the needed feature here is the
>> attribute, which certainly wasn't available in older gcc.
>>
>> With that the question is whether the Clang case won't also need
>> a version check, since I can't imagine them having supported this
>> prior to gcc introducing it.
> Clang has an substantially more sane way than GCC of checking for
> individual features. I will introduce and use the
> __has_{attribute,feature}() infrastructure to tests like this.
>
> Experimentally, Clang 3.5 does have ms_abi support
Looking at it further, this entire block is unsed. Nothing in tree
refers to uefi_call_wrapper() or EFI_FUNCTION_WRAPPER other than this
small bit; all declarations use EFIABI directly.
i.e. this:
diff --git a/xen/include/asm-x86/x86_64/efibind.h
b/xen/include/asm-x86/x86_64/efibind.h
index af5f424..b013db1 100644
--- a/xen/include/asm-x86/x86_64/efibind.h
+++ b/xen/include/asm-x86/x86_64/efibind.h
@@ -274,17 +274,6 @@ typedef uint64_t UINTN;
#endif
#endif
-#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
-#define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__)
-#else
-/* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
-#ifdef EFI_FUNCTION_WRAPPER
-UINTN uefi_call_wrapper(void *func, unsigned long va_num, ...);
-#else
-#error "EFI_FUNCTION_WRAPPER must be defined for x86_64 architecture"
-#endif
-#endif
-
#ifdef _MSC_EXTENSIONS
#pragma warning ( disable : 4731 ) // Suppress warnings about
modification of EBP
#endif
works correctly for GCC and clang. Would dropping this completely be
acceptable?
~Andrew
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 8/8] x86/efi: Generate uefi_call_wrapper() when compiling with clang
2016-02-10 19:11 ` Andrew Cooper
@ 2016-02-11 10:45 ` Jan Beulich
0 siblings, 0 replies; 26+ messages in thread
From: Jan Beulich @ 2016-02-11 10:45 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 10.02.16 at 20:11, <andrew.cooper3@citrix.com> wrote:
> On 10/02/16 13:41, Andrew Cooper wrote:
>> On 10/02/16 13:31, Jan Beulich wrote:
>>>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote:
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>>
>>>> What is the GCC version check supposed to be achieving here? GCC has
>>>> supported this syntax since 3.0
>>> This is best answered by looking at where we've got these headers
>>> from - the gnu-efi package. It has ...
>>>
>>>> --- a/xen/include/asm-x86/x86_64/efibind.h
>>>> +++ b/xen/include/asm-x86/x86_64/efibind.h
>>>> @@ -274,7 +274,7 @@ typedef uint64_t UINTN;
>>>> #endif
>>>> #endif
>>>>
>>>> -#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
>>>> +#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) || __clang__
>>>> #define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__)
>>>> #else
>>>> /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
>>> /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
>>> #if defined(HAVE_USE_MS_ABI)
>>> #define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__)
>>> #else
>>> UINTN uefi_call_wrapper(void *func, unsigned long va_num, ...);
>>> #endif
>>> #define EFI_FUNCTION __attribute__((ms_abi))
>>>
>>> I think this makes clear that the needed feature here is the
>>> attribute, which certainly wasn't available in older gcc.
>>>
>>> With that the question is whether the Clang case won't also need
>>> a version check, since I can't imagine them having supported this
>>> prior to gcc introducing it.
>> Clang has an substantially more sane way than GCC of checking for
>> individual features. I will introduce and use the
>> __has_{attribute,feature}() infrastructure to tests like this.
>>
>> Experimentally, Clang 3.5 does have ms_abi support
>
> Looking at it further, this entire block is unsed. Nothing in tree
> refers to uefi_call_wrapper() or EFI_FUNCTION_WRAPPER other than this
> small bit; all declarations use EFIABI directly.
>
> i.e. this:
>
> diff --git a/xen/include/asm-x86/x86_64/efibind.h
> b/xen/include/asm-x86/x86_64/efibind.h
> index af5f424..b013db1 100644
> --- a/xen/include/asm-x86/x86_64/efibind.h
> +++ b/xen/include/asm-x86/x86_64/efibind.h
> @@ -274,17 +274,6 @@ typedef uint64_t UINTN;
> #endif
> #endif
>
> -#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
> -#define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__)
> -#else
> -/* for x86_64, EFI_FUNCTION_WRAPPER must be defined */
> -#ifdef EFI_FUNCTION_WRAPPER
> -UINTN uefi_call_wrapper(void *func, unsigned long va_num, ...);
> -#else
> -#error "EFI_FUNCTION_WRAPPER must be defined for x86_64 architecture"
> -#endif
> -#endif
> -
> #ifdef _MSC_EXTENSIONS
> #pragma warning ( disable : 4731 ) // Suppress warnings about
> modification of EBP
> #endif
>
> works correctly for GCC and clang. Would dropping this completely be
> acceptable?
We perhaps might as well; I had mainly kept it to stay as close to
the original header as possible (without leaving around a latent
build breakage).
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/8] xen/x86: Fix build with Clang 3.5
2016-02-09 20:01 [PATCH 0/8] xen/x86: Fix build with Clang 3.5 Andrew Cooper
` (7 preceding siblings ...)
2016-02-09 20:01 ` [PATCH 8/8] x86/efi: Generate uefi_call_wrapper() when compiling " Andrew Cooper
@ 2016-02-09 21:09 ` Doug Goldstein
2016-02-10 9:28 ` Ian Campbell
9 siblings, 0 replies; 26+ messages in thread
From: Doug Goldstein @ 2016-02-09 21:09 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: Jun Nakajima, Kevin Tian, Ian Campbell, George Dunlap,
Tim Deegan, Jan Beulich
[-- Attachment #1.1: Type: text/plain, Size: 269 bytes --]
On 2/9/16 2:01 PM, Andrew Cooper wrote:
> This is a misc assortment of patches to fix the build with Clang 3.5. The
> result also appears to function.
>
Manually ran the build here:
https://travis-ci.org/cardoe/xen/builds/108116078
--
Doug Goldstein
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 0/8] xen/x86: Fix build with Clang 3.5
2016-02-09 20:01 [PATCH 0/8] xen/x86: Fix build with Clang 3.5 Andrew Cooper
` (8 preceding siblings ...)
2016-02-09 21:09 ` [PATCH 0/8] xen/x86: Fix build with Clang 3.5 Doug Goldstein
@ 2016-02-10 9:28 ` Ian Campbell
9 siblings, 0 replies; 26+ messages in thread
From: Ian Campbell @ 2016-02-10 9:28 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: George Dunlap, Kevin Tian, Tim Deegan, Jun Nakajima, Jan Beulich
On Tue, 2016-02-09 at 20:01 +0000, Andrew Cooper wrote:
> This is a misc assortment of patches to fix the build with Clang 3.5.
FWIW clang 3.5 appears to be in Debian Jessie, which means the path to
having a build job in osstest is far shorter than it once was if anyone is
interested in adding such a thing.
Ian.
^ permalink raw reply [flat|nested] 26+ messages in thread