fix qemu-kvm to build when gdbstub is disabled

PATCH In review Jes Sorensen 4 participants 16 messages May 6, 2009 – May 7, 2009 KVM

lore

Share & tools

lore

Jes Sorensen

author

2009-05-06 15:42 UTC #0 lore raw

PATCH 1/1 fix qemu-kvm to build when gdbstub is disabled

Hi,

The latest changes to qemu-kvm breaks miserably if one tries to build without CONFIG_GDBSTUB.

Jes

Make qemu-kvm build again when CONFIG_GDBSTUB is not set.

Signed-off-by Jes Sorensen <[email protected]>

---
 gdbstub.h |    6 ++++++
 vl.c      |    2 --
 2 files changed, 6 insertions(+), 2 deletions(-)

gdbstub.h +6 -0

Index: qemu-kvm/gdbstub.h
===================================================================
@@ -15,7 +15,13 @@
 
 void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...);
 int use_gdb_syscalls(void);
+#ifdef CONFIG_GDBSTUB
 void gdb_set_stop_cpu(CPUState *env);
+#else
+static inline void gdb_set_stop_cpu(CPUState *env)
+{
+}
+#endif
 #ifdef CONFIG_USER_ONLY
 int gdb_queuesig (void);
 int gdb_handlesig (CPUState *, int);

vl.c +0 -2

Index: qemu-kvm/vl.c
===================================================================
@@ -4417,13 +4417,11 @@
         }
         if (cpu_can_run(env))
             ret = qemu_cpu_exec(env);
-#ifndef CONFIG_GDBSTUB
         if (ret == EXCP_DEBUG) {
             gdb_set_stop_cpu(env);
             debug_requested = 1;
             break;
         }
-#endif
     }
 }

Jes Sorensen

author

2009-05-07 08:14 UTC #2 lore raw

Quoted 9 lines · Xiantao

vl.c +0 -1
@@ -4417,13 +4417,11 @@
         }
         if (cpu_can_run(env))
             ret = qemu_cpu_exec(env);
-#ifndef CONFIG_GDBSTUB
^^^^^^^^^^^
Don't know why change #ifdef to #ifndef in upstream, and I remember it should be ifdef before. I believe this stuff should be compiled only if CONFIG_GDBSTUB is defined.

Yes, I made get_set_top_cpu() a noop instead when compiled with CONFIG_GDBSTUB disabled, that way we avoid a bunch of these silly #ifdefs.

Cheers,
Jes

Avi Kivity

2009-05-07 09:31 UTC #3 lore raw

In reply to Zhang, Xiantao

Quoted 17 lines · Xiantao

Quoted 7 lines · Jes Sorensen
Hi,
The latest changes to qemu-kvm breaks miserably if one tries to build without CONFIG_GDBSTUB.
Jes
vl.c +0 -1
@@ -4417,13 +4417,11 @@
         }
         if (cpu_can_run(env))
             ret = qemu_cpu_exec(env);
-#ifndef CONFIG_GDBSTUB

Quoted 3 lines

^^^^^^^^^^^
Don't know why change #ifdef to #ifndef in upstream, and I remember it should be ifdef before. I believe this stuff should be compiled only if CONFIG_GDBSTUB is defined.

This was introduced by

commit 704aec581c1683750e313832ba3aa4813d59cbd0

Author: Xiantao Zhang <[email protected]>
Date:   Thu Nov 27 17:23:27 2008 +0800

Build fix for !CONFIG_GDBSTUB case

Once CONFIG_GDBSTUB not configured, compile will generate error

In upstream. Please fix it in upstream and qemu-kvm.git will get the fix from there.

Signature · 1 line

error compiling committee.c: too many arguments to function

Zhang, Xiantao

2009-05-07 09:50 UTC #4 lore raw

Quoted 30 lines · Avi Kivity

Quoted 17 lines · Xiantao
Quoted 7 lines · Jes Sorensen
Hi,
The latest changes to qemu-kvm breaks miserably if one tries to build without CONFIG_GDBSTUB.
Jes
vl.c +0 -1
@@ -4417,13 +4417,11 @@
         }
         if (cpu_can_run(env))
             ret = qemu_cpu_exec(env);
-#ifndef CONFIG_GDBSTUB
Quoted 5 lines
^^^^^^^^^^^
Don't know why change #ifdef to #ifndef in upstream, and I remember it should be ifdef before. I believe this stuff should be compiled only if CONFIG_GDBSTUB is defined.
This was introduced by
commit 704aec581c1683750e313832ba3aa4813d59cbd0
Author: Xiantao Zhang <[email protected]>
Date:   Thu Nov 27 17:23:27 2008 +0800

Quoted 6 lines

Build fix for !CONFIG_GDBSTUB case
Once CONFIG_GDBSTUB not configured, compile will generate error
In upstream. Please fix it in upstream and qemu-kvm.git will get the fix from there.

The original patch is 8c4379cc made by me in kvm-userspace.git and I used #ifdef, but accidently changed to #ifndef in qemu-kvm.git. So maybe this typo is introduced by Anthony Liguori when rewrote this patch for qemu-kvm.git ? Xiantao

Avi Kivity

2009-05-07 09:59 UTC #5 lore raw

Quoted 36 lines · Xiantao

Quoted 34 lines · Avi Kivity
Quoted 18 lines · Xiantao
Quoted 8 lines · Jes Sorensen
Hi,
The latest changes to qemu-kvm breaks miserably if one tries to build without CONFIG_GDBSTUB.
Jes
vl.c +0 -1
@@ -4417,13 +4417,11 @@
         }
         if (cpu_can_run(env))
             ret = qemu_cpu_exec(env);
-#ifndef CONFIG_GDBSTUB
Quoted 6 lines
^^^^^^^^^^^
Don't know why change #ifdef to #ifndef in upstream, and I remember it should be ifdef before. I believe this stuff should be compiled only if CONFIG_GDBSTUB is defined.
This was introduced by
commit 704aec581c1683750e313832ba3aa4813d59cbd0
Author: Xiantao Zhang <[email protected]>
Date:   Thu Nov 27 17:23:27 2008 +0800
Build fix for !CONFIG_GDBSTUB case

Quoted 9 lines

Quoted 5 lines
Once CONFIG_GDBSTUB not configured, compile will generate error
In upstream. Please fix it in upstream and qemu-kvm.git will get the fix from there.
The original patch is 8c4379cc made by me in kvm-userspace.git and I used #ifdef, but accidently changed to #ifndef in qemu-kvm.git. So maybe this typo is introduced by Anthony Liguori when rewrote this patch for qemu-kvm.git ? Xiantao

That commit got translated to fe538e6f in qemu-kvm.git.

Anthony, what's the reason for the difference between fe538e6f and 704aec58?

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

2009-05-07 10:51 UTC #6 lore raw

In reply to Avi Kivity

Quoted 26 lines · Avi Kivity

Quoted 17 lines · Xiantao
Quoted 7 lines · Jes Sorensen
Hi,
The latest changes to qemu-kvm breaks miserably if one tries to build without CONFIG_GDBSTUB.
Jes
vl.c +0 -1
@@ -4417,13 +4417,11 @@
         }
         if (cpu_can_run(env))
             ret = qemu_cpu_exec(env);
-#ifndef CONFIG_GDBSTUB
Quoted 4 lines
^^^^^^^^^^^
Don't know why change #ifdef to #ifndef in upstream, and I remember it should be ifdef before. I believe this stuff should be compiled only if CONFIG_GDBSTUB is defined.
This was introduced by
commit 704aec581c1683750e313832ba3aa4813d59cbd0

Quoted 8 lines

Author: Xiantao Zhang <[email protected]>
Date:   Thu Nov 27 17:23:27 2008 +0800
Build fix for !CONFIG_GDBSTUB case
Once CONFIG_GDBSTUB not configured, compile will generate error
In upstream. Please fix it in upstream and qemu-kvm.git will get the fix from there.

Given that CONFIG_GDBSTUB is always true upstream, I tend to say:
Finally drop it upstream and keep this (or a more arch-local) workaround for missing ia64 gdbstub support downstream.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Avi Kivity

2009-05-07 11:00 UTC #7 lore raw

Quoted 8 lines · Jan Kiszka

Quoted 3 lines
In upstream. Please fix it in upstream and qemu-kvm.git will get the fix from there.
Given that CONFIG_GDBSTUB is always true upstream, I tend to say:
Finally drop it upstream and keep this (or a more arch-local) workaround for missing ia64 gdbstub support downstream.

I agree, unless

- we want to make gdbstub support configurable (don't see any overwhelming reason for this, but maybe others do)
- we want to merge ia64 kvm support upstream, and don't want to impose gdbstub support (though I'd recommend properly implementing gdbstub)

In any case, I'm okay with dropping the check upstream and applying the local fixup.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

2009-05-07 11:13 UTC #8 lore raw

Quoted 12 lines · Avi Kivity

Quoted 8 lines · Jan Kiszka
Quoted 3 lines
In upstream. Please fix it in upstream and qemu-kvm.git will get the fix from there.
Given that CONFIG_GDBSTUB is always true upstream, I tend to say:
Finally drop it upstream and keep this (or a more arch-local) workaround for missing ia64 gdbstub support downstream.
I agree, unless

Quoted 7 lines

- we want to make gdbstub support configurable (don't see any overwhelming reason for this, but maybe others do)
- we want to merge ia64 kvm support upstream, and don't want to impose gdbstub support (though I'd recommend properly implementing gdbstub)
In any case, I'm okay with dropping the check upstream and applying the local fixup.

The last ia64 tcg patch I saw one or two days ago was missing gdb support, too (and may suffer from some brokenness for that reason). But I would suggest for both cases (tcg and kvm) to temporarily provide stubs/placeholders instead of keeping the central #ifdefs.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Avi Kivity

2009-05-07 11:28 UTC #9 lore raw

Quoted 7 lines · Jan Kiszka

Quoted 7 lines
I agree, unless
- we want to make gdbstub support configurable (don't see any overwhelming reason for this, but maybe others do)
- we want to merge ia64 kvm support upstream, and don't want to impose gdbstub support (though I'd recommend properly implementing gdbstub)

Quoted 6 lines

Quoted 3 lines
In any case, I'm okay with dropping the check upstream and applying the local fixup.
The last ia64 tcg patch I saw one or two days ago was missing gdb support, too (and may suffer from some brokenness for that reason).

I didn't see ia64/tcg. Did you mean s390? That was host only IIRC.

Quoted 4 lines

But
I would suggest for both cases (tcg and kvm) to temporarily provide stubs/placeholders instead of keeping the central #ifdefs.

Agreed.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

2009-05-07 11:38 UTC #10 lore raw

Quoted 8 lines · Avi Kivity

Quoted 7 lines · Jan Kiszka
Quoted 7 lines
I agree, unless
- we want to make gdbstub support configurable (don't see any overwhelming reason for this, but maybe others do)
- we want to merge ia64 kvm support upstream, and don't want to impose gdbstub support (though I'd recommend properly implementing gdbstub)

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
In any case, I'm okay with dropping the check upstream and applying the local fixup.
The last ia64 tcg patch I saw one or two days ago was missing gdb support, too (and may suffer from some brokenness for that reason).
I didn't see ia64/tcg. Did you mean s390? That was host only IIRC.

Ah, you are right.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Jes Sorensen

author

2009-05-07 12:34 UTC #11 lore raw

In reply to Avi Kivity

Quoted 8 lines · Avi Kivity

I agree, unless
- we want to make gdbstub support configurable (don't see any overwhelming reason for this, but maybe others do)
- we want to merge ia64 kvm support upstream, and don't want to impose gdbstub support (though I'd recommend properly implementing gdbstub)
In any case, I'm okay with dropping the check upstream and applying the local fixup.

Hi,

Here's a patch that fixes the #ifndef to make it the #ifdef as it was intended.

I am quite fine with us trying to drop all the #ifdefs and introduce noop wrappers for archs that do not provide the gdbstubs (ie. ia64), but to start with we better just fix the #ifndef to make it behave like it was originally intended.

Cheers,
Jes

Fix incorrect #ifndef for CONFIG_GETSTUB, which should have been an #ifdef.

Signed-off-by Jes Sorensen <[email protected]>

---
 vl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

vl.c +1 -1

Index: qemu/vl.c
===================================================================
@@ -4350,7 +4350,7 @@
         }
         if (cpu_can_run(env))
             ret = qemu_cpu_exec(env);
-#ifndef CONFIG_GDBSTUB
+#ifdef CONFIG_GDBSTUB
         if (ret == EXCP_DEBUG) {
             gdb_set_stop_cpu(env);
             debug_requested = 1;

Jan Kiszka

2009-05-07 12:59 UTC #12 lore raw

Quoted 13 lines · Jes Sorensen

Quoted 9 lines · Avi Kivity
I agree, unless
- we want to make gdbstub support configurable (don't see any overwhelming reason for this, but maybe others do)
- we want to merge ia64 kvm support upstream, and don't want to impose gdbstub support (though I'd recommend properly implementing gdbstub)
In any case, I'm okay with dropping the check upstream and applying the local fixup.
Hi,

Quoted 7 lines

Here's a patch that fixes the #ifndef to make it the #ifdef as it was intended.
I am quite fine with us trying to drop all the #ifdefs and introduce noop wrappers for archs that do not provide the gdbstubs (ie. ia64), but to start with we better just fix the #ifndef to make it behave like it was originally intended.

As the original change this patch (correctly) fixes broke gdbstub support in qemu-kvm for all archs + it is still a deviation from upstream, please take the chance and go for ia64 stubs.

BTW, I'm planning to submit a cleanup patch for CONFIG_GDBSTUB to upstream soon.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Jes Sorensen

author

2009-05-07 13:03 UTC #13 lore raw

Quoted 3 lines · Jan Kiszka

As the original change this patch (correctly) fixes broke gdbstub support in qemu-kvm for all archs + it is still a deviation from upstream, please take the chance and go for ia64 stubs.

Jan,

There's far more to do in upstream than just gdbstubs for ia64 to be usable :-(

We'll get to gdbstubs at some point, but there's too many other higher priority items to deal with first.

Cheers,
Jes

Jan Kiszka

2009-05-07 13:07 UTC #14 lore raw

Quoted 5 lines · Jes Sorensen

Quoted 3 lines · Jan Kiszka
As the original change this patch (correctly) fixes broke gdbstub support in qemu-kvm for all archs + it is still a deviation from upstream, please take the chance and go for ia64 stubs.

Quoted 7 lines

Jan,
There's far more to do in upstream than just gdbstubs for ia64 to be usable :-(
We'll get to gdbstubs at some point, but there's too many other higher priority items to deal with first.

I'm not asking for full gdbstub support at this point, just for a build workaround at arch-level (or in arch-specific blocks, in gdbstub.c e.g.) instead of patching generic code with global risk of regressions.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Jes Sorensen

author

2009-05-07 13:31 UTC #15 lore raw

Quoted 3 lines · Jan Kiszka

I'm not asking for full gdbstub support at this point, just for a build workaround at arch-level (or in arch-specific blocks, in gdbstub.c e.g.) instead of patching generic code with global risk of regressions.

Oh, I see.

I already submitted that to Avi, making gdb_set_stop_cpu() a noop on ia64. Since there's no target-ia64/ directory in mainline qemu.git yet, that fix will have to live in qemu-kvm for the time being.

I am fine if you wish to remove all the #ifdef CONFIG_GDBSTUB bits.

Cheers,
Jes