thr3ads.net - Xen devel - [PATCH 0 of 6] Make xen build with clang/LLVM again [Apr 2012]

If this information is useful, please help other people find it:
Share via:

Tim Deegan

2012-Apr-05 12:07 UTC

[PATCH 0 of 6] Make xen build with clang/LLVM again

This series makes the hypervisor build with clang/LLVM again,
after a certain amount of bit-rot. 

Some of these are obvious bug-fixes which are picked up by clang''s 
slightly different -W* implementation.  Some (the spinlock change
in particular) are working around deficiencies in clang/llvm.  I''m 
happy to change those ones to avoid any concerns people have about
damaging the GCC build. 

Cheers,

Tim.

Tim Deegan

2012-Apr-05 12:07 UTC

head link

[PATCH 1 of 6] xen: Add -Wno-unused-value to the clang CFLAGS

# HG changeset patch
# User Tim Deegan <tim@xen.org>
# Date 1333626954 -3600
# Node ID 8518fb0c8c996dca67efd39d31962a6d3502c2ed
# Parent  d690c7e896a26c54a5ab85458824059de72d5cba
xen: Add -Wno-unused-value to the clang CFLAGS

clang complains about a lot of functions and macros whose return value
is unused.  I started on patches to drop some functions'' return values
and scatter (void)s around callers, but it was getting too messy.
Just turn off the warning instead.

Signed-off-by: Tim Deegan <tim@xen.org>

diff -r d690c7e896a2 -r 8518fb0c8c99 Config.mk
--- a/Config.mk	Thu Apr 05 11:06:03 2012 +0100
+++ b/Config.mk	Thu Apr 05 12:55:54 2012 +0100
@@ -159,7 +159,8 @@ CFLAGS += -Wall -Wstrict-prototypes
 
 # Clang complains about macros that expand to ''if ( ( foo == bar ) )
...''
 # and is over-zealous with the printf format lint
-CFLAGS-$(clang) += -Wno-parentheses -Wno-format
+# and is a bit too fierce about unused return values
+CFLAGS-$(clang) += -Wno-parentheses -Wno-format -Wno-unused-value
 
 $(call cc-option-add,HOSTCFLAGS,HOSTCC,-Wdeclaration-after-statement)
 $(call cc-option-add,CFLAGS,CC,-Wdeclaration-after-statement)

Tim Deegan

2012-Apr-05 12:07 UTC

head link

[PATCH 2 of 6] x86: don''t use .subsection to out-of-line failure path in spinlock.h

# HG changeset patch
# User Tim Deegan <tim@xen.org>
# Date 1333626954 -3600
# Node ID 0ecf439475e12f185553f42f56f099be5f328cce
# Parent  8518fb0c8c996dca67efd39d31962a6d3502c2ed
x86: don''t use .subsection to out-of-line failure path in spinlock.h

LLVM''s assembler doesn''t support the .subsection directive. 
Instead,
leave the failure path inline with an unconditional jump past it in
the success path (so the conditional jump is still forwards).

Signed-off-by: Tim Deegan <tim@xen.org>

diff -r 8518fb0c8c99 -r 0ecf439475e1 xen/include/asm-x86/spinlock.h
--- a/xen/include/asm-x86/spinlock.h	Thu Apr 05 12:55:54 2012 +0100
+++ b/xen/include/asm-x86/spinlock.h	Thu Apr 05 12:55:54 2012 +0100
@@ -44,12 +44,11 @@ static always_inline int _raw_read_trylo
 
     asm volatile (
         "    lock; decl %0         \n"
-        "    jns 2f                \n"
-        "1:  .subsection 1         \n"
-        "2:  lock; incl %0         \n"
+        "    jns 1f                \n"
+        "    jmp 2f                \n"
+        "1:  lock; incl %0         \n"
         "    decl %1               \n"
-        "    jmp 1b                \n"
-        "    .subsection 0         \n"
+        "2:                        \n"
         : "=m" (rw->lock), "=r" (acquired) :
"1" (1) : "memory" );
 
     return acquired;

Tim Deegan

2012-Apr-05 12:07 UTC

head link

[PATCH 3 of 6] x86/mm: Another couple of comparisons of unsigned vars with < 0

# HG changeset patch
# User Tim Deegan <tim@xen.org>
# Date 1333626954 -3600
# Node ID 5eeaa7b25a60327c48bf17472e9a00bdfc67378f
# Parent  0ecf439475e12f185553f42f56f099be5f328cce
x86/mm: Another couple of comparisons of unsigned vars with < 0.

Adding the explicit (unsigned) casts in case enums ever end up signed.

Signed-off-by: Tim Deegan <tim@xen.org>

diff -r 0ecf439475e1 -r 5eeaa7b25a60 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Thu Apr 05 12:55:54 2012 +0100
+++ b/xen/arch/x86/mm/p2m.c	Thu Apr 05 12:55:54 2012 +0100
@@ -1305,7 +1305,7 @@ int p2m_set_mem_access(struct domain *d,
         p2m->default_access,
     };
 
-    if ( access >= HVMMEM_access_default || access < 0 )
+    if ( (unsigned) access >= HVMMEM_access_default )
         return -EINVAL;
 
     a = memaccess[access];
