sim: arm: add support for handling core dumps

Message ID AM8PR10MB465804161D9A886045785A32EF0A9@AM8PR10MB4658.EURPRD10.PROD.OUTLOOK.COM
State New
Headers show
Series
  • sim: arm: add support for handling core dumps
Related show

Commit Message

Simon Marchi via Gdb-patches June 21, 2021, 6:30 a.m.
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.

BR Fredrik

Comments

Simon Marchi via Gdb-patches June 22, 2021, 3:20 a.m. | #1
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 ?

> --- 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
Simon Marchi via Gdb-patches June 24, 2021, 1:01 p.m. | #2
> 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
Simon Marchi via Gdb-patches June 29, 2021, 9:11 a.m. | #3
I reviewed comments and decided to make a complete retake on this.
Here is a patch that adds IDs for D0-D31, then let any calling ARM simulator implementation handle the results.

I tested 'gcore' on target sim for ARM on this, and it worked well on my tests.
I also understood better why I got these 4GB corefiles, since I did not set SP (ARM simulator is not supporting Cortex),
so as SP was 0, then after some calls, the SP was like 0xFFFFFFxx, so stack was like 4GB up in memory space.
When I added code lines at start to set SP to a lower address, the corefile was getting smaller correct size.

BR Fredrik


diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 857a52a9a51..2593d89269f 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -4246,6 +4246,9 @@ 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;
 
+  if (regnum >= ARM_D0_REGNUM && regnum <= ARM_FPSCR_REGNUM)
+      return regnum - ARM_D0_REGNUM + SIM_ARM_D0_REGNUM;
+
   if (reg < NUM_GREGS)
     return SIM_ARM_R0_REGNUM + reg;
   reg -= NUM_GREGS;
diff --git a/include/gdb/sim-arm.h b/include/gdb/sim-arm.h
index 8aafa045a0e..28d336be525 100644
--- a/include/gdb/sim-arm.h
+++ b/include/gdb/sim-arm.h
@@ -98,7 +98,41 @@ enum sim_arm_regs
   SIM_ARM_IWMMXT_COP1R12_REGNUM,
   SIM_ARM_IWMMXT_COP1R13_REGNUM,
   SIM_ARM_IWMMXT_COP1R14_REGNUM,
-  SIM_ARM_IWMMXT_COP1R15_REGNUM
+  SIM_ARM_IWMMXT_COP1R15_REGNUM,
+  SIM_ARM_D0_REGNUM,
+  SIM_ARM_D1_REGNUM,
+  SIM_ARM_D2_REGNUM,
+  SIM_ARM_D3_REGNUM,
+  SIM_ARM_D4_REGNUM,
+  SIM_ARM_D5_REGNUM,
+  SIM_ARM_D6_REGNUM,
+  SIM_ARM_D7_REGNUM,
+  SIM_ARM_D8_REGNUM,
+  SIM_ARM_D9_REGNUM,
+  SIM_ARM_D10_REGNUM,
+  SIM_ARM_D11_REGNUM,
+  SIM_ARM_D12_REGNUM,
+  SIM_ARM_D13_REGNUM,
+  SIM_ARM_D14_REGNUM,
+  SIM_ARM_D15_REGNUM,
+  SIM_ARM_D16_REGNUM,
+  SIM_ARM_D17_REGNUM,
+  SIM_ARM_D18_REGNUM,
+  SIM_ARM_D19_REGNUM,
+  SIM_ARM_D20_REGNUM,
+  SIM_ARM_D21_REGNUM,
+  SIM_ARM_D22_REGNUM,
+  SIM_ARM_D23_REGNUM,
+  SIM_ARM_D24_REGNUM,
+  SIM_ARM_D25_REGNUM,
+  SIM_ARM_D26_REGNUM,
+  SIM_ARM_D27_REGNUM,
+  SIM_ARM_D28_REGNUM,
+  SIM_ARM_D29_REGNUM,
+  SIM_ARM_D30_REGNUM,
+  SIM_ARM_D31_REGNUM,
+  SIM_ARM_FPSCR_REGNUM,
+  SIM_ARM_NUM_REGS
 };
 
 #endif
