[RFC] dwarf2_read_address(): sign extend as appropriate

[RFC] dwarf2_read_address(): sign extend as appropriate

[Kevin Buettner]

Any comments regarding the patch below?

I wrote it last summer, so my memory about it is rather hazy aside
from the fact that it fixed a bunch of C++ frame base problems on
mips64 targets.

It shouldn't affect any architecture other than mips because mips is
the only one which defines an integer_to_address method.  It also
addresses Andrew's FIXME comment from nearly four years ago...

	* dwarf2expr.c (unsigned_address_type): Add forward declaration.
	(dwarf2_read_address): Sign extend return address as required by
	target architecture.

Index: dwarf2expr.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2expr.c,v
retrieving revision 1.19
diff -u -p -r1.19 dwarf2expr.c
--- dwarf2expr.c	9 Jan 2007 17:58:50 -0000	1.19
+++ dwarf2expr.c	20 Apr 2007 23:23:50 -0000
@@ -33,6 +33,7 @@
 
 static void execute_stack_op (struct dwarf_expr_context *,
 			      gdb_byte *, gdb_byte *);
+static struct type * unsigned_address_type (void);
 
 /* Create a new context for the expression evaluator.  */
 
@@ -205,9 +206,24 @@ dwarf2_read_address (gdb_byte *buf, gdb_
     error (_("dwarf2_read_address: Corrupted DWARF expression."));
 
   *bytes_read = TARGET_ADDR_BIT / TARGET_CHAR_BIT;
-  /* NOTE: cagney/2003-05-22: This extract is assuming that a DWARF 2
-     address is always unsigned.  That may or may not be true.  */
-  result = extract_unsigned_integer (buf, TARGET_ADDR_BIT / TARGET_CHAR_BIT);
+
+  /* Some architectures (such as MIPS) use signed addresses.  Those that
+     do will have registered a gdbarch_integer_to_address method.  Use
+     that method if it exists, otherwise fall back to extracting an
+     unsigned integer as the address.
+     
+     Note:  kevinb/2006-07-10:  The use of `unsigned_address_type' in
+     the gdbarch_integer_to_address() call below refers to the type of
+     `buf' and has no bearing on the signedness of the address being
+     returned.  In all of the integer-to-address conversion methods
+     being used by GDB at the time that this comment was written, this
+     type was used only to determine the size of `buf'.  */
+  if (gdbarch_integer_to_address_p (current_gdbarch))
+    result = gdbarch_integer_to_address (current_gdbarch,
+                                         unsigned_address_type(), buf);
+  else
+    result = extract_unsigned_integer (buf,
+                                       TARGET_ADDR_BIT / TARGET_CHAR_BIT);
   return result;
 }
 

Re: [RFC] dwarf2_read_address(): sign extend as appropriate

[Ulrich Weigand]

Kevin Buettner wrote:

> Any comments regarding the patch below?
> 
> I wrote it last summer, so my memory about it is rather hazy aside
> from the fact that it fixed a bunch of C++ frame base problems on
> mips64 targets.
> 
> It shouldn't affect any architecture other than mips because mips is
> the only one which defines an integer_to_address method.  It also
> addresses Andrew's FIXME comment from nearly four years ago...
> 
> 	* dwarf2expr.c (unsigned_address_type): Add forward declaration.
> 	(dwarf2_read_address): Sign extend return address as required by
> 	target architecture.

I've recently changed a very similar place, dwarf_expr_read_reg in
dwarf2loc.c, to use a new "address_from_register" function to solve
the same problem:
http://sourceware.org/ml/gdb-patches/2006-11/msg00134.html

Is there any reason you couldn't use the same function here?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFC] dwarf2_read_address(): sign extend as appropriate

[Kevin Buettner]

On Mon, 23 Apr 2007 17:05:20 +0200 (CEST)
"Ulrich Weigand" <uweigand@de.ibm.com> wrote:

> Kevin Buettner wrote:
> 
> > Any comments regarding the patch below?
> > 
> > I wrote it last summer, so my memory about it is rather hazy aside
> > from the fact that it fixed a bunch of C++ frame base problems on
> > mips64 targets.
> > 
> > It shouldn't affect any architecture other than mips because mips is
> > the only one which defines an integer_to_address method.  It also
> > addresses Andrew's FIXME comment from nearly four years ago...
> > 
> > 	* dwarf2expr.c (unsigned_address_type): Add forward declaration.
> > 	(dwarf2_read_address): Sign extend return address as required by
> > 	target architecture.
> 
> I've recently changed a very similar place, dwarf_expr_read_reg in
> dwarf2loc.c, to use a new "address_from_register" function to solve
> the same problem:
> http://sourceware.org/ml/gdb-patches/2006-11/msg00134.html
> 
> Is there any reason you couldn't use the same function here?

