[RFA] string_to_core_addr fix

[RFA] string_to_core_addr fix

[Martin M. Hunt]

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

This is necessary for 64-bit targets where sometimes 32-bit
values must be sign-extended to 64-bits.

2002-10-10  Martin M. Hunt  <hunt@redhat.com>

	* utils.c (string_to_core_addr): After turning string into
	a number, convert to a CORE_ADDR using POINTER_TO_ADDRESS
	which will do necessary sign-extension, etc.

-- 
Martin Hunt
GDB Engineer
Red Hat, Inc.

[-- Attachment #2: p --]
[-- Type: text/x-diff, Size: 486 bytes --]

Index: utils.c
===================================================================
RCS file: /cvs/src/src/gdb/utils.c,v
retrieving revision 1.80
diff -u -p -r1.80 utils.c
--- utils.c	20 Sep 2002 00:24:01 -0000	1.80
+++ utils.c	10 Oct 2002 22:06:50 -0000
@@ -2649,7 +2649,7 @@ string_to_core_addr (const char *my_stri
 	    internal_error (__FILE__, __LINE__, "invalid decimal");
 	}
     }
-  return addr;
+  return POINTER_TO_ADDRESS (builtin_type_void_data_ptr, &addr); 
 }
 
 char *

Re: [RFA] string_to_core_addr fix

[Kevin Buettner]

On Oct 10,  3:07pm, Martin M. Hunt wrote:

> This is necessary for 64-bit targets where sometimes 32-bit
> values must be sign-extended to 64-bits.
> 
> 2002-10-10  Martin M. Hunt  <hunt@redhat.com>
> 
> 	* utils.c (string_to_core_addr): After turning string into
> 	a number, convert to a CORE_ADDR using POINTER_TO_ADDRESS
> 	which will do necessary sign-extension, etc.
>
> Index: utils.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/utils.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 utils.c
> --- utils.c	20 Sep 2002 00:24:01 -0000	1.80
> +++ utils.c	10 Oct 2002 22:06:50 -0000
> @@ -2649,7 +2649,7 @@ string_to_core_addr (const char *my_stri
>  	    internal_error (__FILE__, __LINE__, "invalid decimal");
>  	}
>      }
> -  return addr;
> +  return POINTER_TO_ADDRESS (builtin_type_void_data_ptr, &addr); 
>  }
>  
>  char *

While I agree that something like this is needed, I'm not convinced that
using POINTER_TO_ADDRESS on a CORE_ADDR is right.  By default,
unsigned_pointer_to_address() is used.  It looks like this:

    /* Given a pointer of type TYPE in target form in BUF, return the
       address it represents.  */
    CORE_ADDR
    unsigned_pointer_to_address (struct type *type, void *buf)
    {
      return extract_address (buf, TYPE_LENGTH (type));
    }

The problem is that ``addr'' is an address in host format (i.e, a CORE_ADDR),
not a target address.  I suspect you'll get incorrect results if the
host and target are of different endianness or if sizeof (CORE_ADDR) !=
TYPE_LENGTH (type).

I think you could get the right results by writing addr to a buffer
(maybe using store_typed_address) and then using extract_typed_address(),
but there may be a more straightforward way to do it.

Kevin

Re: [RFA] string_to_core_addr fix

[Martin M. Hunt]

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

On Thursday 10 October 2002 03:40 pm, Kevin Buettner wrote:
> On Oct 10,  3:07pm, Martin M. Hunt wrote:
> > This is necessary for 64-bit targets where sometimes 32-bit
> > values must be sign-extended to 64-bits.
> >
> > 2002-10-10  Martin M. Hunt  <hunt@redhat.com>
> >
> > 	* utils.c (string_to_core_addr): After turning string into
> > 	a number, convert to a CORE_ADDR using POINTER_TO_ADDRESS
> > 	which will do necessary sign-extension, etc.
> >
> > Index: utils.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/utils.c,v
> > retrieving revision 1.80
> > diff -u -p -r1.80 utils.c
> > --- utils.c	20 Sep 2002 00:24:01 -0000	1.80
> > +++ utils.c	10 Oct 2002 22:06:50 -0000
> > @@ -2649,7 +2649,7 @@ string_to_core_addr (const char *my_stri
> >  	    internal_error (__FILE__, __LINE__, "invalid decimal");
> >  	}
> >      }
> > -  return addr;
> > +  return POINTER_TO_ADDRESS (builtin_type_void_data_ptr, &addr);
> >  }
> >
> >  char *
>
> While I agree that something like this is needed, I'm not convinced that
> using POINTER_TO_ADDRESS on a CORE_ADDR is right.  By default,
> unsigned_pointer_to_address() is used.  It looks like this:
>
>     /* Given a pointer of type TYPE in target form in BUF, return the
>        address it represents.  */
>     CORE_ADDR
>     unsigned_pointer_to_address (struct type *type, void *buf)
>     {
>       return extract_address (buf, TYPE_LENGTH (type));
>     }
>
> The problem is that ``addr'' is an address in host format (i.e, a
> CORE_ADDR), not a target address.  I suspect you'll get incorrect results
> if the host and target are of different endianness or if sizeof (CORE_ADDR)
> != TYPE_LENGTH (type).
>
> I think you could get the right results by writing addr to a buffer
> (maybe using store_typed_address) and then using extract_typed_address(),
> but there may be a more straightforward way to do it.

For years we used parse_and_eval_address() in Insight until earlier this year 
when those calls were replaced with string_to_core_addr(), breaking all mips 
targets.  parse_and_eval_address() internally calls INTEGER_TO_ADDRESS() so I 
probably should use that. In fact I decided to just do what 
parse_and_eval_address() did but apparently submitted the wrong version.

Revised patch attached.

Martin


[-- Attachment #2: p --]
[-- Type: text/x-diff, Size: 512 bytes --]

Index: utils.c
===================================================================
RCS file: /cvs/src/src/gdb/utils.c,v
retrieving revision 1.80
diff -u -p -r1.80 utils.c
--- utils.c	20 Sep 2002 00:24:01 -0000	1.80
+++ utils.c	11 Oct 2002 04:29:51 -0000
@@ -2649,6 +2649,8 @@ string_to_core_addr (const char *my_stri
 	    internal_error (__FILE__, __LINE__, "invalid decimal");
 	}
     }
+  if (INTEGER_TO_ADDRESS_P ())
+    addr = INTEGER_TO_ADDRESS (builtin_type_void_data_ptr, &addr); 
   return addr;
 }
 

Re: [RFA] string_to_core_addr fix

[Kevin Buettner]

On Oct 10,  9:29pm, Martin M. Hunt wrote:

> For years we used parse_and_eval_address() in Insight until earlier this year 
> when those calls were replaced with string_to_core_addr(), breaking all mips 
> targets.  parse_and_eval_address() internally calls INTEGER_TO_ADDRESS() so I 
> probably should use that. In fact I decided to just do what 
> parse_and_eval_address() did but apparently submitted the wrong version.
> 
> Revised patch attached.
> 
> Index: utils.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/utils.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 utils.c
> --- utils.c	20 Sep 2002 00:24:01 -0000	1.80
> +++ utils.c	11 Oct 2002 04:29:51 -0000
> @@ -2649,6 +2649,8 @@ string_to_core_addr (const char *my_stri
>  	    internal_error (__FILE__, __LINE__, "invalid decimal");
>  	}
>      }
> +  if (INTEGER_TO_ADDRESS_P ())
> +    addr = INTEGER_TO_ADDRESS (builtin_type_void_data_ptr, &addr); 
>    return addr;
>  }

Okay, this version looks reasonable.  (Approved.)  Make sure you
update the ChangeLog entry to match.

Kevin

Re: [RFA] string_to_core_addr fix

[Andrew Cagney]

> On Oct 10,  9:29pm, Martin M. Hunt wrote:
> 
> 
>> For years we used parse_and_eval_address() in Insight until earlier this year 
>> when those calls were replaced with string_to_core_addr(), breaking all mips 
>> targets.  parse_and_eval_address() internally calls INTEGER_TO_ADDRESS() so I 
>> probably should use that. In fact I decided to just do what 
>> parse_and_eval_address() did but apparently submitted the wrong version.

Insight parse_and_eval_address() was simply bogus.  See the thread 
around the original introduction of these functions.  A short summary is 
that parse_and_eval_address() does conversions like you describe and 
none are needed.  Instead functions that parse in, write out, raw 
CORE_ADDR values are needed.

Switching to string_to_core_addr() and core_addr_to_string() flushed out 
a heap of address conversion problems for the d10v, and I beleive, the MIPS.

>> Revised patch attached.
>> 
>> Index: utils.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/utils.c,v
>> retrieving revision 1.80
>> diff -u -p -r1.80 utils.c
>> --- utils.c	20 Sep 2002 00:24:01 -0000	1.80
>> +++ utils.c	11 Oct 2002 04:29:51 -0000
>> @@ -2649,6 +2649,8 @@ string_to_core_addr (const char *my_stri
>>  	    internal_error (__FILE__, __LINE__, "invalid decimal");
>>  	}
>>      }
>> +  if (INTEGER_TO_ADDRESS_P ())
>> +    addr = INTEGER_TO_ADDRESS (builtin_type_void_data_ptr, &addr); 
>>    return addr;
>>  }
> 
> 
> Okay, this version looks reasonable.  (Approved.)  Make sure you
> update the ChangeLog entry to match.

I believe that this change is wrong and should be reverted.

Per above, string_to_core_addr() and core_addr_to_string() scan/print 
raw CORE_ADDR values.  If there is some sort of conversion problem going 
on then this  indicates that something isn't writing out / reading a raw 
CORE_ADDR value correctly (or some code is trying to use the function 
incorrectly).

