Make xen build with clang/LLVM again

PATCH Superseded Tim Deegan 3 participants 20 messages Apr 5, 2012

lore

Share & tools

lore

Tim Deegan

author

2012-04-05 12:07 UTC #0 lore raw

PATCH 1/1 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

author

2012-04-05 12:07 UTC #1 lore raw

PATCH 1/1 xen: Add -Wno-unused-value to the clang CFLAGS

# HG changeset patch
# User Tim Deegan <[email protected]>
# 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 <[email protected]>

Config.mk +2 -1

diff -r d690c7e896a2 -r 8518fb0c8c99 Config.mk
@@ -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

author

2012-04-05 12:07 UTC #2 lore raw

PATCH 1/1 x86: don't use .subsection to out-of-line failure path in spinlock.h

In reply to Tim Deegan

# HG changeset patch
# User Tim Deegan <[email protected]>
# 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 <[email protected]>

xen/include/asm-x86/spinlock.h +4 -5

diff -r 8518fb0c8c99 -r 0ecf439475e1 xen/include/asm-x86/spinlock.h
@@ -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;

Jan Beulich

2012-04-05 13:28 UTC #3 lore raw

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

Quoted 11 lines

Quoted 1 line
Quoted 1 line · Tim Deegan
# HG changeset patch
# User Tim Deegan <[email protected]>
# 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

Quoted 27 lines

Signed-off-by Tim Deegan <[email protected]>

diff -r 8518fb0c8c99 -r 0ecf439475e1 xen/include/asm-x86/spinlock.h

xen/include/asm-x86/spinlock.h +4 -5

@@ -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
[email protected]
http://lists.xen.org/xen-devel

Tim Deegan

author

2012-04-05 14:05 UTC #4 lore raw

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

Quoted 12 lines · Jan Beulich

Quoted 11 lines
Quoted 1 line
Quoted 1 line · Tim Deegan
# HG changeset patch
# User Tim Deegan <[email protected]>
# 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).

Quoted 5 lines

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.

Quoted 2 lines

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?

xen/include/asm-x86/spinlock.h +2 -2

diff -r ea2bf1456a52 xen/include/asm-x86/spinlock.h
@@ -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;

Jan Beulich

2012-04-05 14:11 UTC #5 lore raw

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

Quoted 14 lines

Quoted 1 line
Quoted 1 line · Tim Deegan
Quoted 12 lines · Jan Beulich
Quoted 11 lines
Quoted 1 line
Quoted 1 line · Tim Deegan
# HG changeset patch
# User Tim Deegan <[email protected]>
# 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).

Quoted 8 lines

Quoted 5 lines
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)?

Quoted 4 lines

Quoted 2 lines
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

Quoted 16 lines

xen/include/asm-x86/spinlock.h +2 -2

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

author

2012-04-05 12:07 UTC #6 lore raw

PATCH 1/1 x86/mm: Another couple of comparisons of unsigned vars with < 0

In reply to Tim Deegan

# HG changeset patch
# User Tim Deegan <[email protected]>
# 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 <[email protected]>

xen/arch/x86/mm/p2m.c +2 -2

diff -r 0ecf439475e1 -r 5eeaa7b25a60 xen/arch/x86/mm/p2m.c
@@ -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];

Jan Beulich

2012-04-05 13:31 UTC #7 lore raw

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

Quoted 9 lines

Quoted 1 line
Quoted 1 line · Tim Deegan
# HG changeset patch
# User Tim Deegan <[email protected]>
# 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

Quoted 28 lines

Signed-off-by Tim Deegan <[email protected]>

diff -r 0ecf439475e1 -r 5eeaa7b25a60 xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c +2 -2

@@ -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
[email protected]
http://lists.xen.org/xen-devel

Tim Deegan

author

2012-04-05 14:08 UTC #8 lore raw

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

Quoted 8 lines · Jan Beulich

Quoted 8 lines
Quoted 1 line
Quoted 1 line · Tim Deegan
# HG changeset patch
# User Tim Deegan <[email protected]>
# Date 1333626954 -3600
# Node ID 5eeaa7b25a60327c48bf17472e9a00bdfc67378f
# Parent 0ecf439475e12f185553f42f56f099be5f328cce x86/mm: Another couple of comparisons of unsigned vars with < 0.

