[PATCH 00/34] Add clang support for ARM and cleanups

[PATCH 00/34] Add clang support for ARM and cleanups

[Julien Grall]

Hello all,

The main goal of this patch series is to add support for Clang on ARM.
The work has been done with Clang 3.5 which is not yet release but have
a good support for ARM32. I think clang 3.4 may also be able to compile
Xen but I didn't try.

With the first part of the serie (patch #1-#19 and patch #33), I'm able to boot
Xen compiled by Clang on midway. The patch #33 workaround for a bug in Clang
(http://llvm.org/bugs/show_bug.cgi?id=19199). I'm not sure if the patch would
be acceptable for Xen upstream.

While I was working on Clang 3.5, I played with -Wmissing-prototypes (patch #20-#30)
and discovered some exported functions which were:
    - the prototype were not declared before because of a missing include
    - never used

Finally the last part of this series (patch #31-33) contains a start of
bug fixes to be able to compile the tools with Clang. I didn't include
the patch https://patches.linaro.org/26826/.

The branch will all the patch applied (+few required patches) can be found here:
    git clone git://xenbits.xen.org/people/julieng/xen-unstable.git -b clang

Any comments and questions are welcomed.

Sincerely yours,

Julien Grall (34):
  xen: clang: Disable initializer-overrides warning
  xen: clang: Disable built-in assembler
  xen: clang: Support correctly cross-compile
  xen/compiler: Introduce always_unused define
  xen/xsm: flask: Remove unused function avc_sidcmp
  xen/x86: shadow: sh_next_page is only used when GUEST_PAGING_LEVELS =
    2
  xen/common: sched_sedf: Remove unused functions
  xen/common: rcupdate: Remove unused function rcu_batch_after
  xen/common: tmem: Remove dumb check in do_tmem_destroy_pool
  xen/crypto: rijndael: Fix compilation with Clang 3.5
  xen/arm: Introduce __builtin_stack_pointer
  xen/arm: psci: Don't need to check if vcpuid is negative
  xen/arm: gic: Introduce GIC_SGI_MAX
  xen/arm: mm: Mark check_memory_layout_alignment_constraints as unused
  xen/arm: traps: Mark check_stack_alignment_constraints as unused
  xen/arm: Guard correctly asm-arm/platforms/midway.h
  xen/arm: Guard correctly asm-arm/platform/omap5.h
  xen/arm: omap5: Correctly constify platform compatibility list
  xen/arm: Provide eabi wrapper for __aeabi_mem* functions
  xen/common: cpupool: cpupool_unassign_cpu is only used internally
  xen/common: domctl: Some functions are only used internally
  xen/common: tmem: Some functions are only used internally
  xen/sched: credit2: Some functions are only used internally
  xen/arm: setup: setup_cache is only used internally
  xen/arm: traps: show_stack is only used internally
  xen/arm: traps: Drop dump_guest_s1_walk
  xen/arm: time: move ticks_to_ns and ns_to_ticks in asm/time.h
  xen/xsm: flask: flask_disable is only used internally
  xen/xsm: flask: ss: remove unused function determine_oocontext
  xen: Add missing includes on different files
  tools: Disable ignored-attributes warning when compiling with clang
  tools/libxl: list_domains: shutdown is typed unsigned
  tools/libxl: libxl__json_object_to_yajl_gen should return
    yajl_gen_status
  DO NOT APPLY xen/common: kernel: Workaround clang 3.5

 config/StdGNU.mk                       |    6 +++-
 tools/Rules.mk                         |    2 ++
 tools/libxl/libxl_internal.h           |    2 +-
 tools/libxl/libxl_json.c               |   18 +++++-----
 tools/libxl/xl_cmdimpl.c               |    3 +-
 xen/Rules.mk                           |    3 ++
 xen/arch/arm/Makefile                  |    1 +
 xen/arch/arm/eabi.c                    |   29 +++++++++++++++
 xen/arch/arm/gic.c                     |    7 ++--
 xen/arch/arm/hvm.c                     |    1 +
 xen/arch/arm/mm.c                      |    4 ++-
 xen/arch/arm/platforms/omap5.c         |    2 +-
 xen/arch/arm/setup.c                   |    2 +-
 xen/arch/arm/shutdown.c                |    1 +
 xen/arch/arm/smp.c                     |    1 +
 xen/arch/arm/time.c                    |   11 +-----
 xen/arch/arm/traps.c                   |   61 ++------------------------------
 xen/arch/arm/vpsci.c                   |    2 +-
 xen/arch/arm/vtimer.c                  |    4 +--
 xen/arch/x86/mm/shadow/multi.c         |    2 ++
 xen/common/cpupool.c                   |    2 +-
 xen/common/domctl.c                    |   20 +++++------
 xen/common/event_channel.c             |    1 +
 xen/common/grant_table.c               |    1 +
 xen/common/kernel.c                    |    5 +--
 xen/common/multicall.c                 |    3 ++
 xen/common/rcupdate.c                  |    6 ----
 xen/common/sched_credit2.c             |   11 +++---
 xen/common/sched_sedf.c                |   12 -------
 xen/common/sort.c                      |    1 +
 xen/common/tmem.c                      |    9 +++--
 xen/crypto/rijndael.c                  |    3 ++
 xen/drivers/video/arm_hdlcd.c          |    1 +
 xen/include/asm-arm/current.h          |   17 +++++++--
 xen/include/asm-arm/gic.h              |    2 ++
 xen/include/asm-arm/platforms/midway.h |    2 +-
 xen/include/asm-arm/platforms/omap5.h  |    2 +-
 xen/include/asm-arm/time.h             |   13 +++++++
 xen/include/xen/compiler.h             |    2 ++
 xen/xsm/flask/avc.c                    |    5 ---
 xen/xsm/flask/flask_op.c               |    2 +-
 xen/xsm/flask/ss/conditional.h         |    1 +
 xen/xsm/flask/ss/services.c            |   14 --------
 43 files changed, 141 insertions(+), 156 deletions(-)
 create mode 100644 xen/arch/arm/eabi.c

-- 
1.7.10.4

[PATCH 01/34] xen: clang: Disable initializer-overrides warning

[Julien Grall]

The commit 819c084 "x86/MTRR: optionally print boot time state" introduces use
of the pattern of initializing an array with a range-based default entry.
On clang it results as a compilation failure:
generic.c:95:32: error: initializer overrides prior initialization of this subobject [-Werror,-Winitializer-overrides]
                [MTRR_TYPE_UNCACHABLE]     = "uncachable",
                                             ^~~~~~~~~~~~
generic.c:94:32: note: previous initialization is here
                [0 ... MTRR_NUM_TYPES - 1] = "?",
                                             ^~~
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/Rules.mk |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index 42c713f..18fbd62 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -68,6 +68,8 @@ ifneq ($(max_phys_irqs),)
 CFLAGS-y                += -DMAX_PHYS_IRQS=$(max_phys_irqs)
 endif
 
+CFLAGS-$(clang)         += -Wno-initializer-overrides
+
 AFLAGS-y                += -D__ASSEMBLY__ -include $(BASEDIR)/include/xen/config.h
 
 # Clang's built-in assembler can't handle .code16/.code32/.code64 yet
-- 
1.7.10.4

Re: [PATCH 01/34] xen: clang: Disable initializer-overrides warning

[Jan Beulich]

>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -68,6 +68,8 @@ ifneq ($(max_phys_irqs),)
>  CFLAGS-y                += -DMAX_PHYS_IRQS=$(max_phys_irqs)
>  endif
>  
> +CFLAGS-$(clang)         += -Wno-initializer-overrides

And this is supported by all clang versions we permit to be used, i.e.
doesn't need to be done conditionally?

Jan

Re: [PATCH 01/34] xen: clang: Disable initializer-overrides warning

[Julien Grall]

Hi Jan,

On 03/26/2014 11:51 AM, Jan Beulich wrote:
>>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
>> --- a/xen/Rules.mk
>> +++ b/xen/Rules.mk
>> @@ -68,6 +68,8 @@ ifneq ($(max_phys_irqs),)
>>  CFLAGS-y                += -DMAX_PHYS_IRQS=$(max_phys_irqs)
>>  endif
>>  
>> +CFLAGS-$(clang)         += -Wno-initializer-overrides
> 
> And this is supported by all clang versions we permit to be used, i.e.
> doesn't need to be done conditionally?

I've tried to compile with clang 3.0-3.5 (used by Debian wheezy).

In another dimension can we really say we are supporting clang when most
of the time it's not possible to compile Xen with it? For instance,
building Xen 4.4 with Clang doesn't work.

I think we might need to have a build test for clang.

Regards,

-- 
Julien Grall

Re: [PATCH 01/34] xen: clang: Disable initializer-overrides warning

[Tim Deegan]

At 15:00 +0000 on 26 Mar (1395842446), Julien Grall wrote:
> Hi Jan,
> 
> On 03/26/2014 11:51 AM, Jan Beulich wrote:
> >>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
> >> --- a/xen/Rules.mk
> >> +++ b/xen/Rules.mk
> >> @@ -68,6 +68,8 @@ ifneq ($(max_phys_irqs),)
> >>  CFLAGS-y                += -DMAX_PHYS_IRQS=$(max_phys_irqs)
> >>  endif
> >>  
> >> +CFLAGS-$(clang)         += -Wno-initializer-overrides
> > 
> > And this is supported by all clang versions we permit to be used, i.e.
> > doesn't need to be done conditionally?
> 
> I've tried to compile with clang 3.0-3.5 (used by Debian wheezy).
> 
> In another dimension can we really say we are supporting clang when most
> of the time it's not possible to compile Xen with it? For instance,
> building Xen 4.4 with Clang doesn't work.
> I think we might need to have a build test for clang.

Most of the breakage seems to come with newer versions of clang,
rather than newer versions of Xen.  Still, a clang build test would be
a good idea.

Cheers,

Tim.

Re: [PATCH 01/34] xen: clang: Disable initializer-overrides warning

[Julien Grall]

On 03/26/2014 03:09 PM, Tim Deegan wrote:
> At 15:00 +0000 on 26 Mar (1395842446), Julien Grall wrote:
>> Hi Jan,
>>
>> On 03/26/2014 11:51 AM, Jan Beulich wrote:
>>>>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
>>>> --- a/xen/Rules.mk
>>>> +++ b/xen/Rules.mk
>>>> @@ -68,6 +68,8 @@ ifneq ($(max_phys_irqs),)
>>>>  CFLAGS-y                += -DMAX_PHYS_IRQS=$(max_phys_irqs)
>>>>  endif
>>>>  
>>>> +CFLAGS-$(clang)         += -Wno-initializer-overrides
>>>
>>> And this is supported by all clang versions we permit to be used, i.e.
>>> doesn't need to be done conditionally?
>>
>> I've tried to compile with clang 3.0-3.5 (used by Debian wheezy).
>>
>> In another dimension can we really say we are supporting clang when most
>> of the time it's not possible to compile Xen with it? For instance,
>> building Xen 4.4 with Clang doesn't work.
>> I think we might need to have a build test for clang.
> 
> Most of the breakage seems to come with newer versions of clang,
> rather than newer versions of Xen.  Still, a clang build test would be
> a good idea.

I was unable to build Xen 4.4 (and onwards) with clang 3.0 which is not new.

-- 
Julien Grall

[PATCH 02/34] xen: clang: Disable built-in assembler

[Julien Grall]

Clang 3.5 is trying to parse every assembly line in C file. The files
asm-offsets.c are taking advantage of the inline assembly but doesn't
produce valid assembly.

x86_64/asm-offsets.c:26:5: error: unexpected token at start of statement
    OFFSET(UREGS_r15, struct cpu_user_regs, r15);
        ^
x86_64/asm-offsets.c:22:5: note: expanded from macro 'OFFSET'
    DEFINE(_sym, offsetof(_str, _mem));
        ^
x86_64/asm-offsets.c:18:31: note: expanded from macro 'DEFINE'
    __asm__ __volatile__ ( "\n->" #_sym " %0 " #_val : : "i" (_val) )
                                  ^
<inline asm>:2:1: note: instantiated into assembly here
->UREGS_r15 $0 offsetof(struct cpu_user_regs, r15)

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Keir Fraser <keir@xen.org>
---
 xen/Rules.mk |    1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index 18fbd62..ce54926 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -74,6 +74,7 @@ AFLAGS-y                += -D__ASSEMBLY__ -include $(BASEDIR)/include/xen/config
 
 # Clang's built-in assembler can't handle .code16/.code32/.code64 yet
 AFLAGS-$(clang)         += -no-integrated-as
+CFLAGS-$(clang)         += -no-integrated-as
 
 ALL_OBJS := $(ALL_OBJS-y)
 
-- 
1.7.10.4

Re: [PATCH 02/34] xen: clang: Disable built-in assembler

[Jan Beulich]

>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -74,6 +74,7 @@ AFLAGS-y                += -D__ASSEMBLY__ -include 
> $(BASEDIR)/include/xen/config
>  
>  # Clang's built-in assembler can't handle .code16/.code32/.code64 yet
>  AFLAGS-$(clang)         += -no-integrated-as
> +CFLAGS-$(clang)         += -no-integrated-as

Iirc Tim had found and worked around other built-in assembler issues
in the past, so if this is to be done unconditionally I wonder whether
we shouldn't then drop those workarounds.

Jan

Re: [PATCH 02/34] xen: clang: Disable built-in assembler

[Tim Deegan]

At 11:53 +0000 on 26 Mar (1395831232), Jan Beulich wrote:
> >>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
> > --- a/xen/Rules.mk
> > +++ b/xen/Rules.mk
> > @@ -74,6 +74,7 @@ AFLAGS-y                += -D__ASSEMBLY__ -include 
> > $(BASEDIR)/include/xen/config
> >  
> >  # Clang's built-in assembler can't handle .code16/.code32/.code64 yet
> >  AFLAGS-$(clang)         += -no-integrated-as
> > +CFLAGS-$(clang)         += -no-integrated-as
> 
> Iirc Tim had found and worked around other built-in assembler issues
> in the past, so if this is to be done unconditionally I wonder whether
> we shouldn't then drop those workarounds.

I would prefer, wherever possible, to make things work with the clang
assembler rather than rely on the binutils one forever.

BTW, I haven't looked at any of this series in detail yet but I'm
planning to go through it all tomorrow.

Cheers,

Tim.

Re: [PATCH 02/34] xen: clang: Disable built-in assembler

[Julien Grall]

Hi Tim,

On 03/26/2014 01:16 PM, Tim Deegan wrote:
> At 11:53 +0000 on 26 Mar (1395831232), Jan Beulich wrote:
>>>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
>>> --- a/xen/Rules.mk
>>> +++ b/xen/Rules.mk
>>> @@ -74,6 +74,7 @@ AFLAGS-y                += -D__ASSEMBLY__ -include 
>>> $(BASEDIR)/include/xen/config
>>>  
>>>  # Clang's built-in assembler can't handle .code16/.code32/.code64 yet
>>>  AFLAGS-$(clang)         += -no-integrated-as
>>> +CFLAGS-$(clang)         += -no-integrated-as
>>
>> Iirc Tim had found and worked around other built-in assembler issues
>> in the past, so if this is to be done unconditionally I wonder whether
>> we shouldn't then drop those workarounds.
> 
> I would prefer, wherever possible, to make things work with the clang
> assembler rather than rely on the binutils one forever.

The clang integrated assembler is too powerful for some part of Xen :).
Every inline assembly code is parsing by the assembler to check the syntax.

This will result to failure to generate asm-offsets.c because of the ->
in the code (see arch/arm/arm32/asm-offsets.c: DEFINE/BLANK macros).
Indeed, the -> is not a valid assembler syntax.

> BTW, I haven't looked at any of this series in detail yet but I'm
> planning to go through it all tomorrow.

There still have few issues to build the tools and Xen x86_64 with clang
3.5.

I also would like to see if we can re-enable some warnings (see
Config.mk) with newer version of clang.

Regards,

-- 
Julien Grall

Re: [PATCH 02/34] xen: clang: Disable built-in assembler

[Jan Beulich]

>>> On 26.03.14 at 16:08, <julien.grall@linaro.org> wrote:
> Hi Tim,
> 
> On 03/26/2014 01:16 PM, Tim Deegan wrote:
>> At 11:53 +0000 on 26 Mar (1395831232), Jan Beulich wrote:
>>>>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
>>>> --- a/xen/Rules.mk
>>>> +++ b/xen/Rules.mk
>>>> @@ -74,6 +74,7 @@ AFLAGS-y                += -D__ASSEMBLY__ -include 
>>>> $(BASEDIR)/include/xen/config
>>>>  
>>>>  # Clang's built-in assembler can't handle .code16/.code32/.code64 yet
>>>>  AFLAGS-$(clang)         += -no-integrated-as
>>>> +CFLAGS-$(clang)         += -no-integrated-as
>>>
>>> Iirc Tim had found and worked around other built-in assembler issues
>>> in the past, so if this is to be done unconditionally I wonder whether
>>> we shouldn't then drop those workarounds.
>> 
>> I would prefer, wherever possible, to make things work with the clang
>> assembler rather than rely on the binutils one forever.
> 
> The clang integrated assembler is too powerful for some part of Xen :).
> Every inline assembly code is parsing by the assembler to check the syntax.
> 
> This will result to failure to generate asm-offsets.c because of the ->
> in the code (see arch/arm/arm32/asm-offsets.c: DEFINE/BLANK macros).
> Indeed, the -> is not a valid assembler syntax.

But what business does the compiler have to pass the assembly
code to its internal assembler when all it was asked was to output
assembly? That may be acceptable as an optional internal
consistency check (verifying the compiler produced valid assembly),
but shouldn't constrain the user from using the compiler for things
where it's known the output isn't going to be valid assembly.

That said, I think it wouldn't be too difficult to change the
asm-offsets logic to produce something that does look like valid
assembly.

Jan

Re: [PATCH 02/34] xen: clang: Disable built-in assembler

[Tim Deegan]

At 15:30 +0000 on 26 Mar (1395844252), Jan Beulich wrote:
> >>> On 26.03.14 at 16:08, <julien.grall@linaro.org> wrote:
> > Hi Tim,
> > 
> > On 03/26/2014 01:16 PM, Tim Deegan wrote:
> >> At 11:53 +0000 on 26 Mar (1395831232), Jan Beulich wrote:
> >>>>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
> >>>> --- a/xen/Rules.mk
> >>>> +++ b/xen/Rules.mk
> >>>> @@ -74,6 +74,7 @@ AFLAGS-y                += -D__ASSEMBLY__ -include 
> >>>> $(BASEDIR)/include/xen/config
> >>>>  
> >>>>  # Clang's built-in assembler can't handle .code16/.code32/.code64 yet
> >>>>  AFLAGS-$(clang)         += -no-integrated-as
> >>>> +CFLAGS-$(clang)         += -no-integrated-as
> >>>
> >>> Iirc Tim had found and worked around other built-in assembler issues
> >>> in the past, so if this is to be done unconditionally I wonder whether
> >>> we shouldn't then drop those workarounds.
> >> 
> >> I would prefer, wherever possible, to make things work with the clang
> >> assembler rather than rely on the binutils one forever.
> > 
> > The clang integrated assembler is too powerful for some part of Xen :).
> > Every inline assembly code is parsing by the assembler to check the syntax.
> > 
> > This will result to failure to generate asm-offsets.c because of the ->
> > in the code (see arch/arm/arm32/asm-offsets.c: DEFINE/BLANK macros).
> > Indeed, the -> is not a valid assembler syntax.
> 
> But what business does the compiler have to pass the assembly
> code to its internal assembler when all it was asked was to output
> assembly? That may be acceptable as an optional internal
> consistency check (verifying the compiler produced valid assembly),
> but shouldn't constrain the user from using the compiler for things
> where it's known the output isn't going to be valid assembly.
> 
> That said, I think it wouldn't be too difficult to change the
> asm-offsets logic to produce something that does look like valid
> assembly.

The patch below works for me (at least as far as building
asm-offsets.h on x86) by wrapping everything in a string.  I did try
just prefixing with '#' but clang 3.5 also strips the comments out.
That seems unhelpful, since I know some people put comments in their
inline assembler too. :(

---8<---

asm-offsets: encode magic asm-offset runes as strings.

Newer versions of clang attempt to parse inline assembler even when
not asked to assemble it.  Wrap our not-for-assembly runes as strings
of the form ``.ascii "==>MAGIC RUNES<=="'' so clang doesn't choke on them.

Reported-by: Julien Grall <julien.grall@linaro.org>
Suggested-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Tim Deegan <tim@xen.org>

---
It should be possible to go futher and assemble the #define in
the C macro, just leaving sed to handle extracting them, but that's for
another day.

diff --git a/xen/Makefile b/xen/Makefile
index 79fa8f2..11aef27 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -150,7 +150,7 @@ include/asm-$(TARGET_ARCH)/asm-offsets.h: arch/$(TARGET_ARCH)/asm-offsets.s
 	  echo "#ifndef __ASM_OFFSETS_H__"; \
 	  echo "#define __ASM_OFFSETS_H__"; \
 	  echo ""; \
-	  sed -ne "/^->/{s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; s:->::; p;}"; \
+	  sed -ne "/==>/{s:^.*==>\(.*\)<==.*:\1:; s:^\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; p;}"; \
 	  echo ""; \
 	  echo "#endif") <$< >$@
 
diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c
index ac628c0..db4bced 100644
--- a/xen/arch/arm/arm32/asm-offsets.c
+++ b/xen/arch/arm/arm32/asm-offsets.c
@@ -14,9 +14,9 @@
 #include <asm/procinfo.h>
 
 #define DEFINE(_sym, _val) \
-    __asm__ __volatile__ ( "\n->" #_sym " %0 " #_val : : "i" (_val) )
+    __asm__ __volatile__ ( "\n.ascii\"==>" #_sym " %0 " #_val "<==\"": : "i" (_val) )
 #define BLANK() \
-    __asm__ __volatile__ ( "\n->" : : )
+    __asm__ __volatile__ ( "\n.ascii\"==><==\"" : : )
 #define OFFSET(_sym, _str, _mem) \
     DEFINE(_sym, offsetof(_str, _mem));
 
diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c
index d7572fa..769416e 100644
--- a/xen/arch/arm/arm64/asm-offsets.c
+++ b/xen/arch/arm/arm64/asm-offsets.c
@@ -13,9 +13,9 @@
 #include <asm/current.h>
 
 #define DEFINE(_sym, _val) \
-    __asm__ __volatile__ ( "\n->" #_sym " %0 " #_val : : "i" (_val) )
+    __asm__ __volatile__ ( "\n.ascii\"==>" #_sym " %0 " #_val "<==\"": : "i" (_val) )
 #define BLANK() \
-    __asm__ __volatile__ ( "\n->" : : )
+    __asm__ __volatile__ ( "\n.ascii\"==><==\"" : : )
 #define OFFSET(_sym, _str, _mem) \
     DEFINE(_sym, offsetof(_str, _mem));
 
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index b0098b3..e677214 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -15,9 +15,9 @@
 #include <xen/multiboot.h>
 
 #define DEFINE(_sym, _val) \
-    __asm__ __volatile__ ( "\n->" #_sym " %0 " #_val : : "i" (_val) )
+    __asm__ __volatile__ ( "\n.ascii\"==>" #_sym " %0 " #_val "<==\"": : "i" (_val) )
 #define BLANK() \
-    __asm__ __volatile__ ( "\n->" : : )
+    __asm__ __volatile__ ( "\n.ascii\"==><==\"" : : )
 #define OFFSET(_sym, _str, _mem) \
     DEFINE(_sym, offsetof(_str, _mem));

Re: [PATCH 02/34] xen: clang: Disable built-in assembler

[Jan Beulich]

>>> On 27.03.14 at 19:01, <tim@xen.org> wrote:
> The patch below works for me (at least as far as building
> asm-offsets.h on x86) by wrapping everything in a string.  I did try
> just prefixing with '#' but clang 3.5 also strips the comments out.
> That seems unhelpful, since I know some people put comments in their
> inline assembler too. :(

Looks generally okay, but in order for it to be as simple (and hence
understandable) as possible ...

> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -150,7 +150,7 @@ include/asm-$(TARGET_ARCH)/asm-offsets.h: arch/$(TARGET_ARCH)/asm-offsets.s
>  	  echo "#ifndef __ASM_OFFSETS_H__"; \
>  	  echo "#define __ASM_OFFSETS_H__"; \
>  	  echo ""; \
> -	  sed -ne "/^->/{s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; s:->::; p;}"; \
> +	  sed -ne "/==>/{s:^.*==>\(.*\)<==.*:\1:; s:^\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; p;}"; \

... I think you should drop the ^ anchoring here, at least for the first
expression (.* will match from the beginning of the string anyway). I
also wonder whether, now that we're intending to make use of it
elsewhere anyway, you shouldn't pass -r too, allowing all the escapes
on the parentheses to be dropped.

Jan

Re: [PATCH 02/34] xen: clang: Disable built-in assembler

[Tim Deegan]

At 08:14 +0000 on 28 Mar (1395990898), Jan Beulich wrote:
> >>> On 27.03.14 at 19:01, <tim@xen.org> wrote:
> > The patch below works for me (at least as far as building
> > asm-offsets.h on x86) by wrapping everything in a string.  I did try
> > just prefixing with '#' but clang 3.5 also strips the comments out.
> > That seems unhelpful, since I know some people put comments in their
> > inline assembler too. :(
> 
> Looks generally okay, but in order for it to be as simple (and hence
> understandable) as possible ...
> 
> > --- a/xen/Makefile
> > +++ b/xen/Makefile
> > @@ -150,7 +150,7 @@ include/asm-$(TARGET_ARCH)/asm-offsets.h: arch/$(TARGET_ARCH)/asm-offsets.s
> >  	  echo "#ifndef __ASM_OFFSETS_H__"; \
> >  	  echo "#define __ASM_OFFSETS_H__"; \
> >  	  echo ""; \
> > -	  sed -ne "/^->/{s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; s:->::; p;}"; \
> > +	  sed -ne "/==>/{s:^.*==>\(.*\)<==.*:\1:; s:^\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; p;}"; \
> 
> ... I think you should drop the ^ anchoring here, at least for the first
> expression (.* will match from the beginning of the string anyway). 

Ack, will do.

> I also wonder whether, now that we're intending to make use of it
> elsewhere anyway, you shouldn't pass -r too, allowing all the escapes
> on the parentheses to be dropped.

OK.  Do you think it would be worth shuffling most of the creation of
the #define into the C side too?  I think we can do everything except
the $ or # prefix of the immediate value.  That would leave the RE looking
something like this: sed -ner "/==>/{s:.*==>(.*)<==.*:\1:; s:[\$$#]::; p;}".

Cheers,

Tim.

Re: [PATCH 02/34] xen: clang: Disable built-in assembler

[Jan Beulich]

>>> On 28.03.14 at 12:28, <tim@xen.org> wrote:
> At 08:14 +0000 on 28 Mar (1395990898), Jan Beulich wrote:
>> I also wonder whether, now that we're intending to make use of it
>> elsewhere anyway, you shouldn't pass -r too, allowing all the escapes
>> on the parentheses to be dropped.
> 
> OK.  Do you think it would be worth shuffling most of the creation of
> the #define into the C side too?  I think we can do everything except
> the $ or # prefix of the immediate value.  That would leave the RE looking
> something like this: sed -ner "/==>/{s:.*==>(.*)<==.*:\1:; s:[\$$#]::; p;}".

Yes, I think anything allowing this ugly sed expression to be reduced
would be beneficial.

Jan

[PATCH v2] asm-offsets: encode magic asm-offset runes as strings.

[Tim Deegan]

Newer versions of clang attempt to parse inline assembler even when
not asked to assemble it.  Wrap our not-for-assembly runes as strings
of the form ``.ascii "==>MAGIC RUNES<=="'' so clang doesn't choke on
them.

While we're at it, assemble more of the final output line in the C
file, to make the sed expression shorter.

Reported-by: Julien Grall <julien.grall@linaro.org>
Suggested-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Tim Deegan <tim@xen.org>
---
v2: make up the '#define' in the C source rather then in sed.
Not quite as neat as I'd hoped: need to match an extra space
to stop the second sed expression eating the # in #define.

diff --git a/xen/Makefile b/xen/Makefile
index 79fa8f2..afbc962 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -150,7 +150,7 @@ include/asm-$(TARGET_ARCH)/asm-offsets.h: arch/$(TARGET_ARCH)/asm-offsets.s
 	  echo "#ifndef __ASM_OFFSETS_H__"; \
 	  echo "#define __ASM_OFFSETS_H__"; \
 	  echo ""; \
-	  sed -ne "/^->/{s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; s:->::; p;}"; \
+	  sed -rne "/==>/{s:.*==>(.*)<==.*:\1:; s: [\$$#]: :; p;}"; \
 	  echo ""; \
 	  echo "#endif") <$< >$@
 
diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c
index ac628c0..cd1dff7 100644
--- a/xen/arch/arm/arm32/asm-offsets.c
+++ b/xen/arch/arm/arm32/asm-offsets.c
@@ -13,11 +13,12 @@
 #include <asm/current.h>
 #include <asm/procinfo.h>
 
-#define DEFINE(_sym, _val) \
-    __asm__ __volatile__ ( "\n->" #_sym " %0 " #_val : : "i" (_val) )
-#define BLANK() \
-    __asm__ __volatile__ ( "\n->" : : )
-#define OFFSET(_sym, _str, _mem) \
+#define DEFINE(_sym, _val)                                                 \
+    asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
+                  : : "i" (_val) )
+#define BLANK()                                                            \
+    asm volatile ( "\n.ascii\"==><==\"" : : )
+#define OFFSET(_sym, _str, _mem)                                           \
     DEFINE(_sym, offsetof(_str, _mem));
 
 void __dummy__(void)
diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c
index d7572fa..a3ce816 100644
--- a/xen/arch/arm/arm64/asm-offsets.c
+++ b/xen/arch/arm/arm64/asm-offsets.c
@@ -12,11 +12,12 @@
 #include <public/xen.h>
 #include <asm/current.h>
 
-#define DEFINE(_sym, _val) \
-    __asm__ __volatile__ ( "\n->" #_sym " %0 " #_val : : "i" (_val) )
-#define BLANK() \
-    __asm__ __volatile__ ( "\n->" : : )
-#define OFFSET(_sym, _str, _mem) \
+#define DEFINE(_sym, _val)                                                 \
+    asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
+                  : : "i" (_val) )
+#define BLANK()                                                            \
+    asm volatile ( "\n.ascii\"==><==\"" : : )
+#define OFFSET(_sym, _str, _mem)                                           \
     DEFINE(_sym, offsetof(_str, _mem));
 
 void __dummy__(void)
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index b0098b3..9acb648 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -14,11 +14,12 @@
 #include <asm/hardirq.h>
 #include <xen/multiboot.h>
 
-#define DEFINE(_sym, _val) \
-    __asm__ __volatile__ ( "\n->" #_sym " %0 " #_val : : "i" (_val) )
-#define BLANK() \
-    __asm__ __volatile__ ( "\n->" : : )
-#define OFFSET(_sym, _str, _mem) \
+#define DEFINE(_sym, _val)                                                 \
+    asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
+                  : : "i" (_val) )
+#define BLANK()                                                            \
+    asm volatile ( "\n.ascii\"==><==\"" : : )
+#define OFFSET(_sym, _str, _mem)                                           \
     DEFINE(_sym, offsetof(_str, _mem));
 
 void __dummy__(void)

Re: [PATCH v2] asm-offsets: encode magic asm-offset runes as strings.

[Ian Campbell]

On Thu, 2014-04-03 at 18:07 +0200, Tim Deegan wrote:
> Newer versions of clang attempt to parse inline assembler even when
> not asked to assemble it.  Wrap our not-for-assembly runes as strings
> of the form ``.ascii "==>MAGIC RUNES<=="'' so clang doesn't choke on
> them.
> 
> While we're at it, assemble more of the final output line in the C
> file, to make the sed expression shorter.
> 
> Reported-by: Julien Grall <julien.grall@linaro.org>
> Suggested-by: Jan Beulich <JBeulich@suse.com>
> Signed-off-by: Tim Deegan <tim@xen.org>

For ARM size, assuming you've build tested it:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.

Re: [PATCH v2] asm-offsets: encode magic asm-offset runes as strings.

[Jan Beulich]

>>> On 03.04.14 at 18:07, <tim@xen.org> wrote:
> Newer versions of clang attempt to parse inline assembler even when
> not asked to assemble it.  Wrap our not-for-assembly runes as strings
> of the form ``.ascii "==>MAGIC RUNES<=="'' so clang doesn't choke on
> them.
> 
> While we're at it, assemble more of the final output line in the C
> file, to make the sed expression shorter.
> 
> Reported-by: Julien Grall <julien.grall@linaro.org>
> Suggested-by: Jan Beulich <JBeulich@suse.com>
> Signed-off-by: Tim Deegan <tim@xen.org>

x86 side
Acked-by: Jan Beulich <jbeulich@suse.com>

Albeit considering the similarity (if not being fully identical) I wonder
whether these definitions shouldn't be moved into a central place.

Jan

> ---
> v2: make up the '#define' in the C source rather then in sed.
> Not quite as neat as I'd hoped: need to match an extra space
> to stop the second sed expression eating the # in #define.
> 
> diff --git a/xen/Makefile b/xen/Makefile
> index 79fa8f2..afbc962 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -150,7 +150,7 @@ include/asm-$(TARGET_ARCH)/asm-offsets.h: 
> arch/$(TARGET_ARCH)/asm-offsets.s
>  	  echo "#ifndef __ASM_OFFSETS_H__"; \
>  	  echo "#define __ASM_OFFSETS_H__"; \
>  	  echo ""; \
> -	  sed -ne "/^->/{s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; 
> s:->::; p;}"; \
> +	  sed -rne "/==>/{s:.*==>(.*)<==.*:\1:; s: [\$$#]: :; p;}"; \
>  	  echo ""; \
>  	  echo "#endif") <$< >$@
>  
> diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c
> index ac628c0..cd1dff7 100644
> --- a/xen/arch/arm/arm32/asm-offsets.c
> +++ b/xen/arch/arm/arm32/asm-offsets.c
> @@ -13,11 +13,12 @@
>  #include <asm/current.h>
>  #include <asm/procinfo.h>
>  
> -#define DEFINE(_sym, _val) \
> -    __asm__ __volatile__ ( "\n->" #_sym " %0 " #_val : : "i" (_val) )
> -#define BLANK() \
> -    __asm__ __volatile__ ( "\n->" : : )
> -#define OFFSET(_sym, _str, _mem) \
> +#define DEFINE(_sym, _val)                                                 
> \
> +    asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
> +                  : : "i" (_val) )
> +#define BLANK()                                                            
> \
> +    asm volatile ( "\n.ascii\"==><==\"" : : )
> +#define OFFSET(_sym, _str, _mem)                                           
> \
>      DEFINE(_sym, offsetof(_str, _mem));
>  
>  void __dummy__(void)
> diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c
> index d7572fa..a3ce816 100644
> --- a/xen/arch/arm/arm64/asm-offsets.c
> +++ b/xen/arch/arm/arm64/asm-offsets.c
> @@ -12,11 +12,12 @@
>  #include <public/xen.h>
>  #include <asm/current.h>
>  
> -#define DEFINE(_sym, _val) \
> -    __asm__ __volatile__ ( "\n->" #_sym " %0 " #_val : : "i" (_val) )
> -#define BLANK() \
> -    __asm__ __volatile__ ( "\n->" : : )
> -#define OFFSET(_sym, _str, _mem) \
> +#define DEFINE(_sym, _val)                                                 
> \
> +    asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
> +                  : : "i" (_val) )
> +#define BLANK()                                                            
> \
> +    asm volatile ( "\n.ascii\"==><==\"" : : )
> +#define OFFSET(_sym, _str, _mem)                                           
> \
>      DEFINE(_sym, offsetof(_str, _mem));
>  
>  void __dummy__(void)
> diff --git a/xen/arch/x86/x86_64/asm-offsets.c 
> b/xen/arch/x86/x86_64/asm-offsets.c
> index b0098b3..9acb648 100644
> --- a/xen/arch/x86/x86_64/asm-offsets.c
> +++ b/xen/arch/x86/x86_64/asm-offsets.c
> @@ -14,11 +14,12 @@
>  #include <asm/hardirq.h>
>  #include <xen/multiboot.h>
>  
> -#define DEFINE(_sym, _val) \
> -    __asm__ __volatile__ ( "\n->" #_sym " %0 " #_val : : "i" (_val) )
> -#define BLANK() \
> -    __asm__ __volatile__ ( "\n->" : : )
> -#define OFFSET(_sym, _str, _mem) \
> +#define DEFINE(_sym, _val)                                                 
> \
> +    asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
> +                  : : "i" (_val) )
> +#define BLANK()                                                            
> \
> +    asm volatile ( "\n.ascii\"==><==\"" : : )
> +#define OFFSET(_sym, _str, _mem)                                           
> \
>      DEFINE(_sym, offsetof(_str, _mem));
>  
>  void __dummy__(void)

Re: [PATCH v2] asm-offsets: encode magic asm-offset runes as strings.

[Julien Grall]

Hi Tim,

I will resend a second version of my clang series soon. Can I add this
patch into it?

Thanks,

On 04/03/2014 05:07 PM, Tim Deegan wrote:
> Newer versions of clang attempt to parse inline assembler even when
> not asked to assemble it.  Wrap our not-for-assembly runes as strings
> of the form ``.ascii "==>MAGIC RUNES<=="'' so clang doesn't choke on
> them.
> 
> While we're at it, assemble more of the final output line in the C
> file, to make the sed expression shorter.
> 
> Reported-by: Julien Grall <julien.grall@linaro.org>
> Suggested-by: Jan Beulich <JBeulich@suse.com>
> Signed-off-by: Tim Deegan <tim@xen.org>
> ---
> v2: make up the '#define' in the C source rather then in sed.
> Not quite as neat as I'd hoped: need to match an extra space
> to stop the second sed expression eating the # in #define.
> 
> diff --git a/xen/Makefile b/xen/Makefile
> index 79fa8f2..afbc962 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -150,7 +150,7 @@ include/asm-$(TARGET_ARCH)/asm-offsets.h: arch/$(TARGET_ARCH)/asm-offsets.s
>  	  echo "#ifndef __ASM_OFFSETS_H__"; \
>  	  echo "#define __ASM_OFFSETS_H__"; \
>  	  echo ""; \
> -	  sed -ne "/^->/{s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; s:->::; p;}"; \
> +	  sed -rne "/==>/{s:.*==>(.*)<==.*:\1:; s: [\$$#]: :; p;}"; \
>  	  echo ""; \
>  	  echo "#endif") <$< >$@
>  
> diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c
> index ac628c0..cd1dff7 100644
> --- a/xen/arch/arm/arm32/asm-offsets.c
> +++ b/xen/arch/arm/arm32/asm-offsets.c
> @@ -13,11 +13,12 @@
>  #include <asm/current.h>
>  #include <asm/procinfo.h>
>  
> -#define DEFINE(_sym, _val) \
> -    __asm__ __volatile__ ( "\n->" #_sym " %0 " #_val : : "i" (_val) )
> -#define BLANK() \
> -    __asm__ __volatile__ ( "\n->" : : )
> -#define OFFSET(_sym, _str, _mem) \
> +#define DEFINE(_sym, _val)                                                 \
> +    asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
> +                  : : "i" (_val) )
> +#define BLANK()                                                            \
> +    asm volatile ( "\n.ascii\"==><==\"" : : )
> +#define OFFSET(_sym, _str, _mem)                                           \
>      DEFINE(_sym, offsetof(_str, _mem));
>  
>  void __dummy__(void)
> diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c
> index d7572fa..a3ce816 100644
> --- a/xen/arch/arm/arm64/asm-offsets.c
> +++ b/xen/arch/arm/arm64/asm-offsets.c
> @@ -12,11 +12,12 @@
>  #include <public/xen.h>
>  #include <asm/current.h>
>  
> -#define DEFINE(_sym, _val) \
> -    __asm__ __volatile__ ( "\n->" #_sym " %0 " #_val : : "i" (_val) )
> -#define BLANK() \
> -    __asm__ __volatile__ ( "\n->" : : )
> -#define OFFSET(_sym, _str, _mem) \
> +#define DEFINE(_sym, _val)                                                 \
> +    asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
> +                  : : "i" (_val) )
> +#define BLANK()                                                            \
> +    asm volatile ( "\n.ascii\"==><==\"" : : )
> +#define OFFSET(_sym, _str, _mem)                                           \
>      DEFINE(_sym, offsetof(_str, _mem));
>  
>  void __dummy__(void)
> diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
> index b0098b3..9acb648 100644
> --- a/xen/arch/x86/x86_64/asm-offsets.c
> +++ b/xen/arch/x86/x86_64/asm-offsets.c
> @@ -14,11 +14,12 @@
>  #include <asm/hardirq.h>
>  #include <xen/multiboot.h>
>  
> -#define DEFINE(_sym, _val) \
> -    __asm__ __volatile__ ( "\n->" #_sym " %0 " #_val : : "i" (_val) )
> -#define BLANK() \
> -    __asm__ __volatile__ ( "\n->" : : )
> -#define OFFSET(_sym, _str, _mem) \
> +#define DEFINE(_sym, _val)                                                 \
> +    asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
> +                  : : "i" (_val) )
> +#define BLANK()                                                            \
> +    asm volatile ( "\n.ascii\"==><==\"" : : )
> +#define OFFSET(_sym, _str, _mem)                                           \
>      DEFINE(_sym, offsetof(_str, _mem));
>  
>  void __dummy__(void)
> 


-- 
Julien Grall

Re: [PATCH v2] asm-offsets: encode magic asm-offset runes as strings.

[Tim Deegan]

At 14:17 +0100 on 23 Apr (1398259077), Julien Grall wrote:
> Hi Tim,
> 
> I will resend a second version of my clang series soon. Can I add this
> patch into it?

Actually it's already acked and was just waiting for me to commit it,
which I've just done.  So, no need. :)

Cheers,

Tim.

Re: [PATCH v2] asm-offsets: encode magic asm-offset runes as strings.

[Julien Grall]

On 04/24/2014 11:45 AM, Tim Deegan wrote:
> At 14:17 +0100 on 23 Apr (1398259077), Julien Grall wrote:
>> Hi Tim,
>>
>> I will resend a second version of my clang series soon. Can I add this
>> patch into it?
> 
> Actually it's already acked and was just waiting for me to commit it,
> which I've just done.  So, no need. :)

Thanks! I will drop patch #2 of my series.


Regards,


-- 
Julien Grall

Re: [PATCH 02/34] xen: clang: Disable built-in assembler

[Julien Grall]

Hi Tim,

On 27/03/14 18:01, Tim Deegan wrote:
>
> The patch below works for me (at least as far as building
> asm-offsets.h on x86) by wrapping everything in a string.  I did try
> just prefixing with '#' but clang 3.5 also strips the comments out.
> That seems unhelpful, since I know some people put comments in their
> inline assembler too. :(

I'm able to build correctly x86 with your patch, and this patch (e.g #2) 
reverted.

But for ARM ... it breaks in another place :(

vfp.c:8:25: error: invalid operand for instruction
     v->arch.vfp.fpexc = READ_CP32(FPEXC);
<inline asm>:1:6: note: instantiated into assembly here
         mrc p10, 7, r1, c8, c0, 0;
             ^

Coprocessor p10 (and p11) are used for Neon instruction are clang 
doesn't allow to use it directly. 
(http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131104/194170.html).

Depends on which part of the ARM ARM you are reading, p10 and p11 should 
not be used directly with mrc/mcr instruction ... but gas accept it.

I guess the best solution is to use directly the VFP instructions but it 
would mean to re-enable VFP at compile time in Xen (see 
http://www.gossamer-threads.com/lists/xen/devel/284653?do=post_view_threaded).

Regards,

-- 
Julien Grall

Re: [PATCH 02/34] xen: clang: Disable built-in assembler

[Jan Beulich]

>>> On 29.03.14 at 23:55, <julien.grall@linaro.org> wrote:
> But for ARM ... it breaks in another place :(
> 
> vfp.c:8:25: error: invalid operand for instruction
>      v->arch.vfp.fpexc = READ_CP32(FPEXC);
> <inline asm>:1:6: note: instantiated into assembly here
>          mrc p10, 7, r1, c8, c0, 0;
>              ^
> 
> Coprocessor p10 (and p11) are used for Neon instruction are clang 
> doesn't allow to use it directly. 
> (http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131104/194170.htm 
> l).
> 
> Depends on which part of the ARM ARM you are reading, p10 and p11 should 
> not be used directly with mrc/mcr instruction ... but gas accept it.
> 
> I guess the best solution is to use directly the VFP instructions but it 
> would mean to re-enable VFP at compile time in Xen (see 
> http://www.gossamer-threads.com/lists/xen/devel/284653?do=post_view_threaded)
> .

Or use .arch_extension around the instruction?

Jan

Re: [PATCH 02/34] xen: clang: Disable built-in assembler

[Ian Campbell]

On Sat, 2014-03-29 at 22:55 +0000, Julien Grall wrote:
> Hi Tim,
> 
> On 27/03/14 18:01, Tim Deegan wrote:
> >
> > The patch below works for me (at least as far as building
> > asm-offsets.h on x86) by wrapping everything in a string.  I did try
> > just prefixing with '#' but clang 3.5 also strips the comments out.
> > That seems unhelpful, since I know some people put comments in their
> > inline assembler too. :(
> 
> I'm able to build correctly x86 with your patch, and this patch (e.g #2) 
> reverted.
> 
> But for ARM ... it breaks in another place :(
> 
> vfp.c:8:25: error: invalid operand for instruction
>      v->arch.vfp.fpexc = READ_CP32(FPEXC);
> <inline asm>:1:6: note: instantiated into assembly here
>          mrc p10, 7, r1, c8, c0, 0;
>              ^
> 
> Coprocessor p10 (and p11) are used for Neon instruction are clang 
> doesn't allow to use it directly. 
> (http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131104/194170.html).
> 
> Depends on which part of the ARM ARM you are reading, p10 and p11 should 
> not be used directly with mrc/mcr instruction ... but gas accept it.
> 
> I guess the best solution is to use directly the VFP instructions but it 
> would mean to re-enable VFP at compile time in Xen (see 
> http://www.gossamer-threads.com/lists/xen/devel/284653?do=post_view_threaded).

Do the VFP instructions have different encodings? I thought this was an
assembler mnemonic difference only.

Ian.

Re: [PATCH 02/34] xen: clang: Disable built-in assembler

[Julien Grall]

On 04/01/2014 02:11 PM, Ian Campbell wrote:
> On Sat, 2014-03-29 at 22:55 +0000, Julien Grall wrote:
>> Hi Tim,
>>
>> On 27/03/14 18:01, Tim Deegan wrote:
>>>
>>> The patch below works for me (at least as far as building
>>> asm-offsets.h on x86) by wrapping everything in a string.  I did try
>>> just prefixing with '#' but clang 3.5 also strips the comments out.
>>> That seems unhelpful, since I know some people put comments in their
>>> inline assembler too. :(
>>
>> I'm able to build correctly x86 with your patch, and this patch (e.g #2) 
>> reverted.
>>
>> But for ARM ... it breaks in another place :(
>>
>> vfp.c:8:25: error: invalid operand for instruction
>>      v->arch.vfp.fpexc = READ_CP32(FPEXC);
>> <inline asm>:1:6: note: instantiated into assembly here
>>          mrc p10, 7, r1, c8, c0, 0;
>>              ^
>>
>> Coprocessor p10 (and p11) are used for Neon instruction are clang 
>> doesn't allow to use it directly. 
>> (http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131104/194170.html).
>>
>> Depends on which part of the ARM ARM you are reading, p10 and p11 should 
>> not be used directly with mrc/mcr instruction ... but gas accept it.
>>
>> I guess the best solution is to use directly the VFP instructions but it 
>> would mean to re-enable VFP at compile time in Xen (see 
>> http://www.gossamer-threads.com/lists/xen/devel/284653?do=post_view_threaded).
> 
> Do the VFP instructions have different encodings? I thought this was an
> assembler mnemonic difference only.

Strictly speaking it works only because vldm* as the same encoding as
ldc*. But the processor will decode the instruction as vldm* (and
execute as it is).

LLVM assembly parser prevents the user to use ldc* in the assembly as
specified by the ARM ARM.

-- 
Julien Grall

Re: [PATCH 02/34] xen: clang: Disable built-in assembler

[Ian Campbell]

On Tue, 2014-04-01 at 15:50 +0100, Julien Grall wrote:
> On 04/01/2014 02:11 PM, Ian Campbell wrote:
> > On Sat, 2014-03-29 at 22:55 +0000, Julien Grall wrote:
> >> Hi Tim,
> >>
> >> On 27/03/14 18:01, Tim Deegan wrote:
> >>>
> >>> The patch below works for me (at least as far as building
> >>> asm-offsets.h on x86) by wrapping everything in a string.  I did try
> >>> just prefixing with '#' but clang 3.5 also strips the comments out.
> >>> That seems unhelpful, since I know some people put comments in their
> >>> inline assembler too. :(
> >>
> >> I'm able to build correctly x86 with your patch, and this patch (e.g #2) 
> >> reverted.
> >>
> >> But for ARM ... it breaks in another place :(
> >>
> >> vfp.c:8:25: error: invalid operand for instruction
> >>      v->arch.vfp.fpexc = READ_CP32(FPEXC);
> >> <inline asm>:1:6: note: instantiated into assembly here
> >>          mrc p10, 7, r1, c8, c0, 0;
> >>              ^
> >>
> >> Coprocessor p10 (and p11) are used for Neon instruction are clang 
> >> doesn't allow to use it directly. 
> >> (http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131104/194170.html).
> >>
> >> Depends on which part of the ARM ARM you are reading, p10 and p11 should 
> >> not be used directly with mrc/mcr instruction ... but gas accept it.
> >>
> >> I guess the best solution is to use directly the VFP instructions but it 
> >> would mean to re-enable VFP at compile time in Xen (see 
> >> http://www.gossamer-threads.com/lists/xen/devel/284653?do=post_view_threaded).
> > 
> > Do the VFP instructions have different encodings? I thought this was an
> > assembler mnemonic difference only.
> 
> Strictly speaking it works only because vldm* as the same encoding as
> ldc*. But the processor will decode the instruction as vldm* (and
> execute as it is).

Are those the instructions in question though? The thing you quoted was
about reading FPEXC which is an mrc cp instruction vs vmrs I think. They
do indeed have identical encodings, so I'm not entirely sure what the
big deal is about which assembler mnemonic gets used is, the process has
no idea what we wrote in the source.

> LLVM assembly parser prevents the user to use ldc* in the assembly as
> specified by the ARM ARM.

Re: [PATCH 02/34] xen: clang: Disable built-in assembler

[Julien Grall]

On 04/01/2014 04:28 PM, Ian Campbell wrote:
> On Tue, 2014-04-01 at 15:50 +0100, Julien Grall wrote:
>> On 04/01/2014 02:11 PM, Ian Campbell wrote:
>>> On Sat, 2014-03-29 at 22:55 +0000, Julien Grall wrote:
>>>> Hi Tim,
>>>>
>>>> On 27/03/14 18:01, Tim Deegan wrote:
>>>>>
>>>>> The patch below works for me (at least as far as building
>>>>> asm-offsets.h on x86) by wrapping everything in a string.  I did try
>>>>> just prefixing with '#' but clang 3.5 also strips the comments out.
>>>>> That seems unhelpful, since I know some people put comments in their
>>>>> inline assembler too. :(
>>>>
>>>> I'm able to build correctly x86 with your patch, and this patch (e.g #2) 
>>>> reverted.
>>>>
>>>> But for ARM ... it breaks in another place :(
>>>>
>>>> vfp.c:8:25: error: invalid operand for instruction
>>>>      v->arch.vfp.fpexc = READ_CP32(FPEXC);
>>>> <inline asm>:1:6: note: instantiated into assembly here
>>>>          mrc p10, 7, r1, c8, c0, 0;
>>>>              ^
>>>>
>>>> Coprocessor p10 (and p11) are used for Neon instruction are clang 
>>>> doesn't allow to use it directly. 
>>>> (http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131104/194170.html).
>>>>
>>>> Depends on which part of the ARM ARM you are reading, p10 and p11 should 
>>>> not be used directly with mrc/mcr instruction ... but gas accept it.
>>>>
>>>> I guess the best solution is to use directly the VFP instructions but it 
>>>> would mean to re-enable VFP at compile time in Xen (see 
>>>> http://www.gossamer-threads.com/lists/xen/devel/284653?do=post_view_threaded).
>>>
>>> Do the VFP instructions have different encodings? I thought this was an
>>> assembler mnemonic difference only.
>>
>> Strictly speaking it works only because vldm* as the same encoding as
>> ldc*. But the processor will decode the instruction as vldm* (and
>> execute as it is).
> 
> Are those the instructions in question though? The thing you quoted was
> about reading FPEXC which is an mrc cp instruction vs vmrs I think. They
> do indeed have identical encodings, so I'm not entirely sure what the
> big deal is about which assembler mnemonic gets used is, the process has
> no idea what we wrote in the source.

Oh sorry, I took one in random. Every VFP instructions has the same
issue. I will raise the bug to Clang. And see if why they choose this
solution.

-- 
Julien Grall

[PATCH 03/34] xen: clang: Support correctly cross-compile

[Julien Grall]

Clang uses "-target" option for cross-compilation.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Keir Fraser <keir@xen.org>
---
 config/StdGNU.mk |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/config/StdGNU.mk b/config/StdGNU.mk
index 25fc594..496e1d9 100644
--- a/config/StdGNU.mk
+++ b/config/StdGNU.mk
@@ -1,7 +1,11 @@
 AS         = $(CROSS_COMPILE)as
 LD         = $(CROSS_COMPILE)ld
 ifeq ($(clang),y)
-CC         = $(CROSS_COMPILE)clang
+ifneq ($(CROSS_COMPILE),)
+CC         = clang -target $(CROSS_COMPILE:-=)
+else
+CC         = clang
+endif
 LD_LTO     = $(CROSS_COMPILE)llvm-ld
 else
 CC         = $(CROSS_COMPILE)gcc
-- 
1.7.10.4

[PATCH 04/34] xen/compiler: Introduce always_unused define

[Julien Grall]

This define will be used to denote function which are never used but contains
usefull code (e.g BUILD_BUG_ON stuff).

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Keir Fraser <keir@xen.org>
---
 xen/include/xen/compiler.h |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 6e07990..07e8b80 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -16,6 +16,8 @@
 
 #define noreturn      __attribute__((noreturn))
 
+#define always_unused __attribute__((unused))
+
 #define __packed      __attribute__((packed))
 
 #if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__ < 5))
-- 
1.7.10.4

Re: [PATCH 04/34] xen/compiler: Introduce always_unused define

[Stefano Stabellini]

On Tue, 25 Mar 2014, Julien Grall wrote:
> This define will be used to denote function which are never used but contains
                                       ^functions                      ^contain
> usefull code (e.g BUILD_BUG_ON stuff).
  ^ useful

> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Keir Fraser <keir@xen.org>
> ---
>  xen/include/xen/compiler.h |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
> index 6e07990..07e8b80 100644
> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -16,6 +16,8 @@
>  
>  #define noreturn      __attribute__((noreturn))
>  
> +#define always_unused __attribute__((unused))
> +
>  #define __packed      __attribute__((packed))
>  
>  #if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__ < 5))
> -- 
> 1.7.10.4
> 

Re: [PATCH 04/34] xen/compiler: Introduce always_unused define

[Julien Grall]

Hi Stefano,

On 03/25/2014 05:34 PM, Stefano Stabellini wrote:
> On Tue, 25 Mar 2014, Julien Grall wrote:
>> This define will be used to denote function which are never used but contains
>                                        ^functions                      ^contain
>> usefull code (e.g BUILD_BUG_ON stuff).
>   ^ useful

I will fix all the typoes on the next version.

Regards,

-- 
Julien Grall

[PATCH 05/34] xen/xsm: flask: Remove unused function avc_sidcmp

[Julien Grall]

Fix compilation with Clang 3.5:

avc.c:657:19: error: unused function 'avc_sidcmp' [-Werror,-Wunused-function]
static inline int avc_sidcmp(u32 x, u32 y)
                      ^

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 xen/xsm/flask/avc.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/xen/xsm/flask/avc.c b/xen/xsm/flask/avc.c
index a26b34a..fc6580e 100644
--- a/xen/xsm/flask/avc.c
+++ b/xen/xsm/flask/avc.c
@@ -654,11 +654,6 @@ int avc_add_callback(int (*callback)(u32 event, u32 ssid, u32 tsid, u16 tclass,
     return rc;
 }
 
-static inline int avc_sidcmp(u32 x, u32 y)
-{
-    return (x == y || x == SECSID_WILD || y == SECSID_WILD);
-}
-
 /**
  * avc_update_node Update an AVC entry
  * @event : Updating event
-- 
1.7.10.4

Re: [PATCH 05/34] xen/xsm: flask: Remove unused function avc_sidcmp

[Daniel De Graaf]

On 03/25/2014 12:55 PM, Julien Grall wrote:
> Fix compilation with Clang 3.5:
>
> avc.c:657:19: error: unused function 'avc_sidcmp' [-Werror,-Wunused-function]
> static inline int avc_sidcmp(u32 x, u32 y)
>                        ^
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH 05/34] xen/xsm: flask: Remove unused function avc_sidcmp

[Jan Beulich]

>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
> Fix compilation with Clang 3.5:
> 
> avc.c:657:19: error: unused function 'avc_sidcmp' [-Werror,-Wunused-function]
> static inline int avc_sidcmp(u32 x, u32 y)

Despite Daniel having acked this already, this seems conceptually wrong
to me: static inline functions are quite frequently unused (and I assume
the compiler warns about them only if they're not in a header file), and
hence the compiler shouldn't be warning about them in the first place.

Jan

Re: [PATCH 05/34] xen/xsm: flask: Remove unused function avc_sidcmp

[Julien Grall]

On 03/26/2014 11:57 AM, Jan Beulich wrote:
>>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
>> Fix compilation with Clang 3.5:
>>
>> avc.c:657:19: error: unused function 'avc_sidcmp' [-Werror,-Wunused-function]
>> static inline int avc_sidcmp(u32 x, u32 y)
> 
> Despite Daniel having acked this already, this seems conceptually wrong
> to me: static inline functions are quite frequently unused (and I assume
> the compiler warns about them only if they're not in a header file), and
> hence the compiler shouldn't be warning about them in the first place.

This function has not been used seen 2007. I think this is the only
static inline function not defined in headers which is not used.

Why shouldn't the compiler warn about it? Rather than static inline
function defined in the header, this kind function is dead code. If we
want to keep it we should at least mark them as unused.

IHMO I don't think we need to keep function that weren't used since ages
and I bet it was a forgotten when the code was reworked a long time ago.

Regards,

-- 
Julien Grall

Re: [PATCH 05/34] xen/xsm: flask: Remove unused function avc_sidcmp

[Jan Beulich]

>>> On 26.03.14 at 17:11, <julien.grall@linaro.org> wrote:
> On 03/26/2014 11:57 AM, Jan Beulich wrote:
>>>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
>>> Fix compilation with Clang 3.5:
>>>
>>> avc.c:657:19: error: unused function 'avc_sidcmp' [-Werror,-Wunused-function]
>>> static inline int avc_sidcmp(u32 x, u32 y)
>> 
>> Despite Daniel having acked this already, this seems conceptually wrong
>> to me: static inline functions are quite frequently unused (and I assume
>> the compiler warns about them only if they're not in a header file), and
>> hence the compiler shouldn't be warning about them in the first place.
> 
> This function has not been used seen 2007. I think this is the only
> static inline function not defined in headers which is not used.
> 
> Why shouldn't the compiler warn about it? Rather than static inline
> function defined in the header, this kind function is dead code. If we
> want to keep it we should at least mark them as unused.
> 
> IHMO I don't think we need to keep function that weren't used since ages
> and I bet it was a forgotten when the code was reworked a long time ago.

Yes, and my comment wasn't about this specific function, but the
general pattern: You justified your change with the build breakage,
whereas you could have stated what you said just now in your
reply. IOW I'm fine with you cleaning up things that were more or
less obviously forgotten in an earlier change. But code adjustments
just to satisfy an overly picky compiler aren't that nice. After all
such functions can serve a purpose (and I think if you looked around
you'd find other unused stuff that could be deleted yet - so far - isn't
being, as being potentially useful in the future), and the compiler here
- from what I can tell - is warning on these simply because they aren't
in header files. Which in turn raises the question how the compiler
knows what a header file is (remember that we have quite a few
instances of .c files including other .c files, and I'd bet the compiler
treats these as header files too). Summary: Likely the compiler is
issuing this sort of warning inconsistently, and hence it would better
not issue the warning at all (or at least provide a way to suppress it).

Jan

Re: [PATCH 05/34] xen/xsm: flask: Remove unused function avc_sidcmp

[Julien Grall]

On 03/26/2014 04:42 PM, Jan Beulich wrote:
>>>> On 26.03.14 at 17:11, <julien.grall@linaro.org> wrote:
>> On 03/26/2014 11:57 AM, Jan Beulich wrote:
>>>>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
>>>> Fix compilation with Clang 3.5:
>>>>
>>>> avc.c:657:19: error: unused function 'avc_sidcmp' [-Werror,-Wunused-function]
>>>> static inline int avc_sidcmp(u32 x, u32 y)
>>>
>>> Despite Daniel having acked this already, this seems conceptually wrong
>>> to me: static inline functions are quite frequently unused (and I assume
>>> the compiler warns about them only if they're not in a header file), and
>>> hence the compiler shouldn't be warning about them in the first place.
>>
>> This function has not been used seen 2007. I think this is the only
>> static inline function not defined in headers which is not used.
>>
>> Why shouldn't the compiler warn about it? Rather than static inline
>> function defined in the header, this kind function is dead code. If we
>> want to keep it we should at least mark them as unused.
>>
>> IHMO I don't think we need to keep function that weren't used since ages
>> and I bet it was a forgotten when the code was reworked a long time ago.
> 
> Yes, and my comment wasn't about this specific function, but the
> general pattern: You justified your change with the build breakage,
> whereas you could have stated what you said just now in your
> reply. IOW I'm fine with you cleaning up things that were more or
> less obviously forgotten in an earlier change. But code adjustments
> just to satisfy an overly picky compiler aren't that nice. After all
> such functions can serve a purpose (and I think if you looked around
> you'd find other unused stuff that could be deleted yet - so far - isn't
> being, as being potentially useful in the future), and the compiler here
> - from what I can tell - is warning on these simply because they aren't
> in header files. Which in turn raises the question how the compiler
> knows what a header file is (remember that we have quite a few
> instances of .c files including other .c files, and I'd bet the compiler
> treats these as header files too). Summary: Likely the compiler is
> issuing this sort of warning inconsistently, and hence it would better
> not issue the warning at all (or at least provide a way to suppress it).

Thanks for your comment. I will update the different commit message.

The new version of clang has amazing feature like guard checking (see
patch #17)... I was wondering the same thing when I first discovered
this kind of errors.

I guess with the '# 1 "/local/.../.h" hints the compiler is able to know
where does the error come from.

I will try to find why clang is considering static inline invalid in .c
context.

-- 
Julien Grall

Re: [PATCH 05/34] xen/xsm: flask: Remove unused function avc_sidcmp

[Julien Grall]

On 03/26/2014 05:06 PM, Julien Grall wrote:
> On 03/26/2014 04:42 PM, Jan Beulich wrote:
>>>>> On 26.03.14 at 17:11, <julien.grall@linaro.org> wrote:
>>> On 03/26/2014 11:57 AM, Jan Beulich wrote:
>>>>>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
>>>>> Fix compilation with Clang 3.5:
>>>>>
>>>>> avc.c:657:19: error: unused function 'avc_sidcmp' [-Werror,-Wunused-function]
>>>>> static inline int avc_sidcmp(u32 x, u32 y)
>>>>
>>>> Despite Daniel having acked this already, this seems conceptually wrong
>>>> to me: static inline functions are quite frequently unused (and I assume
>>>> the compiler warns about them only if they're not in a header file), and
>>>> hence the compiler shouldn't be warning about them in the first place.
>>>
>>> This function has not been used seen 2007. I think this is the only
>>> static inline function not defined in headers which is not used.
>>>
>>> Why shouldn't the compiler warn about it? Rather than static inline
>>> function defined in the header, this kind function is dead code. If we
>>> want to keep it we should at least mark them as unused.
>>>
>>> IHMO I don't think we need to keep function that weren't used since ages
>>> and I bet it was a forgotten when the code was reworked a long time ago.
>>
>> Yes, and my comment wasn't about this specific function, but the
>> general pattern: You justified your change with the build breakage,
>> whereas you could have stated what you said just now in your
>> reply. IOW I'm fine with you cleaning up things that were more or
>> less obviously forgotten in an earlier change. But code adjustments
>> just to satisfy an overly picky compiler aren't that nice. After all
>> such functions can serve a purpose (and I think if you looked around
>> you'd find other unused stuff that could be deleted yet - so far - isn't
>> being, as being potentially useful in the future), and the compiler here
>> - from what I can tell - is warning on these simply because they aren't
>> in header files. Which in turn raises the question how the compiler
>> knows what a header file is (remember that we have quite a few
>> instances of .c files including other .c files, and I'd bet the compiler
>> treats these as header files too). Summary: Likely the compiler is
>> issuing this sort of warning inconsistently, and hence it would better
>> not issue the warning at all (or at least provide a way to suppress it).
> 
> Thanks for your comment. I will update the different commit message.
> 
> The new version of clang has amazing feature like guard checking (see
> patch #17)... I was wondering the same thing when I first discovered
> this kind of errors.
> 
> I guess with the '# 1 "/local/.../.h" hints the compiler is able to know
> where does the error come from.
> 
> I will try to find why clang is considering static inline invalid in .c
> context.
> 

Here we go, this has been explicitly added in clang a while ago:

commit 39bd371610af27b073c792c54c6c28133329f6ad
Date:   Tue Sep 10 03:05:56 2013 +0000

    Make -Wunused warning rules more consistent.
    
    This patch does a few different things.
    
    This patch improves unused var diags for const vars: we no longer
    unconditionally suppress diagnostics for const vars, instead only suppressing
    the diagnostic when the declaration appears to be useful.
    
    This patch also makes us more consistently use whether a variable/function
    is declared in the main file to suppress diagnostics where appropriate.
    
    Fixes <rdar://problem/14907887>.
    
    git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@190382 91177308-0d34-0410-b5e6-96231b3b80d8

-- 
Julien Grall

[PATCH 06/34] xen/x86: shadow: sh_next_page is only used when GUEST_PAGING_LEVELS = 2

[Julien Grall]

Fix compilation with clang 3.5:

multi.c:429:21: error: unused function 'sh_next_page' [-Werror,-Wunused-function]
static inline mfn_t sh_next_page(mfn_t smfn)
                    ^

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/mm/shadow/multi.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 9dfa345..c468d4d 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -425,6 +425,7 @@ sh_guest_get_eff_l1e(struct vcpu *v, unsigned long addr, void *eff_l1e)
  * 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)
 {
@@ -443,6 +444,7 @@ static inline mfn_t sh_next_page(mfn_t smfn)
     ASSERT(!mfn_to_page(next)->u.sh.head);
     return next;
 }
+#endif
 
 static inline u32
 guest_index(void *ptr)
-- 
1.7.10.4

[PATCH 07/34] xen/common: sched_sedf: Remove unused functions

[Julien Grall]

Clang 3.5 will fail to compile xen/common/sched_sedf.c because some function
are not used:

sched_sedf.c:141:20: error: unused function 'extraq_add_head' [-Werror,-Wunused-function]
static inline void extraq_add_head(struct vcpu *d, int i)
                       ^
sched_sedf.c:147:20: error: unused function 'extraq_add_tail' [-Werror,-Wunused-function]
static inline void extraq_add_tail(struct vcpu *d, int i)

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/common/sched_sedf.c |   12 ------------
 1 file changed, 12 deletions(-)

diff --git a/xen/common/sched_sedf.c b/xen/common/sched_sedf.c
index 7c24171..0c9011a 100644
--- a/xen/common/sched_sedf.c
+++ b/xen/common/sched_sedf.c
@@ -138,18 +138,6 @@ static inline int extraq_on(struct vcpu *d, int i)
             (EXTRALIST(d,i)->next != EXTRALIST(d,i)));
 }
 
-static inline void extraq_add_head(struct vcpu *d, int i)
-{
-    list_add(EXTRALIST(d,i), EXTRAQ(d->processor,i));
-    ASSERT(extraq_on(d, i));
-}
-
-static inline void extraq_add_tail(struct vcpu *d, int i)
-{
-    list_add_tail(EXTRALIST(d,i), EXTRAQ(d->processor,i));
-    ASSERT(extraq_on(d, i));
-}
-
 static inline void extraq_del(struct vcpu *d, int i)
 {
     struct list_head *list = EXTRALIST(d,i);
-- 
1.7.10.4

Re: [PATCH 07/34] xen/common: sched_sedf: Remove unused functions

[George Dunlap]

On 03/25/2014 04:55 PM, Julien Grall wrote:
> Clang 3.5 will fail to compile xen/common/sched_sedf.c because some function
> are not used:
>
> sched_sedf.c:141:20: error: unused function 'extraq_add_head' [-Werror,-Wunused-function]
> static inline void extraq_add_head(struct vcpu *d, int i)
>                         ^
> sched_sedf.c:147:20: error: unused function 'extraq_add_tail' [-Werror,-Wunused-function]
> static inline void extraq_add_tail(struct vcpu *d, int i)
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

> ---
>   xen/common/sched_sedf.c |   12 ------------
>   1 file changed, 12 deletions(-)
>
> diff --git a/xen/common/sched_sedf.c b/xen/common/sched_sedf.c
> index 7c24171..0c9011a 100644
> --- a/xen/common/sched_sedf.c
> +++ b/xen/common/sched_sedf.c
> @@ -138,18 +138,6 @@ static inline int extraq_on(struct vcpu *d, int i)
>               (EXTRALIST(d,i)->next != EXTRALIST(d,i)));
>   }
>
> -static inline void extraq_add_head(struct vcpu *d, int i)
> -{
> -    list_add(EXTRALIST(d,i), EXTRAQ(d->processor,i));
> -    ASSERT(extraq_on(d, i));
> -}
> -
> -static inline void extraq_add_tail(struct vcpu *d, int i)
> -{
> -    list_add_tail(EXTRALIST(d,i), EXTRAQ(d->processor,i));
> -    ASSERT(extraq_on(d, i));
> -}
> -
>   static inline void extraq_del(struct vcpu *d, int i)
>   {
>       struct list_head *list = EXTRALIST(d,i);
>

[PATCH 08/34] xen/common: rcupdate: Remove unused function rcu_batch_after

[Julien Grall]

Clang 3.5 will fail to compile xen/common/rcupdate.c because rcu_batch_after
is statically defined but not used:

rcupdate.c:146:19: error: unused function 'rcu_batch_after' [-Werror,-Wunused-function]
static inline int rcu_batch_after(long a, long b)

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Keir Fraser <keir@xen.org>
---
 xen/common/rcupdate.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
index e9979cd..f534c1e 100644
--- a/xen/common/rcupdate.c
+++ b/xen/common/rcupdate.c
@@ -142,12 +142,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)
 {
-- 
1.7.10.4

[PATCH 09/34] xen/common: tmem: Remove dumb check in do_tmem_destroy_pool

[Julien Grall]

do_tmem_destroy_pool is checking if pools == NULL. But, pools is a fixed
array.

Clang 3.5 will fail to compile xen/common/tmem.c with the following error:
tmem.c:1848:18: error: comparison of array 'client->pools' equal to a null pointer is always false [-Werror,-Wtautological-pointer-compare]
    if ( client->pools == NULL )

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/common/tmem.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/common/tmem.c b/xen/common/tmem.c
index 02e7e2e..0a24b3f 100644
--- a/xen/common/tmem.c
+++ b/xen/common/tmem.c
@@ -1845,8 +1845,6 @@ static int do_tmem_destroy_pool(uint32_t pool_id)
     struct client *client = current->domain->tmem_client;
     struct tmem_pool *pool;
 
-    if ( client->pools == NULL )
-        return 0;
     if ( pool_id >= MAX_POOLS_PER_DOMAIN )
         return 0;
     if ( (pool = client->pools[pool_id]) == NULL )
-- 
1.7.10.4

Re: [PATCH 09/34] xen/common: tmem: Remove dumb check in do_tmem_destroy_pool

[Andrew Cooper]

On 25/03/14 16:55, Julien Grall wrote:
> do_tmem_destroy_pool is checking if pools == NULL. But, pools is a fixed
> array.
>
> Clang 3.5 will fail to compile xen/common/tmem.c with the following error:
> tmem.c:1848:18: error: comparison of array 'client->pools' equal to a null pointer is always false [-Werror,-Wtautological-pointer-compare]
>     if ( client->pools == NULL )

Coverity-ID:1055632

~Andrew

> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/common/tmem.c |    2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
> index 02e7e2e..0a24b3f 100644
> --- a/xen/common/tmem.c
> +++ b/xen/common/tmem.c
> @@ -1845,8 +1845,6 @@ static int do_tmem_destroy_pool(uint32_t pool_id)
>      struct client *client = current->domain->tmem_client;
>      struct tmem_pool *pool;
>  
> -    if ( client->pools == NULL )
> -        return 0;
>      if ( pool_id >= MAX_POOLS_PER_DOMAIN )
>          return 0;
>      if ( (pool = client->pools[pool_id]) == NULL )

Re: [PATCH 09/34] xen/common: tmem: Remove dumb check in do_tmem_destroy_pool

[Julien Grall]

Hi Andrew,

I just saw that a patch was already sent few months ago
(http://lists.xenproject.org/archives/html/xen-devel/2013-11/msg03664.html),
but never upstreamed.

Konrad, I'm fine to drop this patch if you plan to send your series.

Regards,

On 03/25/2014 06:05 PM, Andrew Cooper wrote:
> On 25/03/14 16:55, Julien Grall wrote:
>> do_tmem_destroy_pool is checking if pools == NULL. But, pools is a fixed
>> array.
>>
>> Clang 3.5 will fail to compile xen/common/tmem.c with the following error:
>> tmem.c:1848:18: error: comparison of array 'client->pools' equal to a null pointer is always false [-Werror,-Wtautological-pointer-compare]
>>     if ( client->pools == NULL )
> 
> Coverity-ID:1055632
> 
> ~Andrew
> 
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> ---
>>  xen/common/tmem.c |    2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
>> index 02e7e2e..0a24b3f 100644
>> --- a/xen/common/tmem.c
>> +++ b/xen/common/tmem.c
>> @@ -1845,8 +1845,6 @@ static int do_tmem_destroy_pool(uint32_t pool_id)
>>      struct client *client = current->domain->tmem_client;
>>      struct tmem_pool *pool;
>>  
>> -    if ( client->pools == NULL )
>> -        return 0;
>>      if ( pool_id >= MAX_POOLS_PER_DOMAIN )
>>          return 0;
>>      if ( (pool = client->pools[pool_id]) == NULL )
> 


-- 
Julien Grall

Re: [PATCH 09/34] xen/common: tmem: Remove dumb check in do_tmem_destroy_pool

[Konrad Rzeszutek Wilk]

On Tue, Mar 25, 2014 at 06:18:21PM +0000, Julien Grall wrote:
> Hi Andrew,
> 
> I just saw that a patch was already sent few months ago
> (http://lists.xenproject.org/archives/html/xen-devel/2013-11/msg03664.html),
> but never upstreamed.
> 
> Konrad, I'm fine to drop this patch if you plan to send your series.

Nah, lets use this patch. Thanks!

Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

> 
> Regards,
> 
> On 03/25/2014 06:05 PM, Andrew Cooper wrote:
> > On 25/03/14 16:55, Julien Grall wrote:
> >> do_tmem_destroy_pool is checking if pools == NULL. But, pools is a fixed
> >> array.
> >>
> >> Clang 3.5 will fail to compile xen/common/tmem.c with the following error:
> >> tmem.c:1848:18: error: comparison of array 'client->pools' equal to a null pointer is always false [-Werror,-Wtautological-pointer-compare]
> >>     if ( client->pools == NULL )
> > 
> > Coverity-ID:1055632
> > 
> > ~Andrew
> > 
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> ---
> >>  xen/common/tmem.c |    2 --
> >>  1 file changed, 2 deletions(-)
> >>
> >> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
> >> index 02e7e2e..0a24b3f 100644
> >> --- a/xen/common/tmem.c
> >> +++ b/xen/common/tmem.c
> >> @@ -1845,8 +1845,6 @@ static int do_tmem_destroy_pool(uint32_t pool_id)
> >>      struct client *client = current->domain->tmem_client;
> >>      struct tmem_pool *pool;
> >>  
> >> -    if ( client->pools == NULL )
> >> -        return 0;
> >>      if ( pool_id >= MAX_POOLS_PER_DOMAIN )
> >>          return 0;
> >>      if ( (pool = client->pools[pool_id]) == NULL )
> > 
> 
> 
> -- 
> Julien Grall
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 09/34] xen/common: tmem: Remove dumb check in do_tmem_destroy_pool [and 2 more messages]

[Ian Jackson]

On 25/03/14 16:55, Julien Grall wrote:
> do_tmem_destroy_pool is checking if pools == NULL. But, pools is a fixed
> array.

This patch (committed to staging as ac0f56a2) has turned up in my
backport list somehow, even though (a) it wouldn't be my
responsibility as it's a hypervisor patch and (b) I can find no trace
of it in my mailbox to explain why.

My backport list only records the identity of the patch and the date
when I added it to the list (8th of April 2014 in this case).  I
normally use my email to find out why.

Anyway, I thought I'd email Jan as stable maintainer to decide what to
do about it.  I'm removing it from my own backport list.

Ian.

Re: [PATCH 09/34] xen/common: tmem: Remove dumb check in do_tmem_destroy_pool [and 2 more messages]

[Jan Beulich]

>>> On 22.05.14 at 18:01, <Ian.Jackson@eu.citrix.com> wrote:
> On 25/03/14 16:55, Julien Grall wrote:
>> do_tmem_destroy_pool is checking if pools == NULL. But, pools is a fixed
>> array.
> 
> This patch (committed to staging as ac0f56a2) has turned up in my
> backport list somehow, even though (a) it wouldn't be my
> responsibility as it's a hypervisor patch and (b) I can find no trace
> of it in my mailbox to explain why.
> 
> My backport list only records the identity of the patch and the date
> when I added it to the list (8th of April 2014 in this case).  I
> normally use my email to find out why.

Perhaps just a one-off in picking commit IDs from git log output?

> Anyway, I thought I'd email Jan as stable maintainer to decide what to
> do about it.  I'm removing it from my own backport list.

I see no reason to apply any tmem patches to stable trees until
tmem's status changes back to supported, or unless being urged
by the tmem maintainer.

Jan

Re: [PATCH 09/34] xen/common: tmem: Remove dumb check in do_tmem_destroy_pool [and 2 more messages]

[Ian Jackson]

Jan Beulich writes ("Re: [Xen-devel] [PATCH 09/34] xen/common: tmem: Remove dumb check in do_tmem_destroy_pool [and 2 more messages]"):
> On 22.05.14 at 18:01, <Ian.Jackson@eu.citrix.com> wrote:
> > On 25/03/14 16:55, Julien Grall wrote:
> > This patch (committed to staging as ac0f56a2) has turned up in my
> > backport list somehow, even though (a) it wouldn't be my
> > responsibility as it's a hypervisor patch and (b) I can find no trace
> > of it in my mailbox to explain why.
> > 
> > My backport list only records the identity of the patch and the date
> > when I added it to the list (8th of April 2014 in this case).  I
> > normally use my email to find out why.
> 
> Perhaps just a one-off in picking commit IDs from git log output?

I thought of that but the adjacent commits are not candidates either.

> > Anyway, I thought I'd email Jan as stable maintainer to decide what to
> > do about it.  I'm removing it from my own backport list.
> 
> I see no reason to apply any tmem patches to stable trees until
> tmem's status changes back to supported, or unless being urged
> by the tmem maintainer.

Right.

Thanks and sorry for the noise.

Ian.

Re: [PATCH 09/34] xen/common: tmem: Remove dumb check in do_tmem_destroy_pool [and 2 more messages]

[Jan Beulich]

>>> On 23.05.14 at 12:23, <Ian.Jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [Xen-devel] [PATCH 09/34] xen/common: tmem: Remove 
> dumb check in do_tmem_destroy_pool [and 2 more messages]"):
>> On 22.05.14 at 18:01, <Ian.Jackson@eu.citrix.com> wrote:
>> > On 25/03/14 16:55, Julien Grall wrote:
>> > This patch (committed to staging as ac0f56a2) has turned up in my
>> > backport list somehow, even though (a) it wouldn't be my
>> > responsibility as it's a hypervisor patch and (b) I can find no trace
>> > of it in my mailbox to explain why.
>> > 
>> > My backport list only records the identity of the patch and the date
>> > when I added it to the list (8th of April 2014 in this case).  I
>> > normally use my email to find out why.
>> 
>> Perhaps just a one-off in picking commit IDs from git log output?
> 
> I thought of that but the adjacent commits are not candidates either.
> 
>> > Anyway, I thought I'd email Jan as stable maintainer to decide what to
>> > do about it.  I'm removing it from my own backport list.
>> 
>> I see no reason to apply any tmem patches to stable trees until
>> tmem's status changes back to supported, or unless being urged
>> by the tmem maintainer.
> 
> Right.

But I saw you applied it to all trees already...

Jan

[PATCH 10/34] xen/crypto: rijndael: Fix compilation with Clang 3.5

[Julien Grall]

Td0, Td1, Td2, Td3, Td4 are only used when NEED_RIJNDAEL is defined.

rijndael.c:383:18: error: unused variable 'Td0' [-Werror,-Wunused-const-variable]
static const u32 Td0[256] = {
                 ^
rijndael.c:449:18: error: unused variable 'Td1' [-Werror,-Wunused-const-variable]
static const u32 Td1[256] = {
                 ^
rijndael.c:515:18: error: unused variable 'Td2' [-Werror,-Wunused-const-variable]
static const u32 Td2[256] = {
                 ^
rijndael.c:581:18: error: unused variable 'Td3' [-Werror,-Wunused-const-variable]
static const u32 Td3[256] = {
                 ^
rijndael.c:647:18: error: unused variable 'Td4' [-Werror,-Wunused-const-variable]
static const u32 Td4[256] = {

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Keir Fraser <keir@xen.org>
---
 xen/crypto/rijndael.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/crypto/rijndael.c b/xen/crypto/rijndael.c
index b7cd0d1..9de0992 100644
--- a/xen/crypto/rijndael.c
+++ b/xen/crypto/rijndael.c
@@ -380,6 +380,8 @@ static const u32 Te4[256] = {
     0x41414141U, 0x99999999U, 0x2d2d2d2dU, 0x0f0f0f0fU,
     0xb0b0b0b0U, 0x54545454U, 0xbbbbbbbbU, 0x16161616U,
 };
+
+#ifdef NEED_RIJNDAEL_DECRYPT
 static const u32 Td0[256] = {
     0x51f4a750U, 0x7e416553U, 0x1a17a4c3U, 0x3a275e96U,
     0x3bab6bcbU, 0x1f9d45f1U, 0xacfa58abU, 0x4be30393U,
@@ -710,6 +712,7 @@ static const u32 Td4[256] = {
     0xe1e1e1e1U, 0x69696969U, 0x14141414U, 0x63636363U,
     0x55555555U, 0x21212121U, 0x0c0c0c0cU, 0x7d7d7d7dU,
 };
+#endif
 static const u32 rcon[] = {
 	0x01000000, 0x02000000, 0x04000000, 0x08000000,
 	0x10000000, 0x20000000, 0x40000000, 0x80000000,
-- 
1.7.10.4

Re: [PATCH 10/34] xen/crypto: rijndael: Fix compilation with Clang 3.5

[Keir Fraser]

Julien Grall wrote:
> Td0,  Td1,  Td2,  Td3,  Td4  are only used when NEED_RIJNDAEL is defined.
>
> rijndael.c:383:18: error: unused variable 'Td0' [-Werror,-Wunused-const-variable]
> static const u32 Td0[256] = {
>                   ^
> rijndael.c:449:18: error: unused variable 'Td1' [-Werror,-Wunused-const-variable]
> static const u32 Td1[256] = {
>                   ^
> rijndael.c:515:18: error: unused variable 'Td2' [-Werror,-Wunused-const-variable]
> static const u32 Td2[256] = {
>                   ^
> rijndael.c:581:18: error: unused variable 'Td3' [-Werror,-Wunused-const-variable]
> static const u32 Td3[256] = {
>                   ^
> rijndael.c:647:18: error: unused variable 'Td4' [-Werror,-Wunused-const-variable]
> static const u32 Td4[256] = {
>
> Signed-off-by: Julien Grall<julien.grall@linaro.org>
> Cc: Keir Fraser<keir@xen.org>
Acked-by: Keir Fraser <keir@xen.org>

[PATCH 11/34] xen/arm: Introduce __builtin_stack_pointer

[Julien Grall]

Clang doesn't support named register. Introduce __builtin_stack_pointer
to be able to use named register with gcc.

In file included from arm32/asm-offsets.c:10:
In file included from xen/xen/include/xen/sched.h:18:
In file included from xen/xen/include/xen/smp.h:4:
In file included from xen/xen/include/asm/smp.h:8:
xen/xen/include/asm/current.h:31:33: error: variable 'sp' is uninitialized when used here [-Werror,-Wuninitialized]
    return (struct cpu_info *)((sp & ~(STACK_SIZE - 1)) + STACK_SIZE - sizeof(struct cpu_info));
                                ^~
xen/xen/include/asm/current.h:30:30: note: initialize the variable 'sp' to silence this warning
    register unsigned long sp asm ("sp");
                             ^
                              = 0

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/include/asm-arm/current.h |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/current.h b/xen/include/asm-arm/current.h
index 65c0cdf..0d4a8c0 100644
--- a/xen/include/asm-arm/current.h
+++ b/xen/include/asm-arm/current.h
@@ -25,10 +25,23 @@ struct cpu_info {
     unsigned int pad;
 };
 
+#ifdef __clang__
+#define __builtin_stack_pointer() ({                \
+    unsigned long current_sp;                       \
+    asm ("mov %0, sp" : "=r" (current_sp));         \
+    current_sp;                                     \
+})
+#else
+#define __builtin_stack_pointer() ({                \
+    register unsigned long current_sp asm ("sp");   \
+    current_sp;                                     \
+})
+#endif
+
 static inline struct cpu_info *get_cpu_info(void)
 {
-    register unsigned long sp asm ("sp");
-    return (struct cpu_info *)((sp & ~(STACK_SIZE - 1)) + STACK_SIZE - sizeof(struct cpu_info));
+    return (struct cpu_info *)((__builtin_stack_pointer() & ~(STACK_SIZE - 1)) +
+                               STACK_SIZE - sizeof(struct cpu_info));
 }
 
 #define guest_cpu_user_regs() (&get_cpu_info()->guest_cpu_user_regs)
-- 
1.7.10.4

Re: [PATCH 11/34] xen/arm: Introduce __builtin_stack_pointer

[Ian Campbell]

On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> Clang doesn't support named register. Introduce __builtin_stack_pointer
> to be able to use named register with gcc.

I think gcc considers the __builtin namespace to be its own, not sure
about clang.

I think get_stack_pointer() would be a fine name for this macro. It
seems like the clang version should work for both gcc and clang.

Google seems to suggest that __builtin_stack_pointer might become a real
compiler builtin at some point.

Re: [PATCH 11/34] xen/arm: Introduce __builtin_stack_pointer

[Julien Grall]

Hi Ian,

On 03/25/2014 05:18 PM, Ian Campbell wrote:
> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
>> Clang doesn't support named register. Introduce __builtin_stack_pointer
>> to be able to use named register with gcc.
> 
> I think gcc considers the __builtin namespace to be its own, not sure
> about clang.

I'm able to compile on GCC without any issue.

> I think get_stack_pointer() would be a fine name for this macro. It
> seems like the clang version should work for both gcc and clang.
> 
> Google seems to suggest that __builtin_stack_pointer might become a real
> compiler builtin at some point.

I took the idea to the llvmlinux project:
http://git.linuxfoundation.org/?p=llvmlinux.git;a=blob;f=arch/arm/patches/current_stack_pointer_arm.patch;

But I can rename into get_stack_pointer for the next version.

Regards,

-- 
Julien Grall

Re: [PATCH 11/34] xen/arm: Introduce __builtin_stack_pointer

[Ian Campbell]

On Tue, 2014-03-25 at 18:01 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 03/25/2014 05:18 PM, Ian Campbell wrote:
> > On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> >> Clang doesn't support named register. Introduce __builtin_stack_pointer
> >> to be able to use named register with gcc.
> > 
> > I think gcc considers the __builtin namespace to be its own, not sure
> > about clang.
> 
> I'm able to compile on GCC without any issue.

That's not the point. The point of a namespacing rule is that gcc can
add such a function at any time it likes, and if that breaks your
application then that is tough luck.

> > I think get_stack_pointer() would be a fine name for this macro. It
> > seems like the clang version should work for both gcc and clang.
> > 
> > Google seems to suggest that __builtin_stack_pointer might become a real
> > compiler builtin at some point.
> 
> I took the idea to the llvmlinux project:
> http://git.linuxfoundation.org/?p=llvmlinux.git;a=blob;f=arch/arm/patches/current_stack_pointer_arm.patch;
> 
> But I can rename into get_stack_pointer for the next version.

Sounds good, thanks.

Ian.

Re: [PATCH 11/34] xen/arm: Introduce __builtin_stack_pointer

[Julien Grall]

On 26/03/14 10:31, Ian Campbell wrote:
> On Tue, 2014-03-25 at 18:01 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 03/25/2014 05:18 PM, Ian Campbell wrote:
>>> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
>>>> Clang doesn't support named register. Introduce __builtin_stack_pointer
>>>> to be able to use named register with gcc.
>>>
>>> I think gcc considers the __builtin namespace to be its own, not sure
>>> about clang.
>>
>> I'm able to compile on GCC without any issue.
>
> That's not the point. The point of a namespacing rule is that gcc can
> add such a function at any time it likes, and if that breaks your
> application then that is tough luck.

Thanks, I didn't find anything useful on internet about namespace. It 
seems the common rule is __foo is reserved by the compiler.

>>> I think get_stack_pointer() would be a fine name for this macro. It
>>> seems like the clang version should work for both gcc and clang.
>>>
>>> Google seems to suggest that __builtin_stack_pointer might become a real
>>> compiler builtin at some point.
>>
>> I took the idea to the llvmlinux project:
>> http://git.linuxfoundation.org/?p=llvmlinux.git;a=blob;f=arch/arm/patches/current_stack_pointer_arm.patch;
>>
>> But I can rename into get_stack_pointer for the next version.
>
> Sounds good, thanks.

I will rename it and send the pathc.

Regards,

-- 
Julien Grall

Re: [PATCH 11/34] xen/arm: Introduce __builtin_stack_pointer

[Ian Campbell]

On Wed, 2014-03-26 at 10:38 +0000, Julien Grall wrote:
> 
> On 26/03/14 10:31, Ian Campbell wrote:
> > On Tue, 2014-03-25 at 18:01 +0000, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 03/25/2014 05:18 PM, Ian Campbell wrote:
> >>> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> >>>> Clang doesn't support named register. Introduce __builtin_stack_pointer
> >>>> to be able to use named register with gcc.
> >>>
> >>> I think gcc considers the __builtin namespace to be its own, not sure
> >>> about clang.
> >>
> >> I'm able to compile on GCC without any issue.
> >
> > That's not the point. The point of a namespacing rule is that gcc can
> > add such a function at any time it likes, and if that breaks your
> > application then that is tough luck.
> 
> Thanks, I didn't find anything useful on internet about namespace. It 
> seems the common rule is __foo is reserved by the compiler.

Yes, I think that comes from the C standard. odd numbers of  _ are for
the libc (by POSIX I think) and even numbers for the compiler (by the C
std).

[PATCH 12/34] xen/arm: psci: Don't need to check if vcpuid is negative

[Julien Grall]

vcpuid is unsigned int, therefore the value will never be negative.

It fixes compilation with clang 3.5:
vpsci.c:29:18: error: comparison of unsigned expression < 0 is always false [-Werror,-Wtautological-compare]
    if ( (vcpuid < 0) || (vcpuid >= MAX_VIRT_CPUS) )
          ~~~~~~ ^ ~

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/arch/arm/vpsci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
index 1ceb8cb..abd3ade 100644
--- a/xen/arch/arm/vpsci.c
+++ b/xen/arch/arm/vpsci.c
@@ -26,7 +26,7 @@ int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point)
     int rc;
     int is_thumb = entry_point & 1;
 
-    if ( (vcpuid < 0) || (vcpuid >= MAX_VIRT_CPUS) )
+    if ( vcpuid >= MAX_VIRT_CPUS )
         return PSCI_EINVAL;
 
     if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
-- 
1.7.10.4

Re: [PATCH 12/34] xen/arm: psci: Don't need to check if vcpuid is negative

[Ian Campbell]

On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> vcpuid is unsigned int, therefore the value will never be negative.
> 
> It fixes compilation with clang 3.5:
> vpsci.c:29:18: error: comparison of unsigned expression < 0 is always false [-Werror,-Wtautological-compare]
>     if ( (vcpuid < 0) || (vcpuid >= MAX_VIRT_CPUS) )
>           ~~~~~~ ^ ~

These sorts of compiler warnings annoy me because one thing the existing
check protects against is someone who changes the type of the variable
to a signed type (and vcpuid is often held in a signed type inside Xen I
think), at which point the condition can become true.

But any way, I expect I'm in the minority here, and in this specific
case the type of vcpuid is mandated by the PSCI standard (I think) so:

> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.

[PATCH 13/34] xen/arm: gic: Introduce GIC_SGI_MAX

[Julien Grall]

All the functions that send an SGI takes an enum. Therefore checking everytime
if the value is in the range is not correct.

Introduce GIC_SGI_MAX to check the enum will never reach more than 16 values.

This is fix the compilation with Clang 3.5:

gic.c:515:15: error: comparison of constant 16 with expression of type 'enum gic_sgi' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
   ASSERT(sgi < 16); /* There are only 16 SGIs */
          ~~~ ^ ~~
xen/xen/include/xen/lib.h:43:26: note: expanded from macro 'ASSERT'
    do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
                         ^
xen/xen/include/xen/compiler.h:11:41: note: expanded from macro 'unlikely'
 #define unlikely(x)   __builtin_expect((x),0)

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/arch/arm/gic.c        |    7 ++++---
 xen/include/asm-arm/gic.h |    2 ++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 0095b97..41142a5 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -481,7 +481,8 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
     unsigned int mask = 0;
     cpumask_t online_mask;
 
-    ASSERT(sgi < 16); /* There are only 16 SGIs */
+    BUILD_BUG_ON(GIC_SGI_MAX >= 16);
+    ASSERT(sgi != GIC_SGI_MAX);
 
     cpumask_and(&online_mask, cpumask, &cpu_online_map);
     mask = gic_cpu_mask(&online_mask);
@@ -501,7 +502,7 @@ void send_SGI_one(unsigned int cpu, enum gic_sgi sgi)
 
 void send_SGI_self(enum gic_sgi sgi)
 {
-    ASSERT(sgi < 16); /* There are only 16 SGIs */
+    ASSERT(sgi != GIC_SGI_MAX);
 
     dsb(sy);
 
@@ -511,7 +512,7 @@ void send_SGI_self(enum gic_sgi sgi)
 
 void send_SGI_allbutself(enum gic_sgi sgi)
 {
-   ASSERT(sgi < 16); /* There are only 16 SGIs */
+   ASSERT(sgi != GIC_SGI_MAX);
 
    dsb(sy);
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 071280b..968125d 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -205,6 +205,8 @@ enum gic_sgi {
     GIC_SGI_EVENT_CHECK = 0,
     GIC_SGI_DUMP_STATE  = 1,
     GIC_SGI_CALL_FUNCTION = 2,
+    /* GIC_SGI_MAX must be the last type of the enum */
+    GIC_SGI_MAX,
 };
 extern void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi);
 extern void send_SGI_one(unsigned int cpu, enum gic_sgi sgi);
-- 
1.7.10.4

Re: [PATCH 13/34] xen/arm: gic: Introduce GIC_SGI_MAX

[Stefano Stabellini]

On Tue, 25 Mar 2014, Julien Grall wrote:
> All the functions that send an SGI takes an enum. Therefore checking everytime
> if the value is in the range is not correct.
> 
> Introduce GIC_SGI_MAX to check the enum will never reach more than 16 values.
> 
> This is fix the compilation with Clang 3.5:
> 
> gic.c:515:15: error: comparison of constant 16 with expression of type 'enum gic_sgi' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
>    ASSERT(sgi < 16); /* There are only 16 SGIs */
>           ~~~ ^ ~~
> xen/xen/include/xen/lib.h:43:26: note: expanded from macro 'ASSERT'
>     do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
>                          ^
> xen/xen/include/xen/compiler.h:11:41: note: expanded from macro 'unlikely'
>  #define unlikely(x)   __builtin_expect((x),0)
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> Cc: Tim Deegan <tim@xen.org>
> ---
>  xen/arch/arm/gic.c        |    7 ++++---
>  xen/include/asm-arm/gic.h |    2 ++
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 0095b97..41142a5 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -481,7 +481,8 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
>      unsigned int mask = 0;
>      cpumask_t online_mask;
>  
> -    ASSERT(sgi < 16); /* There are only 16 SGIs */
> +    BUILD_BUG_ON(GIC_SGI_MAX >= 16);
> +    ASSERT(sgi != GIC_SGI_MAX);

These new checks wouldn't be able to cover the following case, while the
previous check could:

enum gic_sgi sgi = (enum gic_sgi) integer_greater_than_16;
send_SGI_mask(something, sgi);

Re: [PATCH 13/34] xen/arm: gic: Introduce GIC_SGI_MAX

[Julien Grall]

Hi Stefano,

On 25/03/14 18:19, Stefano Stabellini wrote:
> On Tue, 25 Mar 2014, Julien Grall wrote:
>> All the functions that send an SGI takes an enum. Therefore checking everytime
>> if the value is in the range is not correct.
>>
>> Introduce GIC_SGI_MAX to check the enum will never reach more than 16 values.
>>
>> This is fix the compilation with Clang 3.5:
>>
>> gic.c:515:15: error: comparison of constant 16 with expression of type 'enum gic_sgi' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
>>     ASSERT(sgi < 16); /* There are only 16 SGIs */
>>            ~~~ ^ ~~
>> xen/xen/include/xen/lib.h:43:26: note: expanded from macro 'ASSERT'
>>      do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
>>                           ^
>> xen/xen/include/xen/compiler.h:11:41: note: expanded from macro 'unlikely'
>>   #define unlikely(x)   __builtin_expect((x),0)
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
>> Cc: Tim Deegan <tim@xen.org>
>> ---
>>   xen/arch/arm/gic.c        |    7 ++++---
>>   xen/include/asm-arm/gic.h |    2 ++
>>   2 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 0095b97..41142a5 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -481,7 +481,8 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
>>       unsigned int mask = 0;
>>       cpumask_t online_mask;
>>
>> -    ASSERT(sgi < 16); /* There are only 16 SGIs */
>> +    BUILD_BUG_ON(GIC_SGI_MAX >= 16);
>> +    ASSERT(sgi != GIC_SGI_MAX);
>
> These new checks wouldn't be able to cover the following case, while the
> previous check could:
>
> enum gic_sgi sgi = (enum gic_sgi) integer_greater_than_16;
> send_SGI_mask(something, sgi);

Why people would do that?

I think instead of an ASSERT, sgi & 0xff might better. It won't be 
harmless for the GIC, even debug is turned off. Right now, the 
developper can put the GIC in wrong state if the value is not in this range.

If people wants to give a number instead of an enum, let them go to the 
hell! They should have used the right type.

Regards,

-- 
Julien Grall

Re: [PATCH 13/34] xen/arm: gic: Introduce GIC_SGI_MAX

[Ian Campbell]

On Tue, 2014-03-25 at 23:23 +0000, Julien Grall wrote:
> Hi Stefano,
> 
> On 25/03/14 18:19, Stefano Stabellini wrote:
> > On Tue, 25 Mar 2014, Julien Grall wrote:
> >> All the functions that send an SGI takes an enum. Therefore checking everytime
> >> if the value is in the range is not correct.
> >>
> >> Introduce GIC_SGI_MAX to check the enum will never reach more than 16 values.
> >>
> >> This is fix the compilation with Clang 3.5:
> >>
> >> gic.c:515:15: error: comparison of constant 16 with expression of type 'enum gic_sgi' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
> >>     ASSERT(sgi < 16); /* There are only 16 SGIs */
> >>            ~~~ ^ ~~
> >> xen/xen/include/xen/lib.h:43:26: note: expanded from macro 'ASSERT'
> >>      do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
> >>                           ^
> >> xen/xen/include/xen/compiler.h:11:41: note: expanded from macro 'unlikely'
> >>   #define unlikely(x)   __builtin_expect((x),0)
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> Cc: Ian Campbell <ian.campbell@citrix.com>
> >> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> >> Cc: Tim Deegan <tim@xen.org>
> >> ---
> >>   xen/arch/arm/gic.c        |    7 ++++---
> >>   xen/include/asm-arm/gic.h |    2 ++
> >>   2 files changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >> index 0095b97..41142a5 100644
> >> --- a/xen/arch/arm/gic.c
> >> +++ b/xen/arch/arm/gic.c
> >> @@ -481,7 +481,8 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
> >>       unsigned int mask = 0;
> >>       cpumask_t online_mask;
> >>
> >> -    ASSERT(sgi < 16); /* There are only 16 SGIs */
> >> +    BUILD_BUG_ON(GIC_SGI_MAX >= 16);
> >> +    ASSERT(sgi != GIC_SGI_MAX);
> >
> > These new checks wouldn't be able to cover the following case, while the
> > previous check could:
> >
> > enum gic_sgi sgi = (enum gic_sgi) integer_greater_than_16;
> > send_SGI_mask(something, sgi);
> 
> Why people would do that?

enums and ints are often, for better or worse, interchangeable. That's
why the existing assert is there, to catch people who are inadvertently
doing something like this. (I don't think the cast in Stefano's example
is strictly needed, so a real case is less likely to leap out into your
face during review).

> I think instead of an ASSERT, sgi & 0xff might better. It won't be 
> harmless for the GIC, even debug is turned off. Right now, the 
> developper can put the GIC in wrong state if the value is not in this range.

The whole point of this assert is to help catch programmer mistakes. If
the programmers and review process was perfect then the assert would be
unnecessary.

Does ASSERT(sgi < GIC_SGI_MAX) not compiler without warnings?

> If people wants to give a number instead of an enum, let them go to the 
> hell! They should have used the right type.

As you have discovered in libxl it is sometimes necessary to take an
enum through an integer for various tedious API/ABI reasons.

Ian.

Re: [PATCH 13/34] xen/arm: gic: Introduce GIC_SGI_MAX

[Julien Grall]

On 26/03/14 09:03, Ian Campbell wrote:

> enums and ints are often, for better or worse, interchangeable. That's
> why the existing assert is there, to catch people who are inadvertently
> doing something like this. (I don't think the cast in Stefano's example
> is strictly needed, so a real case is less likely to leap out into your
> face during review).

It's a shame that the compiler is not able to warn when an int is 
implicitly cast into an enum.

>> I think instead of an ASSERT, sgi & 0xff might better. It won't be
>> harmless for the GIC, even debug is turned off. Right now, the
>> developper can put the GIC in wrong state if the value is not in this range.
>
> The whole point of this assert is to help catch programmer mistakes. If
> the programmers and review process was perfect then the assert would be
> unnecessary.

It's valid for the compiler to do some optimization and think this 
ASSERT is not neccessary. So we don't catch programmer mistakes.

If we want to keep the assert for this reason, we will have also to add 
sgi & 0xff to protect non-debug build and compiler which remove this assert.

> Does ASSERT(sgi < GIC_SGI_MAX) not compiler without warnings?

I forgot to try this solution. Surprisingly, the compiler is able to 
compile correctly this code. So I can replace the ASSERT(sgi < 16) with 
ASSERT(sgi < GIC_SGI_MAX) + BUILD_BUG_ON.

Regards,

-- 
Julien Grall

Re: [PATCH 13/34] xen/arm: gic: Introduce GIC_SGI_MAX

[Ian Campbell]

On Wed, 2014-03-26 at 09:41 +0000, Julien Grall wrote:
> 
> On 26/03/14 09:03, Ian Campbell wrote:
> 
> > enums and ints are often, for better or worse, interchangeable. That's
> > why the existing assert is there, to catch people who are inadvertently
> > doing something like this. (I don't think the cast in Stefano's example
> > is strictly needed, so a real case is less likely to leap out into your
> > face during review).
> 
> It's a shame that the compiler is not able to warn when an int is 
> implicitly cast into an enum.
> 
> >> I think instead of an ASSERT, sgi & 0xff might better. It won't be
> >> harmless for the GIC, even debug is turned off. Right now, the
> >> developper can put the GIC in wrong state if the value is not in this range.
> >
> > The whole point of this assert is to help catch programmer mistakes. If
> > the programmers and review process was perfect then the assert would be
> > unnecessary.
> 
> It's valid for the compiler to do some optimization and think this 
> ASSERT is not neccessary. So we don't catch programmer mistakes.

If the compiler is able to prove that it can optimise the ASSERT away
then the programmer has not made a mistake, I think.

> If we want to keep the assert for this reason, we will have also to add 
> sgi & 0xff to protect non-debug build and compiler which remove this assert.

The purpose of the assert in a debug build is so that we can assume it
is ok in a non-debug build.
> 
> > Does ASSERT(sgi < GIC_SGI_MAX) not compiler without warnings?
> 
> I forgot to try this solution. Surprisingly, the compiler is able to 
> compile correctly this code. So I can replace the ASSERT(sgi < 16) with 
> ASSERT(sgi < GIC_SGI_MAX) + BUILD_BUG_ON.

Good, that sounds like the preferable approach then.

Ian.

Re: [PATCH 13/34] xen/arm: gic: Introduce GIC_SGI_MAX

[Julien Grall]

On 26/03/14 09:03, Ian Campbell wrote:
 > Does ASSERT(sgi < GIC_SGI_MAX) not compiler without warnings?

I reworked this patch (see below):

commit a07ef8994c10ae45c0fa42040e802006f7a510c6
Author: Julien Grall <julien.grall@linaro.org>
Date:   Wed Mar 19 20:51:51 2014 +0000

    xen/arm: gic: Introduce GIC_SGI_MAX
    
    All the functions that send an SGI takes an enum. Therefore checking everytime
    if the value is in the range is not correct.
    
    Introduce GIC_SGI_MAX to check the enum will never reach more than 16 values and
    use it to check if the developper will use a wrong SGI by mistake.
    
    This is fix the compilation with Clang 3.5:
    
    gic.c:515:15: error: comparison of constant 16 with expression of type 'enum gic_sgi' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
       ASSERT(sgi < 16); /* There are only 16 SGIs */
              ~~~ ^ ~~
    xen/xen/include/xen/lib.h:43:26: note: expanded from macro 'ASSERT'
        do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
                             ^
    xen/xen/include/xen/compiler.h:11:41: note: expanded from macro 'unlikely'
     #define unlikely(x)   __builtin_expect((x),0)
    
    Signed-off-by: Julien Grall <julien.grall@linaro.org>
    Cc: Ian Campbell <ian.campbell@citrix.com>
    Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
    Cc: Tim Deegan <tim@xen.org>
    
    ---
        Changes in v2:
            - Replace ASSERT(sgi != GIC_SGI_MAX) by ASSERT(sgi < GIC_SGI_MAX)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index b03f2c2..2b9cdc5 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -473,7 +473,8 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
     unsigned int mask = 0;
     cpumask_t online_mask;
 
-    ASSERT(sgi < 16); /* There are only 16 SGIs */
+    BUILD_BUG_ON(GIC_SGI_MAX >= 16);
+    ASSERT(sgi < GIC_SGI_MAX);
 
     cpumask_and(&online_mask, cpumask, &cpu_online_map);
     mask = gic_cpu_mask(&online_mask);
@@ -493,7 +494,7 @@ void send_SGI_one(unsigned int cpu, enum gic_sgi sgi)
 
 void send_SGI_self(enum gic_sgi sgi)
 {
-    ASSERT(sgi < 16); /* There are only 16 SGIs */
+    ASSERT(sgi < GIC_SGI_MAX);
 
     dsb(sy);
 
@@ -503,7 +504,7 @@ void send_SGI_self(enum gic_sgi sgi)
 
 void send_SGI_allbutself(enum gic_sgi sgi)
 {
-   ASSERT(sgi < 16); /* There are only 16 SGIs */
+   ASSERT(sgi < GIC_SGI_MAX);
 
    dsb(sy);
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 76586ab..a4e513f 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -198,6 +198,8 @@ enum gic_sgi {
     GIC_SGI_EVENT_CHECK = 0,
     GIC_SGI_DUMP_STATE  = 1,
     GIC_SGI_CALL_FUNCTION = 2,
+    /* GIC_SGI_MAX must be the last type of the enum */
+    GIC_SGI_MAX,
 };
 extern void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi);
 extern void send_SGI_one(unsigned int cpu, enum gic_sgi sgi);

-- 
Julien Grall

Re: [PATCH 13/34] xen/arm: gic: Introduce GIC_SGI_MAX

[Stefano Stabellini]

On Wed, 26 Mar 2014, Julien Grall wrote:
> 
> 
> On 26/03/14 09:03, Ian Campbell wrote:
>  > Does ASSERT(sgi < GIC_SGI_MAX) not compiler without warnings?
> 
> I reworked this patch (see below):
> 
> commit a07ef8994c10ae45c0fa42040e802006f7a510c6
> Author: Julien Grall <julien.grall@linaro.org>
> Date:   Wed Mar 19 20:51:51 2014 +0000
> 
>     xen/arm: gic: Introduce GIC_SGI_MAX
>     
>     All the functions that send an SGI takes an enum. Therefore checking everytime
>     if the value is in the range is not correct.
>     
>     Introduce GIC_SGI_MAX to check the enum will never reach more than 16 values and
>     use it to check if the developper will use a wrong SGI by mistake.
>     
>     This is fix the compilation with Clang 3.5:
>     
>     gic.c:515:15: error: comparison of constant 16 with expression of type 'enum gic_sgi' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
>        ASSERT(sgi < 16); /* There are only 16 SGIs */
>               ~~~ ^ ~~
>     xen/xen/include/xen/lib.h:43:26: note: expanded from macro 'ASSERT'
>         do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
>                              ^
>     xen/xen/include/xen/compiler.h:11:41: note: expanded from macro 'unlikely'
>      #define unlikely(x)   __builtin_expect((x),0)
>     
>     Signed-off-by: Julien Grall <julien.grall@linaro.org>
>     Cc: Ian Campbell <ian.campbell@citrix.com>
>     Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
>     Cc: Tim Deegan <tim@xen.org>

I think that this is fine.

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>     ---
>         Changes in v2:
>             - Replace ASSERT(sgi != GIC_SGI_MAX) by ASSERT(sgi < GIC_SGI_MAX)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index b03f2c2..2b9cdc5 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -473,7 +473,8 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
>      unsigned int mask = 0;
>      cpumask_t online_mask;
>  
> -    ASSERT(sgi < 16); /* There are only 16 SGIs */
> +    BUILD_BUG_ON(GIC_SGI_MAX >= 16);
> +    ASSERT(sgi < GIC_SGI_MAX);
>  
>      cpumask_and(&online_mask, cpumask, &cpu_online_map);
>      mask = gic_cpu_mask(&online_mask);
> @@ -493,7 +494,7 @@ void send_SGI_one(unsigned int cpu, enum gic_sgi sgi)
>  
>  void send_SGI_self(enum gic_sgi sgi)
>  {
> -    ASSERT(sgi < 16); /* There are only 16 SGIs */
> +    ASSERT(sgi < GIC_SGI_MAX);
>  
>      dsb(sy);
>  
> @@ -503,7 +504,7 @@ void send_SGI_self(enum gic_sgi sgi)
>  
>  void send_SGI_allbutself(enum gic_sgi sgi)
>  {
> -   ASSERT(sgi < 16); /* There are only 16 SGIs */
> +   ASSERT(sgi < GIC_SGI_MAX);
>  
>     dsb(sy);
>  
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 76586ab..a4e513f 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -198,6 +198,8 @@ enum gic_sgi {
>      GIC_SGI_EVENT_CHECK = 0,
>      GIC_SGI_DUMP_STATE  = 1,
>      GIC_SGI_CALL_FUNCTION = 2,
> +    /* GIC_SGI_MAX must be the last type of the enum */
> +    GIC_SGI_MAX,
>  };
>  extern void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi);
>  extern void send_SGI_one(unsigned int cpu, enum gic_sgi sgi);
> 
> -- 
> Julien Grall
> 

[PATCH 14/34] xen/arm: mm: Mark check_memory_layout_alignment_constraints as unused

[Julien Grall]

Fix compilation with Clang 3.5:

mm.c:148:20: error: unused function 'check_memory_layout_alignment_constraints' [-Werror,-Wunused-function]
static inline void check_memory_layout_alignment_constraints(void) {
                   ^

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/arch/arm/mm.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index d66ce85..fc58fc6 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -144,7 +144,8 @@ unsigned long total_pages;
 extern char __init_begin[], __init_end[];
 
 /* Checking VA memory layout alignment. */
-static inline void check_memory_layout_alignment_constraints(void) {
+static inline always_unused void check_memory_layout_alignment_constraints(void)
+{
     /* 2MB aligned regions */
     BUILD_BUG_ON(XEN_VIRT_START & ~SECOND_MASK);
     BUILD_BUG_ON(FIXMAP_ADDR(0) & ~SECOND_MASK);
-- 
1.7.10.4

Re: [PATCH 14/34] xen/arm: mm: Mark check_memory_layout_alignment_constraints as unused

[Ian Campbell]

On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> Fix compilation with Clang 3.5:
> 
> mm.c:148:20: error: unused function 'check_memory_layout_alignment_constraints' [-Werror,-Wunused-function]
> static inline void check_memory_layout_alignment_constraints(void) {
>                    ^
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH 15/34] xen/arm: traps: Mark check_stack_alignment_constraints as unused

[Julien Grall]

Fix compilation with clang 3.5:

traps.c:48:20: error: unused function 'check_stack_alignment_constraints' [-Werror,-Wunused-function]
static inline void check_stack_alignment_constraints(void) {
                   ^

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/arch/arm/traps.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ec43e65..5578688 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -45,7 +45,8 @@
  * that both the kernel half of struct cpu_user_regs (which is pushed in
  * entry.S) and struct cpu_info (which lives at the bottom of a Xen
  * stack) must be doubleword-aligned in size.  */
-static inline void check_stack_alignment_constraints(void) {
+static inline always_unused void check_stack_alignment_constraints(void)
+{
 #ifdef CONFIG_ARM_64
     BUILD_BUG_ON((sizeof (struct cpu_user_regs)) & 0xf);
     BUILD_BUG_ON((offsetof(struct cpu_user_regs, spsr_el1)) & 0xf);
-- 
1.7.10.4

Re: [PATCH 15/34] xen/arm: traps: Mark check_stack_alignment_constraints as unused

[Ian Campbell]

On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> Fix compilation with clang 3.5:
> 
> traps.c:48:20: error: unused function 'check_stack_alignment_constraints' [-Werror,-Wunused-function]
> static inline void check_stack_alignment_constraints(void) {
>                    ^
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH 16/34] xen/arm: Guard correctly asm-arm/platforms/midway.h

[Julien Grall]

Fix compilation with clang 3.5:
xen/include/asm/platforms/midway.h:1:9: error: '__ASM_ARM_PLATFORMS_MIDWAY_H' is used as a header guard here, followed by #define of a different macro [-Werror,-Wheader-guard]
                                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
xen/include/asm/platforms/midway.h:2:9: note: '__ASM_ASM_PLATFORMS_MIDWAY_H' is defined here; did you mean '__ASM_ARM_PLATFORMS_MIDWAY_H'?
                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
        __ASM_ARM_PLATFORMS_MIDWAY_H

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/include/asm-arm/platforms/midway.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-arm/platforms/midway.h b/xen/include/asm-arm/platforms/midway.h
index 099e435..6914393 100644
--- a/xen/include/asm-arm/platforms/midway.h
+++ b/xen/include/asm-arm/platforms/midway.h
@@ -1,5 +1,5 @@
 #ifndef __ASM_ARM_PLATFORMS_MIDWAY_H
-#define __ASM_ASM_PLATFORMS_MIDWAY_H
+#define __ASM_ARM_PLATFORMS_MIDWAY_H
 
 /* addresses of SREG registers for resetting the SoC */
 #define MW_SREG_PWR_REQ             0xfff3cf00
-- 
1.7.10.4

Re: [PATCH 16/34] xen/arm: Guard correctly asm-arm/platforms/midway.h

[Ian Campbell]

On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> Fix compilation with clang 3.5:
> xen/include/asm/platforms/midway.h:1:9: error: '__ASM_ARM_PLATFORMS_MIDWAY_H' is used as a header guard here, followed by #define of a different macro [-Werror,-Wheader-guard]
>                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> xen/include/asm/platforms/midway.h:2:9: note: '__ASM_ASM_PLATFORMS_MIDWAY_H' is defined here; did you mean '__ASM_ARM_PLATFORMS_MIDWAY_H'?
>                                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>         __ASM_ARM_PLATFORMS_MIDWAY_H
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH 17/34] xen/arm: Guard correctly asm-arm/platform/omap5.h

[Julien Grall]

Fix compilation with clang 3.5:

xen/include/asm/platforms/omap5.h:1:9: error: '__ASM_ARM_PLATFORMS_OMAP5_H' is used as a header guard here, followed by #define of a different macro [-werror,-wheader-guard]
                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
xen/include/asm/platforms/omap5.h:2:9: note: '__ASM_ASM_PLATFORMS_OMAP5_H' is defined here; did you mean '__ASM_ARM_PLATFORMS_OMAP5_H'?
                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~
        __ASM_ARM_PLATFORMS_OMAP5_H

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc:Ian Campbell <ian.campbell@citrix.com>
Cc:Stefano Stabellini <stefano.stabellini@citrix.com>
Cc:Tim Deegan <tim@xen.org>
---
 xen/include/asm-arm/platforms/omap5.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-arm/platforms/omap5.h b/xen/include/asm-arm/platforms/omap5.h
index c559c84..8867b45 100644
--- a/xen/include/asm-arm/platforms/omap5.h
+++ b/xen/include/asm-arm/platforms/omap5.h
@@ -1,5 +1,5 @@
 #ifndef __ASM_ARM_PLATFORMS_OMAP5_H
-#define __ASM_ASM_PLATFORMS_OMAP5_H
+#define __ASM_ARM_PLATFORMS_OMAP5_H
 
 #define REALTIME_COUNTER_BASE                   0x48243200
 #define INCREMENTER_NUMERATOR_OFFSET            0x10
-- 
1.7.10.4

Re: [PATCH 17/34] xen/arm: Guard correctly asm-arm/platform/omap5.h

[Ian Campbell]

On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> Fix compilation with clang 3.5:
> 
> xen/include/asm/platforms/omap5.h:1:9: error: '__ASM_ARM_PLATFORMS_OMAP5_H' is used as a header guard here, followed by #define of a different macro [-werror,-wheader-guard]
>                                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> xen/include/asm/platforms/omap5.h:2:9: note: '__ASM_ASM_PLATFORMS_OMAP5_H' is defined here; did you mean '__ASM_ARM_PLATFORMS_OMAP5_H'?
>                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>         __ASM_ARM_PLATFORMS_OMAP5_H
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by:Ian Campbell <ian.campbell@citrix.com>

(I think this series would have been more manageable if it had fixed
each similar class of error in one patch....)

Re: [PATCH 17/34] xen/arm: Guard correctly asm-arm/platform/omap5.h

[Julien Grall]

On 03/27/2014 04:44 PM, Ian Campbell wrote:

> (I think this series would have been more manageable if it had fixed
> each similar class of error in one patch....)

Sorry, I usually work with very small patch to understand every changes.
I though it was better to send as is it on the ML.

Regards

-- 
Julien Grall

Re: [PATCH 17/34] xen/arm: Guard correctly asm-arm/platform/omap5.h

[Ian Campbell]

On Thu, 2014-03-27 at 16:52 +0000, Julien Grall wrote:
> On 03/27/2014 04:44 PM, Ian Campbell wrote:
> 
> > (I think this series would have been more manageable if it had fixed
> > each similar class of error in one patch....)
> 
> Sorry, I usually work with very small patch to understand every changes.

I don't think a single "fix typos in header guard #defines" patch would
have hindered that, but it would have reduced this series to less than
30 patches, which would have been very worthwhile. Likewise for marking
BUILD_BUG_ON functions as always unused in a single patch and making
things static etc.

> I though it was better to send as is it on the ML.

Please try and keep the sanity of people trying to review dozens of
patches a day in mind.

Thanks,
Ian.

[PATCH 18/34] xen/arm: omap5: Correctly constify platform compatibility list

[Julien Grall]

Fix compilation with clang 3.5:
omap5.c:147:19: error: duplicate 'const' declaration specifier [-Werror,-Wduplicate-decl-specifier]
static const char const *omap5_dt_compat[] __initconst =
                  ^

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/arch/arm/platforms/omap5.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c
index 76d4d9b..98267cf 100644
--- a/xen/arch/arm/platforms/omap5.c
+++ b/xen/arch/arm/platforms/omap5.c
@@ -144,7 +144,7 @@ static int __init omap5_smp_init(void)
     return 0;
 }
 
-static const char const *omap5_dt_compat[] __initconst =
+static const char * const omap5_dt_compat[] __initconst =
 {
     "ti,omap5",
     NULL
-- 
1.7.10.4

Re: [PATCH 18/34] xen/arm: omap5: Correctly constify platform compatibility list

[Ian Campbell]

On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> Fix compilation with clang 3.5:
> omap5.c:147:19: error: duplicate 'const' declaration specifier [-Werror,-Wduplicate-decl-specifier]
> static const char const *omap5_dt_compat[] __initconst =
>                   ^
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH 19/34] xen/arm: Provide eabi wrapper for __aeabi_mem* functions

[Julien Grall]

Clang doesn't provide function __aebai_mem*, implement generically as a wrapper
for mem* function provided by Xen.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/arch/arm/Makefile |    1 +
 xen/arch/arm/eabi.c   |   29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)
 create mode 100644 xen/arch/arm/eabi.c

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 63e0460..43344c0 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -33,6 +33,7 @@ obj-y += hvm.o
 obj-y += device.o
 obj-y += decode.o
 obj-y += processor.o
+obj-$(clang) += eabi.o
 
 #obj-bin-y += ....o
 
diff --git a/xen/arch/arm/eabi.c b/xen/arch/arm/eabi.c
new file mode 100644
index 0000000..f01618c
--- /dev/null
+++ b/xen/arch/arm/eabi.c
@@ -0,0 +1,29 @@
+/*
+ * EABI routines
+ */
+
+#include <xen/types.h>
+#include <xen/lib.h>
+
+void __aeabi_memcpy(void *dest, const void *src, size_t n)
+{
+	(void)memcpy(dest, src, n);
+}
+
+void __aeabi_memmove(void *dest, const void *src, size_t n)
+{
+	(void)memmove(dest, src, n);
+}
+
+void __aeabi_memset(void *s, size_t n, int c)
+{
+	(void)memset(s, c, n);
+}
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.7.10.4

Re: [PATCH 19/34] xen/arm: Provide eabi wrapper for __aeabi_mem* functions

[Ian Campbell]

On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> Clang doesn't provide function __aebai_mem*, implement generically as a wrapper
> for mem* function provided by Xen.

Where are the references to these functions coming from if not from the
compiler?

Are you sure this isn't something usually provided by some clang
equivalent of libgcc?

Ian.

Re: [PATCH 19/34] xen/arm: Provide eabi wrapper for __aeabi_mem* functions

[Julien Grall]

Hi Ian,

On 03/25/2014 05:25 PM, Ian Campbell wrote:
> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
>> Clang doesn't provide function __aebai_mem*, implement generically as a wrapper
>> for mem* function provided by Xen.
> 
> Where are the references to these functions coming from if not from the
> compiler?

Theses functions are called by the code generated by the compiler.
Removing this file will result to the following errors:

/local/home/julien/works/arndale/xen/xen/common/symbols-dummy.o -o /local/home/julien/works/arndale/xen/xen/.xen-syms.0
prelink.o: In function `dump_node':
/local/home/julien/works/arndale/xen/xen/common/device_tree.c:259: undefined reference to `__aeabi_memset'
prelink.o: In function `get_cpu_idle_time':
/local/home/julien/works/arndale/xen/xen/common/schedule.c:178: undefined reference to `__aeabi_memset'
prelink.o: In function `construct_dom0':
/local/home/julien/works/arndale/xen/xen/arch/arm/domain_build.c:1055: undefined reference to `__aeabi_memset'
prelink.o: In function `wallclock_time':
/local/home/julien/works/arndale/xen/xen/arch/arm/time.c:267: undefined reference to `__aeabi_memset'
prelink.o: In function `scheduler_init':
/local/home/julien/works/arndale/xen/xen/common/schedule.c:1368: undefined reference to `__aeabi_memcpy'
/local/home/julien/works/arndale/xen/xen/common/schedule.c:1377: undefined reference to `__aeabi_memcpy'

 
> Are you sure this isn't something usually provided by some clang
> equivalent of libgcc?

Yes. Linux on LLVM has the same issue. See page 22:
https://events.linuxfoundation.org/images/stories/pdf/lcjp2012_charlebois.pdf?a

Regards,

-- 
Julien Grall

Re: [PATCH 19/34] xen/arm: Provide eabi wrapper for __aeabi_mem* functions

[Ian Campbell]

On Tue, 2014-03-25 at 18:12 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 03/25/2014 05:25 PM, Ian Campbell wrote:
> > On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> >> Clang doesn't provide function __aebai_mem*, implement generically as a wrapper
> >> for mem* function provided by Xen.
> > 
> > Where are the references to these functions coming from if not from the
> > compiler?
> 
> Theses functions are called by the code generated by the compiler.

And who normally provides them when building with clang? Some sort of
libclang I guess?

> Removing this file will result to the following errors:
> 
> /local/home/julien/works/arndale/xen/xen/common/symbols-dummy.o -o /local/home/julien/works/arndale/xen/xen/.xen-syms.0
> prelink.o: In function `dump_node':
> /local/home/julien/works/arndale/xen/xen/common/device_tree.c:259: undefined reference to `__aeabi_memset'
> prelink.o: In function `get_cpu_idle_time':
> /local/home/julien/works/arndale/xen/xen/common/schedule.c:178: undefined reference to `__aeabi_memset'
> prelink.o: In function `construct_dom0':
> /local/home/julien/works/arndale/xen/xen/arch/arm/domain_build.c:1055: undefined reference to `__aeabi_memset'
> prelink.o: In function `wallclock_time':
> /local/home/julien/works/arndale/xen/xen/arch/arm/time.c:267: undefined reference to `__aeabi_memset'
> prelink.o: In function `scheduler_init':
> /local/home/julien/works/arndale/xen/xen/common/schedule.c:1368: undefined reference to `__aeabi_memcpy'
> /local/home/julien/works/arndale/xen/xen/common/schedule.c:1377: undefined reference to `__aeabi_memcpy'
> 
>  
> > Are you sure this isn't something usually provided by some clang
> > equivalent of libgcc?
> 
> Yes. Linux on LLVM has the same issue.

Kernels often use -nostdlib and other options which intentionally
disable the default linkage of libgcc/libclang type libraries.

>  See page 22:
> https://events.linuxfoundation.org/images/stories/pdf/lcjp2012_charlebois.pdf?a
> 
> Regards,
> 

Re: [PATCH 19/34] xen/arm: Provide eabi wrapper for __aeabi_mem* functions

[Julien Grall]

On 03/26/2014 10:39 AM, Ian Campbell wrote:
> On Tue, 2014-03-25 at 18:12 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 03/25/2014 05:25 PM, Ian Campbell wrote:
>>> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
>>>> Clang doesn't provide function __aebai_mem*, implement generically as a wrapper
>>>> for mem* function provided by Xen.
>>>
>>> Where are the references to these functions coming from if not from the
>>> compiler?
>>
>> Theses functions are called by the code generated by the compiler.
> 
> And who normally provides them when building with clang? Some sort of
> libclang I guess?

I've tried to compile a same compilation unit with gcc and clang. gcc is
inlining the call to __aebi_*. See below:

Clang assembly:
000002b8 <wallclock_time>:
 2b8:   e92d4800        push    {fp, lr}
 2bc:   e1a0b00d        mov     fp, sp
 2c0:   e3a01024        mov     r1, #36 ; 0x24
 2c4:   e3a02000        mov     r2, #0
 2c8:   ebfffffe        bl      0 <__aeabi_memset>
 2cc:   e8bd8800        pop     {fp, pc}

GCC assembly:

00000378 <wallclock_time>:
 378:   e52db004        push    {fp}            ; (str fp, [sp, #-4]!)
 37c:   e28db000        add     fp, sp, #0
 380:   e3a02000        mov     r2, #0
 384:   e1a03000        mov     r3, r0
 388:   e4832004        str     r2, [r3], #4
 38c:   e5802004        str     r2, [r0, #4]
 390:   e2833004        add     r3, r3, #4
 394:   e4832004        str     r2, [r3], #4
 398:   e4832004        str     r2, [r3], #4
 39c:   e4832004        str     r2, [r3], #4
 3a0:   e4832004        str     r2, [r3], #4
 3a4:   e4832004        str     r2, [r3], #4
 3a8:   e4832004        str     r2, [r3], #4
 3ac:   e5832000        str     r2, [r3]
 3b0:   e24bd000        sub     sp, fp, #0
 3b4:   e49db004        pop     {fp}            ; (ldr fp, [sp], #4)
 3b8:   e12fff1e        bx      lr

Regards,

-- 
Julien Grall

Re: [PATCH 19/34] xen/arm: Provide eabi wrapper for __aeabi_mem* functions

[Julien Grall]

On 03/26/2014 03:38 PM, Julien Grall wrote:
> On 03/26/2014 10:39 AM, Ian Campbell wrote:
>> On Tue, 2014-03-25 at 18:12 +0000, Julien Grall wrote:
>>> Hi Ian,
>>>
>>> On 03/25/2014 05:25 PM, Ian Campbell wrote:
>>>> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
>>>>> Clang doesn't provide function __aebai_mem*, implement generically as a wrapper
>>>>> for mem* function provided by Xen.
>>>>
>>>> Where are the references to these functions coming from if not from the
>>>> compiler?
>>>
>>> Theses functions are called by the code generated by the compiler.
>>
>> And who normally provides them when building with clang? Some sort of
>> libclang I guess?
> 
> I've tried to compile a same compilation unit with gcc and clang. gcc is
> inlining the call to __aebi_*. See below:

I've just noticed it also calls memset/memcpy directly without the
prefix __aebi_.

> Clang assembly:
> 000002b8 <wallclock_time>:
>  2b8:   e92d4800        push    {fp, lr}
>  2bc:   e1a0b00d        mov     fp, sp
>  2c0:   e3a01024        mov     r1, #36 ; 0x24
>  2c4:   e3a02000        mov     r2, #0
>  2c8:   ebfffffe        bl      0 <__aeabi_memset>
>  2cc:   e8bd8800        pop     {fp, pc}
> 
> GCC assembly:
> 
> 00000378 <wallclock_time>:
>  378:   e52db004        push    {fp}            ; (str fp, [sp, #-4]!)
>  37c:   e28db000        add     fp, sp, #0
>  380:   e3a02000        mov     r2, #0
>  384:   e1a03000        mov     r3, r0
>  388:   e4832004        str     r2, [r3], #4
>  38c:   e5802004        str     r2, [r0, #4]
>  390:   e2833004        add     r3, r3, #4
>  394:   e4832004        str     r2, [r3], #4
>  398:   e4832004        str     r2, [r3], #4
>  39c:   e4832004        str     r2, [r3], #4
>  3a0:   e4832004        str     r2, [r3], #4
>  3a4:   e4832004        str     r2, [r3], #4
>  3a8:   e4832004        str     r2, [r3], #4
>  3ac:   e5832000        str     r2, [r3]
>  3b0:   e24bd000        sub     sp, fp, #0
>  3b4:   e49db004        pop     {fp}            ; (ldr fp, [sp], #4)
>  3b8:   e12fff1e        bx      lr
> 
> Regards,
> 


-- 
Julien Grall

Re: [PATCH 19/34] xen/arm: Provide eabi wrapper for __aeabi_mem* functions

[Ian Campbell]

On Wed, 2014-03-26 at 15:38 +0000, Julien Grall wrote:
> On 03/26/2014 10:39 AM, Ian Campbell wrote:
> > On Tue, 2014-03-25 at 18:12 +0000, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 03/25/2014 05:25 PM, Ian Campbell wrote:
> >>> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> >>>> Clang doesn't provide function __aebai_mem*, implement generically as a wrapper
> >>>> for mem* function provided by Xen.
> >>>
> >>> Where are the references to these functions coming from if not from the
> >>> compiler?
> >>
> >> Theses functions are called by the code generated by the compiler.
> > 
> > And who normally provides them when building with clang? Some sort of
> > libclang I guess?
> 
> I've tried to compile a same compilation unit with gcc and clang. gcc is
> inlining the call to __aebi_*.

That is orthogonal to the question I asked.

What in a clang based system normally provides __aeabi_*?

Your choices are "a libclang thing", "the libc", "something else".

Ian.

Re: [PATCH 19/34] xen/arm: Provide eabi wrapper for __aeabi_mem* functions

[Julien Grall]

On 03/26/2014 03:47 PM, Ian Campbell wrote:
> On Wed, 2014-03-26 at 15:38 +0000, Julien Grall wrote:
>> On 03/26/2014 10:39 AM, Ian Campbell wrote:
>>> On Tue, 2014-03-25 at 18:12 +0000, Julien Grall wrote:
>>>> Hi Ian,
>>>>
>>>> On 03/25/2014 05:25 PM, Ian Campbell wrote:
>>>>> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
>>>>>> Clang doesn't provide function __aebai_mem*, implement generically as a wrapper
>>>>>> for mem* function provided by Xen.
>>>>>
>>>>> Where are the references to these functions coming from if not from the
>>>>> compiler?
>>>>
>>>> Theses functions are called by the code generated by the compiler.
>>>
>>> And who normally provides them when building with clang? Some sort of
>>> libclang I guess?
>>
>> I've tried to compile a same compilation unit with gcc and clang. gcc is
>> inlining the call to __aebi_*.
> 
> That is orthogonal to the question I asked.
> 
> What in a clang based system normally provides __aeabi_*?
> 
> Your choices are "a libclang thing", "the libc", "something else".

Sorry I spent the last couple of hours to try to find who is providing
the __aebi_* functions.

For userspace binary it's providing by the glibc.

Regards,

-- 
Julien Grall

Re: [PATCH 19/34] xen/arm: Provide eabi wrapper for __aeabi_mem* functions

[Ian Campbell]

On Wed, 2014-03-26 at 16:01 +0000, Julien Grall wrote:
> On 03/26/2014 03:47 PM, Ian Campbell wrote:
> > On Wed, 2014-03-26 at 15:38 +0000, Julien Grall wrote:
> >> On 03/26/2014 10:39 AM, Ian Campbell wrote:
> >>> On Tue, 2014-03-25 at 18:12 +0000, Julien Grall wrote:
> >>>> Hi Ian,
> >>>>
> >>>> On 03/25/2014 05:25 PM, Ian Campbell wrote:
> >>>>> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> >>>>>> Clang doesn't provide function __aebai_mem*, implement generically as a wrapper
> >>>>>> for mem* function provided by Xen.
> >>>>>
> >>>>> Where are the references to these functions coming from if not from the
> >>>>> compiler?
> >>>>
> >>>> Theses functions are called by the code generated by the compiler.
> >>>
> >>> And who normally provides them when building with clang? Some sort of
> >>> libclang I guess?
> >>
> >> I've tried to compile a same compilation unit with gcc and clang. gcc is
> >> inlining the call to __aebi_*.
> > 
> > That is orthogonal to the question I asked.
> > 
> > What in a clang based system normally provides __aeabi_*?
> > 
> > Your choices are "a libclang thing", "the libc", "something else".
> 
> Sorry I spent the last couple of hours to try to find who is providing
> the __aebi_* functions.
> 
> For userspace binary it's providing by the glibc.

Are we not telling clang not to use with libc (via --nostdlib or
similar)? I thought we did for gcc too but I can't actually see the
code. Actually maybe I wouldn't expect it to suppress this sort of
thing.

I see __eabi_memset even in a gcc based glibc, so I think this isn't
clang specific, just differences in the optmisers/inliners etc.

Anyhow, it looks like this is actually part of the ABI spec:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ihi0043d/index.html

The correct wording for the commit message would therefore be something
like "Provide __aeabi_memset et al which are required by EABI and which
compilers expect to be provided by the libc implementation", or
something like that.

Ian.

Re: [PATCH 19/34] xen/arm: Provide eabi wrapper for __aeabi_mem* functions

[Julien Grall]

On 03/26/2014 04:11 PM, Ian Campbell wrote:
> On Wed, 2014-03-26 at 16:01 +0000, Julien Grall wrote:
>> On 03/26/2014 03:47 PM, Ian Campbell wrote:
>>> On Wed, 2014-03-26 at 15:38 +0000, Julien Grall wrote:
>>>> On 03/26/2014 10:39 AM, Ian Campbell wrote:
>>>>> On Tue, 2014-03-25 at 18:12 +0000, Julien Grall wrote:
>>>>>> Hi Ian,
>>>>>>
>>>>>> On 03/25/2014 05:25 PM, Ian Campbell wrote:
>>>>>>> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
>>>>>>>> Clang doesn't provide function __aebai_mem*, implement generically as a wrapper
>>>>>>>> for mem* function provided by Xen.
>>>>>>>
>>>>>>> Where are the references to these functions coming from if not from the
>>>>>>> compiler?
>>>>>>
>>>>>> Theses functions are called by the code generated by the compiler.
>>>>>
>>>>> And who normally provides them when building with clang? Some sort of
>>>>> libclang I guess?
>>>>
>>>> I've tried to compile a same compilation unit with gcc and clang. gcc is
>>>> inlining the call to __aebi_*.
>>>
>>> That is orthogonal to the question I asked.
>>>
>>> What in a clang based system normally provides __aeabi_*?
>>>
>>> Your choices are "a libclang thing", "the libc", "something else".
>>
>> Sorry I spent the last couple of hours to try to find who is providing
>> the __aebi_* functions.
>>
>> For userspace binary it's providing by the glibc.
> 
> Are we not telling clang not to use with libc (via --nostdlib or
> similar)? I thought we did for gcc too but I can't actually see the
> code. Actually maybe I wouldn't expect it to suppress this sort of
> thing.

We directly use ld to link the binary. So no need of --nosdtlib
--nolibgcc (see arch/arm/Makefile $(TARGET)-syms).

> I see __eabi_memset even in a gcc based glibc, so I think this isn't
> clang specific, just differences in the optmisers/inliners etc.
> 
> Anyhow, it looks like this is actually part of the ABI spec:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ihi0043d/index.html
> 
> The correct wording for the commit message would therefore be something
> like "Provide __aeabi_memset et al which are required by EABI and which
> compilers expect to be provided by the libc implementation", or
> something like that.

Thanks I will rework the commit message.

-- 
Julien Grall

Re: [PATCH 19/34] xen/arm: Provide eabi wrapper for __aeabi_mem* functions

[Tim Deegan]

At 16:11 +0000 on 26 Mar (1395846677), Ian Campbell wrote:
> On Wed, 2014-03-26 at 16:01 +0000, Julien Grall wrote:
> > On 03/26/2014 03:47 PM, Ian Campbell wrote:
> > > On Wed, 2014-03-26 at 15:38 +0000, Julien Grall wrote:
> > >> On 03/26/2014 10:39 AM, Ian Campbell wrote:
> > >>> On Tue, 2014-03-25 at 18:12 +0000, Julien Grall wrote:
> > >>>> Hi Ian,
> > >>>>
> > >>>> On 03/25/2014 05:25 PM, Ian Campbell wrote:
> > >>>>> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> > >>>>>> Clang doesn't provide function __aebai_mem*, implement generically as a wrapper
> > >>>>>> for mem* function provided by Xen.
> > >>>>>
> > >>>>> Where are the references to these functions coming from if not from the
> > >>>>> compiler?
> > >>>>
> > >>>> Theses functions are called by the code generated by the compiler.
> > >>>
> > >>> And who normally provides them when building with clang? Some sort of
> > >>> libclang I guess?
> > >>
> > >> I've tried to compile a same compilation unit with gcc and clang. gcc is
> > >> inlining the call to __aebi_*.
> > > 
> > > That is orthogonal to the question I asked.
> > > 
> > > What in a clang based system normally provides __aeabi_*?
> > > 
> > > Your choices are "a libclang thing", "the libc", "something else".
> > 
> > Sorry I spent the last couple of hours to try to find who is providing
> > the __aebi_* functions.
> > 
> > For userspace binary it's providing by the glibc.
> 
> Are we not telling clang not to use with libc (via --nostdlib or
> similar)? I thought we did for gcc too but I can't actually see the
> code. Actually maybe I wouldn't expect it to suppress this sort of
> thing.
> 
> I see __eabi_memset even in a gcc based glibc, so I think this isn't
> clang specific, just differences in the optmisers/inliners etc.
> 
> Anyhow, it looks like this is actually part of the ABI spec:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ihi0043d/index.html
> 
> The correct wording for the commit message would therefore be something
> like "Provide __aeabi_memset et al which are required by EABI and which
> compilers expect to be provided by the libc implementation", or
> something like that.

+1.  AFAICT these are in the same class as functions like
__aeabi_idiv(), (which we already supply in /arm/arm32/lib/*.S), and
should be handled the same way.

Tim.

Re: [PATCH 19/34] xen/arm: Provide eabi wrapper for __aeabi_mem* functions

[Julien Grall]

On 03/26/2014 04:22 PM, Tim Deegan wrote:
> At 16:11 +0000 on 26 Mar (1395846677), Ian Campbell wrote:

>> The correct wording for the commit message would therefore be something
>> like "Provide __aeabi_memset et al which are required by EABI and which
>> compilers expect to be provided by the libc implementation", or
>> something like that.
> 
> +1.  AFAICT these are in the same class as functions like
> __aeabi_idiv(), (which we already supply in /arm/arm32/lib/*.S), and
> should be handled the same way.

I didn't see any issues with an __aebi_*div* function. Shall we wait
until a failure?

Regards,

-- 
Julien Grall

Re: [PATCH 19/34] xen/arm: Provide eabi wrapper for __aeabi_mem* functions

[Ian Campbell]

On Wed, 2014-03-26 at 16:39 +0000, Julien Grall wrote:
> On 03/26/2014 04:22 PM, Tim Deegan wrote:
> > At 16:11 +0000 on 26 Mar (1395846677), Ian Campbell wrote:
> 
> >> The correct wording for the commit message would therefore be something
> >> like "Provide __aeabi_memset et al which are required by EABI and which
> >> compilers expect to be provided by the libc implementation", or
> >> something like that.
> > 
> > +1.  AFAICT these are in the same class as functions like
> > __aeabi_idiv(), (which we already supply in /arm/arm32/lib/*.S), and
> > should be handled the same way.
> 
> I didn't see any issues with an __aebi_*div* function. Shall we wait
> until a failure?

We already have the ones which get generated in practice.

Ian.

Re: [PATCH 19/34] xen/arm: Provide eabi wrapper for __aeabi_mem* functions

[Tim Deegan]

At 16:39 +0000 on 26 Mar (1395848341), Julien Grall wrote:
> On 03/26/2014 04:22 PM, Tim Deegan wrote:
> > At 16:11 +0000 on 26 Mar (1395846677), Ian Campbell wrote:
> 
> >> The correct wording for the commit message would therefore be something
> >> like "Provide __aeabi_memset et al which are required by EABI and which
> >> compilers expect to be provided by the libc implementation", or
> >> something like that.
> > 
> > +1.  AFAICT these are in the same class as functions like
> > __aeabi_idiv(), (which we already supply in /arm/arm32/lib/*.S), and
> > should be handled the same way.
> 
> I didn't see any issues with an __aebi_*div* function. Shall we wait
> until a failure?

What I was trying to say is: we already supply the __aebi_*div*
functions, so adding the __aeabi_memset ones is correct.

Cheers,

Tim.

Re: [PATCH 19/34] xen/arm: Provide eabi wrapper for __aeabi_mem* functions

[Julien Grall]

On 03/26/2014 04:46 PM, Tim Deegan wrote:
> At 16:39 +0000 on 26 Mar (1395848341), Julien Grall wrote:
>> On 03/26/2014 04:22 PM, Tim Deegan wrote:
>>> At 16:11 +0000 on 26 Mar (1395846677), Ian Campbell wrote:
>>
>>>> The correct wording for the commit message would therefore be something
>>>> like "Provide __aeabi_memset et al which are required by EABI and which
>>>> compilers expect to be provided by the libc implementation", or
>>>> something like that.
>>>
>>> +1.  AFAICT these are in the same class as functions like
>>> __aeabi_idiv(), (which we already supply in /arm/arm32/lib/*.S), and
>>> should be handled the same way.
>>
>> I didn't see any issues with an __aebi_*div* function. Shall we wait
>> until a failure?
> 
> What I was trying to say is: we already supply the __aebi_*div*
> functions, so adding the __aeabi_memset ones is correct.

Sorry I misunderstood your previous comment.

-- 
Julien Grall

[PATCH 20/34] xen/common: cpupool: cpupool_unassign_cpu is only used internally

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Juergen Gross <juergen.gross@ts.fujitsu.com>
---
 xen/common/cpupool.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index 933de78..4a0e569 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -307,7 +307,7 @@ out:
  * - last cpu and still active domains in cpupool
  * - cpu just being unplugged
  */
-int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
+static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
 {
     int work_cpu;
     int ret;
-- 
1.7.10.4

Re: [PATCH 20/34] xen/common: cpupool: cpupool_unassign_cpu is only used internally

[Juergen Gross]

On 25.03.2014 17:55, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Juergen Gross <juergen.gross@ts.fujitsu.com>

Acked-by: Juergen Gross <juergen.gross@ts.fujitsu.com>

> ---
>   xen/common/cpupool.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
> index 933de78..4a0e569 100644
> --- a/xen/common/cpupool.c
> +++ b/xen/common/cpupool.c
> @@ -307,7 +307,7 @@ out:
>    * - last cpu and still active domains in cpupool
>    * - cpu just being unplugged
>    */
> -int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
> +static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
>   {
>       int work_cpu;
>       int ret;
>


-- 
Juergen Gross                 Principal Developer Operating Systems
PBG PDG ES&S SWE OS6                   Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

[PATCH 21/34] xen/common: domctl: Some functions are only used internally

[Julien Grall]

The list of function above are only used internally in common/domctl.c.
    - bitmap_to_xenctl_bitmap
    - xenctl_bitmap_to_bitmap
    - nodemask_to_xenctl_bitmap
    - xenctl_bitmap_to_nodemask

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Keir Fraser <keir@xen.org>
---
 xen/common/domctl.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 7cf610a..5342e5d 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -33,9 +33,9 @@
 static DEFINE_SPINLOCK(domctl_lock);
 DEFINE_SPINLOCK(vcpu_alloc_lock);
 
-int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap,
-                            const unsigned long *bitmap,
-                            unsigned int nbits)
+static int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap,
+                                   const unsigned long *bitmap,
+                                   unsigned int nbits)
 {
     unsigned int guest_bytes, copy_bytes, i;
     uint8_t zero = 0;
@@ -63,9 +63,9 @@ int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap,
     return err;
 }
 
-int xenctl_bitmap_to_bitmap(unsigned long *bitmap,
-                            const struct xenctl_bitmap *xenctl_bitmap,
-                            unsigned int nbits)
+static int xenctl_bitmap_to_bitmap(unsigned long *bitmap,
+                                   const struct xenctl_bitmap *xenctl_bitmap,
+                                   unsigned int nbits)
 {
     unsigned int guest_bytes, copy_bytes;
     int err = 0;
@@ -118,15 +118,15 @@ int xenctl_bitmap_to_cpumask(cpumask_var_t *cpumask,
     return err;
 }
 
-int nodemask_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_nodemap,
-                              const nodemask_t *nodemask)
+static int nodemask_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_nodemap,
+                                     const nodemask_t *nodemask)
 {
     return bitmap_to_xenctl_bitmap(xenctl_nodemap, nodes_addr(*nodemask),
                                    MAX_NUMNODES);
 }
 
-int xenctl_bitmap_to_nodemask(nodemask_t *nodemask,
-                              const struct xenctl_bitmap *xenctl_nodemap)
+static int xenctl_bitmap_to_nodemask(nodemask_t *nodemask,
+                                     const struct xenctl_bitmap *xenctl_nodemap)
 {
     return xenctl_bitmap_to_bitmap(nodes_addr(*nodemask), xenctl_nodemap,
                                    MAX_NUMNODES);
-- 
1.7.10.4

Re: [PATCH 21/34] xen/common: domctl: Some functions are only used internally

[Keir Fraser]

[-- Attachment #1.1: Type: text/plain, Size: 2753 bytes --]

On Tue, Mar 25, 2014 at 4:55 PM, Julien Grall <julien.grall@linaro.org>wrote:

> The list of function above are only used internally in common/domctl.c.
>     - bitmap_to_xenctl_bitmap
>     - xenctl_bitmap_to_bitmap
>     - nodemask_to_xenctl_bitmap
>     - xenctl_bitmap_to_nodemask
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Keir Fraser <keir@xen.org>
>

Acked-by: Keir Fraser <keir@xen.org>


> ---
>  xen/common/domctl.c |   20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 7cf610a..5342e5d 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -33,9 +33,9 @@
>  static DEFINE_SPINLOCK(domctl_lock);
>  DEFINE_SPINLOCK(vcpu_alloc_lock);
>
> -int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap,
> -                            const unsigned long *bitmap,
> -                            unsigned int nbits)
> +static int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap,
> +                                   const unsigned long *bitmap,
> +                                   unsigned int nbits)
>  {
>      unsigned int guest_bytes, copy_bytes, i;
>      uint8_t zero = 0;
> @@ -63,9 +63,9 @@ int bitmap_to_xenctl_bitmap(struct xenctl_bitmap
> *xenctl_bitmap,
>      return err;
>  }
>
> -int xenctl_bitmap_to_bitmap(unsigned long *bitmap,
> -                            const struct xenctl_bitmap *xenctl_bitmap,
> -                            unsigned int nbits)
> +static int xenctl_bitmap_to_bitmap(unsigned long *bitmap,
> +                                   const struct xenctl_bitmap
> *xenctl_bitmap,
> +                                   unsigned int nbits)
>  {
>      unsigned int guest_bytes, copy_bytes;
>      int err = 0;
> @@ -118,15 +118,15 @@ int xenctl_bitmap_to_cpumask(cpumask_var_t *cpumask,
>      return err;
>  }
>
> -int nodemask_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_nodemap,
> -                              const nodemask_t *nodemask)
> +static int nodemask_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_nodemap,
> +                                     const nodemask_t *nodemask)
>  {
>      return bitmap_to_xenctl_bitmap(xenctl_nodemap, nodes_addr(*nodemask),
>                                     MAX_NUMNODES);
>  }
>
> -int xenctl_bitmap_to_nodemask(nodemask_t *nodemask,
> -                              const struct xenctl_bitmap *xenctl_nodemap)
> +static int xenctl_bitmap_to_nodemask(nodemask_t *nodemask,
> +                                     const struct xenctl_bitmap
> *xenctl_nodemap)
>  {
>      return xenctl_bitmap_to_bitmap(nodes_addr(*nodemask), xenctl_nodemap,
>                                     MAX_NUMNODES);
> --
> 1.7.10.4
>
>

[-- Attachment #1.2: Type: text/html, Size: 3583 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

[PATCH 22/34] xen/common: tmem: Some functions are only used internally

[Julien Grall]

The list of function above are only used internally in common/tmem:
    - oid_compare
    - oid_set_invalid
    - oid_hash

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/common/tmem.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/common/tmem.c b/xen/common/tmem.c
index 0a24b3f..5155114 100644
--- a/xen/common/tmem.c
+++ b/xen/common/tmem.c
@@ -829,7 +829,7 @@ static void rtn_free(struct radix_tree_node *rtn, void *arg)
 
 /************ POOL OBJECT COLLECTION MANIPULATION ROUTINES *******************/
 
-int oid_compare(struct oid *left, struct oid *right)
+static int oid_compare(struct oid *left, struct oid *right)
 {
     if ( left->oid[2] == right->oid[2] )
     {
@@ -853,12 +853,12 @@ int oid_compare(struct oid *left, struct oid *right)
         return 1;
 }
 
-void oid_set_invalid(struct oid *oidp)
+static void oid_set_invalid(struct oid *oidp)
 {
     oidp->oid[0] = oidp->oid[1] = oidp->oid[2] = -1UL;
 }
 
-unsigned oid_hash(struct oid *oidp)
+static unsigned oid_hash(struct oid *oidp)
 {
     return (tmem_hash(oidp->oid[0] ^ oidp->oid[1] ^ oidp->oid[2],
                      BITS_PER_LONG) & OBJ_HASH_BUCKETS_MASK);
-- 
1.7.10.4

[PATCH 23/34] xen/sched: credit2: Some functions are only used internally

[Julien Grall]

The list of function above are only used internally in common/domctl.c
    - migrate
    - update_max_weight

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/common/sched_credit2.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 6729564..97f4049 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -696,7 +696,8 @@ void burn_credits(struct csched_runqueue_data *rqd, struct csched_vcpu *svc, s_t
 }
 
 /* Find the domain with the highest weight. */
-void update_max_weight(struct csched_runqueue_data *rqd, int new_weight, int old_weight)
+static void update_max_weight(struct csched_runqueue_data *rqd, int new_weight,
+                              int old_weight)
 {
     /* Try to avoid brute-force search:
      * - If new_weight is larger, max_weigth <- new_weight
@@ -1182,10 +1183,10 @@ static void consider(balance_state_t *st,
 }
 
 
-void migrate(const struct scheduler *ops,
-             struct csched_vcpu *svc, 
-             struct csched_runqueue_data *trqd, 
-             s_time_t now)
+static void migrate(const struct scheduler *ops,
+                    struct csched_vcpu *svc, 
+                    struct csched_runqueue_data *trqd, 
+                    s_time_t now)
 {
     if ( test_bit(__CSFLAG_scheduled, &svc->flags) )
     {
-- 
1.7.10.4

Re: [PATCH 23/34] xen/sched: credit2: Some functions are only used internally

[George Dunlap]

On 03/25/2014 04:55 PM, Julien Grall wrote:
> The list of function above are only used internally in common/domctl.c
>      - migrate
>      - update_max_weight
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

> ---
>   xen/common/sched_credit2.c |   11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 6729564..97f4049 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -696,7 +696,8 @@ void burn_credits(struct csched_runqueue_data *rqd, struct csched_vcpu *svc, s_t
>   }
>
>   /* Find the domain with the highest weight. */
> -void update_max_weight(struct csched_runqueue_data *rqd, int new_weight, int old_weight)
> +static void update_max_weight(struct csched_runqueue_data *rqd, int new_weight,
> +                              int old_weight)
>   {
>       /* Try to avoid brute-force search:
>        * - If new_weight is larger, max_weigth <- new_weight
> @@ -1182,10 +1183,10 @@ static void consider(balance_state_t *st,
>   }
>
>
> -void migrate(const struct scheduler *ops,
> -             struct csched_vcpu *svc,
> -             struct csched_runqueue_data *trqd,
> -             s_time_t now)
> +static void migrate(const struct scheduler *ops,
> +                    struct csched_vcpu *svc,
> +                    struct csched_runqueue_data *trqd,
> +                    s_time_t now)
>   {
>       if ( test_bit(__CSFLAG_scheduled, &svc->flags) )
>       {
>

[PATCH 24/34] xen/arm: setup: setup_cache is only used internally

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/arch/arm/setup.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 08be416..3806429 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -642,7 +642,7 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
 size_t __read_mostly cacheline_bytes;
 
 /* Very early check of the CPU cache properties */
-void __init setup_cache(void)
+static void __init setup_cache(void)
 {
     uint32_t ccsid;
 
-- 
1.7.10.4

Re: [PATCH 24/34] xen/arm: setup: setup_cache is only used internally

[Ian Campbell]

On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:

"only used internally" is an odd way to put it. Something like "is only
used from within the file and can therefore be static" perhaps?

> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 24/34] xen/arm: setup: setup_cache is only used internally

[Julien Grall]

On 03/27/2014 05:07 PM, Ian Campbell wrote:
> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> 
> "only used internally" is an odd way to put it. Something like "is only
> used from within the file and can therefore be static" perhaps?

Right, I was unable to find a good description when I wrote the
different patches. I will rework all the commit messages with "only used
internally" in the title.

>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks,

-- 
Julien Grall

[PATCH 25/34] xen/arm: traps: show_stack is only used internally

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/arch/arm/traps.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 5578688..2109d03 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -922,7 +922,7 @@ static void show_trace(struct cpu_user_regs *regs)
     printk("\n");
 }
 
-void show_stack(struct cpu_user_regs *regs)
+static void show_stack(struct cpu_user_regs *regs)
 {
     register_t *stack = STACK_BEFORE_EXCEPTION(regs), addr;
     int i;
-- 
1.7.10.4

Re: [PATCH 25/34] xen/arm: traps: show_stack is only used internally

[Ian Campbell]

On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH 26/34] xen/arm: traps: Drop dump_guest_s1_walk

[Julien Grall]

This function is not used neither export.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/arch/arm/traps.c |   56 --------------------------------------------------
 1 file changed, 56 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 2109d03..b1475f3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1422,62 +1422,6 @@ static void do_sysreg(struct cpu_user_regs *regs,
 }
 #endif
 
-void dump_guest_s1_walk(struct domain *d, vaddr_t addr)
-{
-    register_t ttbcr = READ_SYSREG(TCR_EL1);
-    uint64_t ttbr0 = READ_SYSREG64(TTBR0_EL1);
-    paddr_t paddr;
-    uint32_t offset;
-    uint32_t *first = NULL, *second = NULL;
-
-    printk("dom%d VA 0x%08"PRIvaddr"\n", d->domain_id, addr);
-    printk("    TTBCR: 0x%08"PRIregister"\n", ttbcr);
-    printk("    TTBR0: 0x%016"PRIx64" = 0x%"PRIpaddr"\n",
-           ttbr0, p2m_lookup(d, ttbr0 & PAGE_MASK, NULL));
-
-    if ( ttbcr & TTBCR_EAE )
-    {
-        printk("Cannot handle LPAE guest PT walk\n");
-        return;
-    }
-    if ( (ttbcr & TTBCR_N_MASK) != 0 )
-    {
-        printk("Cannot handle TTBR1 guest walks\n");
-        return;
-    }
-
-    paddr = p2m_lookup(d, ttbr0 & PAGE_MASK, NULL);
-    if ( paddr == INVALID_PADDR )
-    {
-        printk("Failed TTBR0 maddr lookup\n");
-        goto done;
-    }
-    first = map_domain_page(paddr>>PAGE_SHIFT);
-
-    offset = addr >> (12+10);
-    printk("1ST[0x%"PRIx32"] (0x%"PRIpaddr") = 0x%08"PRIx32"\n",
-           offset, paddr, first[offset]);
-    if ( !(first[offset] & 0x1) ||
-         !(first[offset] & 0x2) )
-        goto done;
-
-    paddr = p2m_lookup(d, first[offset] & PAGE_MASK, NULL);
-
-    if ( paddr == INVALID_PADDR )
-    {
-        printk("Failed L1 entry maddr lookup\n");
-        goto done;
-    }
-    second = map_domain_page(paddr>>PAGE_SHIFT);
-    offset = (addr >> 12) & 0x3FF;
-    printk("2ND[0x%"PRIx32"] (0x%"PRIpaddr") = 0x%08"PRIx32"\n",
-           offset, paddr, second[offset]);
-
-done:
-    if (second) unmap_domain_page(second);
-    if (first) unmap_domain_page(first);
-}
-
 static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
                                       union hsr hsr)
 {
-- 
1.7.10.4

Re: [PATCH 26/34] xen/arm: traps: Drop dump_guest_s1_walk

[Ian Campbell]

On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> This function is not used neither export.

I have used it when debugging stuff, where it is very useful.

Should it not be called from one of the dump_vcpu_state in response to
an exception which implies a guest translation fault?

Re: [PATCH 26/34] xen/arm: traps: Drop dump_guest_s1_walk

[Julien Grall]

On 03/27/2014 05:09 PM, Ian Campbell wrote:
> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
>> This function is not used neither export.
> 
> I have used it when debugging stuff, where it is very useful.

I was not sure if anyone was using it. I can export it in p2m.h.

I'm also thinking to move this function in p2m.c (which is a best place
for this function). What do you think?

> Should it not be called from one of the dump_vcpu_state in response to
> an exception which implies a guest translation fault?
> 

No, we inject directly an exception to the guest since few months. So we
don't have to print the p2m.

Regards,

-- 
Julien Grall

Re: [PATCH 26/34] xen/arm: traps: Drop dump_guest_s1_walk

[Ian Campbell]

On Tue, 2014-04-01 at 17:59 +0100, Julien Grall wrote:
> On 03/27/2014 05:09 PM, Ian Campbell wrote:
> > On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> >> This function is not used neither export.
> > 
> > I have used it when debugging stuff, where it is very useful.
> 
> I was not sure if anyone was using it. I can export it in p2m.h.
> 
> I'm also thinking to move this function in p2m.c (which is a best place
> for this function). What do you think?

It's not really a p2m related function, it's guest stage 1. There isn't
really a good home for it I don't think. I suppose it has similarities
with dump_p2m_lookup/dump_phy_walk and therefore with the core
dump_pt_walk, which would then suggest mm.c as a good home for it.

> > Should it not be called from one of the dump_vcpu_state in response to
> > an exception which implies a guest translation fault?
> > 
> 
> No, we inject directly an exception to the guest since few months. So we
> don't have to print the p2m.

Ah, that's probably when the last caller disappeared.

On second thoughts, when debugging I can always rescue the code from the
git history, so lets go with your first instinct and remove it. The
original patch is:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.

[PATCH 27/34] xen/arm: time: move ticks_to_ns and ns_to_ticks in asm/time.h

[Julien Grall]

Both ticks_to_ns and ns_to_ticks are wrapper to a division. Move it into
asm/time.h to let the compiler inlines them.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/arch/arm/time.c        |   10 ----------
 xen/arch/arm/vtimer.c      |    3 ---
 xen/include/asm-arm/time.h |   13 +++++++++++++
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 8dd4bea..7f4f2b4 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -57,16 +57,6 @@ const struct dt_irq *timer_dt_irq(enum timer_ppi ppi)
     return &timer_irq[ppi];
 }
 
-/*static inline*/ s_time_t ticks_to_ns(uint64_t ticks)
-{
-    return muldiv64(ticks, SECONDS(1), 1000 * cpu_khz);
-}
-
-/*static inline*/ uint64_t ns_to_ticks(s_time_t ns)
-{
-    return muldiv64(ns, 1000 * cpu_khz, SECONDS(1));
-}
-
 /* TODO: On a real system the firmware would have set the frequency in
    the CNTFRQ register.  Also we'd need to use devicetree to find
    the RTC.  When we've seen some real systems, we can delete this.
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 3d6a721..83f4c0f 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -26,9 +26,6 @@
 #include <asm/gic.h>
 #include <asm/regs.h>
 
-extern s_time_t ticks_to_ns(uint64_t ticks);
-extern uint64_t ns_to_ticks(s_time_t ns);
-
 static void phys_timer_expired(void *data)
 {
     struct vtimer *t = data;
diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h
index d10c737..31dab85 100644
--- a/xen/include/asm-arm/time.h
+++ b/xen/include/asm-arm/time.h
@@ -5,6 +5,8 @@
     DT_MATCH_COMPATIBLE("arm,armv7-timer"), \
     DT_MATCH_COMPATIBLE("arm,armv8-timer")
 
+#include <xen/lib.h>
+
 typedef unsigned long cycles_t;
 
 static inline cycles_t get_cycles (void)
@@ -34,6 +36,17 @@ extern void __cpuinit init_timer_interrupt(void);
 /* Counter value at boot time */
 extern uint64_t boot_count;
 
+/* Helpers to convert ticks <=> ns */
+static inline s_time_t ticks_to_ns(uint64_t ticks)
+{
+    return muldiv64(ticks, SECONDS(1), 1000 * cpu_khz);
+}
+
+static inline uint64_t ns_to_ticks(s_time_t ns)
+{
+    return muldiv64(ns, 1000 * cpu_khz, SECONDS(1));
+}
+
 #endif /* __ARM_TIME_H__ */
 /*
  * Local variables:
-- 
1.7.10.4

Re: [PATCH 27/34] xen/arm: time: move ticks_to_ns and ns_to_ticks in asm/time.h

[Ian Campbell]

On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> Both ticks_to_ns and ns_to_ticks are wrapper to a division. Move it into
> asm/time.h to let the compiler inlines them.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH 28/34] xen/xsm: flask: flask_disable is only used internally

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 xen/xsm/flask/flask_op.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/xsm/flask/flask_op.c b/xen/xsm/flask/flask_op.c
index 85a615b..7743aac 100644
--- a/xen/xsm/flask/flask_op.c
+++ b/xen/xsm/flask/flask_op.c
@@ -272,7 +272,7 @@ static int flask_security_sid(struct xen_flask_sid_context *arg)
 
 #ifndef COMPAT
 
-int flask_disable(void)
+static int flask_disable(void)
 {
     static int flask_disabled = 0;
 
-- 
1.7.10.4

Re: [PATCH 28/34] xen/xsm: flask: flask_disable is only used internally

[Daniel De Graaf]

On 03/25/2014 12:55 PM, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>   xen/xsm/flask/flask_op.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/xsm/flask/flask_op.c b/xen/xsm/flask/flask_op.c
> index 85a615b..7743aac 100644
> --- a/xen/xsm/flask/flask_op.c
> +++ b/xen/xsm/flask/flask_op.c
> @@ -272,7 +272,7 @@ static int flask_security_sid(struct xen_flask_sid_context *arg)
>
>   #ifndef COMPAT
>
> -int flask_disable(void)
> +static int flask_disable(void)
>   {
>       static int flask_disabled = 0;
>
>


-- 
Daniel De Graaf
National Security Agency

[PATCH 29/34] xen/xsm: flask: ss: remove unused function determine_oocontext

[Julien Grall]

This function is not used neither exported.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 xen/xsm/flask/ss/services.c |   14 --------------
 1 file changed, 14 deletions(-)

diff --git a/xen/xsm/flask/ss/services.c b/xen/xsm/flask/ss/services.c
index 59234ff..f0e459a 100644
--- a/xen/xsm/flask/ss/services.c
+++ b/xen/xsm/flask/ss/services.c
@@ -2035,20 +2035,6 @@ out:
     return rc;
 }
 
-int determine_ocontext( char *ocontext )
-{
-    if ( strcmp(ocontext, "pirq") == 0 )
-        return OCON_PIRQ;
-    else if ( strcmp(ocontext, "ioport") == 0 )
-        return OCON_IOPORT;
-    else if ( strcmp(ocontext, "iomem") == 0 )
-        return OCON_IOMEM;
-    else if ( strcmp(ocontext, "pcidevice") == 0 )
-        return OCON_DEVICE;
-    else
-        return -1;
-}
-
 int security_ocontext_add( u32 ocon, unsigned long low, unsigned long high
                             ,u32 sid )
 {
-- 
1.7.10.4

Re: [PATCH 29/34] xen/xsm: flask: ss: remove unused function determine_oocontext

[Daniel De Graaf]

On 03/25/2014 12:55 PM, Julien Grall wrote:
> This function is not used neither exported.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

> ---
>   xen/xsm/flask/ss/services.c |   14 --------------
>   1 file changed, 14 deletions(-)
>
> diff --git a/xen/xsm/flask/ss/services.c b/xen/xsm/flask/ss/services.c
> index 59234ff..f0e459a 100644
> --- a/xen/xsm/flask/ss/services.c
> +++ b/xen/xsm/flask/ss/services.c
> @@ -2035,20 +2035,6 @@ out:
>       return rc;
>   }
>
> -int determine_ocontext( char *ocontext )
> -{
> -    if ( strcmp(ocontext, "pirq") == 0 )
> -        return OCON_PIRQ;
> -    else if ( strcmp(ocontext, "ioport") == 0 )
> -        return OCON_IOPORT;
> -    else if ( strcmp(ocontext, "iomem") == 0 )
> -        return OCON_IOMEM;
> -    else if ( strcmp(ocontext, "pcidevice") == 0 )
> -        return OCON_DEVICE;
> -    else
> -        return -1;
> -}
> -
>   int security_ocontext_add( u32 ocon, unsigned long low, unsigned long high
>                               ,u32 sid )
>   {
>


-- 
Daniel De Graaf
National Security Agency

[PATCH 30/34] xen: Add missing includes on different files

[Julien Grall]

There is a bunch of functions in xen code which are declared without the
prototypes defined before. This may lead to runtime issue if the propotype
doesn't match the declaration.

Add missing includes where the prototype of theses functions are defined.

This was spotted by -Wmissing-prototypes, which we can't enable because there
is exported function for assembly. I'm not sure if we need to add a prototype
for them.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 xen/arch/arm/hvm.c             |    1 +
 xen/arch/arm/mm.c              |    1 +
 xen/arch/arm/shutdown.c        |    1 +
 xen/arch/arm/smp.c             |    1 +
 xen/arch/arm/time.c            |    1 +
 xen/arch/arm/vtimer.c          |    1 +
 xen/common/event_channel.c     |    1 +
 xen/common/grant_table.c       |    1 +
 xen/common/multicall.c         |    3 +++
 xen/common/sort.c              |    1 +
 xen/common/tmem.c              |    1 +
 xen/drivers/video/arm_hdlcd.c  |    1 +
 xen/xsm/flask/ss/conditional.h |    1 +
 13 files changed, 15 insertions(+)

diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 471c4cd..b695b26 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -4,6 +4,7 @@
 #include <xen/errno.h>
 #include <xen/guest_access.h>
 #include <xen/sched.h>
+#include <xen/hypercall.h>
 
 #include <xsm/xsm.h>
 
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index fc58fc6..84b6ccc 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -39,6 +39,7 @@
 #include <xen/vmap.h>
 #include <xsm/xsm.h>
 #include <xen/pfn.h>
+#include <asm/setup.h>
 
 struct domain *dom_xen, *dom_io, *dom_cow;
 
diff --git a/xen/arch/arm/shutdown.c b/xen/arch/arm/shutdown.c
index adc0529..43eaf47 100644
--- a/xen/arch/arm/shutdown.c
+++ b/xen/arch/arm/shutdown.c
@@ -4,6 +4,7 @@
 #include <xen/delay.h>
 #include <xen/lib.h>
 #include <xen/smp.h>
+#include <xen/shutdown.h>
 #include <asm/platform.h>
 
 static void raw_machine_reset(void)
diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c
index 30203b8..7bb602d 100644
--- a/xen/arch/arm/smp.c
+++ b/xen/arch/arm/smp.c
@@ -1,4 +1,5 @@
 #include <xen/config.h>
+#include <xen/smp.h>
 #include <asm/system.h>
 #include <asm/smp.h>
 #include <asm/cpregs.h>
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 7f4f2b4..3b0feec 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -29,6 +29,7 @@
 #include <xen/time.h>
 #include <xen/sched.h>
 #include <xen/event.h>
+#include <xen/delay.h>
 #include <asm/system.h>
 #include <asm/time.h>
 #include <asm/gic.h>
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 83f4c0f..6aeae5d 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -25,6 +25,7 @@
 #include <asm/time.h>
 #include <asm/gic.h>
 #include <asm/regs.h>
+#include "vtimer.h"
 
 static void phys_timer_expired(void *data)
 {
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index db952af..e94e37e 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -28,6 +28,7 @@
 #include <xen/keyhandler.h>
 #include <xen/event_fifo.h>
 #include <asm/current.h>
+#include <xen/hypercall.h>
 
 #include <public/xen.h>
 #include <public/event_channel.h>
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 107b000..386cdff 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -37,6 +37,7 @@
 #include <xen/iommu.h>
 #include <xen/paging.h>
 #include <xen/keyhandler.h>
+#include <xen/hypercall.h>
 #include <xsm/xsm.h>
 #include <asm/flushtlb.h>
 
diff --git a/xen/common/multicall.c b/xen/common/multicall.c
index e66c798..bb7550b 100644
--- a/xen/common/multicall.c
+++ b/xen/common/multicall.c
@@ -6,6 +6,9 @@
 #include <xen/types.h>
 #include <xen/lib.h>
 #include <xen/mm.h>
+#ifndef COMPAT
+#include <xen/hypercall.h>
+#endif
 #include <xen/sched.h>
 #include <xen/event.h>
 #include <xen/multicall.h>
diff --git a/xen/common/sort.c b/xen/common/sort.c
index d96fc2a..7069888 100644
--- a/xen/common/sort.c
+++ b/xen/common/sort.c
@@ -5,6 +5,7 @@
  */
 
 #include <xen/types.h>
+#include <xen/sort.h>
 
 static void u32_swap(void *a, void *b, int size)
 {
diff --git a/xen/common/tmem.c b/xen/common/tmem.c
index 5155114..865d154 100644
--- a/xen/common/tmem.c
+++ b/xen/common/tmem.c
@@ -23,6 +23,7 @@
 #include <xen/radix-tree.h>
 #include <xen/list.h>
 #include <xen/init.h>
+#include <xen/hypercall.h>
 
 #define TMEM_SPEC_VERSION 1
 
diff --git a/xen/drivers/video/arm_hdlcd.c b/xen/drivers/video/arm_hdlcd.c
index 841d0d0..04a3091 100644
--- a/xen/drivers/video/arm_hdlcd.c
+++ b/xen/drivers/video/arm_hdlcd.c
@@ -25,6 +25,7 @@
 #include <xen/libfdt/libfdt.h>
 #include <xen/init.h>
 #include <xen/mm.h>
+#include <xen/video.h>
 #include "font.h"
 #include "lfb.h"
 #include "modelines.h"
diff --git a/xen/xsm/flask/ss/conditional.h b/xen/xsm/flask/ss/conditional.h
index d389ecf..65af76b 100644
--- a/xen/xsm/flask/ss/conditional.h
+++ b/xen/xsm/flask/ss/conditional.h
@@ -13,6 +13,7 @@
 #include "avtab.h"
 #include "symtab.h"
 #include "policydb.h"
+#include "../include/conditional.h"
 
 #define COND_EXPR_MAXDEPTH 10
 
-- 
1.7.10.4

Re: [PATCH 30/34] xen: Add missing includes on different files

[Daniel De Graaf]

On 03/25/2014 12:55 PM, Julien Grall wrote:
> There is a bunch of functions in xen code which are declared without the
> prototypes defined before. This may lead to runtime issue if the propotype
> doesn't match the declaration.
>
> Add missing includes where the prototype of theses functions are defined.
>
> This was spotted by -Wmissing-prototypes, which we can't enable because there
> is exported function for assembly. I'm not sure if we need to add a prototype
> for them.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

> ---
>   xen/arch/arm/hvm.c             |    1 +
>   xen/arch/arm/mm.c              |    1 +
>   xen/arch/arm/shutdown.c        |    1 +
>   xen/arch/arm/smp.c             |    1 +
>   xen/arch/arm/time.c            |    1 +
>   xen/arch/arm/vtimer.c          |    1 +
>   xen/common/event_channel.c     |    1 +
>   xen/common/grant_table.c       |    1 +
>   xen/common/multicall.c         |    3 +++
>   xen/common/sort.c              |    1 +
>   xen/common/tmem.c              |    1 +
>   xen/drivers/video/arm_hdlcd.c  |    1 +
>   xen/xsm/flask/ss/conditional.h |    1 +
>   13 files changed, 15 insertions(+)
>
> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> index 471c4cd..b695b26 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -4,6 +4,7 @@
>   #include <xen/errno.h>
>   #include <xen/guest_access.h>
>   #include <xen/sched.h>
> +#include <xen/hypercall.h>
>
>   #include <xsm/xsm.h>
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index fc58fc6..84b6ccc 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -39,6 +39,7 @@
>   #include <xen/vmap.h>
>   #include <xsm/xsm.h>
>   #include <xen/pfn.h>
> +#include <asm/setup.h>
>
>   struct domain *dom_xen, *dom_io, *dom_cow;
>
> diff --git a/xen/arch/arm/shutdown.c b/xen/arch/arm/shutdown.c
> index adc0529..43eaf47 100644
> --- a/xen/arch/arm/shutdown.c
> +++ b/xen/arch/arm/shutdown.c
> @@ -4,6 +4,7 @@
>   #include <xen/delay.h>
>   #include <xen/lib.h>
>   #include <xen/smp.h>
> +#include <xen/shutdown.h>
>   #include <asm/platform.h>
>
>   static void raw_machine_reset(void)
> diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c
> index 30203b8..7bb602d 100644
> --- a/xen/arch/arm/smp.c
> +++ b/xen/arch/arm/smp.c
> @@ -1,4 +1,5 @@
>   #include <xen/config.h>
> +#include <xen/smp.h>
>   #include <asm/system.h>
>   #include <asm/smp.h>
>   #include <asm/cpregs.h>
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 7f4f2b4..3b0feec 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -29,6 +29,7 @@
>   #include <xen/time.h>
>   #include <xen/sched.h>
>   #include <xen/event.h>
> +#include <xen/delay.h>
>   #include <asm/system.h>
>   #include <asm/time.h>
>   #include <asm/gic.h>
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index 83f4c0f..6aeae5d 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -25,6 +25,7 @@
>   #include <asm/time.h>
>   #include <asm/gic.h>
>   #include <asm/regs.h>
> +#include "vtimer.h"
>
>   static void phys_timer_expired(void *data)
>   {
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index db952af..e94e37e 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -28,6 +28,7 @@
>   #include <xen/keyhandler.h>
>   #include <xen/event_fifo.h>
>   #include <asm/current.h>
> +#include <xen/hypercall.h>
>
>   #include <public/xen.h>
>   #include <public/event_channel.h>
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 107b000..386cdff 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -37,6 +37,7 @@
>   #include <xen/iommu.h>
>   #include <xen/paging.h>
>   #include <xen/keyhandler.h>
> +#include <xen/hypercall.h>
>   #include <xsm/xsm.h>
>   #include <asm/flushtlb.h>
>
> diff --git a/xen/common/multicall.c b/xen/common/multicall.c
> index e66c798..bb7550b 100644
> --- a/xen/common/multicall.c
> +++ b/xen/common/multicall.c
> @@ -6,6 +6,9 @@
>   #include <xen/types.h>
>   #include <xen/lib.h>
>   #include <xen/mm.h>
> +#ifndef COMPAT
> +#include <xen/hypercall.h>
> +#endif
>   #include <xen/sched.h>
>   #include <xen/event.h>
>   #include <xen/multicall.h>
> diff --git a/xen/common/sort.c b/xen/common/sort.c
> index d96fc2a..7069888 100644
> --- a/xen/common/sort.c
> +++ b/xen/common/sort.c
> @@ -5,6 +5,7 @@
>    */
>
>   #include <xen/types.h>
> +#include <xen/sort.h>
>
>   static void u32_swap(void *a, void *b, int size)
>   {
> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
> index 5155114..865d154 100644
> --- a/xen/common/tmem.c
> +++ b/xen/common/tmem.c
> @@ -23,6 +23,7 @@
>   #include <xen/radix-tree.h>
>   #include <xen/list.h>
>   #include <xen/init.h>
> +#include <xen/hypercall.h>
>
>   #define TMEM_SPEC_VERSION 1
>
> diff --git a/xen/drivers/video/arm_hdlcd.c b/xen/drivers/video/arm_hdlcd.c
> index 841d0d0..04a3091 100644
> --- a/xen/drivers/video/arm_hdlcd.c
> +++ b/xen/drivers/video/arm_hdlcd.c
> @@ -25,6 +25,7 @@
>   #include <xen/libfdt/libfdt.h>
>   #include <xen/init.h>
>   #include <xen/mm.h>
> +#include <xen/video.h>
>   #include "font.h"
>   #include "lfb.h"
>   #include "modelines.h"
> diff --git a/xen/xsm/flask/ss/conditional.h b/xen/xsm/flask/ss/conditional.h
> index d389ecf..65af76b 100644
> --- a/xen/xsm/flask/ss/conditional.h
> +++ b/xen/xsm/flask/ss/conditional.h
> @@ -13,6 +13,7 @@
>   #include "avtab.h"
>   #include "symtab.h"
>   #include "policydb.h"
> +#include "../include/conditional.h"
>
>   #define COND_EXPR_MAXDEPTH 10
>
>


-- 
Daniel De Graaf
National Security Agency

Re: [PATCH 30/34] xen: Add missing includes on different files

[Jan Beulich]

>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
> --- a/xen/common/multicall.c
> +++ b/xen/common/multicall.c
> @@ -6,6 +6,9 @@
>  #include <xen/types.h>
>  #include <xen/lib.h>
>  #include <xen/mm.h>
> +#ifndef COMPAT
> +#include <xen/hypercall.h>
> +#endif

Is there anything wrong with adding this without the seemingly
unmotivated #ifndef?

Jan

Re: [PATCH 30/34] xen: Add missing includes on different files

[Julien Grall]

On 03/26/2014 12:57 PM, Jan Beulich wrote:
>>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
>> --- a/xen/common/multicall.c
>> +++ b/xen/common/multicall.c
>> @@ -6,6 +6,9 @@
>>  #include <xen/types.h>
>>  #include <xen/lib.h>
>>  #include <xen/mm.h>
>> +#ifndef COMPAT
>> +#include <xen/hypercall.h>
>> +#endif
> 
> Is there anything wrong with adding this without the seemingly
> unmotivated #ifndef?

The prototype in hypercall.h return directly long, but the definition
returns ret_t (which is replaced by int if COMPAT is defined).

This will result to a compilation failure:

In file included from multicall.c:41:0:
../multicall.c:38:1: error: conflicting types for ‘compat_multicall’
In file included from ../multicall.c:9:0,
                 from multicall.c:41:

-- 
Julien Grall

Re: [PATCH 30/34] xen: Add missing includes on different files

[Jan Beulich]

>>> On 26.03.14 at 18:41, <julien.grall@linaro.org> wrote:
> On 03/26/2014 12:57 PM, Jan Beulich wrote:
>>>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
>>> --- a/xen/common/multicall.c
>>> +++ b/xen/common/multicall.c
>>> @@ -6,6 +6,9 @@
>>>  #include <xen/types.h>
>>>  #include <xen/lib.h>
>>>  #include <xen/mm.h>
>>> +#ifndef COMPAT
>>> +#include <xen/hypercall.h>
>>> +#endif
>> 
>> Is there anything wrong with adding this without the seemingly
>> unmotivated #ifndef?
> 
> The prototype in hypercall.h return directly long, but the definition
> returns ret_t (which is replaced by int if COMPAT is defined).
> 
> This will result to a compilation failure:
> 
> In file included from multicall.c:41:0:
> ../multicall.c:38:1: error: conflicting types for ‘compat_multicall’
> In file included from ../multicall.c:9:0,
>                  from multicall.c:41:

Meaning this needs to be dealt with differently: Include the header in
both files _and_ add a declaration of compat_multicall() to it (alongside
the other compat_ ones already there).

Jan

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

Re: [PATCH 30/34] xen: Add missing includes on different files

[Julien Grall]

Hi Jan,

Sorry for the late answer.

On 03/27/2014 07:57 AM, Jan Beulich wrote:
>>>> On 26.03.14 at 18:41, <julien.grall@linaro.org> wrote:
>> On 03/26/2014 12:57 PM, Jan Beulich wrote:
>>>>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote:
>>>> --- a/xen/common/multicall.c
>>>> +++ b/xen/common/multicall.c
>>>> @@ -6,6 +6,9 @@
>>>>  #include <xen/types.h>
>>>>  #include <xen/lib.h>
>>>>  #include <xen/mm.h>
>>>> +#ifndef COMPAT
>>>> +#include <xen/hypercall.h>
>>>> +#endif
>>>
>>> Is there anything wrong with adding this without the seemingly
>>> unmotivated #ifndef?
>>
>> The prototype in hypercall.h return directly long, but the definition
>> returns ret_t (which is replaced by int if COMPAT is defined).
>>
>> This will result to a compilation failure:
>>
>> In file included from multicall.c:41:0:
>> ../multicall.c:38:1: error: conflicting types for ‘compat_multicall’
>> In file included from ../multicall.c:9:0,
>>                  from multicall.c:41:
> 
> Meaning this needs to be dealt with differently: Include the header in
> both files _and_ add a declaration of compat_multicall() to it (alongside
> the other compat_ ones already there).

I gave a look to this solution. It won't works because do_multicall is
replaced by the define in compat/multicall.c:26. It will end up to
multiple definition of compat_multcall.

I'm not sure how to handle it because, AFAIU, compat/multicall.c is
defining some macro to redefine the behavior of multicall.c

Regards,

-- 
Julien Grall

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

Re: [PATCH 30/34] xen: Add missing includes on different files

[Jan Beulich]

>>> On 09.04.14 at 18:06, <julien.grall@linaro.org> wrote:
> On 03/27/2014 07:57 AM, Jan Beulich wrote:
>>>>> On 26.03.14 at 18:41, <julien.grall@linaro.org> wrote:
>>> In file included from multicall.c:41:0:
>>> ../multicall.c:38:1: error: conflicting types for ‘compat_multicall’
>>> In file included from ../multicall.c:9:0,
>>>                  from multicall.c:41:
>> 
>> Meaning this needs to be dealt with differently: Include the header in
>> both files _and_ add a declaration of compat_multicall() to it (alongside
>> the other compat_ ones already there).
> 
> I gave a look to this solution. It won't works because do_multicall is
> replaced by the define in compat/multicall.c:26. It will end up to
> multiple definition of compat_multcall.
> 
> I'm not sure how to handle it because, AFAIU, compat/multicall.c is
> defining some macro to redefine the behavior of multicall.c

Right, but that doesn't prevent the suggested model afaict:

- compat/multicall.c includes xen/hypercall.h, obtaining proper
  prototypes for both do_multicall() and compat_multicall()
- compat/multicall.c re-defines do_multicall (which doesn't affect
  the prototypes already seen)
- compat/multicall.c includes multicall.c
- multicall.c's inclusion of xen/hypercall.h does nothing (thanks
  to the header guard)

Did you indeed try this and it didn't work?

Jan

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

Re: [PATCH 30/34] xen: Add missing includes on different files

[Julien Grall]

On 04/09/2014 05:17 PM, Jan Beulich wrote:
>>>> On 09.04.14 at 18:06, <julien.grall@linaro.org> wrote:
>> On 03/27/2014 07:57 AM, Jan Beulich wrote:
>>>>>> On 26.03.14 at 18:41, <julien.grall@linaro.org> wrote:
>>>> In file included from multicall.c:41:0:
>>>> ../multicall.c:38:1: error: conflicting types for ‘compat_multicall’
>>>> In file included from ../multicall.c:9:0,
>>>>                  from multicall.c:41:
>>>
>>> Meaning this needs to be dealt with differently: Include the header in
>>> both files _and_ add a declaration of compat_multicall() to it (alongside
>>> the other compat_ ones already there).
>>
>> I gave a look to this solution. It won't works because do_multicall is
>> replaced by the define in compat/multicall.c:26. It will end up to
>> multiple definition of compat_multcall.
>>
>> I'm not sure how to handle it because, AFAIU, compat/multicall.c is
>> defining some macro to redefine the behavior of multicall.c
> 
> Right, but that doesn't prevent the suggested model afaict:
> 
> - compat/multicall.c includes xen/hypercall.h, obtaining proper
>   prototypes for both do_multicall() and compat_multicall()
> - compat/multicall.c re-defines do_multicall (which doesn't affect
>   the prototypes already seen)
> - compat/multicall.c includes multicall.c
> - multicall.c's inclusion of xen/hypercall.h does nothing (thanks
>   to the header guard)
> 
> Did you indeed try this and it didn't work?

I've tried another things. Anyway the version below works for me. Thanks for your help.

diff --git a/xen/common/compat/multicall.c b/xen/common/compat/multicall.c
index 95c047a..0399863 100644
--- a/xen/common/compat/multicall.c
+++ b/xen/common/compat/multicall.c
@@ -6,6 +6,7 @@
 #include <xen/types.h>
 #include <xen/multicall.h>
 #include <xen/trace.h>
+#include <xen/hypercall.h>
 
 #define COMPAT
 typedef int ret_t;
@@ -18,7 +19,6 @@ static inline void xlat_multicall_entry(struct mc_state *mcs)
         mcs->compat_call.args[i] = mcs->call.args[i];
 }
 
-DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t);
 #define multicall_entry      compat_multicall_entry
 #define multicall_entry_t    multicall_entry_compat_t
 #define do_multicall_call    compat_multicall_call
diff --git a/xen/common/multicall.c b/xen/common/multicall.c
index bb7550b..e39e724 100644
--- a/xen/common/multicall.c
+++ b/xen/common/multicall.c
@@ -6,9 +6,7 @@
 #include <xen/types.h>
 #include <xen/lib.h>
 #include <xen/mm.h>
-#ifndef COMPAT
 #include <xen/hypercall.h>
-#endif
 #include <xen/sched.h>
 #include <xen/event.h>
 #include <xen/multicall.h>
diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
index a9e5229..baa9cc6 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -176,6 +176,11 @@ compat_set_timer_op(
     u32 lo,
     s32 hi);
 
+DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t);
+extern int
+compat_multicall(XEN_GUEST_HANDLE(multicall_entry_compat_t) call_list,
+                unsigned int nr_calls);
+
 #endif
 
 void arch_get_xen_caps(xen_capabilities_info_t *info);

-- 
Julien Grall

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

Re: [PATCH 30/34] xen: Add missing includes on different files

[Ian Campbell]

On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> This was spotted by -Wmissing-prototypes, which we can't enable because there
> is exported function for assembly. I'm not sure if we need to add a prototype
> for them.

What exactly is the issue here?

Ian.

Re: [PATCH 30/34] xen: Add missing includes on different files

[Julien Grall]

On 03/27/2014 05:11 PM, Ian Campbell wrote:
> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
>> This was spotted by -Wmissing-prototypes, which we can't enable because there
>> is exported function for assembly. I'm not sure if we need to add a prototype
>> for them.
> 
> What exactly is the issue here?

There a bunch of functions (see below for ARM) where the prototype is
not defined before. Mainly because theses functions are used by the
assembly code so we don't need to give a prototype.

do_trap_*
start_xen
start_secondary
leave_hypervisor_tail

Regards,


-- 
Julien Grall

Re: [PATCH 30/34] xen: Add missing includes on different files

[Ian Campbell]

On Thu, 2014-03-27 at 17:30 +0000, Julien Grall wrote:
> On 03/27/2014 05:11 PM, Ian Campbell wrote:
> > On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> >> This was spotted by -Wmissing-prototypes, which we can't enable because there
> >> is exported function for assembly. I'm not sure if we need to add a prototype
> >> for them.
> > 
> > What exactly is the issue here?
> 
> There a bunch of functions (see below for ARM) where the prototype is
> not defined before. Mainly because theses functions are used by the
> assembly code so we don't need to give a prototype.
> 
> do_trap_*
> start_xen
> start_secondary
> leave_hypervisor_tail

Is that all of them? Although their prototypes are useless there are few
enough of them that the benefit of being able to turn on
Wmissing-prototypes might make it worth it.

Unless there is some attribute we can apply which marks these as not
requiring a prototype? Even better if as a side effect the compiler will
warn about calls not from assembly...

Ian.

Re: [PATCH 30/34] xen: Add missing includes on different files

[Julien Grall]

On 03/27/2014 05:39 PM, Ian Campbell wrote:
> On Thu, 2014-03-27 at 17:30 +0000, Julien Grall wrote:
>> On 03/27/2014 05:11 PM, Ian Campbell wrote:
>>> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
>>>> This was spotted by -Wmissing-prototypes, which we can't enable because there
>>>> is exported function for assembly. I'm not sure if we need to add a prototype
>>>> for them.
>>>
>>> What exactly is the issue here?
>>
>> There a bunch of functions (see below for ARM) where the prototype is
>> not defined before. Mainly because theses functions are used by the
>> assembly code so we don't need to give a prototype.
>>
>> do_trap_*
>> start_xen
>> start_secondary
>> leave_hypervisor_tail
> 
> Is that all of them? Although their prototypes are useless there are few
> enough of them that the benefit of being able to turn on
> Wmissing-prototypes might make it worth it.

>From the common code there is 7 others:

core_parking_helper and get_cur_idle_nums (both of them are used on C
code but never defined in an header. I was lazy and I didn't write a patch).

__qdivrem
__divdi3
__umoddi3
__moddi3
__ldivmod_helper

For x86, I didn't yet try to compiled it with -Wmissing-prototypes.

> 
> Unless there is some attribute we can apply which marks these as not
> requiring a prototype?

I will look at it.

> Even better if as a side effect the compiler will
> warn about calls not from assembly...

I would love to see this feature, but I don't think the linker is able
to differentiate call from C and from assembly :).

Regards,

-- 
Julien Grall

Re: [PATCH 30/34] xen: Add missing includes on different files

[Ian Campbell]

On Thu, 2014-03-27 at 17:47 +0000, Julien Grall wrote:
> On 03/27/2014 05:39 PM, Ian Campbell wrote:
> > On Thu, 2014-03-27 at 17:30 +0000, Julien Grall wrote:
> >> On 03/27/2014 05:11 PM, Ian Campbell wrote:
> >>> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> >>>> This was spotted by -Wmissing-prototypes, which we can't enable because there
> >>>> is exported function for assembly. I'm not sure if we need to add a prototype
> >>>> for them.
> >>>
> >>> What exactly is the issue here?
> >>
> >> There a bunch of functions (see below for ARM) where the prototype is
> >> not defined before. Mainly because theses functions are used by the
> >> assembly code so we don't need to give a prototype.
> >>
> >> do_trap_*
> >> start_xen
> >> start_secondary
> >> leave_hypervisor_tail
> > 
> > Is that all of them? Although their prototypes are useless there are few
> > enough of them that the benefit of being able to turn on
> > Wmissing-prototypes might make it worth it.
> 
> From the common code there is 7 others:
> 
> core_parking_helper and get_cur_idle_nums (both of them are used on C
> code but never defined in an header. I was lazy and I didn't write a patch).
> 
> __qdivrem
> __divdi3
> __umoddi3
> __moddi3
> __ldivmod_helper

Still not awful I guess.

Several of these are essentially library functions provided for the
compiler to emit calls to, I wonder if there is some compiler header
which we should be including which would prototype them. Probably not,
worth a look though.

> 
> For x86, I didn't yet try to compiled it with -Wmissing-prototypes.
> 
> > 
> > Unless there is some attribute we can apply which marks these as not
> > requiring a prototype?
> 
> I will look at it.

Thanks.

> > Even better if as a side effect the compiler will
> > warn about calls not from assembly...
> 
> I would love to see this feature, but I don't think the linker is able
> to differentiate call from C and from assembly :).

The compiler could see a call from C code to a function whose prototype
was marked with "called_from_asm_only".

Ian.

Re: [PATCH 30/34] xen: Add missing includes on different files

[Julien Grall]

On 03/28/2014 09:59 AM, Ian Campbell wrote:
> On Thu, 2014-03-27 at 17:47 +0000, Julien Grall wrote:
>> On 03/27/2014 05:39 PM, Ian Campbell wrote:
>>> On Thu, 2014-03-27 at 17:30 +0000, Julien Grall wrote:
>>>> On 03/27/2014 05:11 PM, Ian Campbell wrote:
>>>>> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
>>>>>> This was spotted by -Wmissing-prototypes, which we can't enable because there
>>>>>> is exported function for assembly. I'm not sure if we need to add a prototype
>>>>>> for them.
>>>>>
>>>>> What exactly is the issue here?
>>>>
>>>> There a bunch of functions (see below for ARM) where the prototype is
>>>> not defined before. Mainly because theses functions are used by the
>>>> assembly code so we don't need to give a prototype.
>>>>
>>>> do_trap_*
>>>> start_xen
>>>> start_secondary
>>>> leave_hypervisor_tail
>>>
>>> Is that all of them? Although their prototypes are useless there are few
>>> enough of them that the benefit of being able to turn on
>>> Wmissing-prototypes might make it worth it.
>>
>> From the common code there is 7 others:
>>
>> core_parking_helper and get_cur_idle_nums (both of them are used on C
>> code but never defined in an header. I was lazy and I didn't write a patch).
>>
>> __qdivrem
>> __divdi3
>> __umoddi3
>> __moddi3
>> __ldivmod_helper
> 
> Still not awful I guess.
> 
> Several of these are essentially library functions provided for the
> compiler to emit calls to, I wonder if there is some compiler header
> which we should be including which would prototype them. Probably not,
> worth a look though.

These functions are not used by x86 (because of the if BITS_PER_LONG ==
32), and on ARM we provide eabi_* helpers.

>>
>> For x86, I didn't yet try to compiled it with -Wmissing-prototypes.
>>
>>>
>>> Unless there is some attribute we can apply which marks these as not
>>> requiring a prototype?
>>
>> I will look at it.
> 
> Thanks.
> 
>>> Even better if as a side effect the compiler will
>>> warn about calls not from assembly...
>>
>> I would love to see this feature, but I don't think the linker is able
>> to differentiate call from C and from assembly :).
> 
> The compiler could see a call from C code to a function whose prototype
> was marked with "called_from_asm_only".

I'm afraid there is no __attribute__ feature for a such thing. One
solution could be introduce mismatch section as Linux (e.g: functions
called from assembly are in a specific section).

Regards,

-- 
Julien Grall

Re: [PATCH 30/34] xen: Add missing includes on different files

[Ian Campbell]

On Tue, 2014-04-01 at 18:58 +0100, Julien Grall wrote:
> On 03/28/2014 09:59 AM, Ian Campbell wrote:
> > On Thu, 2014-03-27 at 17:47 +0000, Julien Grall wrote:
> >> On 03/27/2014 05:39 PM, Ian Campbell wrote:
> >>> On Thu, 2014-03-27 at 17:30 +0000, Julien Grall wrote:
> >>>> On 03/27/2014 05:11 PM, Ian Campbell wrote:
> >>>>> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> >>>>>> This was spotted by -Wmissing-prototypes, which we can't enable because there
> >>>>>> is exported function for assembly. I'm not sure if we need to add a prototype
> >>>>>> for them.
> >>>>>
> >>>>> What exactly is the issue here?
> >>>>
> >>>> There a bunch of functions (see below for ARM) where the prototype is
> >>>> not defined before. Mainly because theses functions are used by the
> >>>> assembly code so we don't need to give a prototype.
> >>>>
> >>>> do_trap_*
> >>>> start_xen
> >>>> start_secondary
> >>>> leave_hypervisor_tail
> >>>
> >>> Is that all of them? Although their prototypes are useless there are few
> >>> enough of them that the benefit of being able to turn on
> >>> Wmissing-prototypes might make it worth it.
> >>
> >> From the common code there is 7 others:
> >>
> >> core_parking_helper and get_cur_idle_nums (both of them are used on C
> >> code but never defined in an header. I was lazy and I didn't write a patch).
> >>
> >> __qdivrem
> >> __divdi3
> >> __umoddi3
> >> __moddi3
> >> __ldivmod_helper
> > 
> > Still not awful I guess.
> > 
> > Several of these are essentially library functions provided for the
> > compiler to emit calls to, I wonder if there is some compiler header
> > which we should be including which would prototype them. Probably not,
> > worth a look though.
> 
> These functions are not used by x86 (because of the if BITS_PER_LONG ==
> 32), and on ARM we provide eabi_* helpers.

Not sure I follow, what are you concluding there?

> > The compiler could see a call from C code to a function whose prototype
> > was marked with "called_from_asm_only".
> 
> I'm afraid there is no __attribute__ feature for a such thing. One
> solution could be introduce mismatch section as Linux (e.g: functions
> called from assembly are in a specific section).

[PATCH 31/34] tools: Disable ignored-attributes warning when compiling with clang

[Julien Grall]

Clang 3.5 will fail to build most of the tools because aligned attribute
is not used sometimes:

In file included from xc_core.c:64:
In file included from ./xg_private.h:30:
In file included from ./xenctrl.h:55:
../../tools/include/xen/foreign/x86_64.h:198:47: error: 'aligned' attribute ignored when parsing type [-Werror,-Wignored-attributes]
    __align8__ uint64_t evtchn_pending[sizeof(__align8__ uint64_t) * 8];
                                              ^~~~~~~~~~
../../tools/include/xen/foreign/x86_64.h:13:36: note: expanded from macro '__align8__'
 # define __align8__ __attribute__((aligned (8)))
                                   ^~~~~~~~~~~
../../tools/include/xen/foreign/x86_64.h:199:44: error: 'aligned' attribute ignored when parsing type [-Werror,-Wignored-attributes]
    __align8__ uint64_t evtchn_mask[sizeof(__align8__ uint64_t) * 8];
                                           ^~~~~~~~~~
../../tools/include/xen/foreign/x86_64.h:13:36: note: expanded from macro '__align8__'
 # define __align8__ __attribute__((aligned (8)))

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/Rules.mk |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/Rules.mk b/tools/Rules.mk
index 13d8fc1..6fb746f 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -17,6 +17,8 @@ XEN_LIBXENSTAT     = $(XEN_ROOT)/tools/xenstat/libxenstat/src
 XEN_BLKTAP2        = $(XEN_ROOT)/tools/blktap2
 XEN_LIBVCHAN       = $(XEN_ROOT)/tools/libvchan
 
+CFLAGS-$(clang) += -Wno-ignored-attributes
+
 CFLAGS_xeninclude = -I$(XEN_INCLUDE)
 
 CFLAGS_libxenctrl = -I$(XEN_LIBXC) $(CFLAGS_xeninclude)
-- 
1.7.10.4

Re: [PATCH 31/34] tools: Disable ignored-attributes warning when compiling with clang

[Ian Campbell]

On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> Clang 3.5 will fail to build most of the tools because aligned attribute
> is not used sometimes:
> 
> In file included from xc_core.c:64:
> In file included from ./xg_private.h:30:
> In file included from ./xenctrl.h:55:
> ../../tools/include/xen/foreign/x86_64.h:198:47: error: 'aligned' attribute ignored when parsing type [-Werror,-Wignored-attributes]
>     __align8__ uint64_t evtchn_pending[sizeof(__align8__ uint64_t) * 8];
>                                               ^~~~~~~~~~
> ../../tools/include/xen/foreign/x86_64.h:13:36: note: expanded from macro '__align8__'
>  # define __align8__ __attribute__((aligned (8)))
>                                    ^~~~~~~~~~~
> ../../tools/include/xen/foreign/x86_64.h:199:44: error: 'aligned' attribute ignored when parsing type [-Werror,-Wignored-attributes]
>     __align8__ uint64_t evtchn_mask[sizeof(__align8__ uint64_t) * 8];

Is sizeof(__align8__ uint64_t) != sizeof(uint64_t) under any
circumstances? IOW can't we just drop the __align8__ here?

>                                            ^~~~~~~~~~
> ../../tools/include/xen/foreign/x86_64.h:13:36: note: expanded from macro '__align8__'
>  # define __align8__ __attribute__((aligned (8)))
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/Rules.mk |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/Rules.mk b/tools/Rules.mk
> index 13d8fc1..6fb746f 100644
> --- a/tools/Rules.mk
> +++ b/tools/Rules.mk
> @@ -17,6 +17,8 @@ XEN_LIBXENSTAT     = $(XEN_ROOT)/tools/xenstat/libxenstat/src
>  XEN_BLKTAP2        = $(XEN_ROOT)/tools/blktap2
>  XEN_LIBVCHAN       = $(XEN_ROOT)/tools/libvchan
>  
> +CFLAGS-$(clang) += -Wno-ignored-attributes
> +
>  CFLAGS_xeninclude = -I$(XEN_INCLUDE)
>  
>  CFLAGS_libxenctrl = -I$(XEN_LIBXC) $(CFLAGS_xeninclude)

Re: [PATCH 31/34] tools: Disable ignored-attributes warning when compiling with clang

[Julien Grall]

On 03/27/2014 05:14 PM, Ian Campbell wrote:
> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
>> Clang 3.5 will fail to build most of the tools because aligned attribute
>> is not used sometimes:
>>
>> In file included from xc_core.c:64:
>> In file included from ./xg_private.h:30:
>> In file included from ./xenctrl.h:55:
>> ../../tools/include/xen/foreign/x86_64.h:198:47: error: 'aligned' attribute ignored when parsing type [-Werror,-Wignored-attributes]
>>     __align8__ uint64_t evtchn_pending[sizeof(__align8__ uint64_t) * 8];
>>                                               ^~~~~~~~~~
>> ../../tools/include/xen/foreign/x86_64.h:13:36: note: expanded from macro '__align8__'
>>  # define __align8__ __attribute__((aligned (8)))
>>                                    ^~~~~~~~~~~
>> ../../tools/include/xen/foreign/x86_64.h:199:44: error: 'aligned' attribute ignored when parsing type [-Werror,-Wignored-attributes]
>>     __align8__ uint64_t evtchn_mask[sizeof(__align8__ uint64_t) * 8];
> 
> Is sizeof(__align8__ uint64_t) != sizeof(uint64_t) under any
> circumstances? IOW can't we just drop the __align8__ here?

I don't think. This code is generated by mkheader.py. I will look at it
and see if I can modify it.

Regards,

-- 
Julien Grall

[PATCH] clang: sizeof(type) must not have __attribute__(aligned)

[Marcin Cieslak]

---
 tools/include/xen-foreign/mkheader.py | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/tools/include/xen-foreign/mkheader.py b/tools/include/xen-foreign/mkheader.py
index 0504cb8..1e1e55f 100644
--- a/tools/include/xen-foreign/mkheader.py
+++ b/tools/include/xen-foreign/mkheader.py
@@ -13,6 +13,7 @@ infiles = sys.argv[3:];
 # configuration #2: architecture information
 
 inttypes = {};
+typesizes = {}; # types used for sizeof(type) only
 header = {};
 footer = {};
 
@@ -24,6 +25,13 @@ inttypes["arm32"] = {
     "xen_ulong_t"   : "__align8__ uint64_t",
     "uint64_t"      : "__align8__ uint64_t",
 };
+typesizes["arm32"] = {
+    "unsigned long" : "__danger_unsigned_long_on_arm32",
+    "long"          : "__danger_long_on_arm32",
+    "xen_pfn_t"     : "uint64_t",
+    "xen_ulong_t"   : "uint64_t",
+    "uint64_t"      : "uint64_t",
+};
 header["arm32"] = """
 #define __arm___ARM32 1
 #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
@@ -45,6 +53,13 @@ inttypes["arm64"] = {
     "xen_ulong_t"   : "__align8__ uint64_t",
     "uint64_t"      : "__align8__ uint64_t",
 };
+typesizes["arm64"] = {
+    "unsigned long" : "__danger_unsigned_long_on_arm64",
+    "long"          : "__danger_long_on_arm64",
+    "xen_pfn_t"     : "uint64_t",
+    "xen_ulong_t"   : "uint64_t",
+    "uint64_t"      : "uint64_t",
+};
 header["arm64"] = """
 #define __aarch64___ARM64 1
 #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
@@ -66,6 +81,7 @@ inttypes["x86_32"] = {
     "xen_pfn_t"     : "uint32_t",
     "xen_ulong_t"   : "uint32_t",
 };
+typesizes["x86_32"] = inttypes["x86_32"]
 header["x86_32"] = """
 #define __i386___X86_32 1
 #pragma pack(4)
@@ -81,6 +97,12 @@ inttypes["x86_64"] = {
     "xen_pfn_t"     : "__align8__ uint64_t",
     "xen_ulong_t"   : "__align8__ uint64_t",
 };
+typesizes["x86_64"] = {
+    "unsigned long" : "uint64_t",
+    "long"          : "uint64_t",
+    "xen_pfn_t"     : "uint64_t",
+    "xen_ulong_t"   : "uint64_t",
+};
 header["x86_64"] = """
 #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
 # define __DECL_REG(name) union { uint64_t r ## name, e ## name; }
@@ -196,7 +218,8 @@ for struct in structs:
 integers = inttypes[arch].keys();
 integers.sort(lambda a, b: cmp(len(b),len(a)));
 for type in integers:
-    output = re.sub("\\b%s\\b" % type, inttypes[arch][type], output);
+    output = re.sub("\\b(?<=sizeof\()%s\\b" % type, typesizes[arch][type], output);
+    output = re.sub("\\b(?<!sizeof\()%s\\b" % type, inttypes[arch][type], output);
 
 # print results
 f = open(outfile, "w");
-- 
2.0.2

Re: [PATCH] clang: sizeof(type) must not have __attribute__(aligned)

[Ian Campbell]

On Sat, 2014-09-13 at 18:42 +0000, Marcin Cieslak wrote:

The commit message could go into a bit more detail as to why this is not
allowed. To me it sounds like clang is being overly picky, surely it can
just ignore the alignment in that context?


> +typesizes["arm32"] = {

Duplicating the entire inttypes list is not really very nice.

I would prefer instead to make the existing inttypes more structured and
separate the type itself and the other modifiers/attributes into fields
(of a class or in a tuple) and put them together in the right way as we
need them. Methods on the class or helpers (type.gen_type() gen_sizeof()
etc) would help keep this clean.

Ian.

Re: [PATCH] clang: sizeof(type) must not have __attribute__(aligned)

[Julien Grall]

Hi Ian,

On 16/09/14 09:21, Ian Campbell wrote:
> On Sat, 2014-09-13 at 18:42 +0000, Marcin Cieslak wrote:
>
> The commit message could go into a bit more detail as to why this is not
> allowed. To me it sounds like clang is being overly picky, surely it can
> just ignore the alignment in that context?

Clang will throw a warning when an attribute is unused (see 
-Wignored-attributes). In case of libxc, we compiled with -Werror which 
will turn this warning into an error.


I sent a patch few months ago [1] to disable this option. One of the 
solution was to modify mkheader.py.

Regards,

-- 
Julien Grall

Re: [PATCH] clang: sizeof(type) must not have __attribute__(aligned)

[Ian Campbell]

On Tue, 2014-09-16 at 11:26 -0700, Julien Grall wrote:
> Hi Ian,
> 
> On 16/09/14 09:21, Ian Campbell wrote:
> > On Sat, 2014-09-13 at 18:42 +0000, Marcin Cieslak wrote:
> >
> > The commit message could go into a bit more detail as to why this is not
> > allowed. To me it sounds like clang is being overly picky, surely it can
> > just ignore the alignment in that context?
> 
> Clang will throw a warning when an attribute is unused (see 
> -Wignored-attributes). In case of libxc, we compiled with -Werror which 
> will turn this warning into an error.

Sounds to me like -Wignored-attributes is a bit pointless, perhaps we
shouldn't use it?

Ian.

Re: [PATCH] clang: sizeof(type) must not have __attribute__(aligned)

[Jan Beulich]

>>> On 16.09.14 at 20:32, <ian.campbell@citrix.com> wrote:
> On Tue, 2014-09-16 at 11:26 -0700, Julien Grall wrote:
>> Hi Ian,
>> 
>> On 16/09/14 09:21, Ian Campbell wrote:
>> > On Sat, 2014-09-13 at 18:42 +0000, Marcin Cieslak wrote:
>> >
>> > The commit message could go into a bit more detail as to why this is not
>> > allowed. To me it sounds like clang is being overly picky, surely it can
>> > just ignore the alignment in that context?
>> 
>> Clang will throw a warning when an attribute is unused (see 
>> -Wignored-attributes). In case of libxc, we compiled with -Werror which 
>> will turn this warning into an error.
> 
> Sounds to me like -Wignored-attributes is a bit pointless, perhaps we
> shouldn't use it?

+1

Jan

Re: [PATCH] clang: sizeof(type) must not have __attribute__(aligned)

[Julien Grall]

Hi,

On 17/09/14 03:50, Jan Beulich wrote:
>>>> On 16.09.14 at 20:32, <ian.campbell@citrix.com> wrote:
>> On Tue, 2014-09-16 at 11:26 -0700, Julien Grall wrote:
>>> Hi Ian,
>>>
>>> On 16/09/14 09:21, Ian Campbell wrote:
>>>> On Sat, 2014-09-13 at 18:42 +0000, Marcin Cieslak wrote:
>>>>
>>>> The commit message could go into a bit more detail as to why this is not
>>>> allowed. To me it sounds like clang is being overly picky, surely it can
>>>> just ignore the alignment in that context?
>>>
>>> Clang will throw a warning when an attribute is unused (see
>>> -Wignored-attributes). In case of libxc, we compiled with -Werror which
>>> will turn this warning into an error.
>>
>> Sounds to me like -Wignored-attributes is a bit pointless, perhaps we
>> shouldn't use it?
>
> +1

Ian, shall I resend the patch which disabled this attribute?

https://patches.linaro.org/27062/

Regards,

-- 
Julien Grall

[PATCH 32/34] tools/libxl: list_domains: shutdown is typed unsigned

[Julien Grall]

Don't need to check if shutdown is positive.

Fix compilation with clang 3.5:
xl_cmdimpl.c:3350:34: error: comparison of unsigned expression >= 0 is always true [-Werror,-Wtautological-compare]
                (shutdown_reason >= 0 &&
                 ~~~~~~~~~~~~~~~ ^  ~

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 8389468..e82620d 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3324,8 +3324,7 @@ static void list_domains(int verbose, int context, int claim, int numa,
                 info[i].blocked ? 'b' : '-',
                 info[i].paused ? 'p' : '-',
                 info[i].shutdown ? 's' : '-',
-                (shutdown_reason >= 0 &&
-                 shutdown_reason < sizeof(shutdown_reason_letters)-1
+                (shutdown_reason < sizeof(shutdown_reason_letters)-1
                  ? shutdown_reason_letters[shutdown_reason] : '?'),
                 info[i].dying ? 'd' : '-',
                 ((float)info[i].cpu_time / 1e9));
-- 
1.7.10.4

Re: [PATCH 32/34] tools/libxl: list_domains: shutdown is typed unsigned

[Andrew Cooper]

On 25/03/14 16:55, Julien Grall wrote:
> Don't need to check if shutdown is positive.
>
> Fix compilation with clang 3.5:
> xl_cmdimpl.c:3350:34: error: comparison of unsigned expression >= 0 is always true [-Werror,-Wtautological-compare]
>                 (shutdown_reason >= 0 &&
>                  ~~~~~~~~~~~~~~~ ^  ~

Coverity-ID: 1055616

> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 8389468..e82620d 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3324,8 +3324,7 @@ static void list_domains(int verbose, int context, int claim, int numa,
>                  info[i].blocked ? 'b' : '-',
>                  info[i].paused ? 'p' : '-',
>                  info[i].shutdown ? 's' : '-',
> -                (shutdown_reason >= 0 &&
> -                 shutdown_reason < sizeof(shutdown_reason_letters)-1
> +                (shutdown_reason < sizeof(shutdown_reason_letters)-1
>                   ? shutdown_reason_letters[shutdown_reason] : '?'),
>                  info[i].dying ? 'd' : '-',
>                  ((float)info[i].cpu_time / 1e9));

Re: [PATCH 32/34] tools/libxl: list_domains: shutdown is typed unsigned

[Ian Campbell]

On Tue, 2014-03-25 at 18:22 +0000, Andrew Cooper wrote:
> On 25/03/14 16:55, Julien Grall wrote:
> > Don't need to check if shutdown is positive.
> >
> > Fix compilation with clang 3.5:
> > xl_cmdimpl.c:3350:34: error: comparison of unsigned expression >= 0 is always true [-Werror,-Wtautological-compare]
> >                 (shutdown_reason >= 0 &&
> >                  ~~~~~~~~~~~~~~~ ^  ~
> 
> Coverity-ID: 1055616
> 
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Although that construct looks ripe for refactoring for clarity to me.

Ian.

[PATCH 33/34] tools/libxl: libxl__json_object_to_yajl_gen should return yajl_gen_status

[Julien Grall]

All the functions in lixbl__json_object_to_yajl_gen returns an yajl_gen_status
enum.

Also replace yal_gen_status_ok by yalj_gen_status_ok.

This is fix compilation with clang 3.5:
libxl_json.c:378:16: error: implicit conversion from enumeration type 'yajl_gen_status' to different enumeration type 'yajl_status' [-Werror,-Wenum-conversion]
        return yajl_gen_null(hand);
        ~~~~~~ ^~~~~~~~~~~~~~~~~~~

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_internal.h |    2 +-
 tools/libxl/libxl_json.c     |   18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b3a200d..bfefd31 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1722,7 +1722,7 @@ libxl__json_map_node *libxl__json_map_node_get(const libxl__json_object *o,
 _hidden const libxl__json_object *libxl__json_map_get(const char *key,
                                           const libxl__json_object *o,
                                           libxl__json_node_type expected_type);
-_hidden yajl_status libxl__json_object_to_yajl_gen(libxl__gc *gc_opt,
+_hidden yajl_gen_status libxl__json_object_to_yajl_gen(libxl__gc *gc_opt,
                                                    yajl_gen hand,
                                                    libxl__json_object *param);
 _hidden void libxl__json_object_free(libxl__gc *gc_opt,
diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index d2f7de8..8e28c58 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -366,12 +366,12 @@ const libxl__json_object *libxl__json_map_get(const char *key,
     return NULL;
 }
 
-yajl_status libxl__json_object_to_yajl_gen(libxl__gc *gc,
-                                           yajl_gen hand,
-                                           libxl__json_object *obj)
+yajl_gen_status libxl__json_object_to_yajl_gen(libxl__gc *gc,
+                                               yajl_gen hand,
+                                               libxl__json_object *obj)
 {
     int idx = 0;
-    yajl_status rc;
+    yajl_gen_status rc;
 
     switch (obj->type) {
     case JSON_NULL:
@@ -390,17 +390,17 @@ yajl_status libxl__json_object_to_yajl_gen(libxl__gc *gc,
         libxl__json_map_node *node = NULL;
 
         rc = yajl_gen_map_open(hand);
-        if (rc != yajl_status_ok)
+        if (rc != yajl_gen_status_ok)
             return rc;
         for (idx = 0; idx < obj->u.map->count; idx++) {
             if (flexarray_get(obj->u.map, idx, (void**)&node) != 0)
                 break;
 
             rc = libxl__yajl_gen_asciiz(hand, node->map_key);
-            if (rc != yajl_status_ok)
+            if (rc != yajl_gen_status_ok)
                 return rc;
             rc = libxl__json_object_to_yajl_gen(gc, hand, node->obj);
-            if (rc != yajl_status_ok)
+            if (rc != yajl_gen_status_ok)
                 return rc;
         }
         return yajl_gen_map_close(hand);
@@ -409,13 +409,13 @@ yajl_status libxl__json_object_to_yajl_gen(libxl__gc *gc,
         libxl__json_object *node = NULL;
 
         rc = yajl_gen_array_open(hand);
-        if (rc != yajl_status_ok)
+        if (rc != yajl_gen_status_ok)
             return rc;
         for (idx = 0; idx < obj->u.array->count; idx++) {
             if (flexarray_get(obj->u.array, idx, (void**)&node) != 0)
                 break;
             rc = libxl__json_object_to_yajl_gen(gc, hand, node);
-            if (rc != yajl_status_ok)
+            if (rc != yajl_gen_status_ok)
                 return rc;
         }
         return yajl_gen_array_close(hand);
-- 
1.7.10.4

Re: [PATCH 33/34] tools/libxl: libxl__json_object_to_yajl_gen should return yajl_gen_status

[Ian Campbell]

On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> All the functions in lixbl__json_object_to_yajl_gen returns an yajl_gen_status
> enum.
> 
> Also replace yal_gen_status_ok by yalj_gen_status_ok.

Does this work with both yajl1 and yajl2? We are trying to support both.

Ian.

Re: [PATCH 33/34] tools/libxl: libxl__json_object_to_yajl_gen should return yajl_gen_status

[Julien Grall]

Hi Ian,

On 03/25/2014 05:28 PM, Ian Campbell wrote:
> On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
>> All the functions in lixbl__json_object_to_yajl_gen returns an yajl_gen_status
>> enum.
>>
>> Also replace yal_gen_status_ok by yalj_gen_status_ok.
> 
> Does this work with both yajl1 and yajl2? We are trying to support both.

I checked yajl 1.0.12 API, as yajl2 every yajl_gen_* functions return an
yajl_gen_status.

Every other functions in libxl_json.c was correctly using yajl_gen_status.

Regards,

-- 
Julien Grall

Re: [PATCH 33/34] tools/libxl: libxl__json_object_to_yajl_gen should

[Ian Campbell]

On Thu, 2014-03-27 at 12:37 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 03/25/2014 05:28 PM, Ian Campbell wrote:
> > On Tue, 2014-03-25 at 16:55 +0000, Julien Grall wrote:
> >> All the functions in lixbl__json_object_to_yajl_gen returns an yajl_gen_status
> >> enum.
> >>
> >> Also replace yal_gen_status_ok by yalj_gen_status_ok.
> > 
> > Does this work with both yajl1 and yajl2? We are trying to support both.
> 
> I checked yajl 1.0.12 API, as yajl2 every yajl_gen_* functions return an
> yajl_gen_status.
> 
> Every other functions in libxl_json.c was correctly using yajl_gen_status.

Thanks. Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.

[PATCH 34/34] DO NOT APPLY xen/common: kernel: Workaround clang 3.5

[Julien Grall]

http://llvm.org/bugs/show_bug.cgi?id=19199

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/common/kernel.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index b371f8f..320c3fb 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -342,8 +342,9 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     case XENVER_guest_handle:
     {
-        if ( copy_to_guest(arg, current->domain->handle,
-                           ARRAY_SIZE(current->domain->handle)) )
+        struct domain *d = current->domain;
+
+        if ( copy_to_guest(arg, d->handle, ARRAY_SIZE(d->handle)) )
             return -EFAULT;
         return 0;
     }
-- 
1.7.10.4

Re: [PATCH 00/34] Add clang support for ARM and cleanups

[Stefano Stabellini]

On Tue, 25 Mar 2014, Julien Grall wrote:
> Hello all,
> 
> The main goal of this patch series is to add support for Clang on ARM.
> The work has been done with Clang 3.5 which is not yet release but have
> a good support for ARM32. I think clang 3.4 may also be able to compile
> Xen but I didn't try.
> 
> With the first part of the serie (patch #1-#19 and patch #33), I'm able to boot
> Xen compiled by Clang on midway. The patch #33 workaround for a bug in Clang
> (http://llvm.org/bugs/show_bug.cgi?id=19199). I'm not sure if the patch would
> be acceptable for Xen upstream.
> 
> While I was working on Clang 3.5, I played with -Wmissing-prototypes (patch #20-#30)
> and discovered some exported functions which were:
>     - the prototype were not declared before because of a missing include
>     - never used
> 
> Finally the last part of this series (patch #31-33) contains a start of
> bug fixes to be able to compile the tools with Clang. I didn't include
> the patch https://patches.linaro.org/26826/.
> 
> The branch will all the patch applied (+few required patches) can be found here:
>     git clone git://xenbits.xen.org/people/julieng/xen-unstable.git -b clang
> 
> Any comments and questions are welcomed.

Patches #5 #7 #8 #9 #10 #12 #16 #17 #18 #20 #21 #22 #23 #24 #25 #26 #28
#29 are correct and I think they should be applied.
You can add my reviewed-by to them.


> Sincerely yours,
> 
> Julien Grall (34):
>   xen: clang: Disable initializer-overrides warning
>   xen: clang: Disable built-in assembler
>   xen: clang: Support correctly cross-compile
>   xen/compiler: Introduce always_unused define
>   xen/xsm: flask: Remove unused function avc_sidcmp
>   xen/x86: shadow: sh_next_page is only used when GUEST_PAGING_LEVELS =
>     2
>   xen/common: sched_sedf: Remove unused functions
>   xen/common: rcupdate: Remove unused function rcu_batch_after
>   xen/common: tmem: Remove dumb check in do_tmem_destroy_pool
>   xen/crypto: rijndael: Fix compilation with Clang 3.5
>   xen/arm: Introduce __builtin_stack_pointer
>   xen/arm: psci: Don't need to check if vcpuid is negative
>   xen/arm: gic: Introduce GIC_SGI_MAX
>   xen/arm: mm: Mark check_memory_layout_alignment_constraints as unused
>   xen/arm: traps: Mark check_stack_alignment_constraints as unused
>   xen/arm: Guard correctly asm-arm/platforms/midway.h
>   xen/arm: Guard correctly asm-arm/platform/omap5.h
>   xen/arm: omap5: Correctly constify platform compatibility list
>   xen/arm: Provide eabi wrapper for __aeabi_mem* functions
>   xen/common: cpupool: cpupool_unassign_cpu is only used internally
>   xen/common: domctl: Some functions are only used internally
>   xen/common: tmem: Some functions are only used internally
>   xen/sched: credit2: Some functions are only used internally
>   xen/arm: setup: setup_cache is only used internally
>   xen/arm: traps: show_stack is only used internally
>   xen/arm: traps: Drop dump_guest_s1_walk
>   xen/arm: time: move ticks_to_ns and ns_to_ticks in asm/time.h
>   xen/xsm: flask: flask_disable is only used internally
>   xen/xsm: flask: ss: remove unused function determine_oocontext
>   xen: Add missing includes on different files
>   tools: Disable ignored-attributes warning when compiling with clang
>   tools/libxl: list_domains: shutdown is typed unsigned
>   tools/libxl: libxl__json_object_to_yajl_gen should return
>     yajl_gen_status
>   DO NOT APPLY xen/common: kernel: Workaround clang 3.5
> 
>  config/StdGNU.mk                       |    6 +++-
>  tools/Rules.mk                         |    2 ++
>  tools/libxl/libxl_internal.h           |    2 +-
>  tools/libxl/libxl_json.c               |   18 +++++-----
>  tools/libxl/xl_cmdimpl.c               |    3 +-
>  xen/Rules.mk                           |    3 ++
>  xen/arch/arm/Makefile                  |    1 +
>  xen/arch/arm/eabi.c                    |   29 +++++++++++++++
>  xen/arch/arm/gic.c                     |    7 ++--
>  xen/arch/arm/hvm.c                     |    1 +
>  xen/arch/arm/mm.c                      |    4 ++-
>  xen/arch/arm/platforms/omap5.c         |    2 +-
>  xen/arch/arm/setup.c                   |    2 +-
>  xen/arch/arm/shutdown.c                |    1 +
>  xen/arch/arm/smp.c                     |    1 +
>  xen/arch/arm/time.c                    |   11 +-----
>  xen/arch/arm/traps.c                   |   61 ++------------------------------
>  xen/arch/arm/vpsci.c                   |    2 +-
>  xen/arch/arm/vtimer.c                  |    4 +--
>  xen/arch/x86/mm/shadow/multi.c         |    2 ++
>  xen/common/cpupool.c                   |    2 +-
>  xen/common/domctl.c                    |   20 +++++------
>  xen/common/event_channel.c             |    1 +
>  xen/common/grant_table.c               |    1 +
>  xen/common/kernel.c                    |    5 +--
>  xen/common/multicall.c                 |    3 ++
>  xen/common/rcupdate.c                  |    6 ----
>  xen/common/sched_credit2.c             |   11 +++---
>  xen/common/sched_sedf.c                |   12 -------
>  xen/common/sort.c                      |    1 +
>  xen/common/tmem.c                      |    9 +++--
>  xen/crypto/rijndael.c                  |    3 ++
>  xen/drivers/video/arm_hdlcd.c          |    1 +
>  xen/include/asm-arm/current.h          |   17 +++++++--
>  xen/include/asm-arm/gic.h              |    2 ++
>  xen/include/asm-arm/platforms/midway.h |    2 +-
>  xen/include/asm-arm/platforms/omap5.h  |    2 +-
>  xen/include/asm-arm/time.h             |   13 +++++++
>  xen/include/xen/compiler.h             |    2 ++
>  xen/xsm/flask/avc.c                    |    5 ---
>  xen/xsm/flask/flask_op.c               |    2 +-
>  xen/xsm/flask/ss/conditional.h         |    1 +
>  xen/xsm/flask/ss/services.c            |   14 --------
>  43 files changed, 141 insertions(+), 156 deletions(-)
>  create mode 100644 xen/arch/arm/eabi.c
> 
> -- 
> 1.7.10.4
> 

Re: [PATCH 00/34] Add clang support for ARM and cleanups

[Julien Grall]

Hi Stefano,

On 25/03/14 18:22, Stefano Stabellini wrote:
> On Tue, 25 Mar 2014, Julien Grall wrote:
>> Hello all,
>>
>> The main goal of this patch series is to add support for Clang on ARM.
>> The work has been done with Clang 3.5 which is not yet release but have
>> a good support for ARM32. I think clang 3.4 may also be able to compile
>> Xen but I didn't try.
>>
>> With the first part of the serie (patch #1-#19 and patch #33), I'm able to boot
>> Xen compiled by Clang on midway. The patch #33 workaround for a bug in Clang
>> (http://llvm.org/bugs/show_bug.cgi?id=19199). I'm not sure if the patch would
>> be acceptable for Xen upstream.
>>
>> While I was working on Clang 3.5, I played with -Wmissing-prototypes (patch #20-#30)
>> and discovered some exported functions which were:
>>      - the prototype were not declared before because of a missing include
>>      - never used
>>
>> Finally the last part of this series (patch #31-33) contains a start of
>> bug fixes to be able to compile the tools with Clang. I didn't include
>> the patch https://patches.linaro.org/26826/.
>>
>> The branch will all the patch applied (+few required patches) can be found here:
>>      git clone git://xenbits.xen.org/people/julieng/xen-unstable.git -b clang
>>
>> Any comments and questions are welcomed.
>
> Patches #5 #7 #8 #9 #10 #12 #16 #17 #18 #20 #21 #22 #23 #24 #25 #26 #28
> #29 are correct and I think they should be applied.
> You can add my reviewed-by to them.

Thanks for your review. FYI, every patches can be applied separately.

Regards,

-- 
Julien Grall

Re: [PATCH 00/34] Add clang support for ARM and cleanups

[Tim Deegan]

Hi,

Thanks for this!  

At 16:55 +0000 on 25 Mar (1395762907), Julien Grall wrote:
> The main goal of this patch series is to add support for Clang on ARM.
> The work has been done with Clang 3.5 which is not yet release but have
> a good support for ARM32. I think clang 3.4 may also be able to compile
> Xen but I didn't try.

Xen also builds for me with clang 3.2 with this series applied.

You can add Reviewed-by: Tim Deegan <tim@xen.org> to patch 1 and 3-30,
with the following caveats:

#11: assuming a v2 where the new fn is named get_stack_pointer()
#13: for the v2 you already posted. 
#30: assuming v2 that does what Jan wants with the compat definitions.

I'll look at patch #2 separately.  Xen builds OK without it using clang
3.2; let me upgrade to 3.5 and see if there are better workarounds.

Cheers,

Tim.