[PATCH kvm-unit-tests] build: enable -Werror

[PATCH kvm-unit-tests] build: enable -Werror

[Peter Feiner]

Tested with arch=i386, x86_64, ppc64, and arm64.

Signed-off-by: Peter Feiner <pfeiner@google.com>
---
 Makefile        | 2 +-
 x86/hypercall.c | 2 +-
 x86/vmx_tests.c | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 72e6711..09999c6 100644
--- a/Makefile
+++ b/Makefile
@@ -41,7 +41,7 @@ cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \
               > /dev/null 2>&1; then echo "$(1)"; else echo "$(2)"; fi ;)
 
 CFLAGS += -g
-CFLAGS += $(autodepend-flags) -Wall
+CFLAGS += $(autodepend-flags) -Wall -Werror
 CFLAGS += $(call cc-option, -fomit-frame-pointer, "")
 CFLAGS += $(call cc-option, -fno-stack-protector, "")
 CFLAGS += $(call cc-option, -fno-stack-protector-all, "")
diff --git a/x86/hypercall.c b/x86/hypercall.c
index 3ac5ff9..75179a1 100644
--- a/x86/hypercall.c
+++ b/x86/hypercall.c
@@ -34,7 +34,6 @@ asm ("gp_tss: \n\t"
 	"iretq\n\t"
 	"jmp gp_tss\n\t"
     );
-#endif
 
 static inline int
 test_edge(void)
@@ -47,6 +46,7 @@ test_edge(void)
 	printf("Return from int 13, test_rip = %lx\n", test_rip);
 	return test_rip == (1ul << 47);
 }