We can't use address_from_register() in this instance since
dwarf2_read_address() is not fetching an address from a register, but
rather from some DWARF2 info.

Kevin

Re: [RFC] dwarf2_read_address(): sign extend as appropriate

[Daniel Jacobowitz]

On Mon, Apr 23, 2007 at 09:49:00AM -0700, Kevin Buettner wrote:
> We can't use address_from_register() in this instance since
> dwarf2_read_address() is not fetching an address from a register, but
> rather from some DWARF2 info.

How about value_as_address?  I don't like the need for another call
site for gdbarch_integer_to_address; it's historically tricky...

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] dwarf2_read_address(): sign extend as appropriate

[Kevin Buettner]

On Mon, 23 Apr 2007 12:56:46 -0400
Daniel Jacobowitz <drow@false.org> wrote:

> On Mon, Apr 23, 2007 at 09:49:00AM -0700, Kevin Buettner wrote:
> > We can't use address_from_register() in this instance since
> > dwarf2_read_address() is not fetching an address from a register, but
> > rather from some DWARF2 info.
> 
> How about value_as_address?  I don't like the need for another call
> site for gdbarch_integer_to_address; it's historically tricky...

dwarf2_read_address() isn't reading an address from some value in the
inferior, but rather is decoding some sequence of bytes as an address
in the DWARF2 info.  As such, dwarf2_read_address() doesn't have
anything readily available to pass to value_as_address().  That means
that a suitable value would have to be constructed and then immediately
decomposed, leading to an expression which might look something like this:

    result = value_as_address (value_from_longest 
		                (unsigned_address_type (),
                                 extract_unsigned_integer 
				   (buf,
		                    TARGET_ADDR_BIT / TARGET_CHAR_BIT)));

Please let me know if this is what you had in mind.  If so, I'll
try it out and post the results.

Kevin

Re: [RFC] dwarf2_read_address(): sign extend as appropriate

[Daniel Jacobowitz]

On Mon, Apr 23, 2007 at 02:06:42PM -0700, Kevin Buettner wrote:
> dwarf2_read_address() isn't reading an address from some value in the
> inferior, but rather is decoding some sequence of bytes as an address
> in the DWARF2 info.  As such, dwarf2_read_address() doesn't have
> anything readily available to pass to value_as_address().  That means
> that a suitable value would have to be constructed and then immediately
> decomposed, leading to an expression which might look something like this:
> 
>     result = value_as_address (value_from_longest 
> 		                (unsigned_address_type (),
>                                  extract_unsigned_integer 
> 				   (buf,
> 		                    TARGET_ADDR_BIT / TARGET_CHAR_BIT)));
> 
> Please let me know if this is what you had in mind.  If so, I'll
> try it out and post the results.

That is what I had in mind - we have an interface for doing arithmetic
with target values, but there's all sorts of places we do not use it.
But if you're justifiably offended by the inefficiency, let's not do
it now.  Your original patch is fine with me.

(If you fix the formatting issues "struct type *" shouldn't be
followed by a space, "unsigned_address_type()" should have one.)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] dwarf2_read_address(): sign extend as appropriate

[Kevin Buettner]