diff --git a/sim/arm/wrapper.c b/sim/arm/wrapper.c
index e697d55a6b5..69c5905b1ff 100644
--- a/sim/arm/wrapper.c
+++ b/sim/arm/wrapper.c
@@ -418,6 +418,12 @@ arm_reg_store (SIM_CPU *cpu, int rn, unsigned char *memory, int length)
 {
   init ();
 
+  if (rn >= SIM_ARM_NUM_REGS)
+    {
+      sim_io_eprintf (CPU_STATE (cpu), "Invalid register %d (register store ignored)\n", rn);
+      return 0;
+    }
+
   switch ((enum sim_arm_regs) rn)
     {
     case SIM_ARM_R0_REGNUM:
@@ -509,7 +515,47 @@ arm_reg_store (SIM_CPU *cpu, int rn, unsigned char *memory, int length)
     case SIM_ARM_IWMMXT_COP1R13_REGNUM:
     case SIM_ARM_IWMMXT_COP1R14_REGNUM:
     case SIM_ARM_IWMMXT_COP1R15_REGNUM:
-      return Store_Iwmmxt_Register (rn - SIM_ARM_IWMMXT_COP0R0_REGNUM, memory);
+      if (state->is_iWMMXt)
+        {
+          return Store_Iwmmxt_Register (rn - SIM_ARM_IWMMXT_COP0R0_REGNUM, memory);
+        }
+      return 0;
+
+    case SIM_ARM_D0_REGNUM:
+    case SIM_ARM_D1_REGNUM:
+    case SIM_ARM_D2_REGNUM:
+    case SIM_ARM_D3_REGNUM:
+    case SIM_ARM_D4_REGNUM:
+    case SIM_ARM_D5_REGNUM:
+    case SIM_ARM_D6_REGNUM:
+    case SIM_ARM_D7_REGNUM:
+    case SIM_ARM_D8_REGNUM:
+    case SIM_ARM_D9_REGNUM:
+    case SIM_ARM_D10_REGNUM:
+    case SIM_ARM_D11_REGNUM:
+    case SIM_ARM_D12_REGNUM:
+    case SIM_ARM_D13_REGNUM:
+    case SIM_ARM_D14_REGNUM:
+    case SIM_ARM_D15_REGNUM:
+    case SIM_ARM_D16_REGNUM:
+    case SIM_ARM_D17_REGNUM:
+    case SIM_ARM_D18_REGNUM:
+    case SIM_ARM_D19_REGNUM:
+    case SIM_ARM_D20_REGNUM:
+    case SIM_ARM_D21_REGNUM:
+    case SIM_ARM_D22_REGNUM:
+    case SIM_ARM_D23_REGNUM:
+    case SIM_ARM_D24_REGNUM:
+    case SIM_ARM_D25_REGNUM:
+    case SIM_ARM_D26_REGNUM:
+    case SIM_ARM_D27_REGNUM:
+    case SIM_ARM_D28_REGNUM:
+    case SIM_ARM_D29_REGNUM:
+    case SIM_ARM_D30_REGNUM:
+    case SIM_ARM_D31_REGNUM:
+    case SIM_ARM_FPSCR_REGNUM:
+      /* The current GDB ARM simulator does not support D0-D31 nor FPSCR.  */
+      return 0;
 
     default:
       return 0;
@@ -526,6 +572,12 @@ arm_reg_fetch (SIM_CPU *cpu, int rn, unsigned char *memory, int length)
 
   init ();
 
+  if (rn >= SIM_ARM_NUM_REGS)
+    {
+      sim_io_eprintf (CPU_STATE (cpu), "Invalid register %d (register fetch ignored)\n", rn);
+      return 0;
+    }
+
   switch ((enum sim_arm_regs) rn)
     {
     case SIM_ARM_R0_REGNUM:
@@ -619,7 +671,47 @@ arm_reg_fetch (SIM_CPU *cpu, int rn, unsigned char *memory, int length)
     case SIM_ARM_IWMMXT_COP1R13_REGNUM:
     case SIM_ARM_IWMMXT_COP1R14_REGNUM:
     case SIM_ARM_IWMMXT_COP1R15_REGNUM:
-      return Fetch_Iwmmxt_Register (rn - SIM_ARM_IWMMXT_COP0R0_REGNUM, memory);
+      if (state->is_iWMMXt)
+        {
+          return Fetch_Iwmmxt_Register (rn - SIM_ARM_IWMMXT_COP0R0_REGNUM, memory);
+        }
+      return 0;
+
+    case SIM_ARM_D0_REGNUM:
+    case SIM_ARM_D1_REGNUM:
+    case SIM_ARM_D2_REGNUM:
+    case SIM_ARM_D3_REGNUM:
+    case SIM_ARM_D4_REGNUM:
+    case SIM_ARM_D5_REGNUM:
+    case SIM_ARM_D6_REGNUM:
+    case SIM_ARM_D7_REGNUM:
+    case SIM_ARM_D8_REGNUM:
+    case SIM_ARM_D9_REGNUM:
+    case SIM_ARM_D10_REGNUM:
+    case SIM_ARM_D11_REGNUM:
+    case SIM_ARM_D12_REGNUM:
+    case SIM_ARM_D13_REGNUM:
+    case SIM_ARM_D14_REGNUM:
+    case SIM_ARM_D15_REGNUM:
+    case SIM_ARM_D16_REGNUM:
+    case SIM_ARM_D17_REGNUM:
+    case SIM_ARM_D18_REGNUM:
+    case SIM_ARM_D19_REGNUM:
+    case SIM_ARM_D20_REGNUM:
+    case SIM_ARM_D21_REGNUM:
+    case SIM_ARM_D22_REGNUM:
+    case SIM_ARM_D23_REGNUM:
+    case SIM_ARM_D24_REGNUM:
+    case SIM_ARM_D25_REGNUM:
+    case SIM_ARM_D26_REGNUM:
+    case SIM_ARM_D27_REGNUM:
+    case SIM_ARM_D28_REGNUM:
+    case SIM_ARM_D29_REGNUM:
+    case SIM_ARM_D30_REGNUM:
+    case SIM_ARM_D31_REGNUM:
+    case SIM_ARM_FPSCR_REGNUM:
+      /* The current GDB ARM simulator does not support D0-D31 nor FPSCR.  */
+      return 0;
 
     default:
       return 0;

Patch

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 857a52a9a51..2820e456cf7 100644
--- 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;
diff --git a/sim/arm/wrapper.c b/sim/arm/wrapper.c
index e697d55a6b5..2a8a539b939 100644
--- 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;
+
   switch ((enum sim_arm_regs) rn)
     {
     case SIM_ARM_R0_REGNUM: