[RFA] findvar.c: support LOC_BASEREG[_ARG] on Harvard archs

[RFA] findvar.c: support LOC_BASEREG[_ARG] on Harvard archs

[Nick Duffek]

Hi,

At the moment, GDB doesn't apply the necessary POINTER_TO_ADDRESS
translation to the base register value used for finding LOC_BASEREG and
LOC_BASEREG_ARG variables.

This patch applies that translation.  Okay to apply?

ChangeLog:

	* findvar.c (read_var_value): Filter LOC_BASEREG and
	LOC_BASEREG_ARG register values through POINTER_TO_ADDRESS.

Nick Duffek
<nsd@redhat.com>

[patch follows]

Index: gdb/findvar.c
===================================================================
diff -up gdb/findvar.c gdb/findvar.c
--- gdb/findvar.c	Mon Jul 16 12:41:21 2001
+++ gdb/findvar.c	Mon Jul 16 12:39:40 2001
@@ -612,9 +612,10 @@ addresses have not been bound by the dyn
     case LOC_BASEREG_ARG:
       {
 	char *buf = (char*) alloca (MAX_REGISTER_RAW_SIZE);
+	memset (buf, 0, MAX_REGISTER_RAW_SIZE);
 	get_saved_register (buf, NULL, NULL, frame, SYMBOL_BASEREG (var),
 			    NULL);
-	addr = extract_address (buf, REGISTER_RAW_SIZE (SYMBOL_BASEREG (var)));
+	addr = POINTER_TO_ADDRESS (builtin_type_void_data_ptr, buf);
 	addr += SYMBOL_VALUE (var);
 	break;
       }

Re: [RFA] findvar.c: support LOC_BASEREG[_ARG] on Harvard archs

[Jim Blandy]

Nick Duffek <nsd@redhat.com> writes:
> At the moment, GDB doesn't apply the necessary POINTER_TO_ADDRESS
> translation to the base register value used for finding LOC_BASEREG and
> LOC_BASEREG_ARG variables.
> 
> This patch applies that translation.  Okay to apply?

(I think David Taylor is the maintainer for findvar.c.)

The idea seems fine.  The alternative would be to have that code call
value_from_register and then value_as_pointer, as is done for
LOC_REGPARM_ADDR, rather than banging the bits yourself with
get_saved_register and extract_address.  But if you are going to bang
the bits yourself, you do need to call POINTER_TO_ADDRESS.

