* [Qemu-devel] [PATCH 0/3] Few minor improvements of monitor disas command (v2)
@ 2013-09-24 16:45 Fabien Chouteau
2013-09-24 16:45 ` [Qemu-devel] [PATCH 1/3] Fix coding style Fabien Chouteau
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Fabien Chouteau @ 2013-09-24 16:45 UTC (permalink / raw)
To: qemu-trivial; +Cc: peter.maydell, qemu-devel, afaerber, lcapitulino
1. Fix coding style (missing space and tab indentation)
2. Add symbole lookup (when possible)
3. Add ARM registers definitions
I tried to abstract monitor_defs in a CPU field, but I got lost in the
dependencies maze. I still think this patch is a step forward.
Fabien Chouteau (3):
Fix coding style
Improve Monitor disas with symbol lookup
Add ARM registers definitions in Monitor commands
disas.c | 19 ++++++++++++++-----
monitor.c | 17 +++++++++++++++++
2 files changed, 31 insertions(+), 5 deletions(-)
--
1.7.9.5
^ permalink raw reply [flat|nested] 12+ messages in thread* [Qemu-devel] [PATCH 1/3] Fix coding style 2013-09-24 16:45 [Qemu-devel] [PATCH 0/3] Few minor improvements of monitor disas command (v2) Fabien Chouteau @ 2013-09-24 16:45 ` Fabien Chouteau 2013-09-24 17:33 ` Andreas Färber 2013-09-24 16:46 ` [Qemu-devel] [PATCH 2/3] Improve Monitor disas with symbol lookup Fabien Chouteau 2013-09-24 16:46 ` [Qemu-devel] [PATCH 3/3] Add ARM registers definitions in Monitor commands Fabien Chouteau 2 siblings, 1 reply; 12+ messages in thread From: Fabien Chouteau @ 2013-09-24 16:45 UTC (permalink / raw) To: qemu-trivial; +Cc: peter.maydell, qemu-devel, afaerber, lcapitulino Signed-off-by: Fabien Chouteau <chouteau@adacore.com> --- disas.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/disas.c b/disas.c index 0203ef2..32407de 100644 --- a/disas.c +++ b/disas.c @@ -506,12 +506,13 @@ void monitor_disas(Monitor *mon, CPUArchState *env, return; #endif - for(i = 0; i < nb_insn; i++) { - monitor_printf(mon, "0x" TARGET_FMT_lx ": ", pc); + for (i = 0; i < nb_insn; i++) { + monitor_printf(mon, "0x" TARGET_FMT_lx ": ", pc); count = print_insn(pc, &s.info); - monitor_printf(mon, "\n"); - if (count < 0) - break; + monitor_printf(mon, "\n"); + if (count < 0) { + break; + } pc += count; } } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] Fix coding style 2013-09-24 16:45 ` [Qemu-devel] [PATCH 1/3] Fix coding style Fabien Chouteau @ 2013-09-24 17:33 ` Andreas Färber 0 siblings, 0 replies; 12+ messages in thread From: Andreas Färber @ 2013-09-24 17:33 UTC (permalink / raw) To: Fabien Chouteau; +Cc: qemu-trivial, peter.maydell, qemu-devel, lcapitulino Am 24.09.2013 18:45, schrieb Fabien Chouteau: > Signed-off-by: Fabien Chouteau <chouteau@adacore.com> > --- > disas.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) Reviewed-by: Andreas Färber <afaerber@suse.de> Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 2/3] Improve Monitor disas with symbol lookup 2013-09-24 16:45 [Qemu-devel] [PATCH 0/3] Few minor improvements of monitor disas command (v2) Fabien Chouteau 2013-09-24 16:45 ` [Qemu-devel] [PATCH 1/3] Fix coding style Fabien Chouteau @ 2013-09-24 16:46 ` Fabien Chouteau 2013-09-24 16:46 ` [Qemu-devel] [PATCH 3/3] Add ARM registers definitions in Monitor commands Fabien Chouteau 2 siblings, 0 replies; 12+ messages in thread From: Fabien Chouteau @ 2013-09-24 16:46 UTC (permalink / raw) To: qemu-trivial; +Cc: peter.maydell, qemu-devel, afaerber, lcapitulino Signed-off-by: Fabien Chouteau <chouteau@adacore.com> --- disas.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/disas.c b/disas.c index 32407de..c83bf5b 100644 --- a/disas.c +++ b/disas.c @@ -507,7 +507,15 @@ void monitor_disas(Monitor *mon, CPUArchState *env, #endif for (i = 0; i < nb_insn; i++) { - monitor_printf(mon, "0x" TARGET_FMT_lx ": ", pc); + const char *sym = lookup_symbol(pc); + + monitor_printf(mon, "0x" TARGET_FMT_lx, pc); + if (sym[0] != '\0') { + monitor_printf(mon, " <%s>: ", sym); + } else { + monitor_printf(mon, ": "); + } + count = print_insn(pc, &s.info); monitor_printf(mon, "\n"); if (count < 0) { -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 3/3] Add ARM registers definitions in Monitor commands 2013-09-24 16:45 [Qemu-devel] [PATCH 0/3] Few minor improvements of monitor disas command (v2) Fabien Chouteau 2013-09-24 16:45 ` [Qemu-devel] [PATCH 1/3] Fix coding style Fabien Chouteau 2013-09-24 16:46 ` [Qemu-devel] [PATCH 2/3] Improve Monitor disas with symbol lookup Fabien Chouteau @ 2013-09-24 16:46 ` Fabien Chouteau 2013-09-24 23:53 ` Peter Maydell 2 siblings, 1 reply; 12+ messages in thread From: Fabien Chouteau @ 2013-09-24 16:46 UTC (permalink / raw) To: qemu-trivial; +Cc: peter.maydell, qemu-devel, afaerber, lcapitulino Signed-off-by: Fabien Chouteau <chouteau@adacore.com> --- monitor.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/monitor.c b/monitor.c index 74f3f1b..e40c20d 100644 --- a/monitor.c +++ b/monitor.c @@ -3351,6 +3351,23 @@ static const MonitorDef monitor_defs[] = { { "cleanwin", offsetof(CPUSPARCState, cleanwin) }, { "fprs", offsetof(CPUSPARCState, fprs) }, #endif +#elif defined(TARGET_ARM) + { "r0", offsetof(CPUARMState, regs[0]) }, + { "r1", offsetof(CPUARMState, regs[1]) }, + { "r2", offsetof(CPUARMState, regs[2]) }, + { "r3", offsetof(CPUARMState, regs[3]) }, + { "r4", offsetof(CPUARMState, regs[4]) }, + { "r5", offsetof(CPUARMState, regs[5]) }, + { "r6", offsetof(CPUARMState, regs[6]) }, + { "r7", offsetof(CPUARMState, regs[7]) }, + { "r8", offsetof(CPUARMState, regs[8]) }, + { "r9", offsetof(CPUARMState, regs[9]) }, + { "r10", offsetof(CPUARMState, regs[10]) }, + { "r11", offsetof(CPUARMState, regs[11]) }, + { "r12", offsetof(CPUARMState, regs[12]) }, + { "r13|sp", offsetof(CPUARMState, regs[13]) }, + { "r14|lr", offsetof(CPUARMState, regs[14]) }, + { "r15|pc", offsetof(CPUARMState, regs[15]) }, #endif { NULL }, }; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] Add ARM registers definitions in Monitor commands 2013-09-24 16:46 ` [Qemu-devel] [PATCH 3/3] Add ARM registers definitions in Monitor commands Fabien Chouteau @ 2013-09-24 23:53 ` Peter Maydell 2013-09-25 15:38 ` Fabien Chouteau 0 siblings, 1 reply; 12+ messages in thread From: Peter Maydell @ 2013-09-24 23:53 UTC (permalink / raw) To: Fabien Chouteau Cc: QEMU Trivial, QEMU Developers, Andreas Färber, Luiz Capitulino On 25 September 2013 01:46, Fabien Chouteau <chouteau@adacore.com> wrote: > --- a/monitor.c > +++ b/monitor.c > @@ -3351,6 +3351,23 @@ static const MonitorDef monitor_defs[] = { > { "cleanwin", offsetof(CPUSPARCState, cleanwin) }, > { "fprs", offsetof(CPUSPARCState, fprs) }, > #endif > +#elif defined(TARGET_ARM) > + { "r0", offsetof(CPUARMState, regs[0]) }, > + { "r1", offsetof(CPUARMState, regs[1]) }, > + { "r2", offsetof(CPUARMState, regs[2]) }, > + { "r3", offsetof(CPUARMState, regs[3]) }, > + { "r4", offsetof(CPUARMState, regs[4]) }, > + { "r5", offsetof(CPUARMState, regs[5]) }, > + { "r6", offsetof(CPUARMState, regs[6]) }, > + { "r7", offsetof(CPUARMState, regs[7]) }, > + { "r8", offsetof(CPUARMState, regs[8]) }, > + { "r9", offsetof(CPUARMState, regs[9]) }, > + { "r10", offsetof(CPUARMState, regs[10]) }, > + { "r11", offsetof(CPUARMState, regs[11]) }, > + { "r12", offsetof(CPUARMState, regs[12]) }, > + { "r13|sp", offsetof(CPUARMState, regs[13]) }, > + { "r14|lr", offsetof(CPUARMState, regs[14]) }, > + { "r15|pc", offsetof(CPUARMState, regs[15]) }, > #endif No, I really don't want to see another target #ifdef ladder, please. Put a 'static const MonitorDef *monitor_defs;' into CPUClass, and initialize it in each target's class init function, please. (You'll need to move the appropriate sections of the current static array in monitor.c plus the per-target functions that it references into target-*/cpu.c.) Look at gdb_num_core_regs as an example of where we made this kind of abstraction. -- PMM ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] Add ARM registers definitions in Monitor commands 2013-09-24 23:53 ` Peter Maydell @ 2013-09-25 15:38 ` Fabien Chouteau 2013-09-25 15:51 ` Peter Maydell 0 siblings, 1 reply; 12+ messages in thread From: Fabien Chouteau @ 2013-09-25 15:38 UTC (permalink / raw) To: Peter Maydell Cc: QEMU Trivial, QEMU Developers, Andreas Färber, Luiz Capitulino On 09/25/2013 01:53 AM, Peter Maydell wrote: > > No, I really don't want to see another target #ifdef ladder, please. > Put a 'static const MonitorDef *monitor_defs;' into CPUClass, > and initialize it in each target's class init function, please. > (You'll need to move the appropriate sections of the current > static array in monitor.c plus the per-target functions that > it references into target-*/cpu.c.) Look at gdb_num_core_regs > as an example of where we made this kind of abstraction. > I tried already. Where whould you put the declaration of MonitorDef type? -- Fabien Chouteau ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] Add ARM registers definitions in Monitor commands 2013-09-25 15:38 ` Fabien Chouteau @ 2013-09-25 15:51 ` Peter Maydell 2013-09-25 16:29 ` Fabien Chouteau 0 siblings, 1 reply; 12+ messages in thread From: Peter Maydell @ 2013-09-25 15:51 UTC (permalink / raw) To: Fabien Chouteau Cc: QEMU Trivial, QEMU Developers, Andreas Färber, Luiz Capitulino On 26 September 2013 00:38, Fabien Chouteau <chouteau@adacore.com> wrote: > On 09/25/2013 01:53 AM, Peter Maydell wrote: >> >> No, I really don't want to see another target #ifdef ladder, please. >> Put a 'static const MonitorDef *monitor_defs;' into CPUClass, >> and initialize it in each target's class init function, please. >> (You'll need to move the appropriate sections of the current >> static array in monitor.c plus the per-target functions that >> it references into target-*/cpu.c.) Look at gdb_num_core_regs >> as an example of where we made this kind of abstraction. >> > > I tried already. Where whould you put the declaration of MonitorDef type? It doesn't matter very much, but monitor.h seems the obvious place. You probably don't want qom/cpu.h to have to drag in monitor.h so a 'struct MonitorDef;' forward declaration in cpu.h will let you avoid that (we do that already for a few other structs). -- PMM ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] Add ARM registers definitions in Monitor commands 2013-09-25 15:51 ` Peter Maydell @ 2013-09-25 16:29 ` Fabien Chouteau 2013-09-26 0:05 ` Peter Maydell 0 siblings, 1 reply; 12+ messages in thread From: Fabien Chouteau @ 2013-09-25 16:29 UTC (permalink / raw) To: Peter Maydell Cc: QEMU Trivial, QEMU Developers, Andreas Färber, Luiz Capitulino On 09/25/2013 05:51 PM, Peter Maydell wrote: > On 26 September 2013 00:38, Fabien Chouteau <chouteau@adacore.com> wrote: >> On 09/25/2013 01:53 AM, Peter Maydell wrote: >>> >>> No, I really don't want to see another target #ifdef ladder, please. >>> Put a 'static const MonitorDef *monitor_defs;' into CPUClass, >>> and initialize it in each target's class init function, please. >>> (You'll need to move the appropriate sections of the current >>> static array in monitor.c plus the per-target functions that >>> it references into target-*/cpu.c.) Look at gdb_num_core_regs >>> as an example of where we made this kind of abstraction. >>> >> >> I tried already. Where whould you put the declaration of MonitorDef type? > > It doesn't matter very much, but monitor.h seems the obvious > place. You probably don't want qom/cpu.h to have to drag in > monitor.h so a 'struct MonitorDef;' forward declaration in cpu.h > will let you avoid that (we do that already for a few other structs). > I think that's what I did. I think the problem was to include 'monitor.h' in 'target-*/cpu.c'. -- Fabien Chouteau ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] Add ARM registers definitions in Monitor commands 2013-09-25 16:29 ` Fabien Chouteau @ 2013-09-26 0:05 ` Peter Maydell 2013-09-26 14:50 ` Fabien Chouteau 0 siblings, 1 reply; 12+ messages in thread From: Peter Maydell @ 2013-09-26 0:05 UTC (permalink / raw) To: Fabien Chouteau Cc: QEMU Trivial, QEMU Developers, Andreas Färber, Luiz Capitulino On 26 September 2013 01:29, Fabien Chouteau <chouteau@adacore.com> wrote: > On 09/25/2013 05:51 PM, Peter Maydell wrote: >> On 26 September 2013 00:38, Fabien Chouteau <chouteau@adacore.com> wrote: >> It doesn't matter very much, but monitor.h seems the obvious >> place. You probably don't want qom/cpu.h to have to drag in >> monitor.h so a 'struct MonitorDef;' forward declaration in cpu.h >> will let you avoid that (we do that already for a few other structs). > > I think that's what I did. I think the problem was to include > 'monitor.h' in 'target-*/cpu.c'. Why doesn't that work? In any case, you just need to disentangle stuff so you can get the right definitions and function prototypes available to the target specific code. We could have a target-*/monitor.c if that's an easier approach -- we already have a gdbstub.c, for example. -- PMM ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] Add ARM registers definitions in Monitor commands 2013-09-26 0:05 ` Peter Maydell @ 2013-09-26 14:50 ` Fabien Chouteau 2013-09-26 15:28 ` Peter Maydell 0 siblings, 1 reply; 12+ messages in thread From: Fabien Chouteau @ 2013-09-26 14:50 UTC (permalink / raw) To: Peter Maydell Cc: QEMU Trivial, Luiz Capitulino, QEMU Developers, Andreas Färber On 09/26/2013 02:05 AM, Peter Maydell wrote: > On 26 September 2013 01:29, Fabien Chouteau <chouteau@adacore.com> wrote: >> On 09/25/2013 05:51 PM, Peter Maydell wrote: >>> On 26 September 2013 00:38, Fabien Chouteau <chouteau@adacore.com> wrote: >>> It doesn't matter very much, but monitor.h seems the obvious >>> place. You probably don't want qom/cpu.h to have to drag in >>> monitor.h so a 'struct MonitorDef;' forward declaration in cpu.h >>> will let you avoid that (we do that already for a few other structs). >> >> I think that's what I did. I think the problem was to include >> 'monitor.h' in 'target-*/cpu.c'. > > Why doesn't that work? > The problem is use of 'target_long' in 'monitor.h'. -- Fabien Chouteau ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] Add ARM registers definitions in Monitor commands 2013-09-26 14:50 ` Fabien Chouteau @ 2013-09-26 15:28 ` Peter Maydell 0 siblings, 0 replies; 12+ messages in thread From: Peter Maydell @ 2013-09-26 15:28 UTC (permalink / raw) To: Fabien Chouteau Cc: QEMU Trivial, Luiz Capitulino, QEMU Developers, Andreas Färber On 26 September 2013 23:50, Fabien Chouteau <chouteau@adacore.com> wrote: > On 09/26/2013 02:05 AM, Peter Maydell wrote: >> On 26 September 2013 01:29, Fabien Chouteau <chouteau@adacore.com> wrote: >>> I think that's what I did. I think the problem was to include >>> 'monitor.h' in 'target-*/cpu.c'. >> >> Why doesn't that work? > > The problem is use of 'target_long' in 'monitor.h'. Oh, right, the problem isn't including monitor.h from cpu.c, it's that some target-independent source files include monitor.h so you can't put target-dependent types like target_long in it. There are two fixes for this that spring to mind: (1) lazy approach, wrap the MonitorDef structure definition in #ifdef NEED_CPU_H/#endif. (2) the "remove target-specificisms from what should be generic code" approach: * make MonitorDef use uint64_t rather than target_long for the getter function return type * propagate that type change into functions like get_monitor_def and its callsite in expr_unary * make the types recognized by get_monitor_def be MD_I32 or MD_I64, and not MD_TLONG * make the per-target MonitorDef array entries which currently implicitly use MD_TLONG instead either (a) use MD_I32 or MD_I64 if they're targets which really only have one width or (b) use a locally #defined MD_TLONG if they're accessing CPU struct fields which really are target_long and the CPU comes in both 32 and 64 bit variants. -- PMM ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-09-26 15:28 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-24 16:45 [Qemu-devel] [PATCH 0/3] Few minor improvements of monitor disas command (v2) Fabien Chouteau 2013-09-24 16:45 ` [Qemu-devel] [PATCH 1/3] Fix coding style Fabien Chouteau 2013-09-24 17:33 ` Andreas Färber 2013-09-24 16:46 ` [Qemu-devel] [PATCH 2/3] Improve Monitor disas with symbol lookup Fabien Chouteau 2013-09-24 16:46 ` [Qemu-devel] [PATCH 3/3] Add ARM registers definitions in Monitor commands Fabien Chouteau 2013-09-24 23:53 ` Peter Maydell 2013-09-25 15:38 ` Fabien Chouteau 2013-09-25 15:51 ` Peter Maydell 2013-09-25 16:29 ` Fabien Chouteau 2013-09-26 0:05 ` Peter Maydell 2013-09-26 14:50 ` Fabien Chouteau 2013-09-26 15:28 ` Peter Maydell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).