[RFC] h8300 "info registers" fix

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

[RFC] h8300 "info registers" fix

Yoshinori Sato
A result of ccr and exr mistakes by a "info registers" of h8300 target.
There was two faults.

- A name of E_CCR_REGNUM is not defined with h8300_register_name.
  Accordingly regcache_raw_read of h8300_pseudo_regitser_read fails.
- Assume size of E_PSEUDO_CCR_REGNUM by 1 byte with h8300_register_type.
  But assume 4 bytes from a remote target.
  Accordingly cannot take response of remote side to be really.

I correct an issue of a thing of the patch box which I attached.
The issue was corrected, but not know you whether this is good technique.

--
Yoshinori Sato
<[hidden email]>


h8300-tdep.diff (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: [RFC] h8300 "info registers" fix

Michael Snyder-4

> A result of ccr and exr mistakes by a "info registers" of h8300 target.
> There was two faults.
>
> - A name of E_CCR_REGNUM is not defined with h8300_register_name.
>   Accordingly regcache_raw_read of h8300_pseudo_regitser_read fails.
> - Assume size of E_PSEUDO_CCR_REGNUM by 1 byte with h8300_register_type.
>   But assume 4 bytes from a remote target.
>   Accordingly cannot take response of remote side to be really.
>
> I correct an issue of a thing of the patch box which I attached.
> The issue was corrected, but not know you whether this is good technique.

I'm sorry, but I don't think this is right.  

E_CCR_REGNUM is the physical register which gdb reads from the target.
This is defined to be 4 bytes, even though only 1 byte is significant.
It's just easier to keep most or all of the registers to the same size
within the register message packet.

E_PSEUDO_CCR_REGNUM is a "pseudo-register", which gdb computes internally
(rather than requesting from the target).  It is defined to be 1 byte, and
it is the register that gdb displays to the user.  In this case, the
computation of the pseudo-register is simply copying the one significant
byte from the physical register.

The fact that E_CCR_REGNUM has no display-name is deliberate.  That
register is never displayed, it is only fetched from the target and
used to obtain the value of E_PSEUDO_CCR_REGNUM (which is displayed).

If you are working on the target side (gdb stub, simulator, or gdbserver),
you only have to worry about E_CCR_REGNUM, but you need to supply it as
four bytes.  I forget whether the MSB or the LSB is the significant one,
but you should not have too much trouble to figure it out.  The other
bytes can be any value - they'll be ignored.

Michael


Reply | Threaded
Open this post in threaded view
|

RE: [RFC] h8300 "info registers" fix

Yoshinori Sato
>
>> A result of ccr and exr mistakes by a "info registers" of h8300 target.
>> There was two faults.
>>
>> - A name of E_CCR_REGNUM is not defined with h8300_register_name.
>>   Accordingly regcache_raw_read of h8300_pseudo_regitser_read fails.
>> - Assume size of E_PSEUDO_CCR_REGNUM by 1 byte with h8300_register_type.
>>   But assume 4 bytes from a remote target.
>>   Accordingly cannot take response of remote side to be really.
>>
>> I correct an issue of a thing of the patch box which I attached.
>> The issue was corrected, but not know you whether this is good
>> technique.
>
> I'm sorry, but I don't think this is right.
>
> E_CCR_REGNUM is the physical register which gdb reads from the target.
> This is defined to be 4 bytes, even though only 1 byte is significant.
> It's just easier to keep most or all of the registers to the same size
> within the register message packet.
>
> E_PSEUDO_CCR_REGNUM is a "pseudo-register", which gdb computes internally
> (rather than requesting from the target).  It is defined to be 1 byte, and
> it is the register that gdb displays to the user.  In this case, the
> computation of the pseudo-register is simply copying the one significant
> byte from the physical register.

The size is converted by the access of a pseudo register.

> The fact that E_CCR_REGNUM has no display-name is deliberate.  That
> register is never displayed, it is only fetched from the target and
> used to obtain the value of E_PSEUDO_CCR_REGNUM (which is displayed).

Because legacy_register_sim_regno was not able to judge the register
number correctly, it corrected it.
However, it is necessary to evade this problem with register_sim_regno.
It corrects it like that.

> If you are working on the target side (gdb stub, simulator, or gdbserver),
> you only have to worry about E_CCR_REGNUM, but you need to supply it as
> four bytes.  I forget whether the MSB or the LSB is the significant one,
> but you should not have too much trouble to figure it out.  The other
> bytes can be any value - they'll be ignored.
>
> Michael
>

Thank you for the advice that becomes reference very much.

--
Yoshinori Sato
<[hidden email]>


Reply | Threaded
Open this post in threaded view
|

Re: [RFC] h8300 "info registers" fix

Yoshinori Sato
In reply to this post by Yoshinori Sato
At Wed, 13 Sep 2006 00:04:58 +0900,
Yoshinori Sato wrote:

>
> [1  <text/plain; US-ASCII (7bit)>]
> A result of ccr and exr mistakes by a "info registers" of h8300 target.
> There was two faults.
>
> - A name of E_CCR_REGNUM is not defined with h8300_register_name.
>   Accordingly regcache_raw_read of h8300_pseudo_regitser_read fails.
> - Assume size of E_PSEUDO_CCR_REGNUM by 1 byte with h8300_register_type.
>   But assume 4 bytes from a remote target.
>   Accordingly cannot take response of remote side to be really.
>
> I correct an issue of a thing of the patch box which I attached.
> The issue was corrected, but not know you whether this is good technique.
>
> --
> Yoshinori Sato
> <[hidden email]>
>
I modified it in technique to think that I was appropriate.

--
Yoshinori Sato
<[hidden email]>


h8300-tdep.diff (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: [RFC] h8300 "info registers" fix

Michael Snyder-4

At Wed, 13 Sep 2006 00:04:58 +0900,
Yoshinori Sato wrote:
> I modified it in technique to think that I was appropriate.

OK, much better.  Just one more question.

Your change to h8300_pseudo_register_read (and write): is it because
someone is calling the function with a one-byte buffer?  I assume so,
but who is making that call?

Thanks,
Michael Snyder
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] h8300 "info registers" fix

Daniel Jacobowitz-2
On Wed, Sep 13, 2006 at 01:00:17PM -0700, Michael Snyder wrote:

>
> At Wed, 13 Sep 2006 00:04:58 +0900,
> Yoshinori Sato wrote:
> > I modified it in technique to think that I was appropriate.
>
> OK, much better.  Just one more question.
>
> Your change to h8300_pseudo_register_read (and write): is it because
> someone is calling the function with a one-byte buffer?  I assume so,
> but who is making that call?

I'm not familiar with this target, but are there host endianness
problems with this patch?  Casting an unsigned long * pointer to
a gdb_byte * pointer is very suspicious.

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

RE: [RFC] h8300 "info registers" fix

Michael Snyder-4
Grumble -- is the regcache kept in host order, or target order?
I can never remember.  Anyway, good call.  The read is done using
a gdb_byte[4], while the write is done using an unsigned long.
Surely they can't both be correct -- it should be one or the other.

Besides, a host unsigned long can't be right, 'cause we don't
even know what size it is.  


-----Original Message-----
From: [hidden email] on behalf of Daniel Jacobowitz
Sent: Wed 9/13/2006 1:09 PM
To: [hidden email]
Subject: Re: [RFC] h8300 "info registers" fix
 
On Wed, Sep 13, 2006 at 01:00:17PM -0700, Michael Snyder wrote:

>
> At Wed, 13 Sep 2006 00:04:58 +0900,
> Yoshinori Sato wrote:
> > I modified it in technique to think that I was appropriate.
>
> OK, much better.  Just one more question.
>
> Your change to h8300_pseudo_register_read (and write): is it because
> someone is calling the function with a one-byte buffer?  I assume so,
> but who is making that call?

I'm not familiar with this target, but are there host endianness
problems with this patch?  Casting an unsigned long * pointer to
a gdb_byte * pointer is very suspicious.

--
Daniel Jacobowitz
CodeSourcery

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] h8300 "info registers" fix

Daniel Jacobowitz-2
On Wed, Sep 13, 2006 at 01:20:09PM -0700, Michael Snyder wrote:
> Grumble -- is the regcache kept in host order, or target order?

Target order.

[Which is apparently a bit weird; most debug interfaces I've seen
lately use host order.]

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

RE: [RFC] h8300 "info registers" fix

Michael Snyder-4

Daniel:
>On Wed, Sep 13, 2006 at 01:20:09PM -0700, Michael Snyder wrote:
>> Grumble -- is the regcache kept in host order, or target order?
>
>Target order.
>
>[Which is apparently a bit weird; most debug interfaces I've seen
>lately use host order.]

That's right -- it's a relic of the fact that the original
"register cache" was the register packet itself, which is
generated on the target side.  The target doesn't know anything
about the host, so there's no way that can be in host order.

Anyway, in that case, the gdb_byte[4] approach is more likely
to be correct, eh?

I think I understand the problem now -- the pseudo-register is
only one byte, so it's "natural" to call pseudo_register_read
with a one byte buffer.  But the physical register is four bytes,
so you have to have a four byte buffer to read it.

Wonder why it ever worked?   ;-/

Reply | Threaded
Open this post in threaded view
|

RE: [RFC] h8300 "info registers" fix

Yoshinori Sato
In reply to this post by Michael Snyder-4
>
> At Wed, 13 Sep 2006 00:04:58 +0900,
> Yoshinori Sato wrote:
>> I modified it in technique to think that I was appropriate.
>
> OK, much better.  Just one more question.
>
> Your change to h8300_pseudo_register_read (and write): is it because
> someone is calling the function with a one-byte buffer?  I assume so,
> but who is making that call?

E_PSEUDO_CCR_REGNUM and E_PSEUDO_EXR_REGNUM are defined in builtin_
type_uint8 by h8300_register_type.

>
> Thanks,
> Michael Snyder
>

--
Yoshinori Sato
<[hidden email]>



Reply | Threaded
Open this post in threaded view
|

Re: [RFC] h8300 "info registers" fix

Yoshinori Sato
In reply to this post by Yoshinori Sato
I corrected it not to depend on endian.

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.7901
diff -u -r1.7901 ChangeLog
--- ChangeLog 17 Sep 2006 15:48:34 -0000 1.7901
+++ ChangeLog 19 Sep 2006 15:55:15 -0000
@@ -1,3 +1,11 @@
+2006-09-19  Yoshinori Sato  <[hidden email]>
+
+ * h8300-tdep.c (h8300_pseud_register_read): fix E_CCR_REGNUM
+ and E_EXR_REGNUM size.
+ (h8300_pseudo_register_write): Ditto.
+ (h8300_sim_register_regno): New function.
+ (h8300_h8300_gdbarch_init): add regist h8300_sim_register_regno.
+
 2006-09-17  Vladimir Prus  <[hidden email]>
 
  * mi/mi-cmd-stack.c (mi_cmd_stack_list_args): Don't emit error
Index: h8300-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/h8300-tdep.c,v
retrieving revision 1.103
diff -u -r1.103 h8300-tdep.c
--- h8300-tdep.c 17 Dec 2005 22:34:00 -0000 1.103
+++ h8300-tdep.c 19 Sep 2006 15:55:15 -0000
@@ -1148,10 +1148,20 @@
     struct regcache *regcache, int regno,
     gdb_byte *buf)
 {
+  unsigned long tmp;
+
   if (regno == E_PSEUDO_CCR_REGNUM)
-    regcache_raw_read (regcache, E_CCR_REGNUM, buf);
+    {
+      regcache_raw_read (regcache, E_CCR_REGNUM, (gdb_byte *)&tmp);
+      store_unsigned_integer((gdb_byte *)&tmp, 4, tmp);
+      *buf = tmp;
+    }
   else if (regno == E_PSEUDO_EXR_REGNUM)
-    regcache_raw_read (regcache, E_EXR_REGNUM, buf);
+    {
+      regcache_raw_read (regcache, E_EXR_REGNUM, (gdb_byte *)&tmp);
+      store_unsigned_integer((gdb_byte *)&tmp, 4, tmp);
+      *buf = tmp;
+    }
   else
     regcache_raw_read (regcache, regno, buf);
 }
@@ -1161,10 +1171,18 @@
      struct regcache *regcache, int regno,
      const gdb_byte *buf)
 {
+  unsigned long tmp;
+
   if (regno == E_PSEUDO_CCR_REGNUM)
-    regcache_raw_write (regcache, E_CCR_REGNUM, buf);
+    {
+      tmp = extract_unsigned_integer(buf, 1);
+      regcache_raw_write (regcache, E_CCR_REGNUM, (gdb_byte *)&tmp);
+    }
   else if (regno == E_PSEUDO_EXR_REGNUM)
-    regcache_raw_write (regcache, E_EXR_REGNUM, buf);
+    {
+      tmp = extract_unsigned_integer(buf, 1);
+      regcache_raw_write (regcache, E_EXR_REGNUM, (gdb_byte *)&tmp);
+    }
   else
     regcache_raw_write (regcache, regno, buf);
 }
@@ -1205,6 +1223,19 @@
 No floating-point info available for this processor.\n");
 }
 
