[RFA/RFC] mips tracepoint: fix Bug 12013

[RFA/RFC] mips tracepoint: fix Bug 12013

[Hui Zhu]

http://sourceware.org/bugzilla/show_bug.cgi?id=12013

This bug make mips tracepoint cannot trace the backtrace.

This patch to fix this issue with a directly way just remove the
decline of access to the raw register names.

If you think it's not OK.  What about add a new interface to gdbarch
to access to the raw register names.

Thanks,
Hui


2010-12-19  Hui Zhu  <teawater@gmail.com>

	* mips-tdep.c (mips_register_name): Remove the check.
	(mips_print_registers_info): Remove the gdb_assert.

---
 mips-tdep.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

--- a/mips-tdep.c
+++ b/mips-tdep.c
@@ -453,11 +453,8 @@ mips_register_name (struct gdbarch *gdba

   enum mips_abi abi = mips_abi (gdbarch);

-  /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers,
-     but then don't make the raw register names visible.  */
+  /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers.  */
   int rawnum = regno % gdbarch_num_regs (gdbarch);
-  if (regno < gdbarch_num_regs (gdbarch))
-    return "";

   /* The MIPS integer registers are always mapped from 0 to 31.  The
      names of the registers (which reflects the conventions regarding
@@ -4774,7 +4771,6 @@ mips_print_registers_info (struct gdbarc
 {
   if (regnum != -1)		/* do one specified register */
     {
-      gdb_assert (regnum >= gdbarch_num_regs (gdbarch));
       if (*(gdbarch_register_name (gdbarch, regnum)) == '\0')
 	error (_("Not a valid register for the current processor type"));

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Mark Kettenis]

> From: Hui Zhu <teawater@gmail.com>
> Date: Sun, 19 Dec 2010 16:35:59 +0800
> 
> http://sourceware.org/bugzilla/show_bug.cgi?id=12013
> 
> This bug make mips tracepoint cannot trace the backtrace.
> 
> This patch to fix this issue with a directly way just remove the
> decline of access to the raw register names.
> 
> If you think it's not OK.  What about add a new interface to gdbarch
> to access to the raw register names.

It is a common trick to return an empty register name for a (raw)
register to hide the register from the user.  So I don't think this
diff is ok, since the goal obviously is to hide the raw registers in
mips in favour of the pseudo registers.

I'd say the proper way forward is to teach the trace code to handle
pseudo registers.

> 2010-12-19  Hui Zhu  <teawater@gmail.com>
> 
> 	* mips-tdep.c (mips_register_name): Remove the check.
> 	(mips_print_registers_info): Remove the gdb_assert.
> 

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Hui Zhu]

On Sun, Dec 19, 2010 at 18:39, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>> From: Hui Zhu <teawater@gmail.com>
>> Date: Sun, 19 Dec 2010 16:35:59 +0800
>>
>> http://sourceware.org/bugzilla/show_bug.cgi?id=12013
>>
>> This bug make mips tracepoint cannot trace the backtrace.
>>
>> This patch to fix this issue with a directly way just remove the
>> decline of access to the raw register names.
>>
>> If you think it's not OK.  What about add a new interface to gdbarch
>> to access to the raw register names.
>
> It is a common trick to return an empty register name for a (raw)
> register to hide the register from the user.  So I don't think this
> diff is ok, since the goal obviously is to hide the raw registers in
> mips in favour of the pseudo registers.

Could you tell me what this hide for?  I didn't find who get some
advantage form this part?

>
> I'd say the proper way forward is to teach the trace code to handle
> pseudo registers.

void
regcache_cooked_read (struct regcache *regcache, int regnum, gdb_byte *buf)
{
  gdb_assert (regnum >= 0);
  gdb_assert (regnum < regcache->descr->nr_cooked_registers);
  if (regnum < regcache->descr->nr_raw_registers)
    regcache_raw_read (regcache, regnum, buf);
  else if (regcache->readonly_p
	   && regnum < regcache->descr->nr_cooked_registers
	   && regcache->register_valid_p[regnum])
    /* Read-only register cache, perhaps the cooked value was cached?  */
    memcpy (buf, register_buffer (regcache, regnum),
	    regcache->descr->sizeof_register[regnum]);
  else
    gdbarch_pseudo_register_read (regcache->descr->gdbarch, regcache,
				  regnum, buf);
}

For now, even if gdb doesn't how to handle pseudo, it put it to
gdbarch.  I need add a new interface to gdbarch to handle it if we
really need.

Thanks,
Hui

>
>> 2010-12-19  Hui Zhu  <teawater@gmail.com>
>>
>>       * mips-tdep.c (mips_register_name): Remove the check.
>>       (mips_print_registers_info): Remove the gdb_assert.
>>
>

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Hui Zhu]

Did not get any reason.

Checked in.

Thanks,
Hui

On Sun, Dec 19, 2010 at 20:15, Hui Zhu <teawater@gmail.com> wrote:
> On Sun, Dec 19, 2010 at 18:39, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>> From: Hui Zhu <teawater@gmail.com>
>>> Date: Sun, 19 Dec 2010 16:35:59 +0800
>>>
>>> http://sourceware.org/bugzilla/show_bug.cgi?id=12013
>>>
>>> This bug make mips tracepoint cannot trace the backtrace.
>>>
>>> This patch to fix this issue with a directly way just remove the
>>> decline of access to the raw register names.
>>>
>>> If you think it's not OK.  What about add a new interface to gdbarch
>>> to access to the raw register names.
>>
>> It is a common trick to return an empty register name for a (raw)
>> register to hide the register from the user.  So I don't think this
>> diff is ok, since the goal obviously is to hide the raw registers in
>> mips in favour of the pseudo registers.
>
> Could you tell me what this hide for?  I didn't find who get some
> advantage form this part?
>
>>
>> I'd say the proper way forward is to teach the trace code to handle
>> pseudo registers.
>
> void
> regcache_cooked_read (struct regcache *regcache, int regnum, gdb_byte *buf)
> {
>  gdb_assert (regnum >= 0);
>  gdb_assert (regnum < regcache->descr->nr_cooked_registers);
>  if (regnum < regcache->descr->nr_raw_registers)
>    regcache_raw_read (regcache, regnum, buf);
>  else if (regcache->readonly_p
>           && regnum < regcache->descr->nr_cooked_registers
>           && regcache->register_valid_p[regnum])
>    /* Read-only register cache, perhaps the cooked value was cached?  */
>    memcpy (buf, register_buffer (regcache, regnum),
>            regcache->descr->sizeof_register[regnum]);
>  else
>    gdbarch_pseudo_register_read (regcache->descr->gdbarch, regcache,
>                                  regnum, buf);
> }
>
> For now, even if gdb doesn't how to handle pseudo, it put it to
> gdbarch.  I need add a new interface to gdbarch to handle it if we
> really need.
>
> Thanks,
> Hui
>
>>
>>> 2010-12-19  Hui Zhu  <teawater@gmail.com>
>>>
>>>       * mips-tdep.c (mips_register_name): Remove the check.
>>>       (mips_print_registers_info): Remove the gdb_assert.
>>>
>>
>

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Mark Kettenis]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2653 bytes --]

> From: Hui Zhu <teawater@gmail.com>
> Date: Tue, 21 Dec 2010 22:45:15 +0800
> 
> Did not get any reason.

?

> Checked in.

I told you this is wrong.  You'll have to fix this in a different way.
I'm not familliar with the tracepoint stuff, so I can't really help
you with that.

Please back this out immediately.

> On Sun, Dec 19, 2010 at 20:15, Hui Zhu <teawater@gmail.com> wrote:
> > On Sun, Dec 19, 2010 at 18:39, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >>> From: Hui Zhu <teawater@gmail.com>
> >>> Date: Sun, 19 Dec 2010 16:35:59 +0800
> >>>
> >>> http://sourceware.org/bugzilla/show_bug.cgi?id=12013
> >>>
> >>> This bug make mips tracepoint cannot trace the backtrace.
> >>>
> >>> This patch to fix this issue with a directly way just remove the
> >>> decline of access to the raw register names.
> >>>
> >>> If you think it's not OK.  What about add a new interface to gdbarch
> >>> to access to the raw register names.
> >>
> >> It is a common trick to return an empty register name for a (raw)
> >> register to hide the register from the user.  So I don't think this
> >> diff is ok, since the goal obviously is to hide the raw registers in
> >> mips in favour of the pseudo registers.
> >
> > Could you tell me what this hide for?  I didn't find who get some
> > advantage form this part?
> >
> >>
> >> I'd say the proper way forward is to teach the trace code to handle
> >> pseudo registers.
> >
> > void
> > regcache_cooked_read (struct regcache *regcache, int regnum, gdb_byte *buf)
> > {
> >  gdb_assert (regnum >= 0);
> >  gdb_assert (regnum < regcache->descr->nr_cooked_registers);
> >  if (regnum < regcache->descr->nr_raw_registers)
> >    regcache_raw_read (regcache, regnum, buf);
> >  else if (regcache->readonly_p
> >           && regnum < regcache->descr->nr_cooked_registers
> >           && regcache->register_valid_p[regnum])
> >    /* Read-only register cache, perhaps the cooked value was cached?  */
> >    memcpy (buf, register_buffer (regcache, regnum),
> >            regcache->descr->sizeof_register[regnum]);
> >  else
> >    gdbarch_pseudo_register_read (regcache->descr->gdbarch, regcache,
> >                                  regnum, buf);
> > }
> >
> > For now, even if gdb doesn't how to handle pseudo, it put it to
> > gdbarch.  I need add a new interface to gdbarch to handle it if we
> > really need.
> >
> > Thanks,
> > Hui
> >
> >>
> >>> 2010-12-19  Hui Zhu  <teawater@gmail.com>
> >>>
> >>>       * mips-tdep.c (mips_register_name): Remove the check.
> >>>       (mips_print_registers_info): Remove the gdb_assert.
> >>>
> >>
> >
> 
> 

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Hui Zhu]

On Tue, Dec 21, 2010 at 22:58, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>> From: Hui Zhu <teawater@gmail.com>
>> Date: Tue, 21 Dec 2010 22:45:15 +0800
>>
>> Did not get any reason.
>
> ?
>
>> Checked in.
>
> I told you this is wrong.  You'll have to fix this in a different way.
> I'm not familliar with the tracepoint stuff, so I can't really help
> you with that.

You didn't give me the reason.  I asked you:
>> > Could you tell me what this hide for?  I didn't find who get some
>> > advantage form this part?

But you didn't answer.   I don't care you familliar with trace or not.

Do you familliar with MIPS?
If so, please answer me.  If not, please don't say anything.

Thanks,
Hui

>
> Please back this out immediately.
>
>> On Sun, Dec 19, 2010 at 20:15, Hui Zhu <teawater@gmail.com> wrote:
>> > On Sun, Dec 19, 2010 at 18:39, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>> >>> From: Hui Zhu <teawater@gmail.com>
>> >>> Date: Sun, 19 Dec 2010 16:35:59 +0800
>> >>>
>> >>> http://sourceware.org/bugzilla/show_bug.cgi?id=12013
>> >>>
>> >>> This bug make mips tracepoint cannot trace the backtrace.
>> >>>
>> >>> This patch to fix this issue with a directly way just remove the
>> >>> decline of access to the raw register names.
>> >>>
>> >>> If you think it's not OK.  What about add a new interface to gdbarch
>> >>> to access to the raw register names.
>> >>
>> >> It is a common trick to return an empty register name for a (raw)
>> >> register to hide the register from the user.  So I don't think this
>> >> diff is ok, since the goal obviously is to hide the raw registers in
>> >> mips in favour of the pseudo registers.
>> >
>> > Could you tell me what this hide for?  I didn't find who get some
>> > advantage form this part?
>> >
>> >>
>> >> I'd say the proper way forward is to teach the trace code to handle
>> >> pseudo registers.
>> >
>> > void
>> > regcache_cooked_read (struct regcache *regcache, int regnum, gdb_byte *buf)
>> > {
>> >  gdb_assert (regnum >= 0);
>> >  gdb_assert (regnum < regcache->descr->nr_cooked_registers);
>> >  if (regnum < regcache->descr->nr_raw_registers)
>> >    regcache_raw_read (regcache, regnum, buf);
>> >  else if (regcache->readonly_p
>> >           && regnum < regcache->descr->nr_cooked_registers
>> >           && regcache->register_valid_p[regnum])
>> >    /* Read-only register cache, perhaps the cooked value was cached?  */
>> >    memcpy (buf, register_buffer (regcache, regnum),
>> >            regcache->descr->sizeof_register[regnum]);
>> >  else
>> >    gdbarch_pseudo_register_read (regcache->descr->gdbarch, regcache,
>> >                                  regnum, buf);
>> > }
>> >
>> > For now, even if gdb doesn't how to handle pseudo, it put it to
>> > gdbarch.  I need add a new interface to gdbarch to handle it if we
>> > really need.
>> >
>> > Thanks,
>> > Hui
>> >
>> >>
>> >>> 2010-12-19  Hui Zhu  <teawater@gmail.com>
>> >>>
>> >>>       * mips-tdep.c (mips_register_name): Remove the check.
>> >>>       (mips_print_registers_info): Remove the gdb_assert.
>> >>>
>> >>
>> >
>>
>>
>

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Kevin Buettner]