I decided to give the `value_as_address' approach a try.  It worked
as expected when I tried running it against a MIPS target.

Comments?

	* dwarf2expr.c (unsigned_address_type): Add forward declaration.
	(dwarf2_read_address): Sign extend return address as required by
	target architecture.

Index: dwarf2expr.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2expr.c,v
retrieving revision 1.19
diff -u -p -r1.19 dwarf2expr.c
--- dwarf2expr.c	9 Jan 2007 17:58:50 -0000	1.19
+++ dwarf2expr.c	27 Apr 2007 21:07:10 -0000
@@ -33,6 +33,7 @@
 
 static void execute_stack_op (struct dwarf_expr_context *,
 			      gdb_byte *, gdb_byte *);
+static struct type *unsigned_address_type (void);
 
 /* Create a new context for the expression evaluator.  */
 
@@ -205,9 +206,42 @@ dwarf2_read_address (gdb_byte *buf, gdb_
     error (_("dwarf2_read_address: Corrupted DWARF expression."));
 
   *bytes_read = TARGET_ADDR_BIT / TARGET_CHAR_BIT;
-  /* NOTE: cagney/2003-05-22: This extract is assuming that a DWARF 2
-     address is always unsigned.  That may or may not be true.  */
-  result = extract_unsigned_integer (buf, TARGET_ADDR_BIT / TARGET_CHAR_BIT);
+
+  /* For most architectures, calling extract_unsigned_integer() alone
+     is sufficient for extracting an address.  However, some
+     architectures (e.g. MIPS) use signed addresses and using
+     extract_unsigned_integer() will not produce a correct
+     result.  Turning the unsigned integer into a value and then
+     decomposing that value as an address will cause
+     gdbarch_integer_to_address() to be invoked for those
+     architectures which require it.  Thus, using value_as_address()
+     will produce the correct result for both types of architectures.
+
+     One concern regarding the use of values for this purpose is
+     efficiency.  Obviously, these extra calls will take more time to
+     execute and creating a value takes more space, space which will
+     have to be garbage collected at a later time.  If constructing
+     and then decomposing a value for this purpose proves to be too
+     inefficient, then gdbarch_integer_to_address() can be called
+     directly.  This could be done as follows:
+      
+	if (gdbarch_integer_to_address_p (current_gdbarch))
+	  result = gdbarch_integer_to_address (current_gdbarch,
+					       unsigned_address_type (), buf);
+        else
+          result = extract_unsigned_integer
+	             (buf, TARGET_ADDR_BIT / TARGET_CHAR_BIT); 
+
+     The use of `unsigned_address_type' in the code below (or in the
+     suggested replacement code above) refers to the type of buf and
+     has no bearing on the signedness of the address being returned.  */
+
+  result = value_as_address (value_from_longest 
+			      (unsigned_address_type (),
+			       extract_unsigned_integer 
+				 (buf,
+				  TARGET_ADDR_BIT / TARGET_CHAR_BIT)));
+
   return result;
 }
 

Re: [RFC] dwarf2_read_address(): sign extend as appropriate

[Daniel Jacobowitz]

On Fri, Apr 27, 2007 at 02:10:18PM -0700, Kevin Buettner wrote:
> I decided to give the `value_as_address' approach a try.  It worked
> as expected when I tried running it against a MIPS target.
> 
> Comments?
> 
> 	* dwarf2expr.c (unsigned_address_type): Add forward declaration.
> 	(dwarf2_read_address): Sign extend return address as required by
> 	target architecture.

Fine with me, although I would request you not to put code in the
comments; we can figure it out again later, and	I find commented out
code too confusing.

Do you happen to have more MIPS patches lying around?  I'm in the
middle of enabling CFI, based on some old patches of yours; I
encountered yet another new address sign / size confusion.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] dwarf2_read_address(): sign extend as appropriate

[Kevin Buettner]

On Fri, 27 Apr 2007 17:48:30 -0400
Daniel Jacobowitz <drow@false.org> wrote:

> On Fri, Apr 27, 2007 at 02:10:18PM -0700, Kevin Buettner wrote:
> > I decided to give the `value_as_address' approach a try.  It worked
> > as expected when I tried running it against a MIPS target.
> > 
> > Comments?
> > 
> > 	* dwarf2expr.c (unsigned_address_type): Add forward declaration.
> > 	(dwarf2_read_address): Sign extend return address as required by
> > 	target architecture.
> 
> Fine with me, although I would request you not to put code in the
> comments; we can figure it out again later, and	I find commented out
> code too confusing.

Thanks for the quick review.  I've committed my patch minus the code
that I had placed in the comments.  (See below for the patch that I
ended up committing.)

> Do you happen to have more MIPS patches lying around?  I'm in the
> middle of enabling CFI, based on some old patches of yours; I
> encountered yet another new address sign / size confusion.

I think we do have some other old MIPS patches lying about.  (I am not
the author of all of them.)  I'll make an effort to go through them
early next week and post the ones that still seem relevant.

I also noticed in my debugging earlier this week that registers are
not being correctly displayed for mips64 targets.  I know I had an old
patch that dealt with that problem.  Perhaps I'll dust off that patch
too.

Kevin


	* dwarf2expr.c (unsigned_address_type): Add forward declaration.
	(dwarf2_read_address): Sign extend return address as required by
	target architecture.

Index: dwarf2expr.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2expr.c,v
retrieving revision 1.19
diff -u -p -r1.19 dwarf2expr.c
--- dwarf2expr.c	9 Jan 2007 17:58:50 -0000	1.19
+++ dwarf2expr.c	27 Apr 2007 22:32:28 -0000
@@ -33,6 +33,7 @@
 
 static void execute_stack_op (struct dwarf_expr_context *,
 			      gdb_byte *, gdb_byte *);
+static struct type *unsigned_address_type (void);
 
 /* Create a new context for the expression evaluator.  */
 