+static int
+h8300_register_sim_regno (int regno)
+{
+  switch (regno)
+    {
+    case E_CCR_REGNUM:
+    case E_EXR_REGNUM:
+      return regno;
+    default:
+      return legacy_register_sim_regno (regno);
+    }
+}
+
 static struct gdbarch *
 h8300_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 {
@@ -1319,6 +1350,7 @@
   set_gdbarch_register_type (gdbarch, h8300_register_type);
   set_gdbarch_print_registers_info (gdbarch, h8300_print_registers_info);
   set_gdbarch_print_float_info (gdbarch, h8300_print_float_info);
+  set_gdbarch_register_sim_regno(gdbarch, h8300_register_sim_regno);
 
   /*
    * Frame Info

--
Yoshinori Sato
<[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] h8300 "info registers" fix

Mark Kettenis
In reply to this post by Daniel Jacobowitz-2
> Date: Wed, 13 Sep 2006 16:30:43 -0400
> From: Daniel Jacobowitz <[hidden email]>
>
> On Wed, Sep 13, 2006 at 01:20:09PM -0700, Michael Snyder wrote:
> > Grumble -- is the regcache kept in host order, or target order?
>
> Target order.
>
> [Which is apparently a bit weird; most debug interfaces I've seen
> lately use host order.]

Not weird at all; it's the same convention we use for storing values.

Bet most debug interfaces you've seen lately actually use
little-endian byte order.

Mark
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] h8300 "info registers" fix

Mark Kettenis
In reply to this post by Michael Snyder-4
> Date: Wed, 13 Sep 2006 13:49:26 -0700
> From: "Michael Snyder" <[hidden email]>
>
> Daniel:
> >On Wed, Sep 13, 2006 at 01:20:09PM -0700, Michael Snyder wrote:
> >> Grumble -- is the regcache kept in host order, or target order?
> >
> >Target order.
> >
> >[Which is apparently a bit weird; most debug interfaces I've seen
> >lately use host order.]
>
> That's right -- it's a relic of the fact that the original
> "register cache" was the register packet itself, which is
> generated on the target side.  The target doesn't know anything
> about the host, so there's no way that can be in host order.
>
> Anyway, in that case, the gdb_byte[4] approach is more likely
> to be correct, eh?

Yes, gdb_byte is the type we use for target byte buffers.

> I think I understand the problem now -- the pseudo-register is
> only one byte, so it's "natural" to call pseudo_register_read
> with a one byte buffer.  But the physical register is four bytes,
> so you have to have a four byte buffer to read it.
>
> Wonder why it ever worked?   ;-/

The fact that almost all variables on a 32-bit machines are
word-aligned?

Mark
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] h8300 "info registers" fix

Jim Blandy
In reply to this post by Mark Kettenis

Mark Kettenis <[hidden email]> writes:

>> Date: Wed, 13 Sep 2006 16:30:43 -0400
>> From: Daniel Jacobowitz <[hidden email]>
>>
>> On Wed, Sep 13, 2006 at 01:20:09PM -0700, Michael Snyder wrote:
>> > Grumble -- is the regcache kept in host order, or target order?
>>
>> Target order.
>>
>> [Which is apparently a bit weird; most debug interfaces I've seen
>> lately use host order.]
>
> Not weird at all; it's the same convention we use for storing values.
>
> Bet most debug interfaces you've seen lately actually use
> little-endian byte order.

No, they're actually documented to return them in host byte order.
You read a register, you get an integer.

(Well, Daniel's seen more than me, but this is so of the ones I have
seen.)
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] h8300 "info registers" fix

Daniel Jacobowitz-2
On Fri, Sep 22, 2006 at 08:21:12PM -0700, Jim Blandy wrote:
>
> Mark Kettenis <[hidden email]> writes:
> > Not weird at all; it's the same convention we use for storing values.
> >
> > Bet most debug interfaces you've seen lately actually use
> > little-endian byte order.

They're frequently organized in integer multiples of the word size,
which are documented to return in host uint32_t's.  If the underlying
"register" is really bigger than that, then the stub is responsible
for composing.

> No, they're actually documented to return them in host byte order.
> You read a register, you get an integer.
>
> (Well, Daniel's seen more than me, but this is so of the ones I have
> seen.)

Yep.  I can think offhand of four different JTAG-related debug
interfaces I've worked with in the last six months, all of which did
this.  Then, we transform the number from host endianness (on
the machine running the stub) to target endianness (the device)
before passing it over TCP/IP to GDB.  All a bit messy, but you lose
some no matter which order you pick :-)

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] h8300 "info registers" fix

Mark Kettenis
In reply to this post by Yoshinori Sato
> Date: Wed, 20 Sep 2006 01:28:02 +0900
> From: Yoshinori Sato <[hidden email]>
>
> I corrected it not to depend on endian.
>
> Index: h8300-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/h8300-tdep.c,v
> retrieving revision 1.103
> diff -u -r1.103 h8300-tdep.c
> --- h8300-tdep.c 17 Dec 2005 22:34:00 -0000 1.103
> +++ h8300-tdep.c 19 Sep 2006 15:55:15 -0000
> @@ -1148,10 +1148,20 @@
>      struct regcache *regcache, int regno,
>      gdb_byte *buf)
>  {
> +  unsigned long tmp;
> +
>    if (regno == E_PSEUDO_CCR_REGNUM)
> -    regcache_raw_read (regcache, E_CCR_REGNUM, buf);
> +    {
> +      regcache_raw_read (regcache, E_CCR_REGNUM, (gdb_byte *)&tmp);
> +      store_unsigned_integer((gdb_byte *)&tmp, 4, tmp);
> +      *buf = tmp;
> +    }
>    else if (regno == E_PSEUDO_EXR_REGNUM)
> -    regcache_raw_read (regcache, E_EXR_REGNUM, buf);
> +    {
> +      regcache_raw_read (regcache, E_EXR_REGNUM, (gdb_byte *)&tmp);
> +      store_unsigned_integer((gdb_byte *)&tmp, 4, tmp);
> +      *buf = tmp;
> +    }

This is still wrong.  You'll need to read the raw register into a
properly sized gdb_byte array, take the right bits out of it and then
move it into the buffer.  You probably want something like

  gdb_byte tmp[4];

  regcache_raw_read(regcache, E_CCR_REGNUM, tmp)
  *buf = tmp[0];

or

  gdb_byte tmp[4];

  regcache_raw_read(regcache, E_CCR_REGNUM, tmp)
  *buf = tmp[3];

depending on whether h8300 is little- or big-endian.

Mark
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] h8300 "info registers" fix

Yoshinori Sato
A response is late sorry.

At Sat, 23 Sep 2006 20:00:16 +0200 (CEST),
Mark Kettenis wrote:

>
> > Date: Wed, 20 Sep 2006 01:28:02 +0900
> > From: Yoshinori Sato <[hidden email]>
> >
> > I corrected it not to depend on endian.
> >
> > Index: h8300-tdep.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/h8300-tdep.c,v
> > retrieving revision 1.103
> > diff -u -r1.103 h8300-tdep.c
> > --- h8300-tdep.c 17 Dec 2005 22:34:00 -0000 1.103
> > +++ h8300-tdep.c 19 Sep 2006 15:55:15 -0000
> > @@ -1148,10 +1148,20 @@
> >      struct regcache *regcache, int regno,
> >      gdb_byte *buf)
> >  {
> > +  unsigned long tmp;
> > +
> >    if (regno == E_PSEUDO_CCR_REGNUM)
> > -    regcache_raw_read (regcache, E_CCR_REGNUM, buf);
> > +    {
> > +      regcache_raw_read (regcache, E_CCR_REGNUM, (gdb_byte *)&tmp);
> > +      store_unsigned_integer((gdb_byte *)&tmp, 4, tmp);
> > +      *buf = tmp;
> > +    }
> >    else if (regno == E_PSEUDO_EXR_REGNUM)
> > -    regcache_raw_read (regcache, E_EXR_REGNUM, buf);
> > +    {
> > +      regcache_raw_read (regcache, E_EXR_REGNUM, (gdb_byte *)&tmp);
> > +      store_unsigned_integer((gdb_byte *)&tmp, 4, tmp);
> > +      *buf = tmp;
> > +    }
>
> This is still wrong.  You'll need to read the raw register into a
> properly sized gdb_byte array, take the right bits out of it and then
> move it into the buffer.  You probably want something like
>
>   gdb_byte tmp[4];
>
>   regcache_raw_read(regcache, E_CCR_REGNUM, tmp)
>   *buf = tmp[0];
>
> or
>
>   gdb_byte tmp[4];
>
>   regcache_raw_read(regcache, E_CCR_REGNUM, tmp)
>   *buf = tmp[3];
>
> depending on whether h8300 is little- or big-endian.
>
> Mark


h8300.diff (2K) Download Attachment