On Sun, 19 Dec 2010 20:15:59 +0800
Hui Zhu <teawater@gmail.com> wrote:

> > It is a common trick to return an empty register name for a (raw)
> > register to hide the register from the user. __So I don't think this
> > diff is ok, since the goal obviously is to hide the raw registers in
> > mips in favour of the pseudo registers.
> 
> Could you tell me what this hide for?  I didn't find who get some
> advantage form this part?

I agree with Mark.  We do not want to expose the MIPS raw registers
directly to the user.

It is possible to debug a 64-bit device using a 32-bit programming
model.  In such instances, the raw registers are configured to be
64-bits wide, while the pseudo registers are configured to be 32-bits
wide.  The registers that the user sees - the pseudo registers - match
the user's expectations given the programming model being used.

Your patch exposes the raw registers in such a way that errant code
(within GDB) could present the user with an inconsistent view of the
registers.  This, in my opinion, is not desirable.

Please revert your patch and, in the future, wait until a consensus is
achieved before comitting your work.

Kevin

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Hui Zhu]

On Tue, Dec 21, 2010 at 23:42, Kevin Buettner <kevinb@redhat.com> wrote:
> On Sun, 19 Dec 2010 20:15:59 +0800
> Hui Zhu <teawater@gmail.com> wrote:
>
>> > It is a common trick to return an empty register name for a (raw)
>> > register to hide the register from the user. __So I don't think this
>> > diff is ok, since the goal obviously is to hide the raw registers in
>> > mips in favour of the pseudo registers.
>>
>> Could you tell me what this hide for?  I didn't find who get some
>> advantage form this part?
>
> I agree with Mark.  We do not want to expose the MIPS raw registers
> directly to the user.
>
> It is possible to debug a 64-bit device using a 32-bit programming
> model.  In such instances, the raw registers are configured to be
> 64-bits wide, while the pseudo registers are configured to be 32-bits
> wide.  The registers that the user sees - the pseudo registers - match
> the user's expectations given the programming model being used.

Thanks.  Do you think I can add a special name for these raw registers
then other part can use this raw register if need.

Thanks,
Hui

>
> Your patch exposes the raw registers in such a way that errant code
> (within GDB) could present the user with an inconsistent view of the
> registers.  This, in my opinion, is not desirable.
>
> Please revert your patch and, in the future, wait until a consensus is
> achieved before comitting your work.
>
> Kevin
>

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Kevin Buettner]

On Tue, 21 Dec 2010 23:58:29 +0800
Hui Zhu <teawater@gmail.com> wrote:

> Thanks.  Do you think I can add a special name for these raw registers
> then other part can use this raw register if need.

I have a hunch that such an approach will result in an assertion
failure in mips_print_registers_info() when a raw register name is
used to attempt to print a register.

The other problem with such an approach is that it again exposes the
raw registers to the user.  In this case, instead of using the name
"sp", the user would instead have to use "raw-sp" (or some such). 
This is not completely horrible, but it's certainly not as nice as
allowing the user to continue referring to standard nomenclature when
using the trace machinery.

You might consider implementing a new gdbarch method which provides a
mapping from pseudo register numbers to raw register numbers.  The
trace machinery could use such a mapping to find the corresponding raw
register(s) when presented with a pseudo register.  I can think of
several potential pitfalls with this approach, but I think the idea is
worth exploring.

Kevin

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Hui Zhu]

On Wed, Dec 22, 2010 at 14:04, Kevin Buettner <kevinb@redhat.com> wrote:
> On Tue, 21 Dec 2010 23:58:29 +0800
> Hui Zhu <teawater@gmail.com> wrote:
>
>> Thanks.  Do you think I can add a special name for these raw registers
>> then other part can use this raw register if need.
>
> I have a hunch that such an approach will result in an assertion
> failure in mips_print_registers_info() when a raw register name is
> used to attempt to print a register.
>
> The other problem with such an approach is that it again exposes the
> raw registers to the user.  In this case, instead of using the name
> "sp", the user would instead have to use "raw-sp" (or some such).
> This is not completely horrible, but it's certainly not as nice as
> allowing the user to continue referring to standard nomenclature when
> using the trace machinery.
>
> You might consider implementing a new gdbarch method which provides a
> mapping from pseudo register numbers to raw register numbers.  The
> trace machinery could use such a mapping to find the corresponding raw
> register(s) when presented with a pseudo register.  I can think of
> several potential pitfalls with this approach, but I think the idea is
> worth exploring.
>
> Kevin
>

Thanks Kevin.  I will do it.

And I make a patch to add some comments from your mail to mips_register_name.
Wish it can help other people.

Please help me review it.

Thanks,
Hui

2010-12-22  Hui Zhu  <teawater@gmail.com>

	* mips-tedp.c (mips_register_name): Add comments.

---
 mips-tdep.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

--- a/mips-tdep.c
+++ b/mips-tdep.c
@@ -454,7 +454,12 @@ mips_register_name (struct gdbarch *gdba
   enum mips_abi abi = mips_abi (gdbarch);

   /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers,
-     but then don't make the raw register names visible.  */
+     but then don't make the raw register names visible.
+     Because It is possible to debug a 64-bit device using a 32-bit programming
+     model.  In such instances, the raw registers are configured to be
+     64-bits wide, while the pseudo registers are configured to be 32-bits
+     wide.  The registers that the user sees - the pseudo registers - match
+     the user's expectations given the programming model being used.  */
   int rawnum = regno % gdbarch_num_regs (gdbarch);
   if (regno < gdbarch_num_regs (gdbarch))
     return "";

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Kevin Buettner]

On Wed, 22 Dec 2010 15:12:22 +0800
Hui Zhu <teawater@gmail.com> wrote:

> > You might consider implementing a new gdbarch method which provides a
> > mapping from pseudo register numbers to raw register numbers. __The
> > trace machinery could use such a mapping to find the corresponding raw
> > register(s) when presented with a pseudo register. __I can think of
> > several potential pitfalls with this approach, but I think the idea is
> > worth exploring.
>
> Thanks Kevin.  I will do it.

Please look at Pedro's reply.  He has outlined a better approach.

> And I make a patch to add some comments from your mail to mips_register_name.
> Wish it can help other people.
> 
> Please help me review it.

Okay, see below...

> 2010-12-22  Hui Zhu  <teawater@gmail.com>
> 
> 	* mips-tedp.c (mips_register_name): Add comments.
> 
> ---
>  mips-tdep.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> --- a/mips-tdep.c
> +++ b/mips-tdep.c
> @@ -454,7 +454,12 @@ mips_register_name (struct gdbarch *gdba
>    enum mips_abi abi = mips_abi (gdbarch);
> 
>    /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers,
> -     but then don't make the raw register names visible.  */
> +     but then don't make the raw register names visible.
> +     Because It is possible to debug a 64-bit device using a 32-bit programming
> +     model.  In such instances, the raw registers are configured to be
> +     64-bits wide, while the pseudo registers are configured to be 32-bits
> +     wide.  The registers that the user sees - the pseudo registers - match
> +     the user's expectations given the programming model being used.  */

Could you revise the comment to read as follows?

    /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers,
       but do not make the raw register names visible.  This (upper)
       range of user visible register numbers are the
       pseudo-registers.
       
       This approach was adopted accomodate the following scenario: 
       It is possible to debug a 64-bit device using a 32-bit
       programming model.  In such instances, the raw registers are
       configured to be 64-bits wide, while the pseudo registers are
       configured to be 32-bits wide.  The reigsters that the user
       sees - the pseudo registers - match the users expectations
       given the programming model being used.  */

Please allow several days for others to tweak my suggested wording.  If
there are no further comments on the above wording, feel free to commit
it.

Thanks,

Kevin

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Hui Zhu]

On Wed, Dec 22, 2010 at 23:36, Kevin Buettner <kevinb@redhat.com> wrote:
> On Wed, 22 Dec 2010 15:12:22 +0800
> Hui Zhu <teawater@gmail.com> wrote:
>
>> > You might consider implementing a new gdbarch method which provides a
>> > mapping from pseudo register numbers to raw register numbers. __The
>> > trace machinery could use such a mapping to find the corresponding raw
>> > register(s) when presented with a pseudo register. __I can think of
>> > several potential pitfalls with this approach, but I think the idea is
>> > worth exploring.
>>
>> Thanks Kevin.  I will do it.
>
> Please look at Pedro's reply.  He has outlined a better approach.
>
>> And I make a patch to add some comments from your mail to mips_register_name.
>> Wish it can help other people.
>>
>> Please help me review it.
>
> Okay, see below...
>
>> 2010-12-22  Hui Zhu  <teawater@gmail.com>
>>
>>       * mips-tedp.c (mips_register_name): Add comments.
>>
>> ---
>>  mips-tdep.c |    7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> --- a/mips-tdep.c
>> +++ b/mips-tdep.c
>> @@ -454,7 +454,12 @@ mips_register_name (struct gdbarch *gdba
>>    enum mips_abi abi = mips_abi (gdbarch);
>>
>>    /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers,
>> -     but then don't make the raw register names visible.  */
>> +     but then don't make the raw register names visible.
>> +     Because It is possible to debug a 64-bit device using a 32-bit programming
>> +     model.  In such instances, the raw registers are configured to be
>> +     64-bits wide, while the pseudo registers are configured to be 32-bits
>> +     wide.  The registers that the user sees - the pseudo registers - match
>> +     the user's expectations given the programming model being used.  */
>
> Could you revise the comment to read as follows?
>
>    /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers,
>       but do not make the raw register names visible.  This (upper)
>       range of user visible register numbers are the
>       pseudo-registers.
>
>       This approach was adopted accomodate the following scenario:
>       It is possible to debug a 64-bit device using a 32-bit
>       programming model.  In such instances, the raw registers are
>       configured to be 64-bits wide, while the pseudo registers are
>       configured to be 32-bits wide.  The reigsters that the user
>       sees - the pseudo registers - match the users expectations
>       given the programming model being used.  */
>
> Please allow several days for others to tweak my suggested wording.  If
> there are no further comments on the above wording, feel free to commit
> it.
>
> Thanks,
>
> Kevin
>

OK.   Thanks Kevin.

Best,
Hui


2010-12-23  Kevin Buettner  <kevinb@redhat.com>
            Hui Zhu  <teawater@gmail.com>

	* mips-tedp.c (mips_register_name): Add comments.

---
 mips-tdep.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

--- a/mips-tdep.c
+++ b/mips-tdep.c
@@ -454,7 +454,16 @@ mips_register_name (struct gdbarch *gdba
   enum mips_abi abi = mips_abi (gdbarch);

   /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers,
-     but then don't make the raw register names visible.  */
+     but then don't make the raw register names visible.  This (upper)
+     range of user visible register numbers are the pseudo-registers.
+
+     This approach was adopted accommodate the following scenario:
+     It is possible to debug a 64-bit device using a 32-bit
+     programming model.  In such instances, the raw registers are
+     configured to be 64-bits wide, while the pseudo registers are
+     configured to be 32-bits wide.  The registers that the user
+     sees - the pseudo registers - match the users expectations
+     given the programming model being used.  */
   int rawnum = regno % gdbarch_num_regs (gdbarch);
   if (regno < gdbarch_num_regs (gdbarch))
     return "";

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Hui Zhu]

On Thu, Dec 23, 2010 at 10:57, Hui Zhu <teawater@gmail.com> wrote:
> On Wed, Dec 22, 2010 at 23:36, Kevin Buettner <kevinb@redhat.com> wrote:
>> On Wed, 22 Dec 2010 15:12:22 +0800
>> Hui Zhu <teawater@gmail.com> wrote:
>>
>>> > You might consider implementing a new gdbarch method which provides a
>>> > mapping from pseudo register numbers to raw register numbers. __The
>>> > trace machinery could use such a mapping to find the corresponding raw
>>> > register(s) when presented with a pseudo register. __I can think of
>>> > several potential pitfalls with this approach, but I think the idea is
>>> > worth exploring.
>>>
>>> Thanks Kevin.  I will do it.
>>
>> Please look at Pedro's reply.  He has outlined a better approach.
>>
>>> And I make a patch to add some comments from your mail to mips_register_name.
>>> Wish it can help other people.
>>>
>>> Please help me review it.
>>
>> Okay, see below...
>>
>>> 2010-12-22  Hui Zhu  <teawater@gmail.com>
>>>
>>>       * mips-tedp.c (mips_register_name): Add comments.
>>>
>>> ---
>>>  mips-tdep.c |    7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> --- a/mips-tdep.c
>>> +++ b/mips-tdep.c
>>> @@ -454,7 +454,12 @@ mips_register_name (struct gdbarch *gdba
>>>    enum mips_abi abi = mips_abi (gdbarch);
>>>
>>>    /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers,
>>> -     but then don't make the raw register names visible.  */
>>> +     but then don't make the raw register names visible.
>>> +     Because It is possible to debug a 64-bit device using a 32-bit programming
>>> +     model.  In such instances, the raw registers are configured to be
>>> +     64-bits wide, while the pseudo registers are configured to be 32-bits
>>> +     wide.  The registers that the user sees - the pseudo registers - match
>>> +     the user's expectations given the programming model being used.  */
>>
>> Could you revise the comment to read as follows?
>>
>>    /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers,
>>       but do not make the raw register names visible.  This (upper)
>>       range of user visible register numbers are the
>>       pseudo-registers.
>>
>>       This approach was adopted accomodate the following scenario:
>>       It is possible to debug a 64-bit device using a 32-bit
>>       programming model.  In such instances, the raw registers are
>>       configured to be 64-bits wide, while the pseudo registers are
>>       configured to be 32-bits wide.  The reigsters that the user
>>       sees - the pseudo registers - match the users expectations
>>       given the programming model being used.  */
>>
>> Please allow several days for others to tweak my suggested wording.  If
>> there are no further comments on the above wording, feel free to commit
>> it.

Checked in.

Joel,  do you think this patch can check in to 7.2.1.

Thanks,
Hui

>>
>> Thanks,
>>
>> Kevin
>>
>
> OK.   Thanks Kevin.
>
> Best,
> Hui
>
>
> 2010-12-23  Kevin Buettner  <kevinb@redhat.com>
>            Hui Zhu  <teawater@gmail.com>
>
>        * mips-tedp.c (mips_register_name): Add comments.
>
> ---
>  mips-tdep.c |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> --- a/mips-tdep.c
> +++ b/mips-tdep.c
> @@ -454,7 +454,16 @@ mips_register_name (struct gdbarch *gdba
>   enum mips_abi abi = mips_abi (gdbarch);
>
>   /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers,
> -     but then don't make the raw register names visible.  */
> +     but then don't make the raw register names visible.  This (upper)
> +     range of user visible register numbers are the pseudo-registers.
> +
> +     This approach was adopted accommodate the following scenario:
> +     It is possible to debug a 64-bit device using a 32-bit
> +     programming model.  In such instances, the raw registers are
> +     configured to be 64-bits wide, while the pseudo registers are
> +     configured to be 32-bits wide.  The registers that the user
> +     sees - the pseudo registers - match the users expectations
> +     given the programming model being used.  */
>   int rawnum = regno % gdbarch_num_regs (gdbarch);
>   if (regno < gdbarch_num_regs (gdbarch))
>     return "";
>

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Joel Brobecker]

> Joel,  do you think this patch can check in to 7.2.1.

Yes, please go ahead.

-- 
Joel

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Hui Zhu]

On Mon, Dec 27, 2010 at 21:19, Joel Brobecker <brobecker@adacore.com> wrote:
>> Joel,  do you think this patch can check in to 7.2.1.
>
> Yes, please go ahead.
>
> --
> Joel
>

Thanks.  Checked in to 7.2.1.

Best,
Hui

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Pedro Alves]

