mark raw registers the target doesn't have access to as unavailable

mark raw registers the target doesn't have access to as unavailable

[Pedro Alves]

This change:

<http://sourceware.org/ml/gdb-cvs/2011-03/msg00235.html> :

2011-03-18  Pedro Alves  <pedro@codesourcery.com>

        ...
        * regcache.c: ...
        (regcache_save): Adjust to handle REG_UNAVAILABLE registers.
        ...

Broke amd64 and ppc freebsd gdbs, and probably other targets.  They're
hitting this new assertion:

void
regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,
	       void *src)
{
...
  for (regnum = 0; regnum < dst->descr->nr_cooked_registers; regnum++)
    {
      if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup))
	{
	  enum register_status status = cooked_read (src, regnum, buf);

	  if (status == REG_VALID)
	    memcpy (register_buffer (dst, regnum), buf,
		    register_size (gdbarch, regnum));
	  else
	    {
	      gdb_assert (status != REG_UNKNOWN);
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	      memset (register_buffer (dst, regnum), 0,
		      register_size (gdbarch, regnum));
	    }
	  dst->register_status[regnum] = status;
	}
    }

In fact, the only thing relevant that I changed was adding
the assert.

The idea was, if we've just read a register, we should know whether
its value is valid, or unavailable.  Shouldn't be "unknown" any more.

Now, what I missed is that some targets have debug apis that
don't give debugger access to all the raw registers the architecture
supports.  E.g., from amd64fbsd-tdep.c:

/* Mapping between the general-purpose registers in `struct reg'
   format and GDB's register cache layout.
...
static int amd64fbsd_r_reg_offset[] =
{
  14 * 8,			/* %rax */
...
  21 * 8,			/* %ss */
  -1,				/* %ds */
  -1,				/* %es */
  -1,				/* %fs */
  -1				/* %gs */
};

All those -1's mean that those registers aren't retrievable
from fbsd's `struct reg'.

I see amd64-darwin-tdep.c:amd64_darwin_thread_state_reg_offset
also doesn't expose a few of the segment registers.

I think this calls for marking these registers as "unavailable".
That is, they exist in the target machine, so it's correct for
the target's description to include them, but, GDB just
doesn't know their value.  That's what the patch below does.

Here's the result on amd64-fbsd:

(gdb) info registers
rax            0xffffffffffffffff       -1
rbx            0x1      1
rcx            0x800da59d0      34374048208
rdx            0x7fffffffda48   140737488345672
rsi            0x7fffffffda38   140737488345656
rdi            0x1      1
rbp            0x7fffffffd9e0   0x7fffffffd9e0
rsp            0x7fffffffd9c0   0x7fffffffd9c0
r8             0x3a     58
r9             0x7fffffffef42   140737488351042
r10            0x18     24
r11            0x0      0
r12            0x7fffffffda48   140737488345672
r13            0x7fffffffda38   140737488345656
r14            0x0      0
r15            0x0      0
rip            0x400733 0x400733 <main+67>
eflags         0x293    [ CF AF SF IF ]
cs             0x43     67
ss             0x3b     59
ds             *value not available*
es             *value not available*
fs             *value not available*
gs             *value not available*

Notice the "*value not available*".  Working with that register
will yield:

(gdb) p $ds
$1 = <unavailable>
(gdb) p $ds + 1
value is not available

I think this is appropriate.

The code I'm touching had a comment in place around a similar,
but #if 0'd out assertion, suggesting that the targets should
themselves always indicate the state of all registers.
That would mean going over all code that looks like this:

  for (i = 0; i < ARRAY_SIZE (amd64fbsd_jmp_buf_reg_offset); i++)
    {
      if (amd64fbsd_jmp_buf_reg_offset[i] != -1)
		{
...
		  regcache_raw_supply (regcache, i, buf);
		}
    }

and add an else clause that does 

		  regcache_raw_supply (regcache, i, NULL);

NULL means "unavailable" already.

I think that's overkill: we can just handle this in a centralized
place, like the patch below.  Before the <unavailable> state,
this was a bit dangerous, because all you would see is a register
with value 0.  Now, it's clearly visible and distinct.  Any objection?

I tested this on amd64-linux, and Andreas Tobler
tested this on both amd64- and powerpc-freebsd, all with
no regressions.

