kvm: testsuite: silence warnings on x86_64

PATCH Applied Bernhard Kaindl 3 participants 10 messages Jan 24, 2008 – Jan 31, 2008 4 messages not in the archive KVM

lore

Share & tools

lore

Bernhard Kaindl

author

2008-01-24 16:06 UTC #0 lore raw

PATCH 1/1 kvm: testsuite: silence warnings on x86_64

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

---
 access.c     |    8 +++++++-
 emulator.c   |    3 +--
 lib/printf.c |    1 -
 3 files changed, 8 insertions(+), 4 deletions(-)

user/test/x86/access.c +2 -1

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

user/test/x86/emulator.c +1 -2

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

user/test/x86/lib/printf.c +0 -1

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

A message that is not in the archive (sent off-list or not yet received): alpine.LNX.1.00.0801241628040.5231-aORvohZ8rX8NvFgr0RP9cw@public.gmane.org

Avi Kivity

2008-01-24 16:21 UTC #2 lore raw

Quoted 4 lines · Bernhard Kaindl

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)

Quoted 12 lines

user/test/x86/access.c +1 -0

@@ -1,6 +1,7 @@
 
 #include "smp.h"
 #include "printf.h"
+#include "string.h"
 
 #define true 1
 #define false 0

user/test/x86/access.c line 576

Quoted 8 lines

user/test/x86/access.c +1 -1

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

user/test/x86/access.c line 110

Quoted 11 lines

= i;

user/test/x86/access.c +1 -1

@@ -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 ("&").

Signature · 7 lines

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/

A message that is not in the archive (sent off-list or not yet received): 4798BB24.9080100-atKUWr5tajBWk0Htik3J/[email protected]

Bernhard Kaindl

author

2008-01-24 17:43 UTC #4 lore raw

Quoted 3 lines · Avi Kivity

make -C user test_cases
user/kvmctl user/test/x86/bootstrap user/test/x86/access.flat

Ah, thanks!

Quoted 8 lines

Review
Quoted 2 lines
(quoted diff) +1 -1
-    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.

Quoted 6 lines

Review
Quoted 2 lines
(quoted diff) +1 -1
-	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/lib/printf.c: In function 'vsnprintf':
test/x86/lib/printf.c:95: warning: unused variable 'n'

Signed-off-by Bernhard Kaindl <[email protected]>

user/test/x86/access.c +1 -0

@@ -1,6 +1,7 @@
 
 #include "smp.h"
 #include "printf.h"
+#include "string.h"
 
 #define true 1
 #define false 0

user/test/x86/emulator.c +2 -3

@@ -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");

user/test/x86/lib/printf.c +0 -1

@@ -92,7 +92,6 @@
 
 int vsnprintf(char *buf, int size, const char *fmt, va_list va)
 {
-    int n;
     pstream_t s;
 
     s.buffer = buf;

A message that is not in the archive (sent off-list or not yet received): alpine.LNX.1.00.0801241808470.5231-aORvohZ8rX8NvFgr0RP9cw@public.gmane.org

Avi Kivity

Applied

2008-01-27 07:15 UTC #6 lore raw

Quoted 4 lines · Bernhard Kaindl

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.

Signature · 7 lines

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/

Carlo Marcelo Arenas Belon

2008-01-31 10:23 UTC #7 lore raw

In reply to a message not in the archive

Quoted 9 lines · Avi Kivity

Quoted 4 lines · Bernhard Kaindl
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?

Quoted 13 lines

user/test/x86/access.c +1 -0

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

Quoted 10 lines

user/test/x86/access.c line 576
Quoted 7 lines
user/test/x86/access.c +1 -1
     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 :

user/test/x86/lib/smp.h +1 -1

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

Quoted 2 lines

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

Avi Kivity

2008-01-31 10:38 UTC #8 lore raw

Quoted 9 lines · Carlo Marcelo Arenas Belon

Quoted 7 lines · Avi Kivity
Quoted 5 lines · Bernhard Kaindl
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.

Quoted 7 lines

Quoted 5 lines
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.

Quoted 3 lines

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.

Quoted 4 lines

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

Yes.

Quoted 3 lines

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

Yes.

Signature · 7 lines

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/

Avi Kivity

2008-01-31 10:45 UTC #9 lore raw

In reply to Carlo Marcelo Arenas Belon

Carlo Marcelo Arenas Belon wrote:

[I forgot to reply to the this:]

Quoted 39 lines

Quoted 13 lines
Quoted 13 lines
user/test/x86/access.c +1 -0
@@ -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;
 
Quoted 11 lines
user/test/x86/access.c line 576
Quoted 8 lines
user/test/x86/access.c +1 -1
     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 :
user/test/x86/lib/smp.h +1 -1
@@ -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);

Quoted 4 lines

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.

Quoted 8 lines

Quoted 4 lines
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.

Signature · 7 lines

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/

A message that is not in the archive (sent off-list or not yet received): 47A1A6DB.6080804-atKUWr5tajBWk0Htik3J/[email protected]

Carlo Marcelo Arenas Belon

2008-01-31 12:15 UTC #11 lore raw

cover testsuite: fixes for smp compilation issues

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

Carlo Marcelo Arenas Belon

2008-01-31 12:18 UTC #12 lore raw

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

Fixes :

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

Signed-off-by Carlo Marcelo Arenas Belon <[email protected]>

---
 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(-)

user/test/x86/lib/smp.c +2 -2

index a4cbe61..84ab163 100644
@@ -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);

user/test/x86/lib/smp.h +1 -1

index bcf76a3..1ee0cb0 100644
@@ -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);

user/test/x86/smptest.c +2 -1

index f6ef3e9..c16afd5 100644
@@ -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()

Signature · 7 lines

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/

Carlo Marcelo Arenas Belon

2008-01-31 12:20 UTC #13 lore raw

PATCH 2/2 testsuite: fix building smp.flat

In reply to Carlo Marcelo Arenas Belon

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

Signed-off-by Carlo Marcelo Arenas Belon <[email protected]>

---
 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%)

user/config-x86-common.mak +1 -1

index 8cfdd45..058d516 100644
@@ -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

user/test/x86/smp.c renamed from user/test/x86/smptest.c +0 -0

similarity index 100%
rename from user/test/x86/smptest.c
rename to user/test/x86/smp.c

Signature · 7 lines

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/