On Wednesday 22 December 2010 06:04:28, Kevin Buettner wrote:
> You might consider implementing a new gdbarch method which provides a
> mapping from pseudo register numbers to raw register numbers.  The
> trace machinery could use such a mapping to find the corresponding raw
> register(s) when presented with a pseudo register.  I can think of
> several potential pitfalls with this approach, but I think the idea is
> worth exploring.

Such mapping will necessarily be a temporary kludge -- if such a mapping
were always possible, then we wouldn't have
gdbarch_pseudo_register_read/gdbarch_pseudo_register_read callbacks, but
we'd instead have a gdbarch_pseudo_register_to_raw_register callback,
or some such.

If you're going to add support for collecting pseudo registers, I'd
much rather you added a "gdbarch_pseudo_register_collect" callback,
that emitted the agent expression bits required to collect whatever 
gdbarch_pseudo_register_read needs to "read" the pseudo register
in question at trace buffer inspection time (tfind mode).  In the
mips case, you'd end up with only calls to ax_reg in that new
callback, it looks to me.
(Take a look at dwarf2loc.c:compile_dwarf_to_reg and it's ax_reg call,
for example.)

-- 
Pedro Alves

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Hui Zhu]

On Wed, Dec 22, 2010 at 19:26, Pedro Alves <pedro@codesourcery.com> wrote:
> On Wednesday 22 December 2010 06:04:28, Kevin Buettner wrote:
>> You might consider implementing a new gdbarch method which provides a
>> mapping from pseudo register numbers to raw register numbers.  The
>> trace machinery could use such a mapping to find the corresponding raw
>> register(s) when presented with a pseudo register.  I can think of
>> several potential pitfalls with this approach, but I think the idea is
>> worth exploring.
>
> Such mapping will necessarily be a temporary kludge -- if such a mapping
> were always possible, then we wouldn't have
> gdbarch_pseudo_register_read/gdbarch_pseudo_register_read callbacks, but
> we'd instead have a gdbarch_pseudo_register_to_raw_register callback,
> or some such.
>
> If you're going to add support for collecting pseudo registers, I'd
> much rather you added a "gdbarch_pseudo_register_collect" callback,
> that emitted the agent expression bits required to collect whatever
> gdbarch_pseudo_register_read needs to "read" the pseudo register
> in question at trace buffer inspection time (tfind mode).  In the
> mips case, you'd end up with only calls to ax_reg in that new
> callback, it looks to me.
> (Take a look at dwarf2loc.c:compile_dwarf_to_reg and it's ax_reg call,
> for example.)
>
> --
> Pedro Alves
>

Thanks Pedro.

I make a patch according to your comments.
It add to callback in gdbarch.  They are gdbarch_ax_pseudo_reg and
gdbarch_ax_pseudo_reg_mask.

gdbarch_ax_pseudo_reg will be called by ax_reg.  It assemble code to
push the value of pseudo register number REG on the stack.
gdbarch_ax_pseudo_reg_mask will be called by ax_reg_mask. It add
pseudo register REG to the register mask for expression AX.

And I add function  mips_ax_pseudo_reg and mips_ax_pseudo_reg_mask for them.

After this patch, I try the GDB with KGTP in a mips64b board:
(gdb) actions
Enter actions for tracepoint 1, one per line.
End with a line saying just "end".
>collect *(unsigned char *)($sp)@512
>end
(gdb) tstart
(gdb) tstop
(gdb) tfind
Found trace frame 0, tracepoint 1
#0  vfs_readdir (file=0x9800000007b50200, filler=0xffffffff80213f10
<compat_filldir>,
    buf=0x9800000005b8fe70) at /home/teawater/kernel/linux-2.6/fs/readdir.c:25
25		struct inode *inode = file->f_path.dentry->d_inode;
(gdb) bt
#0  vfs_readdir (file=0x9800000007b50200, filler=0xffffffff80213f10
<compat_filldir>,
    buf=0x9800000005b8fe70) at /home/teawater/kernel/linux-2.6/fs/readdir.c:25
#1  0xffffffff80216208 in compat_sys_getdents (fd=<value optimized
out>, dirent=0x7fa56660, count=5960)
    at /home/teawater/kernel/linux-2.6/fs/compat.c:1017
#2  0xffffffff801121a4 in handle_sysn32 ()
    at /home/teawater/kernel/linux-2.6/arch/mips/kernel/scall64-n32.S:61


Please help me review it.

Best,
Hui


2010-12-26  Hui Zhu  <teawater@gmail.com>

	* ax-gdb.c (gen_expr): Remove pseudo-register check code.
	* ax-general.c (user-regs.h): New include.
	(ax_reg): Call gdbarch_ax_pseudo_reg.
	(ax_reg_mask): Call gdbarch_ax_pseudo_reg_mask.
	* gdbarch.sh (ax_pseudo_reg, ax_pseudo_reg_mask): new callback.
	* mips-tdep.c (ax.h): New include.
	(mips_ax_pseudo_reg, mips_ax_pseudo_reg_mask): New function.
	(mips_gdbarch_init): Set mips_ax_pseudo_reg and
	mips_ax_pseudo_reg_mask.
---
 ax-gdb.c     |    4 ---
 ax-general.c |   75 +++++++++++++++++++++++++++++++++++++++++------------------
 gdbarch.c    |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++
 gdbarch.h    |   20 +++++++++++++++
 gdbarch.sh   |   10 +++++++
 mips-tdep.c  |   55 +++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 204 insertions(+), 26 deletions(-)