Quoted 4 lines

Quoted 1 line
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-04-05 14:19 UTC #9 lore raw

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

Quoted 10 lines

Quoted 1 line
Quoted 1 line · Tim Deegan
Quoted 8 lines · Jan Beulich
Quoted 8 lines
Quoted 1 line
Quoted 1 line · Tim Deegan
# HG changeset patch
# User Tim Deegan <[email protected]>
# Date 1333626954 -3600
# Node ID 5eeaa7b25a60327c48bf17472e9a00bdfc67378f
# Parent 0ecf439475e12f185553f42f56f099be5f328cce x86/mm: Another couple of comparisons of unsigned vars with < 0.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
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

Tim Deegan

author

2012-04-05 15:39 UTC #10 lore raw

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

Quoted 13 lines · Jan Beulich

Quoted 5 lines
Quoted 1 line
Quoted 1 line · Tim Deegan
Quoted 3 lines · Jan Beulich
Quoted 3 lines
Quoted 1 line
Quoted 1 line · Tim Deegan
x86/mm: Another couple of comparisons of unsigned vars with < 0.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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.

Quoted 6 lines

	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.

Quoted 8 lines

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.

Tim Deegan

author

2012-04-05 12:07 UTC #11 lore raw

PATCH 1/1 x86: fix logical ANDs used to mask bitfields

In reply to Tim Deegan

# HG changeset patch
# User Tim Deegan <[email protected]>
# Date 1333626954 -3600
# Node ID dfe9453c066f3fbfa09c847d7ec381cdc0da0f99 # Parent 5eeaa7b25a60327c48bf17472e9a00bdfc67378f
x86: fix logical ANDs used to mask bitfields.

Signed-off-by Tim Deegan <[email protected]>

xen/arch/x86/hvm/svm/svm.c +1 -1

diff -r 5eeaa7b25a60 -r dfe9453c066f xen/arch/x86/hvm/svm/svm.c
@@ -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);
 }

xen/arch/x86/hvm/vmx/vmx.c +2 -2

diff -r 5eeaa7b25a60 -r dfe9453c066f xen/arch/x86/hvm/vmx/vmx.c
@@ -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);

Jan Beulich

Acked-by on v1 1/1

2012-04-05 13:33 UTC #12 lore raw

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

Quoted 9 lines

Quoted 1 line
Quoted 1 line · Tim Deegan
# HG changeset patch
# User Tim Deegan <[email protected]>
# Date 1333626954 -3600
# Node ID dfe9453c066f3fbfa09c847d7ec381cdc0da0f99 # Parent 5eeaa7b25a60327c48bf17472e9a00bdfc67378f
x86: fix logical ANDs used to mask bitfields.
Signed-off-by Tim Deegan <[email protected]>

Acked-by Jan Beulich <[email protected]>

Quoted 38 lines

diff -r 5eeaa7b25a60 -r dfe9453c066f xen/arch/x86/hvm/svm/svm.c

xen/arch/x86/hvm/svm/svm.c +1 -1

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

xen/arch/x86/hvm/vmx/vmx.c +2 -2

@@ -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
[email protected]
http://lists.xen.org/xen-devel

Tim Deegan

author

2012-04-05 12:07 UTC #13 lore raw

PATCH 1/1 x86: fix memset(ptr, 0, sizeof ptr)

In reply to Tim Deegan

# HG changeset patch
# User Tim Deegan <[email protected]>
# Date 1333626954 -3600
# Node ID 5101e5ed24732919076d5285e55c7b53032749c2 # Parent dfe9453c066f3fbfa09c847d7ec381cdc0da0f99
x86: fix memset(ptr, 0, sizeof ptr).

Signed-off-by Tim Deegan <[email protected]>

xen/arch/x86/cpu/mcheck/amd_f10.c +2 -2

diff -r dfe9453c066f -r 5101e5ed2473 xen/arch/x86/cpu/mcheck/amd_f10.c
@@ -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;

