[PATCH 0/3] xtensa: support call0 ABI native linux gdb and gdbserver

[PATCH 0/3] xtensa: support call0 ABI native linux gdb and gdbserver

[Max Filippov]

Hello,

this series adds call0 ABI support to native xtensa-linux gdb and gdbserver
ports.

Max Filippov (3):
  gdb: xtensa: initialize isa in call0_ret
  gdb: xtensa-linux: add call0 support
  gdbserver: xtensa: add call0 support

 gdb/gdbserver/linux-xtensa-low.c | 26 ++++++++++++++++++++++++++
 gdb/xtensa-linux-nat.c           | 30 ++++++++++++++++++++++++++++++
 gdb/xtensa-tdep.c                |  2 ++
 3 files changed, 58 insertions(+)

-- 
2.1.4

[PATCH 3/3] gdbserver: xtensa: add call0 support

[Max Filippov]

Correctly handle a0- registers on requests from remote gdb. This fixes

  'Register 1 is not available'

and subsequent assertion in the remote gdb connecting to the gdbserver:

  'findvar.c:291: internal-error: value_of_register_lazy:
    Assertion `frame_id_p(get_frame_id (frame))' failed.'

The register structure is the same for windowed and call0 ABIs because
currently linux kernel internally requires windowed registers, so they
are always present.

2017-01-18  Max Filippov  <jcmvbkbc@gmail.com>
gdb/gdbserver/
	* linux-xtensa-low.c (C0_NREGS): New definition.
	(xtensa_fill_gregset): Call collect_register for all registers in
	a0_regnum..a0_regnum + C0_NREGS range.
	(xtensa_store_gregset): Call supply_register for all registers in
	a0_regnum..a0_regnum + C0_NREGS range.
---
 gdb/gdbserver/linux-xtensa-low.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/gdb/gdbserver/linux-xtensa-low.c b/gdb/gdbserver/linux-xtensa-low.c
index 98c0bf2..73fbfe2 100644
--- a/gdb/gdbserver/linux-xtensa-low.c
+++ b/gdb/gdbserver/linux-xtensa-low.c
@@ -39,12 +39,15 @@ enum regnum {
 	R_A0 = 64
 };
 
+#define C0_NREGS 16
+
 static void
 xtensa_fill_gregset (struct regcache *regcache, void *buf)
 {
   elf_greg_t* rset = (elf_greg_t*)buf;
   const struct target_desc *tdesc = regcache->tdesc;
   int ar0_regnum;
+  int a0_regnum;
   char *ptr;
   int i;
 
@@ -72,6 +75,17 @@ xtensa_fill_gregset (struct regcache *regcache, void *buf)
   collect_register_by_name (regcache, "ps", (char*)&rset[R_PS]);
   collect_register_by_name (regcache, "windowbase", (char*)&rset[R_WB]);
   collect_register_by_name (regcache, "windowstart", (char*)&rset[R_WS]);
+
+  a0_regnum = find_regno (tdesc, "a0");
+  ptr = (char *)&rset[R_A0 + 4 * rset[R_WB]];
+
+  for (i = a0_regnum; i < a0_regnum + C0_NREGS; i++)
+    {
+      if (4 * rset[R_WB] + i - a0_regnum == XCHAL_NUM_AREGS)
+	ptr = (char *)&rset[R_A0];
+      collect_register (regcache, i, ptr);
+      ptr += register_size (tdesc, i);
+    }
 }
 
 static void
@@ -80,6 +94,7 @@ xtensa_store_gregset (struct regcache *regcache, const void *buf)
   const elf_greg_t* rset = (const elf_greg_t*)buf;
   const struct target_desc *tdesc = regcache->tdesc;
   int ar0_regnum;
+  int a0_regnum;
   char *ptr;
   int i;
 
@@ -94,6 +109,17 @@ xtensa_store_gregset (struct regcache *regcache, const void *buf)
       ptr += register_size (tdesc, i);
     }
 
+  a0_regnum = find_regno (tdesc, "a0");
+  ptr = (char *)&rset[R_A0 + (4 * rset[R_WB]) % XCHAL_NUM_AREGS];
+
+  for (i = a0_regnum; i < a0_regnum + C0_NREGS; i++)
+    {
+      if (4 * rset[R_WB] + i - a0_regnum == XCHAL_NUM_AREGS)
+	ptr = (char *)&rset[R_A0];
+      supply_register (regcache, i, ptr);
+      ptr += register_size (tdesc, i);
+    }
+
   /* Loop registers, if hardware has it.  */
 
 #if XCHAL_HAVE_LOOPS
-- 
2.1.4

Re: [PATCH 3/3] gdbserver: xtensa: add call0 support

[Luis Machado]

On 01/18/2017 05:50 PM, Max Filippov wrote:
> Correctly handle a0- registers on requests from remote gdb. This fixes
>
>   'Register 1 is not available'
>
> and subsequent assertion in the remote gdb connecting to the gdbserver:
>
>   'findvar.c:291: internal-error: value_of_register_lazy:
>     Assertion `frame_id_p(get_frame_id (frame))' failed.'
>
> The register structure is the same for windowed and call0 ABIs because
> currently linux kernel internally requires windowed registers, so they
> are always present.
>
> 2017-01-18  Max Filippov  <jcmvbkbc@gmail.com>
> gdb/gdbserver/

Drop gdb/gdbserver/

> 	* linux-xtensa-low.c (C0_NREGS): New definition.
> 	(xtensa_fill_gregset): Call collect_register for all registers in
> 	a0_regnum..a0_regnum + C0_NREGS range.
> 	(xtensa_store_gregset): Call supply_register for all registers in
> 	a0_regnum..a0_regnum + C0_NREGS range.
> ---
>  gdb/gdbserver/linux-xtensa-low.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/gdb/gdbserver/linux-xtensa-low.c b/gdb/gdbserver/linux-xtensa-low.c
> index 98c0bf2..73fbfe2 100644
> --- a/gdb/gdbserver/linux-xtensa-low.c
> +++ b/gdb/gdbserver/linux-xtensa-low.c
> @@ -39,12 +39,15 @@ enum regnum {
>  	R_A0 = 64
>  };
>
> +#define C0_NREGS 16
> +

This is duplicating code from gdb/xtensa-linux-nat. Common nat code 
could be added to gdb/nat/ in an xtensa-specific file.

>  static void
>  xtensa_fill_gregset (struct regcache *regcache, void *buf)
>  {
>    elf_greg_t* rset = (elf_greg_t*)buf;
>    const struct target_desc *tdesc = regcache->tdesc;
>    int ar0_regnum;
> +  int a0_regnum;
>    char *ptr;
>    int i;
>
> @@ -72,6 +75,17 @@ xtensa_fill_gregset (struct regcache *regcache, void *buf)
>    collect_register_by_name (regcache, "ps", (char*)&rset[R_PS]);
>    collect_register_by_name (regcache, "windowbase", (char*)&rset[R_WB]);
>    collect_register_by_name (regcache, "windowstart", (char*)&rset[R_WS]);
> +
> +  a0_regnum = find_regno (tdesc, "a0");
> +  ptr = (char *)&rset[R_A0 + 4 * rset[R_WB]];

(char *) &rset... with space.

More occurrences of this below.

> +
> +  for (i = a0_regnum; i < a0_regnum + C0_NREGS; i++)
> +    {
> +      if (4 * rset[R_WB] + i - a0_regnum == XCHAL_NUM_AREGS)

This looks slightly error-prone. Adding parenthesis around the 
expression would look better and avoid compiler warnings.

> +	ptr = (char *)&rset[R_A0];

space issue mentioned above.

> +      collect_register (regcache, i, ptr);
> +      ptr += register_size (tdesc, i);
> +    }
>  }
>
>  static void
> @@ -80,6 +94,7 @@ xtensa_store_gregset (struct regcache *regcache, const void *buf)
>    const elf_greg_t* rset = (const elf_greg_t*)buf;
>    const struct target_desc *tdesc = regcache->tdesc;
>    int ar0_regnum;
> +  int a0_regnum;
>    char *ptr;
>    int i;
>
> @@ -94,6 +109,17 @@ xtensa_store_gregset (struct regcache *regcache, const void *buf)
>        ptr += register_size (tdesc, i);
>      }
>
> +  a0_regnum = find_regno (tdesc, "a0");
> +  ptr = (char *)&rset[R_A0 + (4 * rset[R_WB]) % XCHAL_NUM_AREGS];
> +

space issue.