--- a/ax-gdb.c
+++ b/ax-gdb.c
@@ -1978,10 +1978,6 @@ gen_expr (struct expression *exp, union
 	if (reg == -1)
 	  internal_error (__FILE__, __LINE__,
 			  _("Register $%s not available"), name);
-	if (reg >= gdbarch_num_regs (exp->gdbarch))
-	  error (_("'%s' is a pseudo-register; "
-		   "GDB cannot yet trace pseudoregister contents."),
-		 name);
 	value->kind = axs_lvalue_register;
 	value->u.reg = reg;
 	value->type = register_type (exp->gdbarch, reg);
--- a/ax-general.c
+++ b/ax-general.c
@@ -28,6 +28,8 @@
 #include "value.h"
 #include "gdb_string.h"

+#include "user-regs.h"
+
 static void grow_expr (struct agent_expr *x, int n);

 static void append_const (struct agent_expr *x, LONGEST val, int n);
@@ -272,14 +274,28 @@ ax_const_d (struct agent_expr *x, LONGES
 void
 ax_reg (struct agent_expr *x, int reg)
 {
-  /* Make sure the register number is in range.  */
-  if (reg < 0 || reg > 0xffff)
-    error (_("GDB bug: ax-general.c (ax_reg): register number out of range"));
-  grow_expr (x, 3);
-  x->buf[x->len] = aop_reg;
-  x->buf[x->len + 1] = (reg >> 8) & 0xff;
-  x->buf[x->len + 2] = (reg) & 0xff;
-  x->len += 3;
+  if (reg >= gdbarch_num_regs (x->gdbarch))
+    {
+      /* This is a pseudo-register.  */
+      if (!gdbarch_ax_pseudo_reg_p (x->gdbarch))
+	error (_("'%s' is a pseudo-register; "
+		 "GDB cannot yet trace its contents."),
+	       user_reg_map_regnum_to_name (x->gdbarch, reg));
+      if (gdbarch_ax_pseudo_reg (x->gdbarch, x, reg))
+	error (_("Trace '%s' failed."),
+	       user_reg_map_regnum_to_name (x->gdbarch, reg));
+    }
+  else
+    {
+      /* Make sure the register number is in range.  */
+      if (reg < 0 || reg > 0xffff)
+        error (_("GDB bug: ax-general.c (ax_reg): register number out
of range"));
+      grow_expr (x, 3);
+      x->buf[x->len] = aop_reg;
+      x->buf[x->len + 1] = (reg >> 8) & 0xff;
+      x->buf[x->len + 2] = (reg) & 0xff;
+      x->len += 3;
+    }
 }

 /* Assemble code to operate on a trace state variable.  */
@@ -413,23 +429,38 @@ ax_print (struct ui_file *f, struct agen
 void
 ax_reg_mask (struct agent_expr *ax, int reg)
 {
-  int byte = reg / 8;
-
-  /* Grow the bit mask if necessary.  */
-  if (byte >= ax->reg_mask_len)
+  if (reg >= gdbarch_num_regs (ax->gdbarch))
     {
-      /* It's not appropriate to double here.  This isn't a
-	 string buffer.  */
-      int new_len = byte + 1;
-      unsigned char *new_reg_mask = xrealloc (ax->reg_mask,
-					      new_len * sizeof (ax->reg_mask[0]));
-      memset (new_reg_mask + ax->reg_mask_len, 0,
-	      (new_len - ax->reg_mask_len) * sizeof (ax->reg_mask[0]));
-      ax->reg_mask_len = new_len;
-      ax->reg_mask = new_reg_mask;
+      /* This is a pseudo-register.  */
+      if (!gdbarch_ax_pseudo_reg_mask_p (ax->gdbarch))
+	error (_("'%s' is a pseudo-register; "
+		 "GDB cannot yet trace its contents."),
+	       user_reg_map_regnum_to_name (ax->gdbarch, reg));
+      if (gdbarch_ax_pseudo_reg_mask (ax->gdbarch, ax, reg))
+	error (_("Trace '%s' failed."),
+	       user_reg_map_regnum_to_name (ax->gdbarch, reg));
     }
+  else
+    {
+      int byte = reg / 8;

-  ax->reg_mask[byte] |= 1 << (reg % 8);
+      /* Grow the bit mask if necessary.  */
+      if (byte >= ax->reg_mask_len)
+        {
+          /* It's not appropriate to double here.  This isn't a
+	     string buffer.  */
+          int new_len = byte + 1;
+          unsigned char *new_reg_mask = xrealloc (ax->reg_mask,
+					          new_len
+					          * sizeof (ax->reg_mask[0]));
+          memset (new_reg_mask + ax->reg_mask_len, 0,
+	          (new_len - ax->reg_mask_len) * sizeof (ax->reg_mask[0]));
+          ax->reg_mask_len = new_len;
+          ax->reg_mask = new_reg_mask;
+        }
+
+      ax->reg_mask[byte] |= 1 << (reg % 8);
+    }
 }

 /* Given an agent expression AX, fill in requirements and other descriptive
--- a/gdbarch.c
+++ b/gdbarch.c
@@ -164,6 +164,8 @@ struct gdbarch
   gdbarch_pseudo_register_write_ftype *pseudo_register_write;
   int num_regs;
   int num_pseudo_regs;
+  gdbarch_ax_pseudo_reg_ftype *ax_pseudo_reg;
+  gdbarch_ax_pseudo_reg_mask_ftype *ax_pseudo_reg_mask;
   int sp_regnum;
   int pc_regnum;
   int ps_regnum;
@@ -314,6 +316,8 @@ struct gdbarch startup_gdbarch =
   0,  /* pseudo_register_write */
   0,  /* num_regs */
   0,  /* num_pseudo_regs */
+  0,  /* ax_pseudo_reg */
+  0,  /* ax_pseudo_reg_mask */
   -1,  /* sp_regnum */
   -1,  /* pc_regnum */
   -1,  /* ps_regnum */
@@ -594,6 +598,8 @@ verify_gdbarch (struct gdbarch *gdbarch)
   if (gdbarch->num_regs == -1)
     fprintf_unfiltered (log, "\n\tnum_regs");
   /* Skip verify of num_pseudo_regs, invalid_p == 0 */
+  /* Skip verify of ax_pseudo_reg, has predicate */
+  /* Skip verify of ax_pseudo_reg_mask, has predicate */
   /* Skip verify of sp_regnum, invalid_p == 0 */
   /* Skip verify of pc_regnum, invalid_p == 0 */
   /* Skip verify of ps_regnum, invalid_p == 0 */
@@ -761,6 +767,18 @@ gdbarch_dump (struct gdbarch *gdbarch, s
                       "gdbarch_dump: auto_wide_charset = <%s>\n",
                       host_address_to_string (gdbarch->auto_wide_charset));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: gdbarch_ax_pseudo_reg_p() = %d\n",
+                      gdbarch_ax_pseudo_reg_p (gdbarch));
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: ax_pseudo_reg = <%s>\n",
+                      host_address_to_string (gdbarch->ax_pseudo_reg));
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: gdbarch_ax_pseudo_reg_mask_p() = %d\n",
+                      gdbarch_ax_pseudo_reg_mask_p (gdbarch));
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: ax_pseudo_reg_mask = <%s>\n",
+                      host_address_to_string (gdbarch->ax_pseudo_reg_mask));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: believe_pcc_promotion = %s\n",
                       plongest (gdbarch->believe_pcc_promotion));
   fprintf_unfiltered (file,
@@ -1741,6 +1759,54 @@ set_gdbarch_num_pseudo_regs (struct gdba
 }

 int
+gdbarch_ax_pseudo_reg_p (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  return gdbarch->ax_pseudo_reg != NULL;
+}
+
+int
+gdbarch_ax_pseudo_reg (struct gdbarch *gdbarch, struct agent_expr *ax, int reg)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->ax_pseudo_reg != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_ax_pseudo_reg called\n");
+  return gdbarch->ax_pseudo_reg (gdbarch, ax, reg);
+}
+
+void
+set_gdbarch_ax_pseudo_reg (struct gdbarch *gdbarch,
+                           gdbarch_ax_pseudo_reg_ftype ax_pseudo_reg)
+{
+  gdbarch->ax_pseudo_reg = ax_pseudo_reg;
+}
+
+int
+gdbarch_ax_pseudo_reg_mask_p (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  return gdbarch->ax_pseudo_reg_mask != NULL;
+}
+
+int
+gdbarch_ax_pseudo_reg_mask (struct gdbarch *gdbarch, struct
agent_expr *ax, int reg)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->ax_pseudo_reg_mask != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_ax_pseudo_reg_mask called\n");
+  return gdbarch->ax_pseudo_reg_mask (gdbarch, ax, reg);
+}
+
+void
+set_gdbarch_ax_pseudo_reg_mask (struct gdbarch *gdbarch,
+                                gdbarch_ax_pseudo_reg_mask_ftype
ax_pseudo_reg_mask)
+{
+  gdbarch->ax_pseudo_reg_mask = ax_pseudo_reg_mask;
+}
+
+int
 gdbarch_sp_regnum (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
--- a/gdbarch.h
+++ b/gdbarch.h
@@ -53,6 +53,7 @@ struct target_desc;
 struct displaced_step_closure;
 struct core_regset_section;
 struct syscall;
+struct agent_expr;

 /* The architecture associated with the connection to the target.

@@ -232,6 +233,25 @@ extern void set_gdbarch_num_regs (struct
 extern int gdbarch_num_pseudo_regs (struct gdbarch *gdbarch);
 extern void set_gdbarch_num_pseudo_regs (struct gdbarch *gdbarch, int
num_pseudo_regs);

+/* Assemble code to push the value of pseudo register number REG on the
+   stack.
+   Return -1 if something goes wrong, 0 otherwise. */
+
+extern int gdbarch_ax_pseudo_reg_p (struct gdbarch *gdbarch);
+
+typedef int (gdbarch_ax_pseudo_reg_ftype) (struct gdbarch *gdbarch,
struct agent_expr *ax, int reg);
+extern int gdbarch_ax_pseudo_reg (struct gdbarch *gdbarch, struct
agent_expr *ax, int reg);
+extern void set_gdbarch_ax_pseudo_reg (struct gdbarch *gdbarch,
gdbarch_ax_pseudo_reg_ftype *ax_pseudo_reg);
+
+/* Add pseudo register REG to the register mask for expression AX.
+   Return -1 if something goes wrong, 0 otherwise. */
+
+extern int gdbarch_ax_pseudo_reg_mask_p (struct gdbarch *gdbarch);
+
+typedef int (gdbarch_ax_pseudo_reg_mask_ftype) (struct gdbarch
*gdbarch, struct agent_expr *ax, int reg);
+extern int gdbarch_ax_pseudo_reg_mask (struct gdbarch *gdbarch,
struct agent_expr *ax, int reg);
+extern void set_gdbarch_ax_pseudo_reg_mask (struct gdbarch *gdbarch,
gdbarch_ax_pseudo_reg_mask_ftype *ax_pseudo_reg_mask);
+
 /* GDB's standard (or well known) register numbers.  These can map onto
    a real register or a pseudo (computed) register or not be defined at
    all (-1).
--- a/gdbarch.sh
+++ b/gdbarch.sh
@@ -427,6 +427,15 @@ v:int:num_regs:::0:-1
 # combinations of other registers, or they may be computed by GDB.
 v:int:num_pseudo_regs:::0:0::0

+# Assemble code to push the value of pseudo register number REG on the
+# stack.
+# Return -1 if something goes wrong, 0 otherwise.
+M:int:ax_pseudo_reg:struct agent_expr *ax, int reg:ax, reg
+
+# Add pseudo register REG to the register mask for expression AX.
+# Return -1 if something goes wrong, 0 otherwise.
+M:int:ax_pseudo_reg_mask:struct agent_expr *ax, int reg:ax, reg
+
 # GDB's standard (or well known) register numbers.  These can map onto
 # a real register or a pseudo (computed) register or not be defined at
 # all (-1).
@@ -919,6 +928,7 @@ struct target_desc;
 struct displaced_step_closure;
 struct core_regset_section;
 struct syscall;
+struct agent_expr;

 /* The architecture associated with the connection to the target.

--- a/mips-tdep.c
+++ b/mips-tdep.c
@@ -58,6 +58,7 @@
 #include "dwarf2-frame.h"
 #include "user-regs.h"
 #include "valprint.h"
+#include "ax.h"

 static const struct objfile_data *mips_pdr_data;

@@ -616,6 +617,57 @@ mips_pseudo_register_write (struct gdbar
     internal_error (__FILE__, __LINE__, _("bad register size"));
 }

+static int
+mips_ax_pseudo_reg (struct gdbarch *gdbarch, struct agent_expr *ax, int reg)
+{
+  int rawnum = reg % gdbarch_num_regs (gdbarch);
+  gdb_assert (reg >= gdbarch_num_regs (gdbarch)
+	      && reg < 2 * gdbarch_num_regs (gdbarch));
+  if (register_size (gdbarch, rawnum) >= register_size (gdbarch, reg))
+    {
+      ax_reg (ax, rawnum);
+
+      if (register_size (gdbarch, rawnum) > register_size (gdbarch, reg))
+        {
+	  if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
+	      && gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
+	    {
+	      ax->buf[ax->len] = aop_const8;
+	      ax->buf[ax->len + 1] = 32;
+	      ax->buf[ax->len + 2] = aop_rsh_unsigned;
+	      ax->len += 3;
+	    }
+	  else
+	    {
+	      ax->buf[ax->len] = aop_const32;
+	      ax->buf[ax->len + 1] = 0xff;
+	      ax->buf[ax->len + 2] = 0xff;
+	      ax->buf[ax->len + 3] = 0xff;
+	      ax->buf[ax->len + 4] = 0xff;
+	      ax->buf[ax->len + 5] = aop_bit_and;
+	      ax->len += 6;
+	    }
+	}
+    }
+  else
+    internal_error (__FILE__, __LINE__, _("bad register size"));
+
+  return 0;
+}
+
+static int
+mips_ax_pseudo_reg_mask (struct gdbarch *gdbarch,
+			 struct agent_expr *ax, int reg)
+{
+  int rawnum = reg % gdbarch_num_regs (gdbarch);
+  gdb_assert (reg >= gdbarch_num_regs (gdbarch)
+	      && reg < 2 * gdbarch_num_regs (gdbarch));
+
+  ax_reg_mask (ax, rawnum);
+
+  return 0;
+}
+
 /* Table to translate MIPS16 register field to actual register number.  */
 static int mips16_to_32_reg[8] = { 16, 17, 2, 3, 4, 5, 6, 7 };

@@ -5933,6 +5985,9 @@ mips_gdbarch_init (struct gdbarch_info i
   set_gdbarch_pseudo_register_read (gdbarch, mips_pseudo_register_read);
   set_gdbarch_pseudo_register_write (gdbarch, mips_pseudo_register_write);

+  set_gdbarch_ax_pseudo_reg (gdbarch, mips_ax_pseudo_reg);
+  set_gdbarch_ax_pseudo_reg_mask (gdbarch, mips_ax_pseudo_reg_mask);
+
   set_gdbarch_elf_make_msymbol_special (gdbarch,
 					mips_elf_make_msymbol_special);

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Pedro Alves]

On Saturday 25 December 2010 17:09:21, Hui Zhu wrote:
> On Wed, Dec 22, 2010 at 19:26, Pedro Alves <pedro@codesourcery.com> wrote:
...
> I make a patch according to your comments.

Thanks!  I'm liking this.  Would be super cool to have these
implemented on x86/x86_64 as well.

> It add to callback in gdbarch.  They are gdbarch_ax_pseudo_reg and
> gdbarch_ax_pseudo_reg_mask.
> 
> gdbarch_ax_pseudo_reg will be called by ax_reg.  It assemble code to
> push the value of pseudo register number REG on the stack.
> gdbarch_ax_pseudo_reg_mask will be called by ax_reg_mask. It add
> pseudo register REG to the register mask for expression AX.

Well, not to the register mask.  It really isn't garanteed
that a pseudo register maps to a raw register.  It could
map to some value stored at some memory address, so the new
callback may do other things than flipping a bit
in the raw registers to collect mask (hence the ax_reg_mask function
name).  I can see why you named the callbacks like that, and I'll approve
the patch with such names anyway, though I'd prefer naming them
something more descriptive like gdbarch_ax_pseudo_register_collect /
gdbarch_ax_pseudo_register_push_stack.  E.g.,

 # Assemble agent expression bytecode to collect pseudo-register REG.
 # Return -1 if something goes wrong, 0 otherwise.
 M:int:ax_pseudo_register_collect:struct agent_expr *ax, int reg:ax, reg

 # Assemble agent expression bytecode to push the value of pseudo-register
 # REG on the interpreter stack.
 # Return -1 if something goes wrong, 0 otherwise.
 M:int:ax_pseudo_register_push_stack:struct agent_expr *ax, int reg:ax, reg

> 2010-12-26  Hui Zhu  <teawater@gmail.com>
> 
> 	* ax-gdb.c (gen_expr): Remove pseudo-register check code.
> 	* ax-general.c (user-regs.h): New include.
> 	(ax_reg): Call gdbarch_ax_pseudo_reg.
> 	(ax_reg_mask): Call gdbarch_ax_pseudo_reg_mask.
> 	* gdbarch.sh (ax_pseudo_reg, ax_pseudo_reg_mask): new callback.

Capitalize, plural: "New callbacks."

> 	* mips-tdep.c (ax.h): New include.
> 	(mips_ax_pseudo_reg, mips_ax_pseudo_reg_mask): New function.

functions.

> 	(mips_gdbarch_init): Set mips_ax_pseudo_reg and
> 	mips_ax_pseudo_reg_mask.

You need to mention that gdbarch.c and gdbarch.h were regenerated.

>  /* Given an agent expression AX, fill in requirements and other descriptive
> --- a/gdbarch.sh
> +++ b/gdbarch.sh
...
> @@ -919,6 +928,7 @@ struct target_desc;
>  struct displaced_step_closure;
>  struct core_regset_section;
>  struct syscall;
> +struct agent_expr;

You forgot to mention this in the changelog.

> +static int
> +mips_ax_pseudo_reg (struct gdbarch *gdbarch, struct agent_expr *ax, int reg)
> +{
> +  int rawnum = reg % gdbarch_num_regs (gdbarch);
> +  gdb_assert (reg >= gdbarch_num_regs (gdbarch)
> +	      && reg < 2 * gdbarch_num_regs (gdbarch));
> +  if (register_size (gdbarch, rawnum) >= register_size (gdbarch, reg))
> +    {
> +      ax_reg (ax, rawnum);
> +
> +      if (register_size (gdbarch, rawnum) > register_size (gdbarch, reg))
> +        {
> +	  if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
> +	      && gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
> +	    {
> +	      ax->buf[ax->len] = aop_const8;
> +	      ax->buf[ax->len + 1] = 32;
> +	      ax->buf[ax->len + 2] = aop_rsh_unsigned;
> +	      ax->len += 3;
> +	    }
> +	  else
> +	    {
> +	      ax->buf[ax->len] = aop_const32;
> +	      ax->buf[ax->len + 1] = 0xff;
> +	      ax->buf[ax->len + 2] = 0xff;
> +	      ax->buf[ax->len + 3] = 0xff;
> +	      ax->buf[ax->len + 4] = 0xff;
> +	      ax->buf[ax->len + 5] = aop_bit_and;
> +	      ax->len += 6;

Hmm, I'm not sure, but don't you need to sign extend?
mips-tdep.c:mips_pseudo_register_read treats this as signed.

Why do you apply the "and 0xffffffff" logic when
mips64_transfers_32bit_regs_p is false?

> +	    }

Please use the functions in ax-general.c that generate
the bytecode instead of writing to the bytecode buffer
directly (and forgetting to ensure there's enough room in
the buffer): ax_const_l/ax_simple, etc.


The patch you posted got line-wrapped/mangled, and I couldn't
apply it even after trying to manually fix it.  Can you please
make the necessary tweaks to your email client to make that
not happen?  (Me, to send patches with gmail, I find it simpler
to use an email client / SMTP than using gmail's web interface)

-- 
Pedro Alves

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 7399 bytes --]

Thanks for your help Pedro.

On Mon, Dec 27, 2010 at 21:09, Pedro Alves <pedro@codesourcery.com> wrote:
> On Saturday 25 December 2010 17:09:21, Hui Zhu wrote:
>> On Wed, Dec 22, 2010 at 19:26, Pedro Alves <pedro@codesourcery.com> wrote:
> ...
>> I make a patch according to your comments.
>
> Thanks!  I'm liking this.  Would be super cool to have these
> implemented on x86/x86_64 as well.

When this patch ok,  I will post a separate patch for it.

>
>> It add to callback in gdbarch.  They are gdbarch_ax_pseudo_reg and
>> gdbarch_ax_pseudo_reg_mask.
>>
>> gdbarch_ax_pseudo_reg will be called by ax_reg.  It assemble code to
>> push the value of pseudo register number REG on the stack.
>> gdbarch_ax_pseudo_reg_mask will be called by ax_reg_mask. It add
>> pseudo register REG to the register mask for expression AX.
>
> Well, not to the register mask.  It really isn't garanteed
> that a pseudo register maps to a raw register.  It could
> map to some value stored at some memory address, so the new
> callback may do other things than flipping a bit
> in the raw registers to collect mask (hence the ax_reg_mask function
> name).  I can see why you named the callbacks like that, and I'll approve
> the patch with such names anyway, though I'd prefer naming them
> something more descriptive like gdbarch_ax_pseudo_register_collect /
> gdbarch_ax_pseudo_register_push_stack.  E.g.,
>
>  # Assemble agent expression bytecode to collect pseudo-register REG.
>  # Return -1 if something goes wrong, 0 otherwise.
>  M:int:ax_pseudo_register_collect:struct agent_expr *ax, int reg:ax, reg
>
>  # Assemble agent expression bytecode to push the value of pseudo-register
>  # REG on the interpreter stack.
>  # Return -1 if something goes wrong, 0 otherwise.
>  M:int:ax_pseudo_register_push_stack:struct agent_expr *ax, int reg:ax, reg

Agree with you.  These names are more better than what I use.  I will use them.

>
>> 2010-12-26  Hui Zhu  <teawater@gmail.com>
>>
>>       * ax-gdb.c (gen_expr): Remove pseudo-register check code.
>>       * ax-general.c (user-regs.h): New include.
>>       (ax_reg): Call gdbarch_ax_pseudo_reg.
>>       (ax_reg_mask): Call gdbarch_ax_pseudo_reg_mask.
>>       * gdbarch.sh (ax_pseudo_reg, ax_pseudo_reg_mask): new callback.
>
> Capitalize, plural: "New callbacks."
>
>>       * mips-tdep.c (ax.h): New include.
>>       (mips_ax_pseudo_reg, mips_ax_pseudo_reg_mask): New function.
>
> functions.
>
>>       (mips_gdbarch_init): Set mips_ax_pseudo_reg and
>>       mips_ax_pseudo_reg_mask.
>
> You need to mention that gdbarch.c and gdbarch.h were regenerated.
>
>>  /* Given an agent expression AX, fill in requirements and other descriptive
>> --- a/gdbarch.sh
>> +++ b/gdbarch.sh
> ...
>> @@ -919,6 +928,7 @@ struct target_desc;
>>  struct displaced_step_closure;
>>  struct core_regset_section;
>>  struct syscall;
>> +struct agent_expr;
>
> You forgot to mention this in the changelog.
>
>> +static int
>> +mips_ax_pseudo_reg (struct gdbarch *gdbarch, struct agent_expr *ax, int reg)
>> +{
>> +  int rawnum = reg % gdbarch_num_regs (gdbarch);
>> +  gdb_assert (reg >= gdbarch_num_regs (gdbarch)
>> +           && reg < 2 * gdbarch_num_regs (gdbarch));
>> +  if (register_size (gdbarch, rawnum) >= register_size (gdbarch, reg))
>> +    {
>> +      ax_reg (ax, rawnum);
>> +
>> +      if (register_size (gdbarch, rawnum) > register_size (gdbarch, reg))
>> +        {
>> +       if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
>> +           && gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
>> +         {
>> +           ax->buf[ax->len] = aop_const8;
>> +           ax->buf[ax->len + 1] = 32;
>> +           ax->buf[ax->len + 2] = aop_rsh_unsigned;
>> +           ax->len += 3;
>> +         }
>> +       else
>> +         {
>> +           ax->buf[ax->len] = aop_const32;
>> +           ax->buf[ax->len + 1] = 0xff;
>> +           ax->buf[ax->len + 2] = 0xff;
>> +           ax->buf[ax->len + 3] = 0xff;
>> +           ax->buf[ax->len + 4] = 0xff;
>> +           ax->buf[ax->len + 5] = aop_bit_and;
>> +           ax->len += 6;
>
> Hmm, I'm not sure, but don't you need to sign extend?
> mips-tdep.c:mips_pseudo_register_read treats this as signed.
>
> Why do you apply the "and 0xffffffff" logic when
> mips64_transfers_32bit_regs_p is false?

I change this part to:
      if (register_size (gdbarch, rawnum) > register_size (gdbarch, reg))
        {
	  if (!gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
	      || gdbarch_byte_order (gdbarch) != BFD_ENDIAN_BIG)
	    {
	      ax_const_l (ax, 32);
	      ax_simple (ax, aop_lsh);
	    }
	  ax_const_l (ax, 32);
	  ax_simple (ax, aop_rsh_signed);
	}

If mips64_transfers_32bit_regs_p is false or arch is little endian,
use the low 32bit.
If not, use the the high 32bit.

Do you think it is OK?

>
>> +         }
>
> Please use the functions in ax-general.c that generate
> the bytecode instead of writing to the bytecode buffer
> directly (and forgetting to ensure there's enough room in
> the buffer): ax_const_l/ax_simple, etc.
>
>
> The patch you posted got line-wrapped/mangled, and I couldn't
> apply it even after trying to manually fix it.  Can you please
> make the necessary tweaks to your email client to make that
> not happen?  (Me, to send patches with gmail, I find it simpler
> to use an email client / SMTP than using gmail's web interface)
>

So sorry about that.
I have some net issue so I cannot use gmail with SMTP sometimes.  So I
use the attachment to send the patch.

Please help me review the new patch.

Thanks,
Hui

2010-12-28  Hui Zhu  <teawater@gmail.com>

	* gdbarch.sh (ax_pseudo_register_collect,
	ax_pseudo_register_push_stack): new callbacks.
	(agent_expr): New struct.
	* gdbarch.c (gdbarch): Add ax_pseudo_register_collect and
	ax_pseudo_register_push_stack.
	(startup_gdbarch): Add 0 for ax_pseudo_register_collect and
	ax_pseudo_register_push_stack.
	(verify_gdbarch): Add comments for ax_pseudo_register_collect and
	ax_pseudo_register_push_stack.
	(gdbarch_dump): Add fprintf_unfiltered for ax_pseudo_register_collect
	and ax_pseudo_register_push_stack.
	(gdbarch_ax_pseudo_register_collect_p,
	gdbarch_ax_pseudo_register_collect,
	set_gdbarch_ax_pseudo_register_collect,
	gdbarch_ax_pseudo_register_push_stack_p,
	gdbarch_ax_pseudo_register_push_stack,
	set_gdbarch_ax_pseudo_register_push_stack): New functions.
	* gdbarch.h (agent_expr): New struct.
	(gdbarch_ax_pseudo_register_collect_p,
	gdbarch_ax_pseudo_register_collect,
	set_gdbarch_ax_pseudo_register_collect,
	gdbarch_ax_pseudo_register_push_stack_p,
	gdbarch_ax_pseudo_register_push_stack,
	set_gdbarch_ax_pseudo_register_push_stack): New externs.
	(gdbarch_ax_pseudo_register_collect_ftype,
	gdbarch_ax_pseudo_register_push_stack_ftype): New typedeies.
	* ax-gdb.c (gen_expr): Remove pseudo-register check code.
	* ax-general.c (user-regs.h): New include.
	(ax_reg): Call gdbarch_ax_pseudo_register_push_stack.
	(ax_reg_mask): Call gdbarch_ax_pseudo_register_collect.
	* mips-tdep.c (ax.h): New include.
	(mips_ax_pseudo_register_collect,
	mips_ax_pseudo_register_push_stack): New functions.
	(mips_gdbarch_init): Set mips_ax_pseudo_register_collect and
	mips_ax_pseudo_register_push_stack.

[-- Attachment #2: trace-pseudo.txt --]
[-- Type: text/plain, Size: 13332 bytes --]

---
 ax-gdb.c     |    4 ---
 ax-general.c |   75 +++++++++++++++++++++++++++++++++++++++++------------------
 gdbarch.c    |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++
 gdbarch.h    |   20 +++++++++++++++
 gdbarch.sh   |   10 +++++++
 mips-tdep.c  |   48 +++++++++++++++++++++++++++++++++++++
 6 files changed, 197 insertions(+), 26 deletions(-)

--- a/ax-gdb.c
+++ b/ax-gdb.c
@@ -1978,10 +1978,6 @@ gen_expr (struct expression *exp, union
 	if (reg == -1)
 	  internal_error (__FILE__, __LINE__,
 			  _("Register $%s not available"), name);
-	if (reg >= gdbarch_num_regs (exp->gdbarch))
-	  error (_("'%s' is a pseudo-register; "
-		   "GDB cannot yet trace pseudoregister contents."),
-		 name);
 	value->kind = axs_lvalue_register;
 	value->u.reg = reg;
 	value->type = register_type (exp->gdbarch, reg);
--- a/ax-general.c
+++ b/ax-general.c
@@ -28,6 +28,8 @@
 #include "value.h"
 #include "gdb_string.h"
 
+#include "user-regs.h"
+
 static void grow_expr (struct agent_expr *x, int n);
 
 static void append_const (struct agent_expr *x, LONGEST val, int n);
@@ -272,14 +274,28 @@ ax_const_d (struct agent_expr *x, LONGES
 void
 ax_reg (struct agent_expr *x, int reg)
 {
-  /* Make sure the register number is in range.  */
-  if (reg < 0 || reg > 0xffff)
-    error (_("GDB bug: ax-general.c (ax_reg): register number out of range"));
-  grow_expr (x, 3);
-  x->buf[x->len] = aop_reg;
-  x->buf[x->len + 1] = (reg >> 8) & 0xff;
-  x->buf[x->len + 2] = (reg) & 0xff;
-  x->len += 3;
+  if (reg >= gdbarch_num_regs (x->gdbarch))
+    {
+      /* This is a pseudo-register.  */
+      if (!gdbarch_ax_pseudo_register_push_stack_p (x->gdbarch))
+	error (_("'%s' is a pseudo-register; "
+		 "GDB cannot yet trace its contents."),
+	       user_reg_map_regnum_to_name (x->gdbarch, reg));
+      if (gdbarch_ax_pseudo_register_push_stack (x->gdbarch, x, reg))
+	error (_("Trace '%s' failed."),
+	       user_reg_map_regnum_to_name (x->gdbarch, reg));
+    }
+  else
+    {
+      /* Make sure the register number is in range.  */
+      if (reg < 0 || reg > 0xffff)
+        error (_("GDB bug: ax-general.c (ax_reg): register number out of range"));
+      grow_expr (x, 3);
+      x->buf[x->len] = aop_reg;
+      x->buf[x->len + 1] = (reg >> 8) & 0xff;
+      x->buf[x->len + 2] = (reg) & 0xff;
+      x->len += 3;
+    }
 }
 
 /* Assemble code to operate on a trace state variable.  */
@@ -413,23 +429,38 @@ ax_print (struct ui_file *f, struct agen
 void
 ax_reg_mask (struct agent_expr *ax, int reg)
 {
-  int byte = reg / 8;
-
-  /* Grow the bit mask if necessary.  */
-  if (byte >= ax->reg_mask_len)
+  if (reg >= gdbarch_num_regs (ax->gdbarch))
     {
-      /* It's not appropriate to double here.  This isn't a
-	 string buffer.  */
-      int new_len = byte + 1;
-      unsigned char *new_reg_mask = xrealloc (ax->reg_mask,
-					      new_len * sizeof (ax->reg_mask[0]));
-      memset (new_reg_mask + ax->reg_mask_len, 0,
-	      (new_len - ax->reg_mask_len) * sizeof (ax->reg_mask[0]));
-      ax->reg_mask_len = new_len;
-      ax->reg_mask = new_reg_mask;
+      /* This is a pseudo-register.  */
+      if (!gdbarch_ax_pseudo_register_collect_p (ax->gdbarch))
+	error (_("'%s' is a pseudo-register; "
+		 "GDB cannot yet trace its contents."),
+	       user_reg_map_regnum_to_name (ax->gdbarch, reg));
+      if (gdbarch_ax_pseudo_register_collect (ax->gdbarch, ax, reg))
+	error (_("Trace '%s' failed."),
+	       user_reg_map_regnum_to_name (ax->gdbarch, reg));
     }
+  else
+    {
+      int byte = reg / 8;
 
-  ax->reg_mask[byte] |= 1 << (reg % 8);
+      /* Grow the bit mask if necessary.  */
+      if (byte >= ax->reg_mask_len)
+        {
+          /* It's not appropriate to double here.  This isn't a
+	     string buffer.  */
+          int new_len = byte + 1;
+          unsigned char *new_reg_mask = xrealloc (ax->reg_mask,
+					          new_len
+					          * sizeof (ax->reg_mask[0]));
+          memset (new_reg_mask + ax->reg_mask_len, 0,
+	          (new_len - ax->reg_mask_len) * sizeof (ax->reg_mask[0]));
+          ax->reg_mask_len = new_len;
+          ax->reg_mask = new_reg_mask;
+        }
+
+      ax->reg_mask[byte] |= 1 << (reg % 8);
+    }
 }
 
 /* Given an agent expression AX, fill in requirements and other descriptive
--- a/gdbarch.c
+++ b/gdbarch.c
@@ -164,6 +164,8 @@ struct gdbarch
   gdbarch_pseudo_register_write_ftype *pseudo_register_write;
   int num_regs;
   int num_pseudo_regs;
+  gdbarch_ax_pseudo_register_collect_ftype *ax_pseudo_register_collect;
+  gdbarch_ax_pseudo_register_push_stack_ftype *ax_pseudo_register_push_stack;
   int sp_regnum;
   int pc_regnum;
   int ps_regnum;
@@ -314,6 +316,8 @@ struct gdbarch startup_gdbarch =
   0,  /* pseudo_register_write */
   0,  /* num_regs */
   0,  /* num_pseudo_regs */
+  0,  /* ax_pseudo_register_collect */
+  0,  /* ax_pseudo_register_push_stack */
   -1,  /* sp_regnum */
   -1,  /* pc_regnum */
   -1,  /* ps_regnum */
@@ -594,6 +598,8 @@ verify_gdbarch (struct gdbarch *gdbarch)
   if (gdbarch->num_regs == -1)
     fprintf_unfiltered (log, "\n\tnum_regs");
   /* Skip verify of num_pseudo_regs, invalid_p == 0 */
+  /* Skip verify of ax_pseudo_register_collect, has predicate */
+  /* Skip verify of ax_pseudo_register_push_stack, has predicate */
   /* Skip verify of sp_regnum, invalid_p == 0 */
   /* Skip verify of pc_regnum, invalid_p == 0 */
   /* Skip verify of ps_regnum, invalid_p == 0 */
@@ -761,6 +767,18 @@ gdbarch_dump (struct gdbarch *gdbarch, s
                       "gdbarch_dump: auto_wide_charset = <%s>\n",
                       host_address_to_string (gdbarch->auto_wide_charset));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: gdbarch_ax_pseudo_register_collect_p() = %d\n",
+                      gdbarch_ax_pseudo_register_collect_p (gdbarch));
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: ax_pseudo_register_collect = <%s>\n",
+                      host_address_to_string (gdbarch->ax_pseudo_register_collect));
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: gdbarch_ax_pseudo_register_push_stack_p() = %d\n",
+                      gdbarch_ax_pseudo_register_push_stack_p (gdbarch));
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: ax_pseudo_register_push_stack = <%s>\n",
+                      host_address_to_string (gdbarch->ax_pseudo_register_push_stack));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: believe_pcc_promotion = %s\n",
                       plongest (gdbarch->believe_pcc_promotion));
   fprintf_unfiltered (file,
@@ -1741,6 +1759,54 @@ set_gdbarch_num_pseudo_regs (struct gdba
 }
 
 int
+gdbarch_ax_pseudo_register_collect_p (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  return gdbarch->ax_pseudo_register_collect != NULL;
+}
+
+int
+gdbarch_ax_pseudo_register_collect (struct gdbarch *gdbarch, struct agent_expr *ax, int reg)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->ax_pseudo_register_collect != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_ax_pseudo_register_collect called\n");
+  return gdbarch->ax_pseudo_register_collect (gdbarch, ax, reg);
+}
+
+void
+set_gdbarch_ax_pseudo_register_collect (struct gdbarch *gdbarch,
+                                        gdbarch_ax_pseudo_register_collect_ftype ax_pseudo_register_collect)
+{
+  gdbarch->ax_pseudo_register_collect = ax_pseudo_register_collect;
+}
+
+int
+gdbarch_ax_pseudo_register_push_stack_p (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  return gdbarch->ax_pseudo_register_push_stack != NULL;
+}
+
+int
+gdbarch_ax_pseudo_register_push_stack (struct gdbarch *gdbarch, struct agent_expr *ax, int reg)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->ax_pseudo_register_push_stack != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_ax_pseudo_register_push_stack called\n");
+  return gdbarch->ax_pseudo_register_push_stack (gdbarch, ax, reg);
+}
+
+void
+set_gdbarch_ax_pseudo_register_push_stack (struct gdbarch *gdbarch,
+                                           gdbarch_ax_pseudo_register_push_stack_ftype ax_pseudo_register_push_stack)
+{
+  gdbarch->ax_pseudo_register_push_stack = ax_pseudo_register_push_stack;
+}
+
+int
 gdbarch_sp_regnum (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
--- a/gdbarch.h
+++ b/gdbarch.h
@@ -53,6 +53,7 @@ struct target_desc;
 struct displaced_step_closure;
 struct core_regset_section;
 struct syscall;
+struct agent_expr;
 
 /* The architecture associated with the connection to the target.
  
@@ -232,6 +233,25 @@ extern void set_gdbarch_num_regs (struct
 extern int gdbarch_num_pseudo_regs (struct gdbarch *gdbarch);
 extern void set_gdbarch_num_pseudo_regs (struct gdbarch *gdbarch, int num_pseudo_regs);
 
+/* Assemble agent expression bytecode to collect pseudo-register REG.
+   Return -1 if something goes wrong, 0 otherwise. */
+
+extern int gdbarch_ax_pseudo_register_collect_p (struct gdbarch *gdbarch);
+
+typedef int (gdbarch_ax_pseudo_register_collect_ftype) (struct gdbarch *gdbarch, struct agent_expr *ax, int reg);
+extern int gdbarch_ax_pseudo_register_collect (struct gdbarch *gdbarch, struct agent_expr *ax, int reg);
+extern void set_gdbarch_ax_pseudo_register_collect (struct gdbarch *gdbarch, gdbarch_ax_pseudo_register_collect_ftype *ax_pseudo_register_collect);
+
+/* Assemble agent expression bytecode to push the value of pseudo-register
+   REG on the interpreter stack.
+   Return -1 if something goes wrong, 0 otherwise. */
+
+extern int gdbarch_ax_pseudo_register_push_stack_p (struct gdbarch *gdbarch);
+
+typedef int (gdbarch_ax_pseudo_register_push_stack_ftype) (struct gdbarch *gdbarch, struct agent_expr *ax, int reg);
+extern int gdbarch_ax_pseudo_register_push_stack (struct gdbarch *gdbarch, struct agent_expr *ax, int reg);
+extern void set_gdbarch_ax_pseudo_register_push_stack (struct gdbarch *gdbarch, gdbarch_ax_pseudo_register_push_stack_ftype *ax_pseudo_register_push_stack);
+
 /* GDB's standard (or well known) register numbers.  These can map onto
    a real register or a pseudo (computed) register or not be defined at
    all (-1).
--- a/gdbarch.sh
+++ b/gdbarch.sh
@@ -427,6 +427,15 @@ v:int:num_regs:::0:-1
 # combinations of other registers, or they may be computed by GDB.
 v:int:num_pseudo_regs:::0:0::0
 
+# Assemble agent expression bytecode to collect pseudo-register REG.
+# Return -1 if something goes wrong, 0 otherwise.
+M:int:ax_pseudo_register_collect:struct agent_expr *ax, int reg:ax, reg
+
+# Assemble agent expression bytecode to push the value of pseudo-register
+# REG on the interpreter stack.
+# Return -1 if something goes wrong, 0 otherwise.
+M:int:ax_pseudo_register_push_stack:struct agent_expr *ax, int reg:ax, reg
+
 # GDB's standard (or well known) register numbers.  These can map onto
 # a real register or a pseudo (computed) register or not be defined at
 # all (-1).
@@ -919,6 +928,7 @@ struct target_desc;
 struct displaced_step_closure;
 struct core_regset_section;
 struct syscall;
+struct agent_expr;
 
 /* The architecture associated with the connection to the target.
  
--- a/mips-tdep.c
+++ b/mips-tdep.c
@@ -58,6 +58,7 @@
 #include "dwarf2-frame.h"
 #include "user-regs.h"
 #include "valprint.h"
+#include "ax.h"
 
 static const struct objfile_data *mips_pdr_data;
 
@@ -616,6 +617,48 @@ mips_pseudo_register_write (struct gdbar
     internal_error (__FILE__, __LINE__, _("bad register size"));
 }
 
+static int
+mips_ax_pseudo_register_collect (struct gdbarch *gdbarch,
+				 struct agent_expr *ax, int reg)
+{
+  int rawnum = reg % gdbarch_num_regs (gdbarch);
+  gdb_assert (reg >= gdbarch_num_regs (gdbarch)
+	      && reg < 2 * gdbarch_num_regs (gdbarch));
+
+  ax_reg_mask (ax, rawnum);
+
+  return 0;
+}
+
+static int
+mips_ax_pseudo_register_push_stack (struct gdbarch *gdbarch,
+				    struct agent_expr *ax, int reg)
+{
+  int rawnum = reg % gdbarch_num_regs (gdbarch);
+  gdb_assert (reg >= gdbarch_num_regs (gdbarch)
+	      && reg < 2 * gdbarch_num_regs (gdbarch));
+  if (register_size (gdbarch, rawnum) >= register_size (gdbarch, reg))
+    {
+      ax_reg (ax, rawnum);
+
+      if (register_size (gdbarch, rawnum) > register_size (gdbarch, reg))
+        {
+	  if (!gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
+	      || gdbarch_byte_order (gdbarch) != BFD_ENDIAN_BIG)
+	    {
+	      ax_const_l (ax, 32);
+	      ax_simple (ax, aop_lsh);
+	    }
+	  ax_const_l (ax, 32);
+	  ax_simple (ax, aop_rsh_signed);
+	}
+    }
+  else
+    internal_error (__FILE__, __LINE__, _("bad register size"));
+
+  return 0;
+}
+
 /* Table to translate MIPS16 register field to actual register number.  */
 static int mips16_to_32_reg[8] = { 16, 17, 2, 3, 4, 5, 6, 7 };
 
@@ -5933,6 +5976,11 @@ mips_gdbarch_init (struct gdbarch_info i
   set_gdbarch_pseudo_register_read (gdbarch, mips_pseudo_register_read);
   set_gdbarch_pseudo_register_write (gdbarch, mips_pseudo_register_write);
 
+  set_gdbarch_ax_pseudo_register_collect (gdbarch,
+					  mips_ax_pseudo_register_collect);
+  set_gdbarch_ax_pseudo_register_push_stack
+      (gdbarch, mips_ax_pseudo_register_push_stack);
+
   set_gdbarch_elf_make_msymbol_special (gdbarch,
 					mips_elf_make_msymbol_special);
 

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Pedro Alves]

On Tuesday 28 December 2010 08:01:47, Hui Zhu wrote:

> >> +static int
> >> +mips_ax_pseudo_reg (struct gdbarch *gdbarch, struct agent_expr *ax, int reg)
> >> +{
> >> +  int rawnum = reg % gdbarch_num_regs (gdbarch);
> >> +  gdb_assert (reg >= gdbarch_num_regs (gdbarch)
> >> +           && reg < 2 * gdbarch_num_regs (gdbarch));
> >> +  if (register_size (gdbarch, rawnum) >= register_size (gdbarch, reg))
> >> +    {
> >> +      ax_reg (ax, rawnum);
> >> +
> >> +      if (register_size (gdbarch, rawnum) > register_size (gdbarch, reg))
> >> +        {
> >> +       if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
> >> +           && gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
> >> +         {
> >> +           ax->buf[ax->len] = aop_const8;
> >> +           ax->buf[ax->len + 1] = 32;
> >> +           ax->buf[ax->len + 2] = aop_rsh_unsigned;
> >> +           ax->len += 3;
> >> +         }
> >> +       else
> >> +         {
> >> +           ax->buf[ax->len] = aop_const32;
> >> +           ax->buf[ax->len + 1] = 0xff;
> >> +           ax->buf[ax->len + 2] = 0xff;
> >> +           ax->buf[ax->len + 3] = 0xff;
> >> +           ax->buf[ax->len + 4] = 0xff;
> >> +           ax->buf[ax->len + 5] = aop_bit_and;
> >> +           ax->len += 6;
> >
> > Hmm, I'm not sure, but don't you need to sign extend?
> > mips-tdep.c:mips_pseudo_register_read treats this as signed.
> >
> > Why do you apply the "and 0xffffffff" logic when
> > mips64_transfers_32bit_regs_p is false?
> 
> I change this part to:
>       if (register_size (gdbarch, rawnum) > register_size (gdbarch, reg))
>         {
> 	  if (!gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
> 	      || gdbarch_byte_order (gdbarch) != BFD_ENDIAN_BIG)
> 	    {
> 	      ax_const_l (ax, 32);
> 	      ax_simple (ax, aop_lsh);
> 	    }
> 	  ax_const_l (ax, 32);
> 	  ax_simple (ax, aop_rsh_signed);
> 	}
> 
> If mips64_transfers_32bit_regs_p is false or arch is little endian,
> use the low 32bit.
> If not, use the the high 32bit.
> 
> Do you think it is OK?

I'm not sure I understand the double shift logic, but I also don't
care enough to spend the time to understand it either.  :-)  You're now
using the right interfaces, and if you've tested this (e.g., try
collecting an expression that involves adding and subtracting values
of registers, and see if the result is sane, and/or if your agent supports
it, cooking up a small test that involves a conditional tracepoint
involving arithmetic with register values, all this with a 32-bit app
running on a 64-bit board), then it's fine with me.
I also have to say that I'm also not sure what does 
mips64_transfers_32bit_regs_p being true mean WRT the agent's register
size (as opposed to the remote protocol register size), but, I understand
that's a legacy mode, so, we can adjust if turns out we need to do something
else.


> 2010-12-28  Hui Zhu  <teawater@gmail.com>
> 
> 	* gdbarch.sh (ax_pseudo_register_collect,
> 	ax_pseudo_register_push_stack): new callbacks.
> 	(agent_expr): New struct.

Not really new.  Use something like "Forward declare." instead.

> 	* gdbarch.c (gdbarch): Add ax_pseudo_register_collect and
> 	ax_pseudo_register_push_stack.
> 	(startup_gdbarch): Add 0 for ax_pseudo_register_collect and
> 	ax_pseudo_register_push_stack.
> 	(verify_gdbarch): Add comments for ax_pseudo_register_collect and
> 	ax_pseudo_register_push_stack.
> 	(gdbarch_dump): Add fprintf_unfiltered for ax_pseudo_register_collect
> 	and ax_pseudo_register_push_stack.
> 	(gdbarch_ax_pseudo_register_collect_p,
> 	gdbarch_ax_pseudo_register_collect,
> 	set_gdbarch_ax_pseudo_register_collect,
> 	gdbarch_ax_pseudo_register_push_stack_p,
> 	gdbarch_ax_pseudo_register_push_stack,
> 	set_gdbarch_ax_pseudo_register_push_stack): New functions.
> 	* gdbarch.h (agent_expr): New struct.
> 	(gdbarch_ax_pseudo_register_collect_p,
> 	gdbarch_ax_pseudo_register_collect,
> 	set_gdbarch_ax_pseudo_register_collect,
> 	gdbarch_ax_pseudo_register_push_stack_p,
> 	gdbarch_ax_pseudo_register_push_stack,
> 	set_gdbarch_ax_pseudo_register_push_stack): New externs.
> 	(gdbarch_ax_pseudo_register_collect_ftype,
> 	gdbarch_ax_pseudo_register_push_stack_ftype): New typedeies.

That's not what I meant with

> > You need to mention that gdbarch.c and gdbarch.h were regenerated.

Write this, literaly:

	* gdbarch.h, gdbarch.c: Regenerate.

That's all.

> 	* ax-gdb.c (gen_expr): Remove pseudo-register check code.
> 	* ax-general.c (user-regs.h): New include.
> 	(ax_reg): Call gdbarch_ax_pseudo_register_push_stack.
> 	(ax_reg_mask): Call gdbarch_ax_pseudo_register_collect.
> 	* mips-tdep.c (ax.h): New include.
> 	(mips_ax_pseudo_register_collect,
> 	mips_ax_pseudo_register_push_stack): New functions.
> 	(mips_gdbarch_init): Set mips_ax_pseudo_register_collect and
> 	mips_ax_pseudo_register_push_stack.

Otherwise okay.  Thanks!

-- 
Pedro Alves

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Hui Zhu]

Thanks Pedro.  Fixed and checked in.

Joel,  do you think this patch can check in to 7.2.1.

Thanks,
Hui

On Tue, Dec 28, 2010 at 17:52, Pedro Alves <pedro@codesourcery.com> wrote:
> On Tuesday 28 December 2010 08:01:47, Hui Zhu wrote:
>
>> >> +static int
>> >> +mips_ax_pseudo_reg (struct gdbarch *gdbarch, struct agent_expr *ax, int reg)
>> >> +{
>> >> +  int rawnum = reg % gdbarch_num_regs (gdbarch);
>> >> +  gdb_assert (reg >= gdbarch_num_regs (gdbarch)
>> >> +           && reg < 2 * gdbarch_num_regs (gdbarch));
>> >> +  if (register_size (gdbarch, rawnum) >= register_size (gdbarch, reg))
>> >> +    {
>> >> +      ax_reg (ax, rawnum);
>> >> +
>> >> +      if (register_size (gdbarch, rawnum) > register_size (gdbarch, reg))
>> >> +        {
>> >> +       if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
>> >> +           && gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
>> >> +         {
>> >> +           ax->buf[ax->len] = aop_const8;
>> >> +           ax->buf[ax->len + 1] = 32;
>> >> +           ax->buf[ax->len + 2] = aop_rsh_unsigned;
>> >> +           ax->len += 3;
>> >> +         }
>> >> +       else
>> >> +         {
>> >> +           ax->buf[ax->len] = aop_const32;
>> >> +           ax->buf[ax->len + 1] = 0xff;
>> >> +           ax->buf[ax->len + 2] = 0xff;
>> >> +           ax->buf[ax->len + 3] = 0xff;
>> >> +           ax->buf[ax->len + 4] = 0xff;
>> >> +           ax->buf[ax->len + 5] = aop_bit_and;
>> >> +           ax->len += 6;
>> >
>> > Hmm, I'm not sure, but don't you need to sign extend?
>> > mips-tdep.c:mips_pseudo_register_read treats this as signed.
>> >
>> > Why do you apply the "and 0xffffffff" logic when
>> > mips64_transfers_32bit_regs_p is false?
>>
>> I change this part to:
>>       if (register_size (gdbarch, rawnum) > register_size (gdbarch, reg))
>>         {
>>         if (!gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
>>             || gdbarch_byte_order (gdbarch) != BFD_ENDIAN_BIG)
>>           {
>>             ax_const_l (ax, 32);
>>             ax_simple (ax, aop_lsh);
>>           }
>>         ax_const_l (ax, 32);
>>         ax_simple (ax, aop_rsh_signed);
>>       }
>>
>> If mips64_transfers_32bit_regs_p is false or arch is little endian,
>> use the low 32bit.
>> If not, use the the high 32bit.
>>
>> Do you think it is OK?
>
> I'm not sure I understand the double shift logic, but I also don't
> care enough to spend the time to understand it either.  :-)  You're now
> using the right interfaces, and if you've tested this (e.g., try
> collecting an expression that involves adding and subtracting values
> of registers, and see if the result is sane, and/or if your agent supports
> it, cooking up a small test that involves a conditional tracepoint
> involving arithmetic with register values, all this with a 32-bit app
> running on a 64-bit board), then it's fine with me.
> I also have to say that I'm also not sure what does
> mips64_transfers_32bit_regs_p being true mean WRT the agent's register
> size (as opposed to the remote protocol register size), but, I understand
> that's a legacy mode, so, we can adjust if turns out we need to do something
> else.
>
>
>> 2010-12-28  Hui Zhu  <teawater@gmail.com>
>>
>>       * gdbarch.sh (ax_pseudo_register_collect,
>>       ax_pseudo_register_push_stack): new callbacks.
>>       (agent_expr): New struct.
>
> Not really new.  Use something like "Forward declare." instead.
>
>>       * gdbarch.c (gdbarch): Add ax_pseudo_register_collect and
>>       ax_pseudo_register_push_stack.
>>       (startup_gdbarch): Add 0 for ax_pseudo_register_collect and
>>       ax_pseudo_register_push_stack.
>>       (verify_gdbarch): Add comments for ax_pseudo_register_collect and
>>       ax_pseudo_register_push_stack.
>>       (gdbarch_dump): Add fprintf_unfiltered for ax_pseudo_register_collect
>>       and ax_pseudo_register_push_stack.
>>       (gdbarch_ax_pseudo_register_collect_p,
>>       gdbarch_ax_pseudo_register_collect,
>>       set_gdbarch_ax_pseudo_register_collect,
>>       gdbarch_ax_pseudo_register_push_stack_p,
>>       gdbarch_ax_pseudo_register_push_stack,
>>       set_gdbarch_ax_pseudo_register_push_stack): New functions.
>>       * gdbarch.h (agent_expr): New struct.
>>       (gdbarch_ax_pseudo_register_collect_p,
>>       gdbarch_ax_pseudo_register_collect,
>>       set_gdbarch_ax_pseudo_register_collect,
>>       gdbarch_ax_pseudo_register_push_stack_p,
>>       gdbarch_ax_pseudo_register_push_stack,
>>       set_gdbarch_ax_pseudo_register_push_stack): New externs.
>>       (gdbarch_ax_pseudo_register_collect_ftype,
>>       gdbarch_ax_pseudo_register_push_stack_ftype): New typedeies.
>
> That's not what I meant with
>
>> > You need to mention that gdbarch.c and gdbarch.h were regenerated.
>
> Write this, literaly:
>
>        * gdbarch.h, gdbarch.c: Regenerate.
>
> That's all.
>
>>       * ax-gdb.c (gen_expr): Remove pseudo-register check code.
>>       * ax-general.c (user-regs.h): New include.
>>       (ax_reg): Call gdbarch_ax_pseudo_register_push_stack.
>>       (ax_reg_mask): Call gdbarch_ax_pseudo_register_collect.
>>       * mips-tdep.c (ax.h): New include.
>>       (mips_ax_pseudo_register_collect,
>>       mips_ax_pseudo_register_push_stack): New functions.
>>       (mips_gdbarch_init): Set mips_ax_pseudo_register_collect and
>>       mips_ax_pseudo_register_push_stack.
>
> Otherwise okay.  Thanks!
>
> --
> Pedro Alves
>

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Joel Brobecker]

> Joel,  do you think this patch can check in to 7.2.1.

I think that it's borderline, and I'm tempted to say no, considering
that this is not a crash or a regression (just a limitation).  But
it looks relatively safe to me.  So, if Pedro agrees, it's OK with me.

-- 
Joel

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Pedro Alves]

On Tuesday 28 December 2010 17:08:52, Joel Brobecker wrote:
> > Joel,  do you think this patch can check in to 7.2.1.
> 
> I think that it's borderline, and I'm tempted to say no, considering
> that this is not a crash or a regression (just a limitation).  But
> it looks relatively safe to me.  So, if Pedro agrees, it's OK with me.

Yeah.  I understand that without this patch, MIPS tracepoints are
practically useless on 7.2, so on those grounds, I'd be okay.

However, I just went to check whether collecting a pseudo or
user register in x86/x86_64 still gives a reasonable error, and
I now get:

 >./gdb -q ./gdb
 Reading symbols from /home/pedro/gdb/baseline/build/gdb/gdb...done.
 (top-gdb) start
 Temporary breakpoint 3 at 0x4565c3: file ../../src/gdb/gdb.c, line 29.
 Starting program: /home/pedro/gdb/baseline/build/gdb/gdb 
 [Thread debugging using libthread_db enabled]

 Temporary breakpoint 3, main (argc=1, argv=0x7fffffffe108) at ../../src/gdb/gdb.c:29
 29        memset (&args, 0, sizeof args);
 (top-gdb) maint agent $sp
 ../../src/gdb/regcache.c:166: internal-error: register_type: Assertion `regnum >= 0 && regnum < descr->nr_cooked_registers' failed.
 A problem internal to GDB has been detected,
 further debugging may prove unreliable.
 Quit this debugging session? (y or n) 

While before (on 7.2), we'd get:

 (top-gdb) maint agent $sp
 'sp' is a pseudo-register; GDB cannot yet trace pseudoregister contents.

We'll need to get this fixed this before considering a backport to 7.2.

-- 
Pedro Alves

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Pedro Alves]

On Tuesday 28 December 2010 17:23:59, Pedro Alves wrote:
> On Tuesday 28 December 2010 17:08:52, Joel Brobecker wrote:
> > > Joel,  do you think this patch can check in to 7.2.1.
> > 
> > I think that it's borderline, and I'm tempted to say no, considering
> > that this is not a crash or a regression (just a limitation).  But
> > it looks relatively safe to me.  So, if Pedro agrees, it's OK with me.
> 
> Yeah.  I understand that without this patch, MIPS tracepoints are
> practically useless on 7.2, so on those grounds, I'd be okay.
> 
> However, I just went to check whether collecting a pseudo or
> user register in x86/x86_64 still gives a reasonable error, and
> I now get:
> 
>  >./gdb -q ./gdb
>  Reading symbols from /home/pedro/gdb/baseline/build/gdb/gdb...done.
>  (top-gdb) start
>  Temporary breakpoint 3 at 0x4565c3: file ../../src/gdb/gdb.c, line 29.
>  Starting program: /home/pedro/gdb/baseline/build/gdb/gdb 
>  [Thread debugging using libthread_db enabled]
> 
>  Temporary breakpoint 3, main (argc=1, argv=0x7fffffffe108) at ../../src/gdb/gdb.c:29
>  29        memset (&args, 0, sizeof args);
>  (top-gdb) maint agent $sp
>  ../../src/gdb/regcache.c:166: internal-error: register_type: Assertion `regnum >= 0 && regnum < descr->nr_cooked_registers' failed.
>  A problem internal to GDB has been detected,
>  further debugging may prove unreliable.
>  Quit this debugging session? (y or n) 
> 
> While before (on 7.2), we'd get:
> 
>  (top-gdb) maint agent $sp
>  'sp' is a pseudo-register; GDB cannot yet trace pseudoregister contents.
> 
> We'll need to get this fixed this before considering a backport to 7.2.
> 
> 

Fixed for now.  I've reinstated the exact same old error, but under
a stricter check: we now error out for user registers, but let
pseudo-registers pass:

 (top-gdb) maint agent $pc
 'pc' is a pseudo-register; GDB cannot yet trace pseudoregister contents.

Pseudo-registers will error out further on, on the new errors that Hui
added when the new gdbarch callbacks aren't implemented.  On x86_64:

 (top-gdb) maint agent $rsp
 Scope: 0x4565c3
 Reg mask: 80
   0  end

 (top-gdb) maint agent $esp
 'esp' is a pseudo-register; GDB cannot yet trace its contents.

-- 
Pedro Alves

2010-12-28  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* ax-gdb.c (gen_expr) <OP_REGISTER>: Error out if trying to
	collect a user register.
	
---
 gdb/ax-gdb.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: src/gdb/ax-gdb.c
===================================================================
--- src.orig/gdb/ax-gdb.c	2010-12-28 16:22:01.000000000 +0000
+++ src/gdb/ax-gdb.c	2010-12-28 17:54:45.000000000 +0000
@@ -1978,6 +1978,12 @@ gen_expr (struct expression *exp, union 
 	if (reg == -1)
 	  internal_error (__FILE__, __LINE__,
 			  _("Register $%s not available"), name);
+	/* No support for tracing user registers yet.  */
+	if (reg >= gdbarch_num_regs (exp->gdbarch)
+	    + gdbarch_num_pseudo_regs (exp->gdbarch))
+	  error (_("'%s' is a pseudo-register; "
+		   "GDB cannot yet trace pseudoregister contents."),
+		 name);
 	value->kind = axs_lvalue_register;
 	value->u.reg = reg;
 	value->type = register_type (exp->gdbarch, reg);

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 4245 bytes --]

On Wed, Dec 29, 2010 at 02:04, Pedro Alves <pedro@codesourcery.com> wrote:
> On Tuesday 28 December 2010 17:23:59, Pedro Alves wrote:
>> On Tuesday 28 December 2010 17:08:52, Joel Brobecker wrote:
>> > > Joel,  do you think this patch can check in to 7.2.1.
>> >
>> > I think that it's borderline, and I'm tempted to say no, considering
>> > that this is not a crash or a regression (just a limitation).  But
>> > it looks relatively safe to me.  So, if Pedro agrees, it's OK with me.
>>
>> Yeah.  I understand that without this patch, MIPS tracepoints are
>> practically useless on 7.2, so on those grounds, I'd be okay.
>>
>> However, I just went to check whether collecting a pseudo or
>> user register in x86/x86_64 still gives a reasonable error, and
>> I now get:
>>
>>  >./gdb -q ./gdb
>>  Reading symbols from /home/pedro/gdb/baseline/build/gdb/gdb...done.
>>  (top-gdb) start
>>  Temporary breakpoint 3 at 0x4565c3: file ../../src/gdb/gdb.c, line 29.
>>  Starting program: /home/pedro/gdb/baseline/build/gdb/gdb
>>  [Thread debugging using libthread_db enabled]
>>
>>  Temporary breakpoint 3, main (argc=1, argv=0x7fffffffe108) at ../../src/gdb/gdb.c:29
>>  29        memset (&args, 0, sizeof args);
>>  (top-gdb) maint agent $sp
>>  ../../src/gdb/regcache.c:166: internal-error: register_type: Assertion `regnum >= 0 && regnum < descr->nr_cooked_registers' failed.
>>  A problem internal to GDB has been detected,
>>  further debugging may prove unreliable.
>>  Quit this debugging session? (y or n)
>>
>> While before (on 7.2), we'd get:
>>
>>  (top-gdb) maint agent $sp
>>  'sp' is a pseudo-register; GDB cannot yet trace pseudoregister contents.
>>
>> We'll need to get this fixed this before considering a backport to 7.2.
>>
>>
>
> Fixed for now.  I've reinstated the exact same old error, but under
> a stricter check: we now error out for user registers, but let
> pseudo-registers pass:
>
>  (top-gdb) maint agent $pc
>  'pc' is a pseudo-register; GDB cannot yet trace pseudoregister contents.
>
> Pseudo-registers will error out further on, on the new errors that Hui
> added when the new gdbarch callbacks aren't implemented.  On x86_64:
>
>  (top-gdb) maint agent $rsp
>  Scope: 0x4565c3
>  Reg mask: 80
>   0  end
>
>  (top-gdb) maint agent $esp
>  'esp' is a pseudo-register; GDB cannot yet trace its contents.
>
> --
> Pedro Alves
>
> 2010-12-28  Pedro Alves  <pedro@codesourcery.com>
>
>        gdb/
>        * ax-gdb.c (gen_expr) <OP_REGISTER>: Error out if trying to
>        collect a user register.
>
> ---
>  gdb/ax-gdb.c |    6 ++++++
>  1 file changed, 6 insertions(+)
>
> Index: src/gdb/ax-gdb.c
> ===================================================================
> --- src.orig/gdb/ax-gdb.c       2010-12-28 16:22:01.000000000 +0000
> +++ src/gdb/ax-gdb.c    2010-12-28 17:54:45.000000000 +0000
> @@ -1978,6 +1978,12 @@ gen_expr (struct expression *exp, union
>        if (reg == -1)
>          internal_error (__FILE__, __LINE__,
>                          _("Register $%s not available"), name);
> +       /* No support for tracing user registers yet.  */
> +       if (reg >= gdbarch_num_regs (exp->gdbarch)
> +           + gdbarch_num_pseudo_regs (exp->gdbarch))
> +         error (_("'%s' is a pseudo-register; "
> +                  "GDB cannot yet trace pseudoregister contents."),
> +                name);
>        value->kind = axs_lvalue_register;
>        value->u.reg = reg;
>        value->type = register_type (exp->gdbarch, reg);
>

(top-gdb) maintenance agent $pc
'pc' is a pseudo-register; GDB cannot yet trace pseudoregister contents.
(top-gdb) maintenance agent $ax
'ax' is a pseudo-register; GDB cannot yet trace its contents.

This place is a bit confusing.  Do you think we change the
"pseudo-register" to "user-register" in gen_expr?

For example:
(top-gdb) maintenance agent $pc
'pc' is a user-register; GDB cannot yet trace user-register contents.
(top-gdb) maintenance agent $ax
'ax' is a pseudo-register; GDB cannot yet trace its contents.


I make a patch for it.

Thanks,
Hui

2010-12-29  Hui Zhu  <teawater@gmail.com>

	* ax-gdb.c (gen_expr): Change error message.

[-- Attachment #2: gen_expr_change_message.txt --]
[-- Type: text/plain, Size: 578 bytes --]

---
 ax-gdb.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/ax-gdb.c
+++ b/ax-gdb.c
@@ -1981,8 +1981,8 @@ gen_expr (struct expression *exp, union
 	/* No support for tracing user registers yet.  */
 	if (reg >= gdbarch_num_regs (exp->gdbarch)
 	    + gdbarch_num_pseudo_regs (exp->gdbarch))
-	  error (_("'%s' is a pseudo-register; "
-		   "GDB cannot yet trace pseudoregister contents."),
+	  error (_("'%s' is a user-register; "
+		   "GDB cannot yet trace user-register contents."),
 		 name);
 	value->kind = axs_lvalue_register;
 	value->u.reg = reg;

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Pedro Alves]

On Wednesday 29 December 2010 06:07:33, Hui Zhu wrote:
> 2010-12-29  Hui Zhu  <teawater@gmail.com>
> 
>         * ax-gdb.c (gen_expr): Change error message.

Okay.

-- 
Pedro Alves

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Hui Zhu]

Thanks.  Checked in.

Do you think I can merge these 3 patches to 7.2.1?

Thanks,
Hui

On Wed, Dec 29, 2010 at 19:16, Pedro Alves <pedro@codesourcery.com> wrote:
> On Wednesday 29 December 2010 06:07:33, Hui Zhu wrote:
>> 2010-12-29  Hui Zhu  <teawater@gmail.com>
>>
>>         * ax-gdb.c (gen_expr): Change error message.
>
> Okay.
>
> --
> Pedro Alves
>

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Pedro Alves]

On Wednesday 29 December 2010 13:06:19, Hui Zhu wrote:
> Thanks.  Checked in.
> 
> Do you think I can merge these 3 patches to 7.2.1?

Yes.

-- 
Pedro Alves

Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Hui Zhu]

Thanks.  Checked in.

Hui

On Wed, Dec 29, 2010 at 21:15, Pedro Alves <pedro@codesourcery.com> wrote:
> On Wednesday 29 December 2010 13:06:19, Hui Zhu wrote:
>> Thanks.  Checked in.
>>
>> Do you think I can merge these 3 patches to 7.2.1?
>
> Yes.
>
> --
> Pedro Alves
>