[3/3] Add ARM registers definitions in Monitor commands

Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
 monitor.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

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

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?

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

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'.

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

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'.

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