> +  for (i = a0_regnum; i < a0_regnum + C0_NREGS; i++)
> +    {
> +      if (4 * rset[R_WB] + i - a0_regnum == XCHAL_NUM_AREGS)

parenthesis around the expression.

Re: [PATCH 3/3] gdbserver: xtensa: add call0 support

[Max Filippov]

On Mon, Feb 13, 2017 at 10:43 AM, Luis Machado
<lgustavo@codesourcery.com> wrote:
> On 01/18/2017 05:50 PM, Max Filippov wrote:
>>
>> Correctly handle a0- registers on requests from remote gdb. This fixes
>>
>>   'Register 1 is not available'
>>
>> and subsequent assertion in the remote gdb connecting to the gdbserver:
>>
>>   'findvar.c:291: internal-error: value_of_register_lazy:
>>     Assertion `frame_id_p(get_frame_id (frame))' failed.'
>>
>> The register structure is the same for windowed and call0 ABIs because
>> currently linux kernel internally requires windowed registers, so they
>> are always present.
>>
>> 2017-01-18  Max Filippov  <jcmvbkbc@gmail.com>
>> gdb/gdbserver/
>
>
> Drop gdb/gdbserver/
>
>>         * linux-xtensa-low.c (C0_NREGS): New definition.
>>         (xtensa_fill_gregset): Call collect_register for all registers in
>>         a0_regnum..a0_regnum + C0_NREGS range.
>>         (xtensa_store_gregset): Call supply_register for all registers in
>>         a0_regnum..a0_regnum + C0_NREGS range.
>> ---
>>  gdb/gdbserver/linux-xtensa-low.c | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/gdb/gdbserver/linux-xtensa-low.c
>> b/gdb/gdbserver/linux-xtensa-low.c
>> index 98c0bf2..73fbfe2 100644
>> --- a/gdb/gdbserver/linux-xtensa-low.c
>> +++ b/gdb/gdbserver/linux-xtensa-low.c
>> @@ -39,12 +39,15 @@ enum regnum {
>>         R_A0 = 64
>>  };
>>
>> +#define C0_NREGS 16
>> +
>
>
> This is duplicating code from gdb/xtensa-linux-nat. Common nat code could be
> added to gdb/nat/ in an xtensa-specific file.

Ok.

>>  static void
>>  xtensa_fill_gregset (struct regcache *regcache, void *buf)
>>  {
>>    elf_greg_t* rset = (elf_greg_t*)buf;
>>    const struct target_desc *tdesc = regcache->tdesc;
>>    int ar0_regnum;
>> +  int a0_regnum;
>>    char *ptr;
>>    int i;
>>
>> @@ -72,6 +75,17 @@ xtensa_fill_gregset (struct regcache *regcache, void
>> *buf)
>>    collect_register_by_name (regcache, "ps", (char*)&rset[R_PS]);
>>    collect_register_by_name (regcache, "windowbase", (char*)&rset[R_WB]);
>>    collect_register_by_name (regcache, "windowstart", (char*)&rset[R_WS]);
>> +
>> +  a0_regnum = find_regno (tdesc, "a0");
>> +  ptr = (char *)&rset[R_A0 + 4 * rset[R_WB]];
>
>
> (char *) &rset... with space.
>
> More occurrences of this below.

Will fix.

>> +
>> +  for (i = a0_regnum; i < a0_regnum + C0_NREGS; i++)
>> +    {
>> +      if (4 * rset[R_WB] + i - a0_regnum == XCHAL_NUM_AREGS)
>
>
> This looks slightly error-prone. Adding parenthesis around the expression
> would look better and avoid compiler warnings.

Will add. It builds without warnings for me though.

>> +       ptr = (char *)&rset[R_A0];
>
>
> space issue mentioned above.

Will fix.

>> +      collect_register (regcache, i, ptr);
>> +      ptr += register_size (tdesc, i);
>> +    }
>>  }
>>
>>  static void
>> @@ -80,6 +94,7 @@ xtensa_store_gregset (struct regcache *regcache, const
>> void *buf)
>>    const elf_greg_t* rset = (const elf_greg_t*)buf;
>>    const struct target_desc *tdesc = regcache->tdesc;
>>    int ar0_regnum;
>> +  int a0_regnum;
>>    char *ptr;
>>    int i;
>>
>> @@ -94,6 +109,17 @@ xtensa_store_gregset (struct regcache *regcache, const
>> void *buf)
>>        ptr += register_size (tdesc, i);
>>      }
>>
>> +  a0_regnum = find_regno (tdesc, "a0");
>> +  ptr = (char *)&rset[R_A0 + (4 * rset[R_WB]) % XCHAL_NUM_AREGS];
>> +
>
>
> space issue.

Will fix.

>> +  for (i = a0_regnum; i < a0_regnum + C0_NREGS; i++)
>> +    {
>> +      if (4 * rset[R_WB] + i - a0_regnum == XCHAL_NUM_AREGS)
>
>
> parenthesis around the expression.

Will add.

-- 
Thanks.
-- Max

[PATCH 2/3] gdb: xtensa-linux: add call0 support

[Max Filippov]

Correctly handle a0- registers. This allows debugging call0 code in
linux natively.
The register structure is the same for windowed and call0 ABIs because
currently linux kernel internally requires windowed registers, so they are
always present.

2017-01-18  Max Filippov  <jcmvbkbc@gmail.com>
gdb/
	* xtensa-linux-nat.c (C0_NREGS): New definition.
	(fill_gregset): Call regcache_raw_collect for a single specified
	register or for all registers in a0_base..a0_base + C0_NREGS
	range.
	(supply_gregset_reg): Call regcache_raw_supply for a single
	specified register or for all registers in a0_base..a0_base +
	C0_NREGS range.
---
 gdb/xtensa-linux-nat.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/gdb/xtensa-linux-nat.c b/gdb/xtensa-linux-nat.c
index afe15f6..2693939 100644
--- a/gdb/xtensa-linux-nat.c
+++ b/gdb/xtensa-linux-nat.c
@@ -45,6 +45,8 @@
    hardware-specific overlays.  */
 #include "xtensa-xtregs.c"
 
+#define C0_NREGS   16	/* Number of A-registers to track.  */
+
 void
 fill_gregset (const struct regcache *regcache,
 	      gdb_gregset_t *gregsetp, int regnum)
@@ -94,6 +96,20 @@ fill_gregset (const struct regcache *regcache,
 			      gdbarch_tdep (gdbarch)->ar_base + i,
 			      &regs->ar[i]);
     }
+  if (regnum >= gdbarch_tdep (gdbarch)->a0_base
+      && regnum < gdbarch_tdep (gdbarch)->a0_base + C0_NREGS)
+    regcache_raw_collect (regcache, regnum,
+			  &regs->ar[(4 * regs->windowbase + regnum
+				     - gdbarch_tdep (gdbarch)->a0_base)
+			  % gdbarch_tdep (gdbarch)->num_aregs]);
+  else if (regnum == -1)
+    {
+      for (i = 0; i < C0_NREGS; ++i)
+	regcache_raw_collect (regcache,
+			      gdbarch_tdep (gdbarch)->a0_base + i,
+			      &regs->ar[(4 * regs->windowbase + i)
+			      % gdbarch_tdep (gdbarch)->num_aregs]);
+    }
 }
 
 static void
@@ -146,6 +162,20 @@ supply_gregset_reg (struct regcache *regcache,
 			      gdbarch_tdep (gdbarch)->ar_base + i,
 			      &regs->ar[i]);
     }
+  if (regnum >= gdbarch_tdep (gdbarch)->a0_base
+      && regnum < gdbarch_tdep (gdbarch)->a0_base + C0_NREGS)
+    regcache_raw_supply (regcache, regnum,
+			 &regs->ar[(4 * regs->windowbase + regnum
+				    - gdbarch_tdep (gdbarch)->a0_base)
+			 % gdbarch_tdep (gdbarch)->num_aregs]);
+  else if (regnum == -1)
+    {
+      for (i = 0; i < C0_NREGS; ++i)
+	regcache_raw_supply (regcache,
+			     gdbarch_tdep (gdbarch)->a0_base + i,
+			     &regs->ar[(4 * regs->windowbase + i)
+			     % gdbarch_tdep (gdbarch)->num_aregs]);
+    }
 }
 
 void
-- 
2.1.4

Re: [PATCH 2/3] gdb: xtensa-linux: add call0 support

[Luis Machado]

On 01/18/2017 05:50 PM, Max Filippov wrote:
> Correctly handle a0- registers. This allows debugging call0 code in
> linux natively.
> The register structure is the same for windowed and call0 ABIs because
> currently linux kernel internally requires windowed registers, so they are
> always present.
>
> 2017-01-18  Max Filippov  <jcmvbkbc@gmail.com>
> gdb/

drop gdb/

I have no further comments on the patch.

[PATCH 1/3] gdb: xtensa: initialize isa in call0_ret

[Max Filippov]

This fixes segfault in native gdb because isa is not initialized at the
point of call to xtensa_isa_maxlength.

2017-01-18  Max Filippov  <jcmvbkbc@gmail.com>
gdb/
	* xtensa-tdep.c (call0_ret): Initialize xtensa_default_isa
	before use.
---
 gdb/xtensa-tdep.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/xtensa-tdep.c b/gdb/xtensa-tdep.c
index 978b13a..797e728 100644
--- a/gdb/xtensa-tdep.c
+++ b/gdb/xtensa-tdep.c
@@ -2027,6 +2027,8 @@ call0_ret (CORE_ADDR start_pc, CORE_ADDR finish_pc)
   const char *opcname;
   int found_ret = 0;
 
+  if (!xtensa_default_isa)
+    xtensa_default_isa = xtensa_isa_init (0, 0);
   isa = xtensa_default_isa;
   gdb_assert (XTENSA_ISA_BSZ >= xtensa_isa_maxlength (isa));
   ins = xtensa_insnbuf_alloc (isa);
-- 
2.1.4

Re: [PATCH 1/3] gdb: xtensa: initialize isa in call0_ret

[Luis Machado]

On 01/18/2017 05:50 PM, Max Filippov wrote:
> This fixes segfault in native gdb because isa is not initialized at the
> point of call to xtensa_isa_maxlength.
>
> 2017-01-18  Max Filippov  <jcmvbkbc@gmail.com>
> gdb/


Drop gdb/

> diff --git a/gdb/xtensa-tdep.c b/gdb/xtensa-tdep.c
> index 978b13a..797e728 100644
> --- a/gdb/xtensa-tdep.c
> +++ b/gdb/xtensa-tdep.c
> @@ -2027,6 +2027,8 @@ call0_ret (CORE_ADDR start_pc, CORE_ADDR finish_pc)
>    const char *opcname;
>    int found_ret = 0;
>
> +  if (!xtensa_default_isa)
> +    xtensa_default_isa = xtensa_isa_init (0, 0);
>    isa = xtensa_default_isa;
>    gdb_assert (XTENSA_ISA_BSZ >= xtensa_isa_maxlength (isa));
>    ins = xtensa_insnbuf_alloc (isa);
>

I see a couple other uses of this construct for checking if a default 
isa is set or not. How about moving this check to a function that takes 
care of setting the default isa and returns this information to assign 
to "isa"?

Re: [PATCH 1/3] gdb: xtensa: initialize isa in call0_ret

[Max Filippov]

On Mon, Feb 13, 2017 at 10:32 AM, Luis Machado
<lgustavo@codesourcery.com> wrote:
> On 01/18/2017 05:50 PM, Max Filippov wrote:
>>
>> This fixes segfault in native gdb because isa is not initialized at the
>> point of call to xtensa_isa_maxlength.
>>
>> 2017-01-18  Max Filippov  <jcmvbkbc@gmail.com>
>> gdb/
>
>
>
> Drop gdb/
>
>> diff --git a/gdb/xtensa-tdep.c b/gdb/xtensa-tdep.c
>> index 978b13a..797e728 100644
>> --- a/gdb/xtensa-tdep.c
>> +++ b/gdb/xtensa-tdep.c
>> @@ -2027,6 +2027,8 @@ call0_ret (CORE_ADDR start_pc, CORE_ADDR finish_pc)
>>    const char *opcname;
>>    int found_ret = 0;
>>
>> +  if (!xtensa_default_isa)
>> +    xtensa_default_isa = xtensa_isa_init (0, 0);
>>    isa = xtensa_default_isa;
>>    gdb_assert (XTENSA_ISA_BSZ >= xtensa_isa_maxlength (isa));
>>    ins = xtensa_insnbuf_alloc (isa);
>>
>
> I see a couple other uses of this construct for checking if a default isa is
> set or not. How about moving this check to a function that takes care of
> setting the default isa and returns this information to assign to "isa"?

Ok, let me look at it.

-- 
Thanks.
-- Max

Re: [PATCH 0/3] xtensa: support call0 ABI native linux gdb and gdbserver

[Max Filippov]

ping?

On Wed, Jan 18, 2017 at 3:50 PM, Max Filippov <jcmvbkbc@gmail.com> wrote:
> Hello,
>
> this series adds call0 ABI support to native xtensa-linux gdb and gdbserver
> ports.
>
> Max Filippov (3):
>   gdb: xtensa: initialize isa in call0_ret
>   gdb: xtensa-linux: add call0 support
>   gdbserver: xtensa: add call0 support
>
>  gdb/gdbserver/linux-xtensa-low.c | 26 ++++++++++++++++++++++++++
>  gdb/xtensa-linux-nat.c           | 30 ++++++++++++++++++++++++++++++
>  gdb/xtensa-tdep.c                |  2 ++
>  3 files changed, 58 insertions(+)

-- 
Thanks.
-- Max