I think (though I haven't tried it) this bit in corelow.c:get_core_registers:

  /* Mark all registers not found in the core as unavailable.  */
  for (i = 0; i < gdbarch_num_regs (get_regcache_arch (regcache)); i++)
    if (regcache_register_status (regcache, i) == REG_UNKNOWN)
      regcache_raw_supply (regcache, i, NULL);

can go away afterwards as now redundant.

-- 
Pedro Alves

2011-03-23  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* regcache.c (regcache_raw_read): If the target didn't supply an
	given raw register, mark it as unavailable.

---
 gdb/regcache.c |   29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

Index: src/gdb/regcache.c
===================================================================
--- src.orig/gdb/regcache.c	2011-03-23 14:05:54.000000000 +0000
+++ src/gdb/regcache.c	2011-03-23 14:27:34.892156993 +0000
@@ -586,25 +586,20 @@ regcache_raw_read (struct regcache *regc
      to the current thread.  This switching shouldn't be necessary
      only there is still only one target side register cache.  Sigh!
      On the bright side, at least there is a regcache object.  */
-  if (!regcache->readonly_p)
+  if (!regcache->readonly_p
+      && regcache_register_status (regcache, regnum) == REG_UNKNOWN)
     {
-      if (regcache_register_status (regcache, regnum) == REG_UNKNOWN)
-	{
-	  struct cleanup *old_chain = save_inferior_ptid ();
+      struct cleanup *old_chain = save_inferior_ptid ();
 
-	  inferior_ptid = regcache->ptid;
-	  target_fetch_registers (regcache, regnum);
-	  do_cleanups (old_chain);
-	}
-#if 0
-      /* FIXME: cagney/2004-08-07: At present a number of targets
-	 forget (or didn't know that they needed) to set this leading to
-	 panics.  Also is the problem that targets need to indicate
-	 that a register is in one of the possible states: valid,
-	 undefined, unknown.  The last of which isn't yet
-	 possible.  */
-      gdb_assert (regcache_register_status (regcache, regnum) == REG_VALID);
-#endif
+      inferior_ptid = regcache->ptid;
+      target_fetch_registers (regcache, regnum);
+      do_cleanups (old_chain);
+
+      /* A number of targets can't access the whole set of raw
+	 registers (because the debug API provides no means to get at
+	 them).  */
+      if (regcache->register_status[regnum] == REG_UNKNOWN)
+	regcache->register_status[regnum] = REG_UNAVAILABLE;
     }
 
   if (regcache->register_status[regnum] != REG_VALID)

Re: mark raw registers the target doesn't have access to as unavailable

[Joel Brobecker]

> I think that's overkill: we can just handle this in a centralized
> place, like the patch below.  Before the <unavailable> state,
> this was a bit dangerous, because all you would see is a register
> with value 0.  Now, it's clearly visible and distinct.  Any objection?

None from me, I like it!
-- 
Joel

Re: mark raw registers the target doesn't have access to as unavailable

[Mark Kettenis]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Wed, 23 Mar 2011 21:10:52 +0000
> 
> This change:
> 
> <http://sourceware.org/ml/gdb-cvs/2011-03/msg00235.html> :
> 
> 2011-03-18  Pedro Alves  <pedro@codesourcery.com>
> 
>         ...
>         * regcache.c: ...
>         (regcache_save): Adjust to handle REG_UNAVAILABLE registers.
>         ...
> 
> Broke amd64 and ppc freebsd gdbs, and probably other targets.  They're
> hitting this new assertion:
> 
> void
> regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,
> 	       void *src)
> {
> ...
>   for (regnum = 0; regnum < dst->descr->nr_cooked_registers; regnum++)
>     {
>       if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup))
> 	{
> 	  enum register_status status = cooked_read (src, regnum, buf);
> 
> 	  if (status == REG_VALID)
> 	    memcpy (register_buffer (dst, regnum), buf,
> 		    register_size (gdbarch, regnum));
> 	  else
> 	    {
> 	      gdb_assert (status != REG_UNKNOWN);
>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 	      memset (register_buffer (dst, regnum), 0,
> 		      register_size (gdbarch, regnum));
> 	    }
> 	  dst->register_status[regnum] = status;
> 	}
>     }
> 
> In fact, the only thing relevant that I changed was adding
> the assert.
> 
> The idea was, if we've just read a register, we should know whether
> its value is valid, or unavailable.  Shouldn't be "unknown" any more.
> 
> Now, what I missed is that some targets have debug apis that
> don't give debugger access to all the raw registers the architecture
> supports.  E.g., from amd64fbsd-tdep.c:
> 
> /* Mapping between the general-purpose registers in `struct reg'
>    format and GDB's register cache layout.
> ...
> static int amd64fbsd_r_reg_offset[] =
> {
>   14 * 8,			/* %rax */
> ...
>   21 * 8,			/* %ss */
>   -1,				/* %ds */
>   -1,				/* %es */
>   -1,				/* %fs */
>   -1				/* %gs */
> };
> 
> All those -1's mean that those registers aren't retrievable
> from fbsd's `struct reg'.
> 
> I see amd64-darwin-tdep.c:amd64_darwin_thread_state_reg_offset
> also doesn't expose a few of the segment registers.
> 
> I think this calls for marking these registers as "unavailable".
> That is, they exist in the target machine, so it's correct for
> the target's description to include them, but, GDB just
> doesn't know their value.  That's what the patch below does.
> 
> Here's the result on amd64-fbsd:
> 
> (gdb) info registers
> rax            0xffffffffffffffff       -1
> rbx            0x1      1
> rcx            0x800da59d0      34374048208
> rdx            0x7fffffffda48   140737488345672
> rsi            0x7fffffffda38   140737488345656
> rdi            0x1      1
> rbp            0x7fffffffd9e0   0x7fffffffd9e0
> rsp            0x7fffffffd9c0   0x7fffffffd9c0
> r8             0x3a     58
> r9             0x7fffffffef42   140737488351042
> r10            0x18     24
> r11            0x0      0
> r12            0x7fffffffda48   140737488345672
> r13            0x7fffffffda38   140737488345656
> r14            0x0      0
> r15            0x0      0
> rip            0x400733 0x400733 <main+67>
> eflags         0x293    [ CF AF SF IF ]
> cs             0x43     67
> ss             0x3b     59
> ds             *value not available*
> es             *value not available*
> fs             *value not available*
> gs             *value not available*
> 
> Notice the "*value not available*".  Working with that register
> will yield:
> 
> (gdb) p $ds
> $1 = <unavailable>
> (gdb) p $ds + 1
> value is not available
> 
> I think this is appropriate.

I think what you're proposing makes tons of sense.  Please go ahead.

Re: mark raw registers the target doesn't have access to as unavailable

[Pedro Alves]

On Wednesday 23 March 2011 21:44:21, Joel Brobecker wrote:
> > with value 0.  Now, it's clearly visible and distinct.  Any objection?
> 
> None from me, I like it!

...

On Thursday 24 March 2011 10:42:37, Mark Kettenis wrote:
> I think what you're proposing makes tons of sense.  Please go ahead.

Thanks all.  I've checked it in.

-- 
Pedro Alves