I think it would be a little cleaner to use `lookup_pointer_type
(type)' instead of builtin_type_void_data_ptr; that gives more
information to the target's POINTER_TO_ADDRESS function.  ANSI C says
void * can be used as a generic type for all data pointers, but GDB is
supposed to support other languages, too.

You should do similar things for LOC_INDIRECT, LOC_REF_ARG,
LOC_THREAD_LOCAL_STATIC.  I think it's not needed for
LOC_REGPARM_ADDR; value_as_pointer should do the work.




> 
> ChangeLog:
> 
> 	* findvar.c (read_var_value): Filter LOC_BASEREG and
> 	LOC_BASEREG_ARG register values through POINTER_TO_ADDRESS.
> 
> Nick Duffek
> <nsd@redhat.com>
> 
> [patch follows]
> 
> Index: gdb/findvar.c
> ===================================================================
> diff -up gdb/findvar.c gdb/findvar.c
> --- gdb/findvar.c	Mon Jul 16 12:41:21 2001
> +++ gdb/findvar.c	Mon Jul 16 12:39:40 2001
> @@ -612,9 +612,10 @@ addresses have not been bound by the dyn
>      case LOC_BASEREG_ARG:
>        {
>  	char *buf = (char*) alloca (MAX_REGISTER_RAW_SIZE);
> +	memset (buf, 0, MAX_REGISTER_RAW_SIZE);
>  	get_saved_register (buf, NULL, NULL, frame, SYMBOL_BASEREG (var),
>  			    NULL);
> -	addr = extract_address (buf, REGISTER_RAW_SIZE (SYMBOL_BASEREG (var)));
> +	addr = POINTER_TO_ADDRESS (builtin_type_void_data_ptr, buf);
>  	addr += SYMBOL_VALUE (var);
>  	break;
>        }
> 

Re: [RFA] findvar.c: support LOC_BASEREG[_ARG] on Harvard archs

[Nick Duffek]

On 16-Jul-2001, Jim Blandy wrote:

>The alternative would be to have that code call
>value_from_register and then value_as_pointer, as is done for
>LOC_REGPARM_ADDR, rather than banging the bits yourself with
>get_saved_register and extract_address.

Okay, here's an updated patch that uses value_from_register and
value_as_pointer.

>But if you are going to bang
>the bits yourself, you do need to call POINTER_TO_ADDRESS.

I don't think there's any applicable bit-banging here.

>You should do similar things for LOC_INDIRECT,

Aren't LOC_INDIRECT addresses from the symbol table and therefore already
Harvard-adjusted?

>LOC_REF_ARG,

I think that architectures already handle this in their own
FRAME_ARGS_ADDRESS implementations, so this would do double pointer
translation.

>LOC_THREAD_LOCAL_STATIC.

Agreed.  In fact, the code for LOC_THREAD_LOCAL_STATIC is identical to
that for LOC_BASEREG_ARG, so I've just moved LOC_THREAD_LOCAL_STATIC to
the LOC_BASEREG_ARG case in the appended patch.

Nick

Index: gdb/findvar.c
===================================================================
diff -up gdb/findvar.c gdb/findvar.c
--- gdb/findvar.c	Mon Jul 16 15:52:10 2001
+++ gdb/findvar.c	Mon Jul 16 15:51:58 2001
@@ -610,22 +610,15 @@ addresses have not been bound by the dyn
 
     case LOC_BASEREG:
     case LOC_BASEREG_ARG:
-      {
-	char *buf = (char*) alloca (MAX_REGISTER_RAW_SIZE);
-	get_saved_register (buf, NULL, NULL, frame, SYMBOL_BASEREG (var),
-			    NULL);
-	addr = extract_address (buf, REGISTER_RAW_SIZE (SYMBOL_BASEREG (var)));
-	addr += SYMBOL_VALUE (var);
-	break;
-      }
-
     case LOC_THREAD_LOCAL_STATIC:
       {
-	char *buf = (char*) alloca (MAX_REGISTER_RAW_SIZE);
+	value_ptr regval;
 
-	get_saved_register (buf, NULL, NULL, frame, SYMBOL_BASEREG (var),
-			    NULL);
-	addr = extract_address (buf, REGISTER_RAW_SIZE (SYMBOL_BASEREG (var)));
+	regval = value_from_register (lookup_pointer_type (type),
+				      SYMBOL_BASEREG (var), frame);
+	if (regval == NULL)
+	  error ("Value of base register not available.");
+	addr = value_as_pointer (regval);
 	addr += SYMBOL_VALUE (var);
 	break;
       }

Re: [RFA] findvar.c: support LOC_BASEREG[_ARG] on Harvard archs

[Andrew Cagney]

>> -	char *buf = (char*) alloca (MAX_REGISTER_RAW_SIZE);
>> +	value_ptr regval;


value_ptr is really ``struct value *'' so ...


>> +	regval = value_from_register (lookup_pointer_type (type),
>> +				      SYMBOL_BASEREG (var), frame);
>> +	if (regval == NULL)
>> +	  error ("Value of base register not available.");
>> +	addr = value_as_pointer (regval);


I suspect you need xfree() or the like.

	Andrew

Re: [RFA] findvar.c: support LOC_BASEREG[_ARG] on Harvard archs

[Jim Blandy]

Andrew Cagney <ac131313@cygnus.com> writes:

> 
> 
> >> -	char *buf = (char*) alloca (MAX_REGISTER_RAW_SIZE);
> >> +	value_ptr regval;
> 
> 
> value_ptr is really ``struct value *'' so ...
> 
> 
> >> +	regval = value_from_register (lookup_pointer_type (type),
> >> +				      SYMBOL_BASEREG (var), frame);
> >> +	if (regval == NULL)
> >> +	  error ("Value of base register not available.");
> >> +	addr = value_as_pointer (regval);
> 
> 
> I suspect you need xfree() or the like.

No --- all values get put in a global list, `all_values', which gets
freed automatically when the current expression evaluation is complete.
Grep for `all_values' and see how it gets used.

Re: [RFA] findvar.c: support LOC_BASEREG[_ARG] on Harvard archs

[Nick Duffek]

On 16-Jul-2001, Andrew Cagney wrote:

>value_ptr is really ``struct value *'' so ...

>I suspect you need xfree() or the like.

Nope, the value_ptr gets garbage-collected.  value_from_register()
allocates the value_ptr using allocate_value(), which stores the pointer
in all_values to be freed later by free_all_values().

Nick

Re: [RFA] findvar.c: support LOC_BASEREG[_ARG] on Harvard archs

[Nick Duffek]

On 16-Jul-2001, Jim Blandy wrote:

>Nick Duffek <nsd@redhat.com> writes:

>> Aren't LOC_INDIRECT addresses from the symbol table and therefore already
>> Harvard-adjusted?

>SYMBOL_VALUE_ADDRESS (var) is already an address, but the value
>returned by read_memory_unsigned_integer will not be.

Oh, right.

>> I think that architectures already handle this in their own
>> FRAME_ARGS_ADDRESS implementations, so this would do double pointer
>> translation.

>Similar --- read_memory_unsigned_integer will return a value which
>needs to be tweaked.

Likewise.

>Your revisions below look okay to me.

I've committed those,

Nick

Re: [RFA] findvar.c: support LOC_BASEREG[_ARG] on Harvard archs

[Andrew Cagney]

>>Your revisions below look okay to me.
> 
> 
> I've committed those,


Nick, did you hear anything from DavidT?

	Andrew

Re: [RFA] findvar.c: support LOC_BASEREG[_ARG] on Harvard archs

[Nick Duffek]

On 16-Jul-2001, Andrew Cagney wrote:

>Nick, did you hear anything from DavidT?

No,

Nick

Re: [RFA] findvar.c: support LOC_BASEREG[_ARG] on Harvard archs

[Nick Duffek]

On 16-Jul-2001, Andrew Cagney wrote:

>> I've committed those,

>Nick, did you hear anything from DavidT?

Sorry if I jumped the gun there, David.  If you've got any objections to
the patch, let me know and I'll rework/revert it.

Nick

Re: [RFA] findvar.c: support LOC_BASEREG[_ARG] on Harvard archs

[Jim Blandy]

Andrew Cagney <ac131313@cygnus.com> writes:
> > Index: gdb/findvar.c
> > ===================================================================
> > diff -up gdb/findvar.c gdb/findvar.c> --- gdb/findvar.c	Mon Jul 16 12:41:21 2001
> > +++ gdb/findvar.c	Mon Jul 16 12:39:40 2001
> > @@ -612,9 +612,10 @@ addresses have not been bound by the dyn
> >      case LOC_BASEREG_ARG:
> >        {
> >  	char *buf = (char*) alloca (MAX_REGISTER_RAW_SIZE);
> > +	memset (buf, 0, MAX_REGISTER_RAW_SIZE);
> >  	get_saved_register (buf, NULL, NULL, frame, SYMBOL_BASEREG (var),
> >  			    NULL);
> > -	addr = extract_address (buf, REGISTER_RAW_SIZE (SYMBOL_BASEREG (var)));
> > +	addr = POINTER_TO_ADDRESS (builtin_type_void_data_ptr, buf);
> >  	addr += SYMBOL_VALUE (var);
> >  	break;
> 
> 
> FYI, I think this change will break the MIPS o64 ABI in big-endian mode.
> Under that ABI, 8 byte registers are used (and saved on the stack) but 
> sizeof (void*) == 4.  If I'm reading the above change correctly, 
> pointer_to_address() will always extract the value from the first 4 
> bytes of of the register when, for o64/BE, it should be using the second 
> 4 bytes.

That's right, it will.  :(

Probably using register_value and value_as_pointer is best, then.