+#endif
 
 int main(int ac, char **av)
 {
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index d851692..b7dfcbb 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -1174,12 +1174,10 @@ static int vpid_exit_handler()
 	u64 guest_rip;
 	ulong reason;
 	u32 insn_len;
-	u32 exit_qual;
 
 	guest_rip = vmcs_read(GUEST_RIP);
 	reason = vmcs_read(EXI_REASON) & 0xff;
 	insn_len = vmcs_read(EXI_INST_LEN);
-	exit_qual = vmcs_read(EXI_QUALIFICATION);
 
 	switch (reason) {
 	case VMX_VMCALL:
@@ -1392,6 +1390,7 @@ static void dbgctls_main(void)
 	asm volatile("mov %%dr7,%0" : "=r" (dr7));
 	debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
 	/* Commented out: KVM does not support DEBUGCTL so far */
+	assert(debugctl == debugctl);
 	report("Load debug controls", dr7 == 0x404 /* && debugctl == 0x2 */);
 
 	dr7 = 0x408;
@@ -1413,6 +1412,7 @@ static void dbgctls_main(void)
 	asm volatile("mov %%dr7,%0" : "=r" (dr7));
 	debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
 	/* Commented out: KVM does not support DEBUGCTL so far */
+	assert(debugctl == debugctl);
 	report("Guest=host debug controls", dr7 == 0x402 /* && debugctl == 0x1 */);
 
 	dr7 = 0x408;
-- 
2.7.0.rc3.207.g0ac5344

Re: [PATCH kvm-unit-tests] build: enable -Werror

[Thomas Huth]

On 03.03.2016 22:55, Peter Feiner wrote:
> Tested with arch=i386, x86_64, ppc64, and arm64.
> 
> Signed-off-by: Peter Feiner <pfeiner@google.com>
> ---
>  Makefile        | 2 +-
>  x86/hypercall.c | 2 +-
>  x86/vmx_tests.c | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
...
> @@ -1392,6 +1390,7 @@ static void dbgctls_main(void)
>  	asm volatile("mov %%dr7,%0" : "=r" (dr7));
>  	debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
>  	/* Commented out: KVM does not support DEBUGCTL so far */
> +	assert(debugctl == debugctl);
>  	report("Load debug controls", dr7 == 0x404 /* && debugctl == 0x2 */);
>  
>  	dr7 = 0x408;
> @@ -1413,6 +1412,7 @@ static void dbgctls_main(void)
>  	asm volatile("mov %%dr7,%0" : "=r" (dr7));
>  	debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
>  	/* Commented out: KVM does not support DEBUGCTL so far */
> +	assert(debugctl == debugctl);
>  	report("Guest=host debug controls", dr7 == 0x402 /* && debugctl == 0x1 */);

Now these assert()s are really ugly. Wouldn't it be better do comment
out the "debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);" line instead?

 Thomas

Re: [PATCH kvm-unit-tests] build: enable -Werror

[Paolo Bonzini]

On 04/03/2016 09:47, Thomas Huth wrote:
>> >  	asm volatile("mov %%dr7,%0" : "=r" (dr7));
>> >  	debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
>> >  	/* Commented out: KVM does not support DEBUGCTL so far */
>> > +	assert(debugctl == debugctl);
>> >  	report("Guest=host debug controls", dr7 == 0x402 /* && debugctl == 0x1 */);
> Now these assert()s are really ugly. Wouldn't it be better do comment
> out the "debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);" line instead?

This also looks better than the asserts:

	(void)debugctl;

Thomas, if you're okay with it I can do the change locally.

Paolo

Re: [PATCH kvm-unit-tests] build: enable -Werror

[Thomas Huth]

On 04.03.2016 09:49, Paolo Bonzini wrote:
> 
> 
> On 04/03/2016 09:47, Thomas Huth wrote:
>>>>  	asm volatile("mov %%dr7,%0" : "=r" (dr7));
>>>>  	debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
>>>>  	/* Commented out: KVM does not support DEBUGCTL so far */
>>>> +	assert(debugctl == debugctl);
>>>>  	report("Guest=host debug controls", dr7 == 0x402 /* && debugctl == 0x1 */);
>> Now these assert()s are really ugly. Wouldn't it be better do comment
>> out the "debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);" line instead?
> 
> This also looks better than the asserts:
> 
> 	(void)debugctl;
> 
> Thomas, if you're okay with it I can do the change locally.

I personally would rather prefer to put comments around "debugctl =",
but your suggestion looks at least better to me than the solution with
assert() ... (since assert() can also be #undef in normal C projects, so
IMHO it should not be used for something that has influence on the
behavior of the normal C code).

 Thomas

Re: [PATCH kvm-unit-tests] build: enable -Werror

[Peter Feiner]

On Fri, Mar 4, 2016 at 1:00 AM, Thomas Huth <thuth@redhat.com> wrote:
> On 04.03.2016 09:49, Paolo Bonzini wrote:
>>
>>
>> On 04/03/2016 09:47, Thomas Huth wrote:
>>>>>    asm volatile("mov %%dr7,%0" : "=r" (dr7));
>>>>>    debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
>>>>>    /* Commented out: KVM does not support DEBUGCTL so far */
>>>>> +  assert(debugctl == debugctl);
>>>>>    report("Guest=host debug controls", dr7 == 0x402 /* && debugctl == 0x1 */);
>>> Now these assert()s are really ugly. Wouldn't it be better do comment
>>> out the "debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);" line instead?
>>
>> This also looks better than the asserts:
>>
>>       (void)debugctl;
>>
>> Thomas, if you're okay with it I can do the change locally.
>
> I personally would rather prefer to put comments around "debugctl =",
> but your suggestion looks at least better to me than the solution with
> assert() ... (since assert() can also be #undef in normal C projects, so
> IMHO it should not be used for something that has influence on the
> behavior of the normal C code).

I wasn't too happy with the asserts either. I didn't just comment it
all out because I wanted to leave in the rdmsr in case there were side
effects (which there shouldn't be, but that's the point of testing!).
I'm fine with the (void) as well. Paolo, I'll assume you're going to
massage the patch unless you say otherwise.

Thanks,
Peter