[PATCH] kvm: testsuite: silence warnings on x86_64

[PATCH] kvm: testsuite: silence warnings on x86_64

[Bernhard Kaindl]

Hi,
   please find the following trivial warning fix patch for

		kvm-60/user/test/x86/*

   below. Feel free to merge to upstream kvm. It's diffed against kvm-60.

The patch does not change things, just shuts up the warnings.

The deal here is that we check for warnings and are alerted of warnings which
include language like:

	"is used uninitialized in this function"
---------^^ (not may)

which indicate a true programming error and are prominently recognized.

Fixing the same warning message made me fix a terrible bug in libksba
(a library which deals with X.509 certificates, CMS data, and related data
which is used by
GnuPG) which surfaced with the use of the new gcc-4.3 compiler suite.

gcc-4.3 optimizes better and the probabitlity that uninitialized value
actually starts off with a non-zero value really increases with gcc-4.3.

I did not test this patch as I did not find documentation on how to run the
test cases and I could not find a make target to run them from make.

Bernhard

----------------

This patch fixes the following warnings on x86_64:

test/x86/access.c: In function 'ac_test_exec':
test/x86/access.c:541: warning: implicit declaration of function 'strcat'
test/x86/access.c: In function 'main':
test/x86/access.c:577: warning: passing argument 1 of 'smp_init' from incompatible pointer type

test/x86/emulator.c: In function 'test_cmps':
test/x86/emulator.c:26: warning: unused variable 'i'
test/x86/emulator.c: In function 'test_push':
test/x86/emulator.c:115: warning: 'tmp' is used uninitialized in this function

test/x86/lib/printf.c: In function 'vsnprintf':
test/x86/lib/printf.c:95: warning: unused variable 'n'

Note: 'tmp' is only used in inline assembly, so the initialisation of it may be
done in a more elegant way, but just setting it to 0 should hopefully just
achive the same functional result as no initialisation (most of the time...).

Signed-off-by: Bernhard Kaindl <bk-l3A5Bk7waGM@public.gmane.org>
---
 access.c     |    8 +++++++-
 emulator.c   |    3 +--
 lib/printf.c |    1 -
 3 files changed, 8 insertions(+), 4 deletions(-)

--- kvm-60/user/test/x86/access.c
+++ kvm-60/user/test/x86/access.c	2008/01/24 15:14:16
@@ -1,6 +1,7 @@
 
 #include "smp.h"
 #include "printf.h"
+#include "string.h"
 
 #define true 1
 #define false 0
@@ -569,7 +570,7 @@
     int r;
 
     printf("starting test\n\n");
-    smp_init(ac_test_run);
+    smp_init((void (*)(void))ac_test_run);
     r = ac_test_run();
     return r ? 0 : 1;
 }
--- kvm-60/user/test/x86/emulator.c
+++ kvm-60/user/test/x86/emulator.c	2008/01/24 15:08:16
@@ -23,7 +23,6 @@
 	unsigned char m3[1024];
 	void *rsi, *rdi;
 	long rcx, tmp;
-	int i;
 
 	for (int i = 0; i < 100; ++i)
 		m1[i] = m2[i] = m3[i] = i;
@@ -105,7 +104,7 @@
 
 void test_push(void *mem)
 {
-	unsigned long tmp;
+	unsigned long tmp = 0;
 	unsigned long *stack_top = mem + 4096;
 	unsigned long *new_stack_top;
 	unsigned long memw = 0x123456789abcdeful;
--- kvm-60/user/test/x86/lib/printf.c
+++ kvm-60/user/test/x86/lib/printf.c	2008/01/24 15:10:35
@@ -92,7 +92,6 @@
 
 int vsnprintf(char *buf, int size, const char *fmt, va_list va)
 {
-    int n;
     pstream_t s;
 
     s.buffer = buf;

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] kvm: testsuite: silence warnings on x86_64

[Avi Kivity]

Bernhard Kaindl wrote:
> I did not test this patch as I did not find documentation on how to run the
> test cases and I could not find a make target to run them from make.
>
>   
  make -C user test_cases
  user/kvmctl user/test/x86/bootstrap user/test/x86/access.flat

(we should rename user -> test)


> --- kvm-60/user/test/x86/access.c
> +++ kvm-60/user/test/x86/access.c	2008/01/24 15:14:16
> @@ -1,6 +1,7 @@
>  
>  #include "smp.h"
>  #include "printf.h"
> +#include "string.h"
>  
>  #define true 1
>  #define false 0
> @@ -569,7 +570,7 @@
>      int r;
>  
>      printf("starting test\n\n");
> -    smp_init(ac_test_run);
> +    smp_init((void (*)(void))ac_test_run);
>      r = ac_test_run();
>      return r ? 0 : 1;
>  }
>   

Better to add a wrapper that conforms to the expected signature, and 
makes sure the return value of ac_test_run() is not lost.

Haven't run access.flat on smp for a long while; the results should be 
interesting after the page fault scaling work.

>  = i;
> @@ -105,7 +104,7 @@
>  
>  void test_push(void *mem)
>  {
> -	unsigned long tmp;
> +	unsigned long tmp = 0;
>  	unsigned long *stack_top = mem + 4096;
>  	unsigned long *new_stack_top;
>  	unsigned long memw = 0x123456789abcdeful;
>   

I'm needlessly pedantic, but the correct fix is to pass the constraint 
"=&r"(tmp) in the write section.  This tells gcc the register is 
clobbered ("=") and not to pass any inputs in it ("&").

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] kvm: testsuite: silence warnings on x86_64

[Bernhard Kaindl]

On Thu, 24 Jan 2008, Avi Kivity wrote:
>
>  make -C user test_cases
>  user/kvmctl user/test/x86/bootstrap user/test/x86/access.flat

Ah, thanks!

> > -    smp_init(ac_test_run);
> > +    smp_init((void (*)(void))ac_test_run);
> 
> Better to add a wrapper that conforms to the expected signature, and makes
> sure the return value of ac_test_run() is not lost.
> 
> Haven't run access.flat on smp for a long while; the results should be
> interesting after the page fault scaling work.

On my E6850, running 2.6.24-rc7-ish x86_64 kernel with kvm-59,

user/kvmctl -s2 user/test/x86/bootstrap user/test/x86x/(tried all).flat

just hangs without any output, kvmctl consuming 100% of one core.
I could try with kvm-60 at a later time.

Since that means that I can't test any fix I would be able to do along
your suggestion, I removed that hunk from the patch to keep the warning.

> > -	unsigned long tmp;
> > +	unsigned long tmp = 0;
> 
> I'm needlessly pedantic, but the correct fix is to pass the constraint
> "=&r"(tmp) in the write section.  This tells gcc the register is clobbered
> ("=") and not to pass any inputs in it ("&").

I ran the test case now, and interestingly, the testcase appeared indeed
broken here (kernel modules were still kvm-59), and the change above didn't
change that behaviour:

ser/kvmctl   user/test/x86/bootstrap user/test/x86/emulator.flat 
GUEST: paging enabled
GUEST: PASS: mov reg, r/m (1)
GUEST: PASS: repe/cmpsb (1)
GUEST: PASS: repe/cmpsw (1)
GUEST: PASS: repe/cmpll (1)
GUEST: PASS: repe/cmpsq (1)
GUEST: PASS: repe/cmpsb (2)
GUEST: PASS: repe/cmpsw (2)
GUEST: PASS: repe/cmpll (2)
GUEST: PASS: repe/cmpsq (2)
(command hung here, no further output)

Using "=&r"(tmp) fixed this (gcc only accepted this after moving the tmp operand
to the input operands section, but then, only having "=r"(tmp) - (the '=' was demanded
by gcc in the input section)only fixes it so far as this:

GUEST: paging enabled
GUEST: PASS: mov reg, r/m (1)
GUEST: PASS: repe/cmpsb (1)
GUEST: PASS: repe/cmpsw (1)
GUEST: PASS: repe/cmpll (1)
GUEST: PASS: repe/cmpsq (1)
GUEST: PASS: repe/cmpsb (2)
GUEST: PASS: repe/cmpsw (2)
GUEST: PASS: repe/cmpll (2)
GUEST: PASS: repe/cmpsq (2)
GUEST: PASS: push $imm8
GUEST: PASS: push %reg
GUEST: FAIL: push mem
GUEST: PASS: mov %cr8
GUEST: 
GUEST: SUMMARY: 13 tests, 1 failures

and using "=&r"(tmp) all the test work:

GUEST: paging enabled
GUEST: PASS: mov reg, r/m (1)
GUEST: PASS: repe/cmpsb (1)
GUEST: PASS: repe/cmpsw (1)
GUEST: PASS: repe/cmpll (1)
GUEST: PASS: repe/cmpsq (1)
GUEST: PASS: repe/cmpsb (2)
GUEST: PASS: repe/cmpsw (2)
GUEST: PASS: repe/cmpll (2)
GUEST: PASS: repe/cmpsq (2)
GUEST: PASS: push $imm8
GUEST: PASS: push %reg
GUEST: PASS: push mem
GUEST: PASS: mov %cr8
GUEST: 
GUEST: SUMMARY: 13 tests, 0 failures

Please find the updated patch below,
Bernhard

This patch fixes user/test/x86/emulator.flat on kvm-59 with x86_64/E6850
and three other warnings:

test/x86/access.c: In function 'ac_test_exec':
test/x86/access.c:541: warning: implicit declaration of function 'strcat'

test/x86/emulator.c: In function 'test_cmps':
test/x86/emulator.c:26: warning: unused variable 'i'
test/x86/emulator.c: In function 'test_push':
test/x86/emulator.c:115: warning: 'tmp' is used uninitialized in this function

test/x86/lib/printf.c: In function 'vsnprintf':
test/x86/lib/printf.c:95: warning: unused variable 'n'

Signed-off-by: Bernhard Kaindl <bk-l3A5Bk7waGM@public.gmane.org>

--- kvm-60/user/test/x86/access.c
+++ kvm-60/user/test/x86/access.c
@@ -1,6 +1,7 @@
 
 #include "smp.h"
 #include "printf.h"
+#include "string.h"
 
 #define true 1
 #define false 0
--- kvm-60/user/test/x86/emulator.c
+++ kvm-60/user/test/x86/emulator.c
@@ -23,7 +23,6 @@
 	unsigned char m3[1024];
 	void *rsi, *rdi;
 	long rcx, tmp;
-	int i;
 
 	for (int i = 0; i < 100; ++i)
 		m1[i] = m2[i] = m3[i] = i;
@@ -119,8 +118,8 @@
 		     "pushq (%[mem]) \n\t"
 		     "mov %%rsp, %[new_stack_top] \n\t"
 		     "mov %[tmp], %%rsp"
-		     : [new_stack_top]"=r"(new_stack_top)
-		     : [tmp]"r"(tmp), [stack_top]"r"(stack_top),
+		     : [tmp]"=&r"(tmp), [new_stack_top]"=r"(new_stack_top)
+		     : [stack_top]"r"(stack_top),
 		       [reg]"r"(-17l), [mem]"r"(&memw)
 		     : "memory");
 
--- kvm-60/user/test/x86/lib/printf.c
+++ kvm-60/user/test/x86/lib/printf.c	2008/01/24 15:10:35
@@ -92,7 +92,6 @@
 
 int vsnprintf(char *buf, int size, const char *fmt, va_list va)
 {
-    int n;
     pstream_t s;
 
     s.buffer = buf;

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] kvm: testsuite: silence warnings on x86_64

[Avi Kivity]

Bernhard Kaindl wrote:
> This patch fixes user/test/x86/emulator.flat on kvm-59 with x86_64/E6850
> and three other warnings:
>
>   

Applied, thanks.  Please send separate patches for separate problems in 
the future.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] kvm: testsuite: silence warnings on x86_64

[Carlo Marcelo Arenas Belon]

On Thu, Jan 24, 2008 at 06:21:56PM +0200, Avi Kivity wrote:
> Bernhard Kaindl wrote:
> > I did not test this patch as I did not find documentation on how to run the
> > test cases and I could not find a make target to run them from make.
> >
> >   
>   make -C user test_cases
>   user/kvmctl user/test/x86/bootstrap user/test/x86/access.flat
> 
> (we should rename user -> test)

are you interested on patches for that? or does something else need to be done
first?

IMHO will be also a good idea to add a target in the Makefile so that building
the tests will not be part of "all" and will be executed automatically when
invoked with "make check" as it is is done usually.

presume that at least to begin, running all *.flat tests could be a good
starting point for this target?

> > --- kvm-60/user/test/x86/access.c
> > +++ kvm-60/user/test/x86/access.c	2008/01/24 15:14:16
> > @@ -1,6 +1,7 @@
> >  
> >  #include "smp.h"
> >  #include "printf.h"
> > +#include "string.h"
> >  
> >  #define true 1
> >  #define false 0
> > @@ -569,7 +570,7 @@
> >      int r;
> >  
> >      printf("starting test\n\n");
> > -    smp_init(ac_test_run);
> > +    smp_init((void (*)(void))ac_test_run);
> >      r = ac_test_run();
> >      return r ? 0 : 1;
> >  }
> >   
> 
> Better to add a wrapper that conforms to the expected signature, and 
> makes sure the return value of ac_test_run() is not lost.

this will require redefining smp_init as shown by :

--- a/user/test/x86/lib/smp.h
+++ b/user/test/x86/lib/smp.h
@@ -5,7 +5,7 @@ struct spinlock {
     int v;
 };
 
-void smp_init(void (*smp_main)(void));
+void smp_init(int (*smp_main)(void));
 
 int cpu_count(void);
 int smp_id(void);

and will require also fixing the smp.flat test to build againg and to return
a bool when executed (will send patches for doing both later if that is what
you want)

> Haven't run access.flat on smp for a long while; the results should be 
> interesting after the page fault scaling work.

if you meant `kvmctl -s2` it doesn't seem to get pass the kvm device
initialization inside kvmctl for kvm-60.

Carlo

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] kvm: testsuite: silence warnings on x86_64

[Avi Kivity]

Carlo Marcelo Arenas Belon wrote:
> On Thu, Jan 24, 2008 at 06:21:56PM +0200, Avi Kivity wrote:
>   
>> Bernhard Kaindl wrote:
>>     
>>> I did not test this patch as I did not find documentation on how to run the
>>> test cases and I could not find a make target to run them from make.
>>>
>>>   
>>>       
>>   make -C user test_cases
>>   user/kvmctl user/test/x86/bootstrap user/test/x86/access.flat
>>
>> (we should rename user -> test)
>>     
>
> are you interested on patches for that? 

git patches (which have rename detection) please.

>
> IMHO will be also a good idea to add a target in the Makefile so that building
> the tests will not be part of "all" 

No, that means the build will break often.

> and will be executed automatically when
> invoked with "make check" as it is is done usually.
>
>   

Yes.

> presume that at least to begin, running all *.flat tests could be a good
> starting point for this target?
>   

Yes.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] kvm: testsuite: silence warnings on x86_64

[Avi Kivity]

Carlo Marcelo Arenas Belon wrote:

[I forgot to reply to the this:]
>>> --- kvm-60/user/test/x86/access.c
>>> +++ kvm-60/user/test/x86/access.c	2008/01/24 15:14:16
>>> @@ -1,6 +1,7 @@
>>>  
>>>  #include "smp.h"
>>>  #include "printf.h"
>>> +#include "string.h"
>>>  
>>>  #define true 1
>>>  #define false 0
>>> @@ -569,7 +570,7 @@
>>>      int r;
>>>  
>>>      printf("starting test\n\n");
>>> -    smp_init(ac_test_run);
>>> +    smp_init((void (*)(void))ac_test_run);
>>>      r = ac_test_run();
>>>      return r ? 0 : 1;
>>>  }
>>>   
>>>       
>> Better to add a wrapper that conforms to the expected signature, and 
>> makes sure the return value of ac_test_run() is not lost.
>>     
>
> this will require redefining smp_init as shown by :
>
> --- a/user/test/x86/lib/smp.h
> +++ b/user/test/x86/lib/smp.h
> @@ -5,7 +5,7 @@ struct spinlock {
>      int v;
>  };
>  
> -void smp_init(void (*smp_main)(void));
> +void smp_init(int (*smp_main)(void));
>  
>  int cpu_count(void);
>  int smp_id(void);
>
> and will require also fixing the smp.flat test to build againg and to return
> a bool when executed (will send patches for doing both later if that is what
> you want)
>   

Yes please.  smp.flat was only used during smp bringup, but it may be 
useful later.

>   
>> Haven't run access.flat on smp for a long while; the results should be 
>> interesting after the page fault scaling work.
>>     
>
> if you meant `kvmctl -s2` it doesn't seem to get pass the kvm device
> initialization inside kvmctl for kvm-60.
>   

Sigh, we should start automated runs of this thing.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[PATCH 0/2] testsuite: fixes for smp compilation issues

[Carlo Marcelo Arenas Belon]

The following series fixes the last remaining warning from the testsuite in
x86 and together with it the make rules for building the other test case
affected by this changes :

  PATCH 1/2 : make smp_init parameter be a function that returns int
  PATCH 2/2 : fix building smp.flat

Tested in x86_64 without smp

Carlo

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[PATCH 1/2] testsuite: make smp_init parameter be a function that returns int

[Carlo Marcelo Arenas Belon]

Fixes :

  test/x86/access.c:577: warning: passing argument 1 of 'smp_init' from
  incompatible pointer type

Signed-off-by: Carlo Marcelo Arenas Belon <carenas-kLeDWSohozoJb6fo7hG9ng@public.gmane.org>
---
 user/test/x86/lib/smp.c |    4 ++--
 user/test/x86/lib/smp.h |    2 +-
 user/test/x86/smptest.c |    3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/user/test/x86/lib/smp.c b/user/test/x86/lib/smp.c
index a4cbe61..84ab163 100644
--- a/user/test/x86/lib/smp.c
+++ b/user/test/x86/lib/smp.c
@@ -125,7 +125,7 @@ void on_cpu(int cpu, void (*function)(void *data), void *data)
     spin_unlock(&ipi_lock);
 }
 
-static void (*smp_main_func)(void);
+static int (*smp_main_func)(void);
 static volatile int smp_main_running;
 
 asm ("smp_init_entry: \n"
@@ -133,7 +133,7 @@ asm ("smp_init_entry: \n"
      "sti \n"
      "call *smp_main_func");
 
-void smp_init(void (*smp_main)(void))
+void smp_init(int (*smp_main)(void))
 {
     int i;
     void smp_init_entry(void);
diff --git a/user/test/x86/lib/smp.h b/user/test/x86/lib/smp.h
index bcf76a3..1ee0cb0 100644
--- a/user/test/x86/lib/smp.h
+++ b/user/test/x86/lib/smp.h
@@ -5,7 +5,7 @@ struct spinlock {
     int v;
 };
 
-void smp_init(void (*smp_main)(void));
+void smp_init(int (*smp_main)(void));
 
 int cpu_count(void);
 int smp_id(void);
diff --git a/user/test/x86/smptest.c b/user/test/x86/smptest.c
index f6ef3e9..c16afd5 100644
--- a/user/test/x86/smptest.c
+++ b/user/test/x86/smptest.c
@@ -11,11 +11,12 @@ static void ipi_test(void *data)
 	printf("but wrong cpu %d\n", smp_id());
 }
 
-static void smp_main(void)
+static int smp_main(void)
 {
     printf("smp main %d\n", smp_id());
     while (1)
 	asm volatile ("hlt" : : : "memory");
+    return 0;
 }
 
 int main()
-- 
1.5.3.7


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[PATCH 2/2] testsuite: fix building smp.flat

[Carlo Marcelo Arenas Belon]

Rename smptest.c so it matches the right stem for building smp.flat

Signed-off-by: Carlo Marcelo Arenas Belon <carenas-kLeDWSohozoJb6fo7hG9ng@public.gmane.org>
---
 user/config-x86-common.mak         |    2 +-
 user/test/x86/{smptest.c => smp.c} |    0 
 2 files changed, 1 insertions(+), 1 deletions(-)
 rename user/test/x86/{smptest.c => smp.c} (100%)

diff --git a/user/config-x86-common.mak b/user/config-x86-common.mak
index 8cfdd45..058d516 100644
--- a/user/config-x86-common.mak
+++ b/user/config-x86-common.mak
@@ -34,7 +34,7 @@ $(TEST_DIR)/vmexit.flat: $(cstart.o) $(TEST_DIR)/vmexit.o
  
 $(TEST_DIR)/test32.flat: $(TEST_DIR)/test32.o
 
-$(TEST_DIR)/smp.flat: $(cstart.o) $(TEST_DIR)/smptest.o
+$(TEST_DIR)/smp.flat: $(cstart.o) $(TEST_DIR)/smp.o
  
 $(TEST_DIR)/emulator.flat: $(cstart.o) $(TEST_DIR)/vm.o $(TEST_DIR)/print.o
 
diff --git a/user/test/x86/smptest.c b/user/test/x86/smp.c
similarity index 100%
rename from user/test/x86/smptest.c
rename to user/test/x86/smp.c
-- 
1.5.3.7


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/