@@ -1367,7 +1367,7 @@ int p2m_get_mem_access(struct domain *d,
     if ( mfn_x(mfn) == INVALID_MFN )
         return -ESRCH;
     
-    if ( a >= ARRAY_SIZE(memaccess) || a < 0 )
+    if ( (unsigned) a >= ARRAY_SIZE(memaccess) )
         return -ERANGE;
 
     *access =  memaccess[a];

Tim Deegan

2012-Apr-05 12:07 UTC

head link

[PATCH 4 of 6] x86: fix logical ANDs used to mask bitfields

# HG changeset patch
# User Tim Deegan <tim@xen.org>
# Date 1333626954 -3600
# Node ID dfe9453c066f3fbfa09c847d7ec381cdc0da0f99
# Parent  5eeaa7b25a60327c48bf17472e9a00bdfc67378f
x86: fix logical ANDs used to mask bitfields.

Signed-off-by: Tim Deegan <tim@xen.org>

diff -r 5eeaa7b25a60 -r dfe9453c066f xen/arch/x86/hvm/svm/svm.c
--- a/xen/arch/x86/hvm/svm/svm.c	Thu Apr 05 12:55:54 2012 +0100
+++ b/xen/arch/x86/hvm/svm/svm.c	Thu Apr 05 12:55:54 2012 +0100
@@ -752,7 +752,7 @@ static void svm_lwp_interrupt(struct cpu
     ack_APIC_irq();
     vlapic_set_irq(
         vcpu_vlapic(curr),
-        (curr->arch.hvm_svm.guest_lwp_cfg >> 40) && 0xff,
+        (curr->arch.hvm_svm.guest_lwp_cfg >> 40) & 0xff,
         0);
 }
 
diff -r 5eeaa7b25a60 -r dfe9453c066f xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c	Thu Apr 05 12:55:54 2012 +0100
+++ b/xen/arch/x86/hvm/vmx/vmx.c	Thu Apr 05 12:55:54 2012 +0100
@@ -1382,7 +1382,7 @@ void vmx_inject_extint(int trap)
     if ( nestedhvm_vcpu_in_guestmode(v) ) {
         pin_based_cntrl = __get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx, 
                                      PIN_BASED_VM_EXEC_CONTROL);
-        if ( pin_based_cntrl && PIN_BASED_EXT_INTR_MASK ) {
+        if ( pin_based_cntrl & PIN_BASED_EXT_INTR_MASK ) {
             nvmx_enqueue_n2_exceptions (v, 
                INTR_INFO_VALID_MASK | (X86_EVENTTYPE_EXT_INTR<<8) | trap,
                HVM_DELIVER_NO_ERROR_CODE);
@@ -1401,7 +1401,7 @@ void vmx_inject_nmi(void)
     if ( nestedhvm_vcpu_in_guestmode(v) ) {
         pin_based_cntrl = __get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx, 
                                      PIN_BASED_VM_EXEC_CONTROL);
-        if ( pin_based_cntrl && PIN_BASED_NMI_EXITING ) {
+        if ( pin_based_cntrl & PIN_BASED_NMI_EXITING ) {
             nvmx_enqueue_n2_exceptions (v, 
                INTR_INFO_VALID_MASK | (X86_EVENTTYPE_NMI<<8) | TRAP_nmi,
                HVM_DELIVER_NO_ERROR_CODE);

Tim Deegan

2012-Apr-05 12:07 UTC

head link

[PATCH 5 of 6] x86: fix memset(ptr, 0, sizeof ptr)

# HG changeset patch
# User Tim Deegan <tim@xen.org>
# Date 1333626954 -3600
# Node ID 5101e5ed24732919076d5285e55c7b53032749c2
# Parent  dfe9453c066f3fbfa09c847d7ec381cdc0da0f99
x86: fix memset(ptr, 0, sizeof ptr).

Signed-off-by: Tim Deegan <tim@xen.org>

diff -r dfe9453c066f -r 5101e5ed2473 xen/arch/x86/cpu/mcheck/amd_f10.c
--- a/xen/arch/x86/cpu/mcheck/amd_f10.c	Thu Apr 05 12:55:54 2012 +0100
+++ b/xen/arch/x86/cpu/mcheck/amd_f10.c	Thu Apr 05 12:55:54 2012 +0100
@@ -73,9 +73,9 @@ amd_f10_handler(struct mc_info *mi, uint
 		return NULL;
 	}
 
-	memset(mc_ext, 0, sizeof(mc_ext));
+	memset(mc_ext, 0, sizeof(*mc_ext));
 	mc_ext->common.type = MC_TYPE_EXTENDED;
-	mc_ext->common.size = sizeof(mc_ext);
+	mc_ext->common.size = sizeof(*mc_ext);
 	mc_ext->mc_msrs = 3;
 
 	mc_ext->mc_msr[0].reg = MSR_F10_MC4_MISC1;
diff -r dfe9453c066f -r 5101e5ed2473 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Thu Apr 05 12:55:54 2012 +0100
+++ b/xen/arch/x86/mm/p2m.c	Thu Apr 05 12:55:54 2012 +0100
@@ -1236,7 +1236,7 @@ bool_t p2m_mem_access_check(unsigned lon
     if ( req )
     {
         *req_ptr = req;
-        memset(req, 0, sizeof(req));
+        memset(req, 0, sizeof (mem_event_request_t));
         req->reason = MEM_EVENT_REASON_VIOLATION;
 
         /* Pause the current VCPU */

Tim Deegan

2012-Apr-05 12:07 UTC

head link

[PATCH 6 of 6] x86: explicitly mark an __initdata variable as used

# HG changeset patch
# User Tim Deegan <tim@xen.org>
# Date 1333626954 -3600
# Node ID 5a2f5ab5128e4b13b3fd2dbcae1f084bc922584e
# Parent  5101e5ed24732919076d5285e55c7b53032749c2
x86: explicitly mark an __initdata variable as used.

This stops LLVM from replacing it with a different, auto-generated
variable as part of an optimization.  (The auto-generated variable
ends up in the normal data section, failing the check that this
file only contains __initdata vars).

Signed-off-by: Tim Deegan <tim@xen.org>

diff -r 5101e5ed2473 -r 5a2f5ab5128e xen/arch/x86/domain_build.c
--- a/xen/arch/x86/domain_build.c	Thu Apr 05 12:55:54 2012 +0100
+++ b/xen/arch/x86/domain_build.c	Thu Apr 05 12:55:54 2012 +0100
@@ -129,7 +129,7 @@ static struct page_info * __init alloc_c
     struct domain *d, unsigned long max_pages)
 {
     static unsigned int __initdata last_order = MAX_ORDER;
-    static unsigned int __initdata memflags = MEMF_no_dma;
+    static unsigned int __initdata __attribute__((used)) memflags =
MEMF_no_dma;
     struct page_info *page;
     unsigned int order = get_order_from_pages(max_pages), free_order;

Jan Beulich

2012-Apr-05 13:28 UTC

head link

Re: [PATCH 2 of 6] x86: don''t use .subsection to out-of-line failure path in spinlock.h

>>> On 05.04.12 at 14:07, Tim Deegan <tim@xen.org> wrote:
> # HG changeset patch
> # User Tim Deegan <tim@xen.org>
> # Date 1333626954 -3600
> # Node ID 0ecf439475e12f185553f42f56f099be5f328cce
> # Parent  8518fb0c8c996dca67efd39d31962a6d3502c2ed
> x86: don''t use .subsection to out-of-line failure path in
spinlock.h
> 
> LLVM''s assembler doesn''t support the .subsection
directive.  Instead,
> leave the failure path inline with an unconditional jump past it in
> the success path (so the conditional jump is still forwards).
Please don''t - the failure path should really be out-of-line here;
that''s
why the .subsection directive got added in the first place. And there
are more uses of .subsection elsewhere, which I hope you''re not
intending to all undo just because of a shortcoming of a non-standard
assembler. Abstracting this in some way might be doable, e.g.
moving the code into .text.1 or some such when non-gas is used.

Jan
> Signed-off-by: Tim Deegan <tim@xen.org>
> 
> diff -r 8518fb0c8c99 -r 0ecf439475e1 xen/include/asm-x86/spinlock.h
> --- a/xen/include/asm-x86/spinlock.h	Thu Apr 05 12:55:54 2012 +0100
> +++ b/xen/include/asm-x86/spinlock.h	Thu Apr 05 12:55:54 2012 +0100
> @@ -44,12 +44,11 @@ static always_inline int _raw_read_trylo
>  
>      asm volatile (
>          "    lock; decl %0         \n"
> -        "    jns 2f                \n"
> -        "1:  .subsection 1         \n"
> -        "2:  lock; incl %0         \n"
> +        "    jns 1f                \n"
> +        "    jmp 2f                \n"
> +        "1:  lock; incl %0         \n"
>          "    decl %1               \n"
> -        "    jmp 1b                \n"
> -        "    .subsection 0         \n"
> +        "2:                        \n"
>          : "=m" (rw->lock), "=r" (acquired) :
"1" (1) : "memory" );
>  
>      return acquired;
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

Jan Beulich

2012-Apr-05 13:31 UTC

head link

Re: [PATCH 3 of 6] x86/mm: Another couple of comparisons of unsigned vars with < 0

>>> On 05.04.12 at 14:07, Tim Deegan <tim@xen.org> wrote:
> # HG changeset patch
> # User Tim Deegan <tim@xen.org>
> # Date 1333626954 -3600
> # Node ID 5eeaa7b25a60327c48bf17472e9a00bdfc67378f
> # Parent  0ecf439475e12f185553f42f56f099be5f328cce
> x86/mm: Another couple of comparisons of unsigned vars with < 0.
> 
> Adding the explicit (unsigned) casts in case enums ever end up signed.
Ugly, but unavoidable if range comparisons are done on enums (which
is what ought to get fixed instead imo).

Jan
> Signed-off-by: Tim Deegan <tim@xen.org>
> 
> diff -r 0ecf439475e1 -r 5eeaa7b25a60 xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c	Thu Apr 05 12:55:54 2012 +0100
> +++ b/xen/arch/x86/mm/p2m.c	Thu Apr 05 12:55:54 2012 +0100
> @@ -1305,7 +1305,7 @@ int p2m_set_mem_access(struct domain *d,
>          p2m->default_access,
>      };
>  
> -    if ( access >= HVMMEM_access_default || access < 0 )
> +    if ( (unsigned) access >= HVMMEM_access_default )
>          return -EINVAL;
>  
>      a = memaccess[access];
> @@ -1367,7 +1367,7 @@ int p2m_get_mem_access(struct domain *d,
>      if ( mfn_x(mfn) == INVALID_MFN )
>          return -ESRCH;
>      
> -    if ( a >= ARRAY_SIZE(memaccess) || a < 0 )
> +    if ( (unsigned) a >= ARRAY_SIZE(memaccess) )
>          return -ERANGE;
>  
>      *access =  memaccess[a];
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

Jan Beulich

2012-Apr-05 13:33 UTC

head link

Re: [PATCH 4 of 6] x86: fix logical ANDs used to mask bitfields

>>> On 05.04.12 at 14:07, Tim Deegan <tim@xen.org> wrote:
> # HG changeset patch
> # User Tim Deegan <tim@xen.org>
> # Date 1333626954 -3600
> # Node ID dfe9453c066f3fbfa09c847d7ec381cdc0da0f99
> # Parent  5eeaa7b25a60327c48bf17472e9a00bdfc67378f
> x86: fix logical ANDs used to mask bitfields.
> 
> Signed-off-by: Tim Deegan <tim@xen.org>
Acked-by: Jan Beulich <jbeulich@suse.com>
> diff -r 5eeaa7b25a60 -r dfe9453c066f xen/arch/x86/hvm/svm/svm.c
> --- a/xen/arch/x86/hvm/svm/svm.c	Thu Apr 05 12:55:54 2012 +0100
> +++ b/xen/arch/x86/hvm/svm/svm.c	Thu Apr 05 12:55:54 2012 +0100
> @@ -752,7 +752,7 @@ static void svm_lwp_interrupt(struct cpu
>      ack_APIC_irq();
>      vlapic_set_irq(
>          vcpu_vlapic(curr),
> -        (curr->arch.hvm_svm.guest_lwp_cfg >> 40) && 0xff,
> +        (curr->arch.hvm_svm.guest_lwp_cfg >> 40) & 0xff,
>          0);
>  }
>  
> diff -r 5eeaa7b25a60 -r dfe9453c066f xen/arch/x86/hvm/vmx/vmx.c
> --- a/xen/arch/x86/hvm/vmx/vmx.c	Thu Apr 05 12:55:54 2012 +0100
> +++ b/xen/arch/x86/hvm/vmx/vmx.c	Thu Apr 05 12:55:54 2012 +0100
> @@ -1382,7 +1382,7 @@ void vmx_inject_extint(int trap)
>      if ( nestedhvm_vcpu_in_guestmode(v) ) {
>          pin_based_cntrl = __get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx, 
>                                       PIN_BASED_VM_EXEC_CONTROL);
> -        if ( pin_based_cntrl && PIN_BASED_EXT_INTR_MASK ) {
> +        if ( pin_based_cntrl & PIN_BASED_EXT_INTR_MASK ) {
>              nvmx_enqueue_n2_exceptions (v, 
>                 INTR_INFO_VALID_MASK | (X86_EVENTTYPE_EXT_INTR<<8) |
trap,
>                 HVM_DELIVER_NO_ERROR_CODE);
> @@ -1401,7 +1401,7 @@ void vmx_inject_nmi(void)
>      if ( nestedhvm_vcpu_in_guestmode(v) ) {
>          pin_based_cntrl = __get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx, 
>                                       PIN_BASED_VM_EXEC_CONTROL);
> -        if ( pin_based_cntrl && PIN_BASED_NMI_EXITING ) {
> +        if ( pin_based_cntrl & PIN_BASED_NMI_EXITING ) {
>              nvmx_enqueue_n2_exceptions (v, 
>                 INTR_INFO_VALID_MASK | (X86_EVENTTYPE_NMI<<8) |
TRAP_nmi,
>                 HVM_DELIVER_NO_ERROR_CODE);
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

Jan Beulich

2012-Apr-05 13:38 UTC

head link

Re: [PATCH 5 of 6] x86: fix memset(ptr, 0, sizeof ptr)

>>> On 05.04.12 at 14:07, Tim Deegan <tim@xen.org> wrote:
> # HG changeset patch
> # User Tim Deegan <tim@xen.org>
> # Date 1333626954 -3600
> # Node ID 5101e5ed24732919076d5285e55c7b53032749c2
> # Parent  dfe9453c066f3fbfa09c847d7ec381cdc0da0f99
> x86: fix memset(ptr, 0, sizeof ptr).
> 
> Signed-off-by: Tim Deegan <tim@xen.org>
> 
> diff -r dfe9453c066f -r 5101e5ed2473 xen/arch/x86/cpu/mcheck/amd_f10.c
> --- a/xen/arch/x86/cpu/mcheck/amd_f10.c	Thu Apr 05 12:55:54 2012 +0100
> +++ b/xen/arch/x86/cpu/mcheck/amd_f10.c	Thu Apr 05 12:55:54 2012 +0100
> @@ -73,9 +73,9 @@ amd_f10_handler(struct mc_info *mi, uint
>  		return NULL;
>  	}
>  
> -	memset(mc_ext, 0, sizeof(mc_ext));
> +	memset(mc_ext, 0, sizeof(*mc_ext));
>  	mc_ext->common.type = MC_TYPE_EXTENDED;
> -	mc_ext->common.size = sizeof(mc_ext);
> +	mc_ext->common.size = sizeof(*mc_ext);
>  	mc_ext->mc_msrs = 3;
>  
>  	mc_ext->mc_msr[0].reg = MSR_F10_MC4_MISC1;
> diff -r dfe9453c066f -r 5101e5ed2473 xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c	Thu Apr 05 12:55:54 2012 +0100
> +++ b/xen/arch/x86/mm/p2m.c	Thu Apr 05 12:55:54 2012 +0100
> @@ -1236,7 +1236,7 @@ bool_t p2m_mem_access_check(unsigned lon
>      if ( req )
>      {
>          *req_ptr = req;
> -        memset(req, 0, sizeof(req));
> +        memset(req, 0, sizeof (mem_event_request_t));
Why not

        memset(req, 0, sizeof (*req));

? If req''s type changes, you still want to clear all of it (and nothing
more).

Oh, wait - this gets xmalloc()-ed immediately before that if(). Mind
simply switching that one to xzalloc(), and deleting the memset()
altogether? (I wonder how many other xmalloc()/memset() pairs
went in again since the introduction of xzalloc() - one of the
purposes of this was to avoid this particular common mistake.)

Apart from that
Acked-by: Jan Beulich <jbeulich@suse.com>
>          req->reason = MEM_EVENT_REASON_VIOLATION;
>  
>          /* Pause the current VCPU */
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

Jan Beulich

2012-Apr-05 13:44 UTC

head link

Re: [PATCH 6 of 6] x86: explicitly mark an __initdata variable as used

>>> On 05.04.12 at 14:07, Tim Deegan <tim@xen.org> wrote:
> # HG changeset patch
> # User Tim Deegan <tim@xen.org>
> # Date 1333626954 -3600
> # Node ID 5a2f5ab5128e4b13b3fd2dbcae1f084bc922584e
> # Parent  5101e5ed24732919076d5285e55c7b53032749c2
> x86: explicitly mark an __initdata variable as used.
> 
> This stops LLVM from replacing it with a different, auto-generated
> variable as part of an optimization.  (The auto-generated variable
> ends up in the normal data section, failing the check that this
> file only contains __initdata vars).
> 
> Signed-off-by: Tim Deegan <tim@xen.org>
> 
> diff -r 5101e5ed2473 -r 5a2f5ab5128e xen/arch/x86/domain_build.c
> --- a/xen/arch/x86/domain_build.c	Thu Apr 05 12:55:54 2012 +0100
> +++ b/xen/arch/x86/domain_build.c	Thu Apr 05 12:55:54 2012 +0100
> @@ -129,7 +129,7 @@ static struct page_info * __init alloc_c
>      struct domain *d, unsigned long max_pages)
>  {
>      static unsigned int __initdata last_order = MAX_ORDER;
> -    static unsigned int __initdata memflags = MEMF_no_dma;
> +    static unsigned int __initdata __attribute__((used)) memflags =
MEMF_no_dma;
Without a code comment, the mere fact that this is (a) totally
non-obvious, (b) being done differently for two neighboring
variables of otherwise the exact same kind, and (c) probably
a compiler bug makes it quite likely that your change will get
removed again by a future commit.

Furthermore, it being needed on one but not the other variable
makes it highly likely that the same issue could surface at any
time here or elsewhere in the code.

Jan
>      struct page_info *page;
>      unsigned int order = get_order_from_pages(max_pages), free_order;
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

Tim Deegan

2012-Apr-05 14:05 UTC

head link

Re: [PATCH 2 of 6] x86: don''t use .subsection to out-of-line failure path in spinlock.h

At 14:28 +0100 on 05 Apr (1333636106), Jan Beulich
wrote:> >>> On 05.04.12 at 14:07, Tim Deegan <tim@xen.org> wrote:
> > # HG changeset patch
> > # User Tim Deegan <tim@xen.org>
> > # Date 1333626954 -3600
> > # Node ID 0ecf439475e12f185553f42f56f099be5f328cce
> > # Parent  8518fb0c8c996dca67efd39d31962a6d3502c2ed
> > x86: don''t use .subsection to out-of-line failure path in
spinlock.h
> > 
> > LLVM''s assembler doesn''t support the .subsection
directive.  Instead,
> > leave the failure path inline with an unconditional jump past it in
> > the success path (so the conditional jump is still forwards).
> 
> Please don''t - the failure path should really be out-of-line here;
that''s
> why the .subsection directive got added in the first place. And there
> are more uses of .subsection elsewhere, which I hope you''re not
> intending to all undo just because of a shortcoming of a non-standard
> assembler. 
The other uses are all in .S files, so don''t go through
clang''s
assembler.
> Abstracting this in some way might be doable, e.g.
> moving the code into .text.1 or some such when non-gas is used.
OK.  How about something like this?

diff -r ea2bf1456a52 xen/include/asm-x86/spinlock.h
--- a/xen/include/asm-x86/spinlock.h	Thu Apr 05 14:51:31 2012 +0100
+++ b/xen/include/asm-x86/spinlock.h	Thu Apr 05 15:03:31 2012 +0100
@@ -45,11 +45,11 @@ static always_inline int _raw_read_trylo
     asm volatile (
         "    lock; decl %0         \n"
         "    jns 2f                \n"
-        "1:  .subsection 1         \n"
+        "1:  .pushsection .fixup   \n"
         "2:  lock; incl %0         \n"
         "    decl %1               \n"
         "    jmp 1b                \n"
-        "    .subsection 0         \n"
+        "    .popsection           \n"
         : "=m" (rw->lock), "=r" (acquired) :
"1" (1) : "memory" );
 
     return acquired;

Tim Deegan

2012-Apr-05 14:08 UTC

head link

Re: [PATCH 3 of 6] x86/mm: Another couple of comparisons of unsigned vars with < 0

At 14:31 +0100 on 05 Apr (1333636297), Jan Beulich
wrote:> >>> On 05.04.12 at 14:07, Tim Deegan <tim@xen.org> wrote:
> > # HG changeset patch
> > # User Tim Deegan <tim@xen.org>
> > # Date 1333626954 -3600
> > # Node ID 5eeaa7b25a60327c48bf17472e9a00bdfc67378f
> > # Parent  0ecf439475e12f185553f42f56f099be5f328cce
> > x86/mm: Another couple of comparisons of unsigned vars with < 0.
> > 
> > Adding the explicit (unsigned) casts in case enums ever end up signed.
> 
> Ugly, but unavoidable if range comparisons are done on enums (which
> is what ought to get fixed instead imo).
Any suggestions for how?  Turn the array access into a case statement?
That''s even uglier IMO. 

Tim.

Jan Beulich

2012-Apr-05 14:11 UTC

head link

Re: [PATCH 2 of 6] x86: don''t use .subsection to out-of-line failure path in spinlock.h

>>> On 05.04.12 at 16:05, Tim Deegan <tim@xen.org> wrote:
> At 14:28 +0100 on 05 Apr (1333636106), Jan Beulich wrote:
>> >>> On 05.04.12 at 14:07, Tim Deegan <tim@xen.org>
wrote:
>> > # HG changeset patch
>> > # User Tim Deegan <tim@xen.org>
>> > # Date 1333626954 -3600
>> > # Node ID 0ecf439475e12f185553f42f56f099be5f328cce
>> > # Parent  8518fb0c8c996dca67efd39d31962a6d3502c2ed
>> > x86: don''t use .subsection to out-of-line failure path in
spinlock.h
>> > 
>> > LLVM''s assembler doesn''t support the .subsection
directive.  Instead,
>> > leave the failure path inline with an unconditional jump past it
in
>> > the success path (so the conditional jump is still forwards).
>> 
>> Please don''t - the failure path should really be out-of-line
here; that''s
>> why the .subsection directive got added in the first place. And there
>> are more uses of .subsection elsewhere, which I hope you''re
not
>> intending to all undo just because of a shortcoming of a non-standard
>> assembler. 
> 
> The other uses are all in .S files, so don''t go through
clang''s
> assembler.
Oh, so the compiler looks at the assembly itself? Or uses other than
gas for assembling (yet gas gets used for dealing with assembly files)?
>> Abstracting this in some way might be doable, e.g.
>> moving the code into .text.1 or some such when non-gas is used.
> 
> OK.  How about something like this?
This looks reasonable - if you package it so that with gcc it will be
.subsection and only with clang it would become .pushsection/
.popsection. (Also, to be generic, you may need to specify the
attributes of the .fixup section.)

Jan
> --- a/xen/include/asm-x86/spinlock.h	Thu Apr 05 14:51:31 2012 +0100
> +++ b/xen/include/asm-x86/spinlock.h	Thu Apr 05 15:03:31 2012 +0100
> @@ -45,11 +45,11 @@ static always_inline int _raw_read_trylo
>      asm volatile (
>          "    lock; decl %0         \n"
>          "    jns 2f                \n"
> -        "1:  .subsection 1         \n"
> +        "1:  .pushsection .fixup   \n"
>          "2:  lock; incl %0         \n"
>          "    decl %1               \n"
>          "    jmp 1b                \n"
> -        "    .subsection 0         \n"
> +        "    .popsection           \n"
>          : "=m" (rw->lock), "=r" (acquired) :
"1" (1) : "memory" );
>  
>      return acquired;

Tim Deegan

2012-Apr-05 14:11 UTC

head link

Re: [PATCH 6 of 6] x86: explicitly mark an __initdata variable as used

At 14:44 +0100 on 05 Apr (1333637053), Jan Beulich
wrote:> >>> On 05.04.12 at 14:07, Tim Deegan <tim@xen.org> wrote:
> > x86: explicitly mark an __initdata variable as used.
> > 
> > This stops LLVM from replacing it with a different, auto-generated
> > variable as part of an optimization.  (The auto-generated variable
> > ends up in the normal data section, failing the check that this
> > file only contains __initdata vars).
> > 
> > Signed-off-by: Tim Deegan <tim@xen.org>
> > 
> > diff -r 5101e5ed2473 -r 5a2f5ab5128e xen/arch/x86/domain_build.c
> > --- a/xen/arch/x86/domain_build.c	Thu Apr 05 12:55:54 2012 +0100
> > +++ b/xen/arch/x86/domain_build.c	Thu Apr 05 12:55:54 2012 +0100
> > @@ -129,7 +129,7 @@ static struct page_info * __init alloc_c
> >      struct domain *d, unsigned long max_pages)
> >  {
> >      static unsigned int __initdata last_order = MAX_ORDER;
> > -    static unsigned int __initdata memflags = MEMF_no_dma;
> > +    static unsigned int __initdata __attribute__((used)) memflags =
MEMF_no_dma;
> 
> Without a code comment, the mere fact that this is (a) totally
> non-obvious, (b) being done differently for two neighboring
> variables of otherwise the exact same kind, and (c) probably
> a compiler bug makes it quite likely that your change will get
> removed again by a future commit.
Sorry, yes this deserves a code comment.  I''ll add one. 

I agree that this is a compiler bug, but the LLVM developers disagree,
and I have limited time for arguing with compiler devs.
> Furthermore, it being needed on one but not the other variable
> makes it highly likely that the same issue could surface at any
> time here or elsewhere in the code.
I considered putting the __attribute__((used)) into the definition of
__initdata but thought it was more invasive.  I''m happy to reconsider. 

Tim.

Jan Beulich

2012-Apr-05 14:19 UTC

head link

Re: [PATCH 3 of 6] x86/mm: Another couple of comparisons of unsigned vars with < 0

>>> On 05.04.12 at 16:08, Tim Deegan <tim@xen.org> wrote:
> At 14:31 +0100 on 05 Apr (1333636297), Jan Beulich wrote:
>> >>> On 05.04.12 at 14:07, Tim Deegan <tim@xen.org>
wrote:
>> > # HG changeset patch
>> > # User Tim Deegan <tim@xen.org>
>> > # Date 1333626954 -3600
>> > # Node ID 5eeaa7b25a60327c48bf17472e9a00bdfc67378f
>> > # Parent  0ecf439475e12f185553f42f56f099be5f328cce
>> > x86/mm: Another couple of comparisons of unsigned vars with <
0.
>> > 
>> > Adding the explicit (unsigned) casts in case enums ever end up
signed.
>> 
>> Ugly, but unavoidable if range comparisons are done on enums (which
>> is what ought to get fixed instead imo).
> 
> Any suggestions for how?  Turn the array access into a case statement?
> That''s even uglier IMO. 
	switch (<enum-var>) {
	case <low> ... <high>:
		break;
	default:
		<bad>;
	}

doesn''t look that bad. But when wanting to do range comparisons,
an enum may not be the best choice anyway (for the very reason of
it possible being signed, and signed array indexes generally being
less efficient than unsigned ones), and hence using #define-s and
an "unsigned int" variable might be the better way.

Otoh, doesn''t clang have a command line option to control whether
enums are signed or unsigned? I think gcc does.

Jan

Jan Beulich

2012-Apr-05 14:24 UTC

head link

Re: [PATCH 6 of 6] x86: explicitly mark an __initdata variable as used

>>> On 05.04.12 at 16:11, Tim Deegan <tim@xen.org> wrote:
> At 14:44 +0100 on 05 Apr (1333637053), Jan Beulich wrote:
>> >>> On 05.04.12 at 14:07, Tim Deegan <tim@xen.org>
wrote:
>> > x86: explicitly mark an __initdata variable as used.
>> > 
>> > This stops LLVM from replacing it with a different, auto-generated
>> > variable as part of an optimization.  (The auto-generated variable
>> > ends up in the normal data section, failing the check that this
>> > file only contains __initdata vars).
>> > 
>> > Signed-off-by: Tim Deegan <tim@xen.org>
>> > 
>> > diff -r 5101e5ed2473 -r 5a2f5ab5128e xen/arch/x86/domain_build.c
>> > --- a/xen/arch/x86/domain_build.c	Thu Apr 05 12:55:54 2012 +0100
>> > +++ b/xen/arch/x86/domain_build.c	Thu Apr 05 12:55:54 2012 +0100
>> > @@ -129,7 +129,7 @@ static struct page_info * __init alloc_c
>> >      struct domain *d, unsigned long max_pages)
>> >  {
>> >      static unsigned int __initdata last_order = MAX_ORDER;
>> > -    static unsigned int __initdata memflags = MEMF_no_dma;
>> > +    static unsigned int __initdata __attribute__((used)) memflags
=
> MEMF_no_dma;
>> 
>> Without a code comment, the mere fact that this is (a) totally
>> non-obvious, (b) being done differently for two neighboring
>> variables of otherwise the exact same kind, and (c) probably
>> a compiler bug makes it quite likely that your change will get
>> removed again by a future commit.
> 
> Sorry, yes this deserves a code comment.  I''ll add one. 
> 
> I agree that this is a compiler bug, but the LLVM developers disagree,
> and I have limited time for arguing with compiler devs.
Which I can well understand.
>> Furthermore, it being needed on one but not the other variable
>> makes it highly likely that the same issue could surface at any
>> time here or elsewhere in the code.
> 
> I considered putting the __attribute__((used)) into the definition of
> __initdata but thought it was more invasive.  I''m happy to
reconsider.
That''s a reasonable idea imo, and you may want to do this for
other section placement directives (__read_mostly) too. And then
ideally only for clang (at least for the time being). Maybe we should
even have a __section()-like thing as Linux has (just that we''d likely
want one for text and one for data, so that the attribute wouldn''t
needlessly get applied to functions).

Jan

Tim Deegan

2012-Apr-05 15:39 UTC

head link

Re: [PATCH 3 of 6] x86/mm: Another couple of comparisons of unsigned vars with < 0

At 15:19 +0100 on 05 Apr (1333639188), Jan Beulich
wrote:> >>> On 05.04.12 at 16:08, Tim Deegan <tim@xen.org> wrote:
> > At 14:31 +0100 on 05 Apr (1333636297), Jan Beulich wrote:
> >> >>> On 05.04.12 at 14:07, Tim Deegan <tim@xen.org>
wrote:
> >> > x86/mm: Another couple of comparisons of unsigned vars with
< 0.
> >> > 
> >> > Adding the explicit (unsigned) casts in case enums ever end
up signed.
> >> 
> >> Ugly, but unavoidable if range comparisons are done on enums
(which
> >> is what ought to get fixed instead imo).
> > 
> > Any suggestions for how?  Turn the array access into a case statement?
> > That''s even uglier IMO. 
> 
> 	switch (<enum-var>) {
> 	case <low> ... <high>:
> 		break;
> 	default:
> 		<bad>;
> 	}
Hmm.  I really prefer the cast to that.

In both these cases, I could replace this idiom:

 static const array a  = { val_x, val_y, val_z };
 if ( (unsigned) e >= enum_z ) return -EINVAL;
 val = a[e];

with 

 switch (e) {
     case enum_x: val = val_x; break;
     case enum_x: val = val_x; break;
     case enum_x: val = val_x; break;
     default: return -EINVAL;
 }

but unless it''s laid out like this (one line per case) it''ll
get ugly too.

Switching to #defines is a bit tricky as they''re in a public header and
I''m not confident we wouldn''t break someone else''s
compile.
> doesn''t look that bad. But when wanting to do range comparisons,
> an enum may not be the best choice anyway (for the very reason of
> it possible being signed, and signed array indexes generally being
> less efficient than unsigned ones), and hence using #define-s and
> an "unsigned int" variable might be the better way.
> 
> Otoh, doesn''t clang have a command line option to control whether
> enums are signed or unsigned? I think gcc does.
The only gcc flag I can find is -fshort-enums, for making enums into
shorter types.

Cheers,

Tim.

Keir Fraser

2012-Apr-05 15:55 UTC

head link

Re: [PATCH 0 of 6] Make xen build with clang/LLVM again

On 05/04/2012 13:07, "Tim Deegan" <tim@xen.org> wrote:
> This series makes the hypervisor build with clang/LLVM again,
> after a certain amount of bit-rot.
> 
> Some of these are obvious bug-fixes which are picked up by clang''s
> slightly different -W* implementation.  Some (the spinlock change
> in particular) are working around deficiencies in clang/llvm.  I''m
> happy to change those ones to avoid any concerns people have about
> damaging the GCC build.
I don''t have any comments on these beyond Jan''s comments on
patch 2/6. I
don''t care what method you use to keep the out-of-line code ool on
standard
toolchain.

 -- keir
> Cheers,
> 
> Tim.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Xen devel - Apr 2012 - [PATCH 0 of 6] Make xen build with clang/LLVM again

[PATCH 0 of 6] Make xen build with clang/LLVM again

[PATCH 1 of 6] xen: Add -Wno-unused-value to the clang CFLAGS

[PATCH 2 of 6] x86: don''t use .subsection to out-of-line failure path in spinlock.h

[PATCH 3 of 6] x86/mm: Another couple of comparisons of unsigned vars with < 0

[PATCH 4 of 6] x86: fix logical ANDs used to mask bitfields

[PATCH 5 of 6] x86: fix memset(ptr, 0, sizeof ptr)

[PATCH 6 of 6] x86: explicitly mark an __initdata variable as used

Re: [PATCH 2 of 6] x86: don''t use .subsection to out-of-line failure path in spinlock.h

Re: [PATCH 3 of 6] x86/mm: Another couple of comparisons of unsigned vars with < 0

Re: [PATCH 4 of 6] x86: fix logical ANDs used to mask bitfields

Re: [PATCH 5 of 6] x86: fix memset(ptr, 0, sizeof ptr)

Re: [PATCH 6 of 6] x86: explicitly mark an __initdata variable as used

Re: [PATCH 2 of 6] x86: don''t use .subsection to out-of-line failure path in spinlock.h

Re: [PATCH 3 of 6] x86/mm: Another couple of comparisons of unsigned vars with < 0

Re: [PATCH 2 of 6] x86: don''t use .subsection to out-of-line failure path in spinlock.h

Re: [PATCH 6 of 6] x86: explicitly mark an __initdata variable as used

Re: [PATCH 3 of 6] x86/mm: Another couple of comparisons of unsigned vars with < 0

Re: [PATCH 6 of 6] x86: explicitly mark an __initdata variable as used

Re: [PATCH 3 of 6] x86/mm: Another couple of comparisons of unsigned vars with < 0

Re: [PATCH 0 of 6] Make xen build with clang/LLVM again