| Message ID | 150642387446.3900.8629683985208127854.stgit@Misha-PC.lan02.inno |
|---|---|
| State | New |
| Headers | show |
| Series | Windbg supporting | expand |
On Tue, Sep 26, 2017 at 4:04 AM, Mihail Abakumov <mikhail.abakumov@ispras.ru> wrote: > Added some helper features for windbgstub. > > 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> > --- > include/exec/windbgstub-utils.h | 46 +++++++++++++++++++++++++++++++++++++++ > include/exec/windbgstub.h | 3 +++ > windbgstub.c | 1 + > 3 files changed, 50 insertions(+) > > diff --git a/include/exec/windbgstub-utils.h b/include/exec/windbgstub-utils.h > index 2390597f1f..65f336e4bf 100755 > --- a/include/exec/windbgstub-utils.h > +++ b/include/exec/windbgstub-utils.h > @@ -13,7 +13,53 @@ > #define WINDBGSTUB_UTILS_H > > #include "qemu/osdep.h" > +#include "qemu/error-report.h" > +#include "cpu.h" > #include "exec/windbgstub.h" > #include "exec/windbgkd.h" > > +#ifndef TARGET_I386 > +#error Unsupported Architecture > +#endif > +#ifdef TARGET_X86_64 /* Unimplemented yet */ > +#error Unsupported Architecture > +#endif > + > +#if (WINDBG_DEBUG_ON) > + > +# define WINDBG_DEBUG(...) do { \ > + printf("Debug: " __VA_ARGS__); \ > + printf("\n"); \ > +} while (false) > + > +# define WINDBG_ERROR(...) do { \ > + printf("Error: " __VA_ARGS__); \ > + printf("\n"); \ > +} while (false) Use qemu_log() instead of printf(). Have a look as some other files for the usual way we handle debug printing. > + > +#else > + > +# define WINDBG_DEBUG(...) > +# define WINDBG_ERROR(...) error_report(WINDBG ": " __VA_ARGS__) > + > +#endif > + > +#define FMT_ADDR "addr:0x" TARGET_FMT_lx > +#define FMT_ERR "Error:%d" > + > +#define UINT8_P(ptr) ((uint8_t *) (ptr)) > +#define UINT32_P(ptr) ((uint32_t *) (ptr)) > +#define FIELD_P(type, field, ptr) ((typeof_field(type, field) *) (ptr)) > +#define PTR(var) UINT8_P(&var) > + > +#define M64_SIZE sizeof(DBGKD_MANIPULATE_STATE64) > + > +#define sizeof_field(type, field) sizeof(((type *) NULL)->field) > + > +#define READ_VMEM(cpu, addr, type) ({ \ > + type _t; \ > + cpu_memory_rw_debug(cpu, addr, PTR(_t), sizeof(type), 0); \ > + _t; \ > +}) > + > #endif > diff --git a/include/exec/windbgstub.h b/include/exec/windbgstub.h > index 1a6e1cc6e5..703fc26b8f 100755 > --- a/include/exec/windbgstub.h > +++ b/include/exec/windbgstub.h > @@ -12,6 +12,9 @@ > #ifndef WINDBGSTUB_H > #define WINDBGSTUB_H > > +#define WINDBG "windbg" > +#define WINDBG_DEBUG_ON false You should have a check here to see if the user has already set WINDBG_DEBUG_ON to allow people to set it during build time. Thanks, Alistair > + > int windbg_server_start(const char *device); > > #endif > diff --git a/windbgstub.c b/windbgstub.c > index 4951f59203..3830446988 100755 > --- a/windbgstub.c > +++ b/windbgstub.c > @@ -11,6 +11,7 @@ > > #include "qemu/osdep.h" > #include "exec/windbgstub.h" > +#include "exec/windbgstub-utils.h" > > int windbg_server_start(const char *device) > { > >
On 09/26/2017 12:13 PM, Alistair Francis wrote: >> +#if (WINDBG_DEBUG_ON) >> + >> +# define WINDBG_DEBUG(...) do { \ >> + printf("Debug: " __VA_ARGS__); \ >> + printf("\n"); \ >> +} while (false) >> + >> +# define WINDBG_ERROR(...) do { \ >> + printf("Error: " __VA_ARGS__); \ >> + printf("\n"); \ >> +} while (false) > > Use qemu_log() instead of printf(). > > Have a look as some other files for the usual way we handle debug printing. > >> + >> +#else >> + >> +# define WINDBG_DEBUG(...) >> +# define WINDBG_ERROR(...) error_report(WINDBG ": " __VA_ARGS__) What's more - as written, your approach is prone to bit-rot: the debug/error statements are not compared against -Werror except for the rare person that enables debugging. Better is go make the macro unconditionally expand to something that triggers -Wformat checking, but guarded by an if(0) for normal use. Or even switch to trace points rather than debugging statements, so that you can control at runtime how much debugging information you want, rather than having to recompile to turn it on and off.
Alistair Francis писал 2017-09-26 20:13: > On Tue, Sep 26, 2017 at 4:04 AM, Mihail Abakumov > <mikhail.abakumov@ispras.ru> wrote: >> Added some helper features for windbgstub. >> >> 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> >> --- >> include/exec/windbgstub-utils.h | 46 >> +++++++++++++++++++++++++++++++++++++++ >> include/exec/windbgstub.h | 3 +++ >> windbgstub.c | 1 + >> 3 files changed, 50 insertions(+) >> >> diff --git a/include/exec/windbgstub-utils.h >> b/include/exec/windbgstub-utils.h >> index 2390597f1f..65f336e4bf 100755 >> --- a/include/exec/windbgstub-utils.h >> +++ b/include/exec/windbgstub-utils.h >> @@ -13,7 +13,53 @@ >> #define WINDBGSTUB_UTILS_H >> >> #include "qemu/osdep.h" >> +#include "qemu/error-report.h" >> +#include "cpu.h" >> #include "exec/windbgstub.h" >> #include "exec/windbgkd.h" >> >> +#ifndef TARGET_I386 >> +#error Unsupported Architecture >> +#endif >> +#ifdef TARGET_X86_64 /* Unimplemented yet */ >> +#error Unsupported Architecture >> +#endif >> + >> +#if (WINDBG_DEBUG_ON) >> + >> +# define WINDBG_DEBUG(...) do { \ >> + printf("Debug: " __VA_ARGS__); \ >> + printf("\n"); \ >> +} while (false) >> + >> +# define WINDBG_ERROR(...) do { \ >> + printf("Error: " __VA_ARGS__); \ >> + printf("\n"); \ >> +} while (false) > > Use qemu_log() instead of printf(). > > Have a look as some other files for the usual way we handle debug > printing. > Thanks for your feedback. I'm replaced printf() with qemu_log(). You can find new version here: https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03912.html >> + >> +#else >> + >> +# define WINDBG_DEBUG(...) >> +# define WINDBG_ERROR(...) error_report(WINDBG ": " __VA_ARGS__) >> + >> +#endif >> + >> +#define FMT_ADDR "addr:0x" TARGET_FMT_lx >> +#define FMT_ERR "Error:%d" >> + >> +#define UINT8_P(ptr) ((uint8_t *) (ptr)) >> +#define UINT32_P(ptr) ((uint32_t *) (ptr)) >> +#define FIELD_P(type, field, ptr) ((typeof_field(type, field) *) >> (ptr)) >> +#define PTR(var) UINT8_P(&var) >> + >> +#define M64_SIZE sizeof(DBGKD_MANIPULATE_STATE64) >> + >> +#define sizeof_field(type, field) sizeof(((type *) NULL)->field) >> + >> +#define READ_VMEM(cpu, addr, type) ({ \ >> + type _t; \ >> + cpu_memory_rw_debug(cpu, addr, PTR(_t), sizeof(type), 0); \ >> + _t; \ >> +}) >> + >> #endif >> diff --git a/include/exec/windbgstub.h b/include/exec/windbgstub.h >> index 1a6e1cc6e5..703fc26b8f 100755 >> --- a/include/exec/windbgstub.h >> +++ b/include/exec/windbgstub.h >> @@ -12,6 +12,9 @@ >> #ifndef WINDBGSTUB_H >> #define WINDBGSTUB_H >> >> +#define WINDBG "windbg" >> +#define WINDBG_DEBUG_ON false > > You should have a check here to see if the user has already set > WINDBG_DEBUG_ON to allow people to set it during build time. > > Thanks, > Alistair > Yes, you're right. I done it. >> + >> int windbg_server_start(const char *device); >> >> #endif >> diff --git a/windbgstub.c b/windbgstub.c >> index 4951f59203..3830446988 100755 >> --- a/windbgstub.c >> +++ b/windbgstub.c >> @@ -11,6 +11,7 @@ >> >> #include "qemu/osdep.h" >> #include "exec/windbgstub.h" >> +#include "exec/windbgstub-utils.h" >> >> int windbg_server_start(const char *device) >> { >> >> -- Thanks, Mihail Abakumov
Eric Blake писал 2017-09-26 20:27: > On 09/26/2017 12:13 PM, Alistair Francis wrote: > >>> +#if (WINDBG_DEBUG_ON) >>> + >>> +# define WINDBG_DEBUG(...) do { \ >>> + printf("Debug: " __VA_ARGS__); \ >>> + printf("\n"); \ >>> +} while (false) >>> + >>> +# define WINDBG_ERROR(...) do { \ >>> + printf("Error: " __VA_ARGS__); \ >>> + printf("\n"); \ >>> +} while (false) >> >> Use qemu_log() instead of printf(). >> >> Have a look as some other files for the usual way we handle debug >> printing. >> >>> + >>> +#else >>> + >>> +# define WINDBG_DEBUG(...) >>> +# define WINDBG_ERROR(...) error_report(WINDBG ": " __VA_ARGS__) > > What's more - as written, your approach is prone to bit-rot: the > debug/error statements are not compared against -Werror except for the > rare person that enables debugging. Better is go make the macro > unconditionally expand to something that triggers -Wformat checking, > but > guarded by an if(0) for normal use. Or even switch to trace points > rather than debugging statements, so that you can control at runtime > how > much debugging information you want, rather than having to recompile to > turn it on and off. Thank you for your feedback. I corrected it like this #define WINDBG_DEBUG(...) do { \ if (WINDBG_DEBUG_ON) { \ qemu_log(WINDBG ": " __VA_ARGS__); \ qemu_log("\n"); \ } \ } while (false) You can find a new version here: https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03912.html -- Thanks, Mihail Abakumov
diff --git a/include/exec/windbgstub-utils.h b/include/exec/windbgstub-utils.h index 2390597f1f..65f336e4bf 100755 --- a/include/exec/windbgstub-utils.h +++ b/include/exec/windbgstub-utils.h @@ -13,7 +13,53 @@ #define WINDBGSTUB_UTILS_H #include "qemu/osdep.h" +#include "qemu/error-report.h" +#include "cpu.h" #include "exec/windbgstub.h" #include "exec/windbgkd.h" +#ifndef TARGET_I386 +#error Unsupported Architecture +#endif +#ifdef TARGET_X86_64 /* Unimplemented yet */ +#error Unsupported Architecture +#endif + +#if (WINDBG_DEBUG_ON) + +# define WINDBG_DEBUG(...) do { \ + printf("Debug: " __VA_ARGS__); \ + printf("\n"); \ +} while (false) + +# define WINDBG_ERROR(...) do { \ + printf("Error: " __VA_ARGS__); \ + printf("\n"); \ +} while (false) + +#else + +# define WINDBG_DEBUG(...) +# define WINDBG_ERROR(...) error_report(WINDBG ": " __VA_ARGS__) + +#endif + +#define FMT_ADDR "addr:0x" TARGET_FMT_lx +#define FMT_ERR "Error:%d" + +#define UINT8_P(ptr) ((uint8_t *) (ptr)) +#define UINT32_P(ptr) ((uint32_t *) (ptr)) +#define FIELD_P(type, field, ptr) ((typeof_field(type, field) *) (ptr)) +#define PTR(var) UINT8_P(&var) + +#define M64_SIZE sizeof(DBGKD_MANIPULATE_STATE64) + +#define sizeof_field(type, field) sizeof(((type *) NULL)->field) + +#define READ_VMEM(cpu, addr, type) ({ \ + type _t; \ + cpu_memory_rw_debug(cpu, addr, PTR(_t), sizeof(type), 0); \ + _t; \ +}) + #endif diff --git a/include/exec/windbgstub.h b/include/exec/windbgstub.h index 1a6e1cc6e5..703fc26b8f 100755 --- a/include/exec/windbgstub.h +++ b/include/exec/windbgstub.h @@ -12,6 +12,9 @@ #ifndef WINDBGSTUB_H #define WINDBGSTUB_H +#define WINDBG "windbg" +#define WINDBG_DEBUG_ON false + int windbg_server_start(const char *device); #endif diff --git a/windbgstub.c b/windbgstub.c index 4951f59203..3830446988 100755 --- a/windbgstub.c +++ b/windbgstub.c @@ -11,6 +11,7 @@ #include "qemu/osdep.h" #include "exec/windbgstub.h" +#include "exec/windbgstub-utils.h" int windbg_server_start(const char *device) {