xen/arch/x86/mm/p2m.c +1 -1

diff -r dfe9453c066f -r 5101e5ed2473 xen/arch/x86/mm/p2m.c
@@ -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 */

Jan Beulich

Acked-by on v1 1/1

2012-04-05 13:38 UTC #14 lore raw

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

Quoted 29 lines

Quoted 1 line
Quoted 1 line · Tim Deegan
# HG changeset patch
# User Tim Deegan <[email protected]>
# Date 1333626954 -3600
# Node ID 5101e5ed24732919076d5285e55c7b53032749c2 # Parent dfe9453c066f3fbfa09c847d7ec381cdc0da0f99
x86: fix memset(ptr, 0, sizeof ptr).
Signed-off-by Tim Deegan <[email protected]>
diff -r dfe9453c066f -r 5101e5ed2473 xen/arch/x86/cpu/mcheck/amd_f10.c
xen/arch/x86/cpu/mcheck/amd_f10.c +2 -2
@@ -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

xen/arch/x86/mm/p2m.c bool_t p2m_mem_access_check(unsigned lon · line 1239

Quoted 5 lines

xen/arch/x86/mm/p2m.c +1 -1

@@ -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 <[email protected]>

Quoted 8 lines

xen/arch/x86/mm/p2m.c +0 -0
         req->reason = MEM_EVENT_REASON_VIOLATION;
 
         /* Pause the current VCPU */
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Tim Deegan

author

2012-04-05 12:07 UTC #15 lore raw

PATCH 1/1 x86: explicitly mark an __initdata variable as used

In reply to Tim Deegan

# HG changeset patch
# User Tim Deegan <[email protected]>
# 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 <[email protected]>

xen/arch/x86/domain_build.c +1 -1

diff -r 5101e5ed2473 -r 5a2f5ab5128e xen/arch/x86/domain_build.c
@@ -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-04-05 13:44 UTC #16 lore raw

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

Quoted 19 lines

Quoted 1 line
Quoted 1 line · Tim Deegan
# HG changeset patch
# User Tim Deegan <[email protected]>
# 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 <[email protected]>
diff -r 5101e5ed2473 -r 5a2f5ab5128e xen/arch/x86/domain_build.c

xen/arch/x86/domain_build.c static struct page_info * __init alloc_c · line 132

Quoted 5 lines

xen/arch/x86/domain_build.c +1 -1

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

Quoted 8 lines

xen/arch/x86/domain_build.c +0 -0
     struct page_info *page;
     unsigned int order = get_order_from_pages(max_pages), free_order;
 
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Tim Deegan

author

2012-04-05 14:11 UTC #17 lore raw

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

Quoted 10 lines · Jan Beulich

Quoted 10 lines
Quoted 1 line
Quoted 1 line · Tim Deegan
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 <[email protected]>

Quoted 15 lines

xen/arch/x86/domain_build.c static struct page_info * __init alloc_c · line 132
Quoted 9 lines
diff -r 5101e5ed2473 -r 5a2f5ab5128e xen/arch/x86/domain_build.c
xen/arch/x86/domain_build.c +1 -1
@@ -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.

Quoted 3 lines

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-04-05 14:24 UTC #18 lore raw

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

Quoted 29 lines

Quoted 1 line
Quoted 1 line · Tim Deegan
Quoted 10 lines · Jan Beulich
Quoted 10 lines
Quoted 1 line
Quoted 1 line · Tim Deegan
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 <[email protected]>
Quoted 9 lines
xen/arch/x86/domain_build.c static struct page_info * __init alloc_c · line 132
Quoted 9 lines
diff -r 5101e5ed2473 -r 5a2f5ab5128e xen/arch/x86/domain_build.c
xen/arch/x86/domain_build.c +1 -1
@@ -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;
Quoted 6 lines
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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 3 lines
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

Keir Fraser

2012-04-05 15:55 UTC #19 lore raw

In reply to Tim Deegan

Quoted 8 lines · Tim Deegan

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

Quoted 8 lines

Cheers,
Tim.
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel