[PATCH] Make register_valid_p signed

[PATCH] Make register_valid_p signed

[Frédéric Riss]

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

Hello,

I was investigating some behaviour change after a 6.3->6.4 migration and
found this out:

It seems the gdb_byte-ification of regcache has been a bit to zealous.
struct regcache uses an array of chars to store the status of each
register in the cache. This array has been converted to an array of
gdb_bytes which doesn't make any sense as it's not a target buffer, and
moreover it prevents the cache status from being negative. This is an
issue if you look at the register_cached function:

/* REGISTER_CACHED()

   Returns 0 if the value is not in the cache (needs fetch).
          >0 if the value is in the cache.
	  <0 if the value is permanently unavailable (don't ask again).  */

int
register_cached (int regnum)
{
  return current_regcache->register_valid_p[regnum];
}

This means that with the current code, when a target sets as register as
unavailable, it will be wrongly reported as fetched. 

If you wonder if any target uses this, remote.c has code setting
registers as unavailable in remote_fetch_register and
fetch_registers_using_p.

The observable symptom is that for example 'info register' will display
a 0 value instead of "*value not available*". Some variables will also
get a 0 value instead of an unknown value.

The attached patch fixes this by declaring register_valid_p as a signed
char array and also adds a comment (partly copied from the
register_cached one).

:ADDPATCH regcache:

While reading the code to find this out, I also noticed a little
inconsistency. I don't really think this deserves a fix, but I thought
I'd mention it anyway. 
Most people are using dwarf2 as debug format today, and with dwarf2
symbols in registers are marked as LOC_COMPUTED. The read_var_value
function will return NULL when a dwarf2 computed symbol needs access to
a register that the target has marked as unavailable (register_valid_p <
0). This will make functions like print_frame_args output something like
"my_var=???" for the unavailable var value.
Yet if you use another debug format that marks the symbol as
LOC_REGISTER, read_var_value will call error() if it can't read the
register which is a lot more radical than the above behaviour. 


Regards,
Fred

[-- Attachment #2: signed_register_valid_p.patch --]
[-- Type: text/x-patch, Size: 1091 bytes --]

2006-07-21  Frederic Riss  <frederic.riss@st.com>

	* regcache.c (struct regcache): Make register_valid_p a signed char 
	array.


Index: regcache.c
===================================================================
RCS file: /cvs/src/src/gdb/regcache.c,v
retrieving revision 1.138
diff -u -p -p -r1.138 regcache.c
--- regcache.c	17 Dec 2005 22:34:01 -0000	1.138
+++ regcache.c	21 Jul 2006 19:59:38 -0000
@@ -186,7 +186,11 @@ struct regcache
      full [0 .. NUM_REGS + NUM_PSEUDO_REGS) while a read/write
      register cache can only hold [0 .. NUM_REGS).  */
   gdb_byte *registers;
-  gdb_byte *register_valid_p;
+  /* Register cache status:
+     register_valid_p[REG] == 0 if REG value is not in the cache
+                            > 0 if REG value is in the cache
+                            < 0 if REG value is permanently unavailable */
+  signed char *register_valid_p;
   /* Is this a read-only cache?  A read-only cache is used for saving
      the target's register state (e.g, across an inferior function
      call or just before forcing a function return).  A read-only

Re: [PATCH] Make register_valid_p signed

[Daniel Jacobowitz]

On Fri, Jul 21, 2006 at 11:15:49PM +0200, Frédéric Riss wrote:
> The attached patch fixes this by declaring register_valid_p as a signed
> char array and also adds a comment (partly copied from the
> register_cached one).

Oops!  Thank you.  This patch is OK - you have write access, I think?

> While reading the code to find this out, I also noticed a little
> inconsistency. I don't really think this deserves a fix, but I thought
> I'd mention it anyway. 
> Most people are using dwarf2 as debug format today, and with dwarf2
> symbols in registers are marked as LOC_COMPUTED. The read_var_value
> function will return NULL when a dwarf2 computed symbol needs access to
> a register that the target has marked as unavailable (register_valid_p <
> 0). This will make functions like print_frame_args output something like
> "my_var=???" for the unavailable var value.
> Yet if you use another debug format that marks the symbol as
> LOC_REGISTER, read_var_value will call error() if it can't read the
> register which is a lot more radical than the above behaviour. 

You're right, this is a bit strange.  Feel free to fix it if you like;
otherwise I'm sure it will eventually go away next time that code is
cleaned up thoroughly.

:REVIEWMAIL:

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Make register_valid_p signed

[Frédéric Riss]

Le lundi 24 juillet 2006 à 15:41 -0400, Daniel Jacobowitz a écrit :
> On Fri, Jul 21, 2006 at 11:15:49PM +0200, Frédéric Riss wrote:
> > The attached patch fixes this by declaring register_valid_p as a signed
> > char array and also adds a comment (partly copied from the
> > register_cached one).
> 
> Oops!  Thank you.  This patch is OK - you have write access, I think?

Thanks! I commited it.

> > While reading the code to find this out, I also noticed a little
> > inconsistency. I don't really think this deserves a fix, but I thought
> > I'd mention it anyway. 
> > Most people are using dwarf2 as debug format today, and with dwarf2
> > symbols in registers are marked as LOC_COMPUTED. The read_var_value
> > function will return NULL when a dwarf2 computed symbol needs access to
> > a register that the target has marked as unavailable (register_valid_p <
> > 0). This will make functions like print_frame_args output something like
> > "my_var=???" for the unavailable var value.
> > Yet if you use another debug format that marks the symbol as
> > LOC_REGISTER, read_var_value will call error() if it can't read the
> > register which is a lot more radical than the above behaviour. 
> 
> You're right, this is a bit strange.  Feel free to fix it if you like;
> otherwise I'm sure it will eventually go away next time that code is
> cleaned up thoroughly.

I you think it's worth it, I'll give it a try once I get some time.
Should just be a matter of some s/error(.*)/return NULL/