diff mbox series

[v3,35/45] windbg: debug exception subscribing

Message ID 151127343314.6888.13289325558071964830.stgit@Misha-PC.lan02.inno
State New
Headers show
Series Windbg supporting | expand

Commit Message

Mikhail Abakumov Nov. 21, 2017, 2:10 p.m. UTC 
Added handler registration of gdb debug exception. Its exception also can be used for windbg.

Signed-off-by: Mihail Abakumov <mikhail.abakumov@ispras.ru>
Signed-off-by: Pavel Dovgalyuk <dovgaluk@ispras.ru>
Signed-off-by: Dmitriy Koltunov <koltunov@ispras.ru>
---
 cpus.c                  |   18 +++++++++++++++++-
 gdbstub.c               |    4 ++++
 include/sysemu/sysemu.h |    2 ++
 windbgstub.c            |   16 ++++++++++++----
 4 files changed, 35 insertions(+), 5 deletions(-)

Comments

Ladi Prosek Nov. 29, 2017, 7:13 a.m. UTC | #1 
On Tue, Nov 21, 2017 at 3:10 PM, Mihail Abakumov
<mikhail.abakumov@ispras.ru> wrote:
> Added handler registration of gdb debug exception. Its exception also can be used for windbg.
>
> Signed-off-by: Mihail Abakumov <mikhail.abakumov@ispras.ru>
> Signed-off-by: Pavel Dovgalyuk <dovgaluk@ispras.ru>
> Signed-off-by: Dmitriy Koltunov <koltunov@ispras.ru>
> ---
>  cpus.c                  |   18 +++++++++++++++++-
>  gdbstub.c               |    4 ++++
>  include/sysemu/sysemu.h |    2 ++
>  windbgstub.c            |   16 ++++++++++++----
>  4 files changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 9bed61eefc..212553b7e3 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -77,6 +77,8 @@ int64_t max_advance;
>  static QEMUTimer *throttle_timer;
>  static unsigned int throttle_percentage;
>
> +static void (*excp_debug_handler)(CPUState *cpu);
> +
>  #define CPU_THROTTLE_PCT_MIN 1
>  #define CPU_THROTTLE_PCT_MAX 99
>  #define CPU_THROTTLE_TIMESLICE_NS 10000000
> @@ -960,9 +962,23 @@ static bool cpu_can_run(CPUState *cpu)
>      return true;
>  }
>
> +bool register_excp_debug_handler(void (*handler)(CPUState *cpu))
> +{
> +    if (excp_debug_handler == NULL) {
> +        excp_debug_handler = handler;
> +        return true;
> +    } else {
> +        error_report("ERROR: Something debugger already using");

So I take it that -windbg and -gdb cannot be used at the same time.
Should it be handled in a more explicit way with a more user-friendly
error?

Right now I get this error, which is more like an implementation
detail (could the debug handler be refactored into a multicast event
in the future?) and does not even make sense as a sentence.

> +        return false;
> +    }
> +}
> +
>  static void cpu_handle_guest_debug(CPUState *cpu)
>  {
> -    gdb_set_stop_cpu(cpu);
> +    if (excp_debug_handler != NULL) {
> +        excp_debug_handler(cpu);
> +    }
> +
>      qemu_system_debug_request();
>      cpu->stopped = true;
>  }
> diff --git a/gdbstub.c b/gdbstub.c
> index 2a94030d3b..8c76f54117 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2006,6 +2006,10 @@ int gdbserver_start(const char *device)
>      s->mon_chr = mon_chr;
>      s->current_syscall_cb = NULL;
>
> +    if (!register_excp_debug_handler(gdb_set_stop_cpu)) {
> +        exit(1);
> +    }
> +
>      return 0;
>  }
>
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index b21369672a..34588c99b4 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -193,6 +193,8 @@ QemuOpts *qemu_get_machine_opts(void);
>
>  bool defaults_enabled(void);
>
> +bool register_excp_debug_handler(void (*handler)(CPUState *cpu));
> +
>  extern QemuOptsList qemu_legacy_drive_opts;
>  extern QemuOptsList qemu_common_drive_opts;
>  extern QemuOptsList qemu_drive_opts;
> diff --git a/windbgstub.c b/windbgstub.c
> index 489abe6d6c..b33f412659 100755
> --- a/windbgstub.c
> +++ b/windbgstub.c
> @@ -115,16 +115,20 @@ static void windbg_send_control_packet(uint16_t type)
>      windbg_state->ctrl_packet_id ^= 1;
>  }
>
> -static void windbg_vm_stop(void)
> +static void windbg_bp_handler(CPUState *cpu)
>  {
> -    CPUState *cpu = qemu_get_cpu(0);
> -    vm_stop(RUN_STATE_PAUSED);
> -
>      SizedBuf buf = kd_gen_exception_sc(cpu);
>      windbg_send_data_packet(buf.data, buf.size, PACKET_TYPE_KD_STATE_CHANGE64);
>      SBUF_FREE(buf);
>  }
>
> +static void windbg_vm_stop(void)
> +{
> +    CPUState *cpu = qemu_get_cpu(0);
> +    vm_stop(RUN_STATE_PAUSED);
> +    windbg_bp_handler(cpu);
> +}
> +
>  static void windbg_process_manipulate_packet(ParsingContext *ctx)
>  {
>      CPUState *cpu;
> @@ -432,6 +436,10 @@ int windbg_server_start(const char *device)
>
>      qemu_register_reset(windbg_handle_reset, NULL);
>
> +    if (!register_excp_debug_handler(windbg_bp_handler)) {
> +        exit(1);
> +    }
> +
>      atexit(windbg_exit);
>      return 0;
>  }
>
Mikhail Abakumov Dec. 6, 2017, 7:29 a.m. UTC | #2 
Ladi Prosek писал 2017-11-29 10:13:
> On Tue, Nov 21, 2017 at 3:10 PM, Mihail Abakumov
> <mikhail.abakumov@ispras.ru> wrote:
>> Added handler registration of gdb debug exception. Its exception also 
>> can be used for windbg.
>> 
>> Signed-off-by: Mihail Abakumov <mikhail.abakumov@ispras.ru>
>> Signed-off-by: Pavel Dovgalyuk <dovgaluk@ispras.ru>
>> Signed-off-by: Dmitriy Koltunov <koltunov@ispras.ru>
>> ---
>>  cpus.c                  |   18 +++++++++++++++++-
>>  gdbstub.c               |    4 ++++
>>  include/sysemu/sysemu.h |    2 ++
>>  windbgstub.c            |   16 ++++++++++++----
>>  4 files changed, 35 insertions(+), 5 deletions(-)
>> 
>> diff --git a/cpus.c b/cpus.c
>> index 9bed61eefc..212553b7e3 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -77,6 +77,8 @@ int64_t max_advance;
>>  static QEMUTimer *throttle_timer;
>>  static unsigned int throttle_percentage;
>> 
>> +static void (*excp_debug_handler)(CPUState *cpu);
>> +
>>  #define CPU_THROTTLE_PCT_MIN 1
>>  #define CPU_THROTTLE_PCT_MAX 99
>>  #define CPU_THROTTLE_TIMESLICE_NS 10000000
>> @@ -960,9 +962,23 @@ static bool cpu_can_run(CPUState *cpu)
>>      return true;
>>  }
>> 
>> +bool register_excp_debug_handler(void (*handler)(CPUState *cpu))
>> +{
>> +    if (excp_debug_handler == NULL) {
>> +        excp_debug_handler = handler;
>> +        return true;
>> +    } else {
>> +        error_report("ERROR: Something debugger already using");
> 
> So I take it that -windbg and -gdb cannot be used at the same time.
> Should it be handled in a more explicit way with a more user-friendly
> error?
> 
> Right now I get this error, which is more like an implementation
> detail (could the debug handler be refactored into a multicast event
> in the future?) and does not even make sense as a sentence.

Yes. I have added a more user-friendly error. "Something debugger is
already in use. '-gdb' and '-windbg' cannot be used at the same time".
Or what do you mean under the explicit way? How to do it better?

> 
>> +        return false;
>> +    }
>> +}
>> +
>>  static void cpu_handle_guest_debug(CPUState *cpu)
>>  {
>> -    gdb_set_stop_cpu(cpu);
>> +    if (excp_debug_handler != NULL) {
>> +        excp_debug_handler(cpu);
>> +    }
>> +
>>      qemu_system_debug_request();
>>      cpu->stopped = true;
>>  }
>> diff --git a/gdbstub.c b/gdbstub.c
>> index 2a94030d3b..8c76f54117 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -2006,6 +2006,10 @@ int gdbserver_start(const char *device)
>>      s->mon_chr = mon_chr;
>>      s->current_syscall_cb = NULL;
>> 
>> +    if (!register_excp_debug_handler(gdb_set_stop_cpu)) {
>> +        exit(1);
>> +    }
>> +
>>      return 0;
>>  }
>> 
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index b21369672a..34588c99b4 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -193,6 +193,8 @@ QemuOpts *qemu_get_machine_opts(void);
>> 
>>  bool defaults_enabled(void);
>> 
>> +bool register_excp_debug_handler(void (*handler)(CPUState *cpu));
>> +
>>  extern QemuOptsList qemu_legacy_drive_opts;
>>  extern QemuOptsList qemu_common_drive_opts;
>>  extern QemuOptsList qemu_drive_opts;
>> diff --git a/windbgstub.c b/windbgstub.c
>> index 489abe6d6c..b33f412659 100755
>> --- a/windbgstub.c
>> +++ b/windbgstub.c
>> @@ -115,16 +115,20 @@ static void windbg_send_control_packet(uint16_t 
>> type)
>>      windbg_state->ctrl_packet_id ^= 1;
>>  }
>> 
>> -static void windbg_vm_stop(void)
>> +static void windbg_bp_handler(CPUState *cpu)
>>  {
>> -    CPUState *cpu = qemu_get_cpu(0);
>> -    vm_stop(RUN_STATE_PAUSED);
>> -
>>      SizedBuf buf = kd_gen_exception_sc(cpu);
>>      windbg_send_data_packet(buf.data, buf.size, 
>> PACKET_TYPE_KD_STATE_CHANGE64);
>>      SBUF_FREE(buf);
>>  }
>> 
>> +static void windbg_vm_stop(void)
>> +{
>> +    CPUState *cpu = qemu_get_cpu(0);
>> +    vm_stop(RUN_STATE_PAUSED);
>> +    windbg_bp_handler(cpu);
>> +}
>> +
>>  static void windbg_process_manipulate_packet(ParsingContext *ctx)
>>  {
>>      CPUState *cpu;
>> @@ -432,6 +436,10 @@ int windbg_server_start(const char *device)
>> 
>>      qemu_register_reset(windbg_handle_reset, NULL);
>> 
>> +    if (!register_excp_debug_handler(windbg_bp_handler)) {
>> +        exit(1);
>> +    }
>> +
>>      atexit(windbg_exit);
>>      return 0;
>>  }
>>
Ladi Prosek Dec. 6, 2017, 9:23 a.m. UTC | #3 
On Wed, Dec 6, 2017 at 8:29 AM, Mihail Abakumov
<mikhail.abakumov@ispras.ru> wrote:
> Ladi Prosek писал 2017-11-29 10:13:
>
>> On Tue, Nov 21, 2017 at 3:10 PM, Mihail Abakumov
>> <mikhail.abakumov@ispras.ru> wrote:
>>>
>>> Added handler registration of gdb debug exception. Its exception also can
>>> be used for windbg.
>>>
>>> Signed-off-by: Mihail Abakumov <mikhail.abakumov@ispras.ru>
>>> Signed-off-by: Pavel Dovgalyuk <dovgaluk@ispras.ru>
>>> Signed-off-by: Dmitriy Koltunov <koltunov@ispras.ru>
>>> ---
>>>  cpus.c                  |   18 +++++++++++++++++-
>>>  gdbstub.c               |    4 ++++
>>>  include/sysemu/sysemu.h |    2 ++
>>>  windbgstub.c            |   16 ++++++++++++----
>>>  4 files changed, 35 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/cpus.c b/cpus.c
>>> index 9bed61eefc..212553b7e3 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -77,6 +77,8 @@ int64_t max_advance;
>>>  static QEMUTimer *throttle_timer;
>>>  static unsigned int throttle_percentage;
>>>
>>> +static void (*excp_debug_handler)(CPUState *cpu);
>>> +
>>>  #define CPU_THROTTLE_PCT_MIN 1
>>>  #define CPU_THROTTLE_PCT_MAX 99
>>>  #define CPU_THROTTLE_TIMESLICE_NS 10000000
>>> @@ -960,9 +962,23 @@ static bool cpu_can_run(CPUState *cpu)
>>>      return true;
>>>  }
>>>
>>> +bool register_excp_debug_handler(void (*handler)(CPUState *cpu))
>>> +{
>>> +    if (excp_debug_handler == NULL) {
>>> +        excp_debug_handler = handler;
>>> +        return true;
>>> +    } else {
>>> +        error_report("ERROR: Something debugger already using");
>>
>>
>> So I take it that -windbg and -gdb cannot be used at the same time.
>> Should it be handled in a more explicit way with a more user-friendly
>> error?
>>
>> Right now I get this error, which is more like an implementation
>> detail (could the debug handler be refactored into a multicast event
>> in the future?) and does not even make sense as a sentence.
>
>
> Yes. I have added a more user-friendly error. "Something debugger is
> already in use. '-gdb' and '-windbg' cannot be used at the same time".
> Or what do you mean under the explicit way? How to do it better?

Yes, "'-gdb' and '-windbg' cannot be used at the same time" would be
nicer. By being more explicit I mean issuing the error from the code
that parses the options.

In other words, I can imagine an implementation of
register_excp_debug_handler() that allows registering multiple
handlers. But running windbg and gdb at the same would probably still
not be a good idea.

Maybe I'm nit-picking and it's ok for the error to stay here.
Wondering what other reviewers think.

>>
>>> +        return false;
>>> +    }
>>> +}
>>> +
>>>  static void cpu_handle_guest_debug(CPUState *cpu)
>>>  {
>>> -    gdb_set_stop_cpu(cpu);
>>> +    if (excp_debug_handler != NULL) {
>>> +        excp_debug_handler(cpu);
>>> +    }
>>> +
>>>      qemu_system_debug_request();
>>>      cpu->stopped = true;
>>>  }
>>> diff --git a/gdbstub.c b/gdbstub.c
>>> index 2a94030d3b..8c76f54117 100644
>>> --- a/gdbstub.c
>>> +++ b/gdbstub.c
>>> @@ -2006,6 +2006,10 @@ int gdbserver_start(const char *device)
>>>      s->mon_chr = mon_chr;
>>>      s->current_syscall_cb = NULL;
>>>
>>> +    if (!register_excp_debug_handler(gdb_set_stop_cpu)) {
>>> +        exit(1);
>>> +    }
>>> +
>>>      return 0;
>>>  }
>>>
>>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>>> index b21369672a..34588c99b4 100644
>>> --- a/include/sysemu/sysemu.h
>>> +++ b/include/sysemu/sysemu.h
>>> @@ -193,6 +193,8 @@ QemuOpts *qemu_get_machine_opts(void);
>>>
>>>  bool defaults_enabled(void);
>>>
>>> +bool register_excp_debug_handler(void (*handler)(CPUState *cpu));
>>> +
>>>  extern QemuOptsList qemu_legacy_drive_opts;
>>>  extern QemuOptsList qemu_common_drive_opts;
>>>  extern QemuOptsList qemu_drive_opts;
>>> diff --git a/windbgstub.c b/windbgstub.c
>>> index 489abe6d6c..b33f412659 100755
>>> --- a/windbgstub.c
>>> +++ b/windbgstub.c
>>> @@ -115,16 +115,20 @@ static void windbg_send_control_packet(uint16_t
>>> type)
>>>      windbg_state->ctrl_packet_id ^= 1;
>>>  }
>>>
>>> -static void windbg_vm_stop(void)
>>> +static void windbg_bp_handler(CPUState *cpu)
>>>  {
>>> -    CPUState *cpu = qemu_get_cpu(0);
>>> -    vm_stop(RUN_STATE_PAUSED);
>>> -
>>>      SizedBuf buf = kd_gen_exception_sc(cpu);
>>>      windbg_send_data_packet(buf.data, buf.size,
>>> PACKET_TYPE_KD_STATE_CHANGE64);
>>>      SBUF_FREE(buf);
>>>  }
>>>
>>> +static void windbg_vm_stop(void)
>>> +{
>>> +    CPUState *cpu = qemu_get_cpu(0);
>>> +    vm_stop(RUN_STATE_PAUSED);
>>> +    windbg_bp_handler(cpu);
>>> +}
>>> +
>>>  static void windbg_process_manipulate_packet(ParsingContext *ctx)
>>>  {
>>>      CPUState *cpu;
>>> @@ -432,6 +436,10 @@ int windbg_server_start(const char *device)
>>>
>>>      qemu_register_reset(windbg_handle_reset, NULL);
>>>
>>> +    if (!register_excp_debug_handler(windbg_bp_handler)) {
>>> +        exit(1);
>>> +    }
>>> +
>>>      atexit(windbg_exit);
>>>      return 0;
>>>  }
>>>
>
diff mbox series

Patch

diff --git a/cpus.c b/cpus.c
index 9bed61eefc..212553b7e3 100644
--- a/cpus.c
+++ b/cpus.c
@@ -77,6 +77,8 @@  int64_t max_advance;
 static QEMUTimer *throttle_timer;
 static unsigned int throttle_percentage;
 
+static void (*excp_debug_handler)(CPUState *cpu);
+
 #define CPU_THROTTLE_PCT_MIN 1
 #define CPU_THROTTLE_PCT_MAX 99
 #define CPU_THROTTLE_TIMESLICE_NS 10000000
@@ -960,9 +962,23 @@  static bool cpu_can_run(CPUState *cpu)
     return true;
 }
 
+bool register_excp_debug_handler(void (*handler)(CPUState *cpu))
+{
+    if (excp_debug_handler == NULL) {
+        excp_debug_handler = handler;
+        return true;
+    } else {
+        error_report("ERROR: Something debugger already using");
+        return false;
+    }
+}
+
 static void cpu_handle_guest_debug(CPUState *cpu)
 {
-    gdb_set_stop_cpu(cpu);
+    if (excp_debug_handler != NULL) {
+        excp_debug_handler(cpu);
+    }
+
     qemu_system_debug_request();
     cpu->stopped = true;
 }
diff --git a/gdbstub.c b/gdbstub.c
index 2a94030d3b..8c76f54117 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2006,6 +2006,10 @@  int gdbserver_start(const char *device)
     s->mon_chr = mon_chr;
     s->current_syscall_cb = NULL;
 
+    if (!register_excp_debug_handler(gdb_set_stop_cpu)) {
+        exit(1);
+    }
+
     return 0;
 }
 
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b21369672a..34588c99b4 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -193,6 +193,8 @@  QemuOpts *qemu_get_machine_opts(void);
 
 bool defaults_enabled(void);
 
+bool register_excp_debug_handler(void (*handler)(CPUState *cpu));
+
 extern QemuOptsList qemu_legacy_drive_opts;
 extern QemuOptsList qemu_common_drive_opts;
 extern QemuOptsList qemu_drive_opts;
diff --git a/windbgstub.c b/windbgstub.c
index 489abe6d6c..b33f412659 100755
--- a/windbgstub.c
+++ b/windbgstub.c
@@ -115,16 +115,20 @@  static void windbg_send_control_packet(uint16_t type)
     windbg_state->ctrl_packet_id ^= 1;
 }
 
-static void windbg_vm_stop(void)
+static void windbg_bp_handler(CPUState *cpu)
 {
-    CPUState *cpu = qemu_get_cpu(0);
-    vm_stop(RUN_STATE_PAUSED);
-
     SizedBuf buf = kd_gen_exception_sc(cpu);
     windbg_send_data_packet(buf.data, buf.size, PACKET_TYPE_KD_STATE_CHANGE64);
     SBUF_FREE(buf);
 }
 
+static void windbg_vm_stop(void)
+{
+    CPUState *cpu = qemu_get_cpu(0);
+    vm_stop(RUN_STATE_PAUSED);
+    windbg_bp_handler(cpu);
+}
+
 static void windbg_process_manipulate_packet(ParsingContext *ctx)
 {
     CPUState *cpu;
@@ -432,6 +436,10 @@  int windbg_server_start(const char *device)
 
     qemu_register_reset(windbg_handle_reset, NULL);
 
+    if (!register_excp_debug_handler(windbg_bp_handler)) {
+        exit(1);
+    }
+
     atexit(windbg_exit);
     return 0;
 }