[Qemu-devel] [PATCH 1/2] Improve Monitor disas with symbol lookup

[Qemu-devel] [PATCH 1/2] Improve Monitor disas with symbol lookup

[Fabien Chouteau]

Part of M731-018.

Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
 disas.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/disas.c b/disas.c
index 71007fb..3ffb3ae 100644
--- a/disas.c
+++ b/disas.c
@@ -480,11 +480,19 @@ 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)
-	    break;
+        monitor_printf(mon, "\n");
+        if (count < 0) {
+            break;
+        }
         pc += count;
     }
 }
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/2] Add ARM registers definitions in Monitor commands

[Fabien Chouteau]

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 5dc0aa9..78e93af 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3167,6 +3167,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

Re: [Qemu-devel] [PATCH 2/2] Add ARM registers definitions in Monitor commands

[Fabien Chouteau]

Any comments?

Regards,

On 08/02/2013 02:48 PM, Fabien Chouteau wrote:
> 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 5dc0aa9..78e93af 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3167,6 +3167,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 },
>  };
> 


-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH 1/2] Improve Monitor disas with symbol lookup

[Fabien Chouteau]

Any comments?

Regards,

On 08/02/2013 02:48 PM, Fabien Chouteau wrote:
> Part of M731-018.
> 
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> ---
>  disas.c |   16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/disas.c b/disas.c
> index 71007fb..3ffb3ae 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -480,11 +480,19 @@ 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)
> -	    break;
> +        monitor_printf(mon, "\n");
> +        if (count < 0) {
> +            break;
> +        }
>          pc += count;
>      }
>  }
> 


-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH 2/2] Add ARM registers definitions in Monitor commands

[Peter Maydell]

On 19 August 2013 09:26, Fabien Chouteau <chouteau@adacore.com> wrote:
> Any comments?
>
> Regards,
>
> On 08/02/2013 02:48 PM, Fabien Chouteau wrote:
>> 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 5dc0aa9..78e93af 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -3167,6 +3167,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])  },

Rather than adding yet another entry to this target-ifdef
ladder in common code, maybe we can abstract this out to be
a method/field of the CPU object?

Andreas can probably suggest the best approach.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] Improve Monitor disas with symbol lookup

[Peter Maydell]

On 2 August 2013 13:48, Fabien Chouteau <chouteau@adacore.com> wrote:
> Part of M731-018.

What is this a reference to?

> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> ---
>  disas.c |   16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/disas.c b/disas.c
> index 71007fb..3ffb3ae 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -480,11 +480,19 @@ 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, ":  ");
> +        }

It feels to me like this is at the wrong level:
shouldn't it be in the disassembly layer so that you
can get symbols in both monitor disassembly and
debug-log disassembly?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] Improve Monitor disas with symbol lookup

[Fabien Chouteau]

On 08/19/2013 10:33 AM, Peter Maydell wrote:
> On 2 August 2013 13:48, Fabien Chouteau <chouteau@adacore.com> wrote:
>> Part of M731-018.
> 
> What is this a reference to?
> 

I'm sorry this is not supposed to be here. Do you have comments before I
resend the patch?

Regards,

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH 1/2] Improve Monitor disas with symbol lookup

[Andreas Färber]

Am 02.08.2013 14:48, schrieb Fabien Chouteau:
> Part of M731-018.
> 
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> ---
>  disas.c |   16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/disas.c b/disas.c
> index 71007fb..3ffb3ae 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -480,11 +480,19 @@ 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, ":  ");
> +        }

Independent of PMM's comment, I think you meant

+        monitor_printf(mon, "0x" TARGET_FMT_lx, pc);
+        if (sym[0] != '\0') {
+            monitor_printf(mon, " <%s>:  ", sym);
+        } else {
+            monitor_printf(mon, ":  ");
+        }

to keep the output unchanged.

Could you please put the tab fixing into a preceding patch for
readability and prepend a cover letter?

Regards,
Andreas

>          count = print_insn(pc, &s.info);
> -	monitor_printf(mon, "\n");
> -	if (count < 0)
> -	    break;
> +        monitor_printf(mon, "\n");
> +        if (count < 0) {
> +            break;
> +        }
>          pc += count;
>      }
>  }
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/2] Add ARM registers definitions in Monitor commands

[Andreas Färber]

Am 19.08.2013 10:31, schrieb Peter Maydell:
> On 19 August 2013 09:26, Fabien Chouteau <chouteau@adacore.com> wrote:
>> Any comments?
>>
>> Regards,
>>
>> On 08/02/2013 02:48 PM, Fabien Chouteau wrote:
>>> 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 5dc0aa9..78e93af 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -3167,6 +3167,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])  },
> 
> Rather than adding yet another entry to this target-ifdef
> ladder in common code, maybe we can abstract this out to be
> a method/field of the CPU object?
> 
> Andreas can probably suggest the best approach.

I had similar thoughts when I first saw this patch... I haven't looked
closely into monitor yet though (other than seeing it has too much
target dependencies).

Either a MonitorDef* field (null-terminating the array then) or,
depending on where/how this is used, a method in CPUClass might be
options to better abstract this. Given that there is going to be a
little bit of time before 1.7 now we could take some time to rethink our
design.

A related question that remained unanswered for the Program Counter in
the gdbstub context was whether we want QOM properties for such
registers, which would hint at further abstracting such a register list
to reuse it outside monitor.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/2] Improve Monitor disas with symbol lookup

[Fabien Chouteau]

On 08/19/2013 04:14 PM, Andreas Färber wrote:
> Am 02.08.2013 14:48, schrieb Fabien Chouteau:
>> Part of M731-018.
>>
>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>> ---
>>  disas.c |   16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/disas.c b/disas.c
>> index 71007fb..3ffb3ae 100644
>> --- a/disas.c
>> +++ b/disas.c
>> @@ -480,11 +480,19 @@ 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, ":  ");
>> +        }
> 
> Independent of PMM's comment, I think you meant
> 
> +        monitor_printf(mon, "0x" TARGET_FMT_lx, pc);
> +        if (sym[0] != '\0') {
> +            monitor_printf(mon, " <%s>:  ", sym);
> +        } else {
> +            monitor_printf(mon, ":  ");
> +        }
> 
> to keep the output unchanged.
> 
> Could you please put the tab fixing into a preceding patch for
> readability and prepend a cover letter?


OK, will do.

-- 
Fabien Chouteau