Andrew

Re: [RFA] string_to_core_addr fix

[Martin M. Hunt]

On Mon, 2002-10-21 at 13:06, Andrew Cagney wrote:

> Insight parse_and_eval_address() was simply bogus.  See the thread 
> around the original introduction of these functions. 

I looked.  All I found was something about parse_and_eval_address()
being broken for harvard arch.

> A short summary is 
> that parse_and_eval_address() does conversions like you describe and 
> none are needed.  Instead functions that parse in, write out, raw 
> CORE_ADDR values are needed.

I believe we have target addrs and CORE_ADDRs, where CORE_ADDRs are
sometimes target addrs sign-extended to 64-bits.  Is that not right?

If the user types something like "x/10i 0xA0000000" on a Mips
architecture, is the address not sign-extended to a 64-bit CORE_ADDR?

From memory, you print out a target addr by using paddr_nz.  If you
wanted to print a CORE_ADDR you would use core_addr_to_string_nz. You
can read in a CORE_ADDR with string_to_core_addr. So how do you read in
a target addr and have it converted to a CORE_ADDR?

> I believe that this change is wrong and should be reverted.

By your definition of string_to_core_addr below, I agree.  However, this
bug has been here a long time and I would like some agreement on how it
should properly be fixed.

The bug is simply that Insight gets CORE_ADDRs for any symbol lookup. 
It must convert them to strings and uses paddr_nz.  Then the user does
something with that address and Insight converts that address string
back into a CORE_ADDR incorrectly (it doesn't sign-extend to 64-bits,
therefore my patch).
 
> Per above, string_to_core_addr() and core_addr_to_string() scan/print 
> raw CORE_ADDR values.  If there is some sort of conversion problem going 
> on then this  indicates that something isn't writing out / reading a raw 
> CORE_ADDR value correctly (or some code is trying to use the function 
> incorrectly).
> 
> Andrew
> 
> 

Re: [RFA] string_to_core_addr fix

[Andrew Cagney]

> On Mon, 2002-10-21 at 13:06, Andrew Cagney wrote:
> 
> 
>> Insight parse_and_eval_address() was simply bogus.  See the thread 
>> around the original introduction of these functions. 
> 
> 
> I looked.  All I found was something about parse_and_eval_address()
> being broken for harvard arch.

Sigh, looks like it was private e-mail :-(

>> A short summary is 
>> that parse_and_eval_address() does conversions like you describe and 
>> none are needed.  Instead functions that parse in, write out, raw 
>> CORE_ADDR values are needed.
> 
> 
> I believe we have target addrs and CORE_ADDRs, where CORE_ADDRs are
> sometimes target addrs sign-extended to 64-bits.  Is that not right?

(not sure what `sometimes' was bound too)
A CORE_ADDR always contains an address converted to a canonical form. 
For the MIPS (32 or 64 bit), when GDB is debugging a 32 bit ABI, the 
CORE_ADDR will always contain a canonical value that has been created by 
sign-extending the 32 bit pointer or register value.

>>From memory, you print out a target addr by using paddr_nz.  If you
> wanted to print a CORE_ADDR you would use core_addr_to_string_nz. You
> can read in a CORE_ADDR with string_to_core_addr. So how do you read in
> a target addr and have it converted to a CORE_ADDR?

(is core_addr_to_string_nz() used?)

Addresses are ment to be displayed using print_address_numeric() and 
similar.  A user specified value would be parsed with something like 
parse_and_eval_address().

On the other hand, string<->core_addr() is used as a way for Insight to 
create an internal address handle (for saving things like frames).  The 
user should not be able to access or manipulate such values directly.

> 
>> I believe that this change is wrong and should be reverted.
> 
> 
> By your definition of string_to_core_addr below, I agree.  However, this
> bug has been here a long time and I would like some agreement on how it
> should properly be fixed.

I know of several ongoing bugs:

- GDB forgetting to convert a pointer into a core_addr
- GDB/Insight incorrectly interchanging addresses and core_addr

> The bug is simply that Insight gets CORE_ADDRs for any symbol lookup. 
> It must convert them to strings and uses paddr_nz.  Then the user does
> something with that address and Insight converts that address string
> back into a CORE_ADDR incorrectly (it doesn't sign-extend to 64-bits,
> therefore my patch).

That code is definitly wrong.  The equality:
	core_addr == string_to_core_addr (paddr_nz (core_addr))
does NOT hold.

The code should either:

- use string <-> core_addr() and not let the user directly manipulate 
the values (insight could manipulate it though).

- Convert the CORE_ADDR back into an address, let the user manipulate 
the address, and then use parse_and_eval_address() to get the core_addr 
back.  Not sure how well this would go with harvard architectures though 
- for them, a simple address may not be sufficient for re-constructing 
the CORE_ADDR.
	
Any way, the patch should be reverted.

Andrew