Re: [PATCH] sim: arm: add support for handling core dumps

[Alan Hayward via Gdb-patches <gdb-patches@sourceware.org>]

> On 22 Jun 2021, at 04:20, Mike Frysinger via Gdb-patches <gdb-patches@sourceware.org> wrote:
> 
> On 21 Jun 2021 06:30, Fredrik Hederstierna via Gdb-patches wrote:
>> The ARM simulator does not support all registers, and also crash in case of entering zero length when fetching registers.
>> This patch allow gcore to also work in ARM simulator, though by default it seems to dump full memory space 4GB, so core files get very large, though seems to work ok anyhow.
> 
> dumping the entire address space doesn't sound unreasonable for bare metal env.
> you have an ELF which will tell you the memory regions it occupies directly,
> but there's no API i'm aware of to communicate things like stack & heap.
> 
> that said, the arm sim has some non-ideal behavior.  it allocates 2MiB by
> default, but after that, access to anywhere in the 4GiB address space will
> automatically allocate a page if one hasn't yet.  if someone gets around to
> gutting arm's custom memory implementation and switching it to the common
> sim memory core, this would get fixed in the process ...
> 
>> --- a/gdb/arm-tdep.c
>> +++ b/gdb/arm-tdep.c
>> @@ -4246,6 +4246,10 @@ arm_register_sim_regno (struct gdbarch *gdbarch, int regnum)
>>   if (regnum >= ARM_WCGR0_REGNUM && regnum <= ARM_WCGR7_REGNUM)
>>     return regnum - ARM_WCGR0_REGNUM + SIM_ARM_IWMMXT_COP1R8_REGNUM;
>> 
>> +  /* The current GDB ARM simulator does not support D0-D31 nor FPSCR.  */
>> +  if (regnum >= ARM_D0_REGNUM && regnum <= ARM_FPSCR_REGNUM)
>> +    return -1;
>> +
>>   if (reg < NUM_GREGS)
>>     return SIM_ARM_R0_REGNUM + reg;
>>   reg -= NUM_GREGS;
> 
> shouldn't this check be at the end of arm_register_sim_regno instead of
> calling internal error ?

I’m not familiar with the sim, so Im going to assume the comment statement is correct.

With the added code, it means the "if (reg < NUM_FREGS)” and "if (reg < NUM_SREGS)” statements can never be hit.
So, it’s probably worth removing those.

Agree with Mike’s comment - the internal_error at the end of the function needs dropping too. The caller handles the bad value.


>> --- a/sim/arm/wrapper.c
>> +++ b/sim/arm/wrapper.c
>> @@ -526,6 +526,10 @@ arm_reg_fetch (SIM_CPU *cpu, int rn, unsigned char *memory, int length)
>> 
>>   init ();
>> 
>> +  /* Check that memory and length are valid before fetching the register.  */
>> +  if ((memory == NULL) || (length == 0))
>> +    return 0;
> 
> why is the caller passing NULL to the sim ?
> -mike