@@ -205,9 +206,35 @@ dwarf2_read_address (gdb_byte *buf, gdb_
     error (_("dwarf2_read_address: Corrupted DWARF expression."));
 
   *bytes_read = TARGET_ADDR_BIT / TARGET_CHAR_BIT;
-  /* NOTE: cagney/2003-05-22: This extract is assuming that a DWARF 2
-     address is always unsigned.  That may or may not be true.  */
-  result = extract_unsigned_integer (buf, TARGET_ADDR_BIT / TARGET_CHAR_BIT);
+
+  /* For most architectures, calling extract_unsigned_integer() alone
+     is sufficient for extracting an address.  However, some
+     architectures (e.g. MIPS) use signed addresses and using
+     extract_unsigned_integer() will not produce a correct
+     result.  Turning the unsigned integer into a value and then
+     decomposing that value as an address will cause
+     gdbarch_integer_to_address() to be invoked for those
+     architectures which require it.  Thus, using value_as_address()
+     will produce the correct result for both types of architectures.
+
+     One concern regarding the use of values for this purpose is
+     efficiency.  Obviously, these extra calls will take more time to
+     execute and creating a value takes more space, space which will
+     have to be garbage collected at a later time.  If constructing
+     and then decomposing a value for this purpose proves to be too
+     inefficient, then gdbarch_integer_to_address() can be called
+     directly.
+
+     The use of `unsigned_address_type' in the code below refers to
+     the type of buf and has no bearing on the signedness of the
+     address being returned.  */
+
+  result = value_as_address (value_from_longest 
+			      (unsigned_address_type (),
+			       extract_unsigned_integer 
+				 (buf,
+				  TARGET_ADDR_BIT / TARGET_CHAR_BIT)));
+
   return result;
 }
 

Re: [RFC] dwarf2_read_address(): sign extend as appropriate

[Daniel Jacobowitz]

On Fri, Apr 27, 2007 at 03:41:47PM -0700, Kevin Buettner wrote:
> I think we do have some other old MIPS patches lying about.  (I am not
> the author of all of them.)  I'll make an effort to go through them
> early next week and post the ones that still seem relevant.

Thanks.  I may have some Monday myself, if today's testing goes well.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] dwarf2_read_address(): sign extend as appropriate

[Andreas Schwab]

Daniel Jacobowitz <drow@false.org> writes:

> On Mon, Apr 23, 2007 at 09:49:00AM -0700, Kevin Buettner wrote:
>> We can't use address_from_register() in this instance since
>> dwarf2_read_address() is not fetching an address from a register, but
>> rather from some DWARF2 info.
>
> How about value_as_address?

How about extract_typed_address?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFC] dwarf2_read_address(): sign extend as appropriate

[Kevin Buettner]

On Tue, 24 Apr 2007 00:43:05 +0200
Andreas Schwab <schwab@suse.de> wrote:

> Daniel Jacobowitz <drow@false.org> writes:
> 
> > On Mon, Apr 23, 2007 at 09:49:00AM -0700, Kevin Buettner wrote:
> >> We can't use address_from_register() in this instance since
> >> dwarf2_read_address() is not fetching an address from a register, but
> >> rather from some DWARF2 info.
> >
> > How about value_as_address?
> 
> How about extract_typed_address?

I really liked this suggestion when I first saw it because it appears
that it would do the right thing on MIPS and we would avoid creating a
value which we'd immediately take apart again.

However, after further study, I see two issues with using this function:

    1) extract_typed_address() does its conversion using
       POINTER_TO_ADDRESS.  For most architectures, this isn't
       an issue, but I'm not convinced that it will work correctly
       for those which define non-trivial pointer-to-address methods. 

    2) dwarf2_read_address is currently extracting an address which
       is TARGET_ADDR_BIT bits wide.  extract_typed_address() must be
       invoked on a pointer (TYPE_CODE_PTR) or reference (TYPE_CODE_REF)
       type.  If it gets anything else, it generates an internal error.
       If the pointer type passed to extract_typed_address() is created
       via make_pointer_type(), the type width will be TARGET_PTR_BIT
       rather than TARGET_ADDR_BIT.  Thus, I'd expect extract_typed_address()
       to work as expected on architectures for which TARGET_ADDR_BIT
       is the same as TARGET_PTR_BIT, but not otherwise - unless some
       sort of special TARGET_ADDR_BIT width pointer were created for
       just this purpose - but that smells like a hack to me.

Given these problems, I don't think that the use of
extract_typed_address() is suitable for this situation.

That leaves the value_as_address() suggestion.  I don't like that
suggestion, but my only objection is that it is inefficient.  (As far
as I can tell, it would work correctly.)  It seems wasteful (in both
time and space) to create a (struct value *) with the express purpose
of immediately decomposing it, never to be used again.

Kevin