MIPS dwarf2 location lists

MIPS dwarf2 location lists

[Daniel Jacobowitz]

Hi Ulrich,

Your patch here:

Subject: [rfc] Fix address vs. offset handling in DWARF-2 location lists
http://sourceware.org/ml/gdb-patches/2009-07/msg00378.html

stopped us from using dwarf2_read_address for the offsets in location
lists.  The comment there talks specifically about MIPS:

  /* 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.  Make sure we invoke gdbarch_integer_to_address()
     for those architectures which require it.

This comment does apply to the calls you removed.  GCC typically
generates base_address == 0 and puts the whole address in the
offsets.  Therefore they must be sign extended, and it's
gdbarch_integer_to_address which does that.  So now we're getting
zero-extended addresses, and they don't match anything.

This ties in to my message last night, which was about a different
instance of a similar bug:

Subject: CORE_ADDR representation
http://sourceware.org/ml/gdb/2010-02/msg00118.html

Unlike the place referenced there, however, this is just comparing the
results.  So we have a lot of flexibility; the "abstractly right" fix
would work, but so would a simple mask and check.

Do you have any better idea than the attached?

The tree I'm testing in is not unmodified HEAD; I'm not set up for
that at the moment.  I believe it has a number of other patches
of similar nature to this one.

-- 
Daniel Jacobowitz
CodeSourcery

2010-02-18  Daniel Jacobowitz  <dan@codesourcery.com>

	gdb/
	* dwarf2loc.c (find_location_expression): Mask addresses before
	comparing.

--- gdb-merged-localpatches/gdb/dwarf2loc.c	2010-02-11 12:12:07.000000000 -0800
+++ gdb-merged-postmips/gdb/dwarf2loc.c	2010-02-18 06:05:55.000000000 -0800
@@ -105,7 +105,8 @@ find_location_expression (struct dwarf2_
       length = extract_unsigned_integer (loc_ptr, 2, byte_order);
       loc_ptr += 2;
 
-      if (pc >= low && pc < high)
+      if ((pc & base_mask) >= (low & base_mask)
+	  && (pc & base_mask) < (high & base_mask))
 	{
 	  *locexpr_length = length;
 	  return loc_ptr;

Re: MIPS dwarf2 location lists

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> Subject: [rfc] Fix address vs. offset handling in DWARF-2 location lists
> http://sourceware.org/ml/gdb-patches/2009-07/msg00378.html
> 
> stopped us from using dwarf2_read_address for the offsets in location
> lists.  The comment there talks specifically about MIPS:
> 
>   /* 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.  Make sure we invoke gdbarch_integer_to_address()
>      for those architectures which require it.
> 
> This comment does apply to the calls you removed.  GCC typically
> generates base_address == 0 and puts the whole address in the
> offsets.  Therefore they must be sign extended, and it's
> gdbarch_integer_to_address which does that.  So now we're getting
> zero-extended addresses, and they don't match anything.

Hmm.  The problem I was fixing is that on the Cell/B.E., the transformation
of a target integer into an GDB address is more complex than simply choosing
between sign- or zero-extending.

On Cell, the GDB addresses encode the SPU ID for addresses within
SPU contexts attached to the process.  In the target format (which
is also used in object files, including DWARF addresses etc.), an
address is a plain SPU local store address.

When gdbarch_integer_to_address is called, we attach the SPU ID
into the high word of the 64-bit CORE_ADDR; for example, an SPU
local store address of 0x3f000 in SPU context #7 gets encoded
into a CORE_ADDR value of 0x800000070003f000.

gdbarch_integer_to_address is the right place to do this, because
we want the transformation to also happen for addresses entered
as literals on the command line, for example.

But this means that we need to distinguish very clearly between
*addresses* and *offsets*.  A transformation like the above must
only be done on base addresses; offsets are simply treated as
integers that are added onto an already transformed base address,
and must not themselves be transformed as well.

However, it seems that there is still one choice that isn't
fully specified: should an offset be treated as *signed* or
*unsigned* integer?  The DWARF standard is silent on this;
maybe it assumes this doesn't matter as address arithmetic
on these offsets is supposed to be performed within the DWARF
address size precision? 

> This ties in to my message last night, which was about a different
> instance of a similar bug:
> 
> Subject: CORE_ADDR representation
> http://sourceware.org/ml/gdb/2010-02/msg00118.html
> 
> Unlike the place referenced there, however, this is just comparing the
> results.  So we have a lot of flexibility; the "abstractly right" fix
> would work, but so would a simple mask and check.

Masking high bits off will actually break Cell, because it would
cause the SPU ID to be simply ignored in comparisons.  In the past,
the only places where addresses were truncated according to
gdbarch_addr_bit did so to output an address in human- or machine-
readable string form (as user output, or remote/monitor protocol
elements).  At these points, address truncation in a sense serves
as the reverse of the gdbarch_integer_to_address transform; this
is exactly what's needed on Cell.

However, there have been a couple of places recently introduced
where truncated address values are subsequently used as CORE_ADDR
by common code, in particular the place you point out above.  It
seems that this will break Cell as well (and presumably breaks
MIPS as you noticed).  Using address truncation like this doesn't
seem right to me.  (The other recently affected places are
dwarf2loc.c:read_pieced_value and solib-svr4.c:enable_break.)


> Do you have any better idea than the attached?

One way would be to treat address offsets as signed or unsigned
depending on bfd_get_sign_extend_vma.

A maybe more generic way should be to perform the arithmetic
on plain integral values, and only call gdbarch_integer_to_address
on the *sums* (i.e. the final DWARF addresses), along the lines of:

      low = extract_unsigned_integer (loc_ptr, addr_size, byte_order);
      loc_ptr += addr_size;

      high = extract_unsigned_integer (loc_ptr, addr_size, byte_order);
      loc_ptr += addr_size;

      /* A base-address-selection entry.  */
      if (low == base_mask)
        {
          base_address = high;
          continue;
        }

      /* An end-of-list entry.  */
      if (low == 0 && high == 0)
        return NULL;

      /* Otherwise, a location expression entry.  */
      store_unsigned_integer (tmp, addr_size, byte_order, low + base_address);
      low = dwarf2_read_address (gdbarch, tmp, tmp + addr_size, addr_size);

      store_unsigned_integer (tmp, addr_size, byte_order, high + base_address);
      high = dwarf2_read_address (gdbarch, tmp, tmp + addr_size, addr_size);


As to build_section_addr_info_from_objfile, I'm wondering what problem the
masking was supposed to address:

      sap->other[i].addr = (bfd_get_section_vma (objfile->obfd, sec)
                            + objfile->section_offsets->offsets[i]) & mask;

It looks like this happens if an object is loaded below its original load
address (I guess this can happen if a prelinked object is loaded somewhere
else), and thus the section_offsets need to be negative, which may not
work as intended as CORE_ADDR is unsigned.  It seems that whoever computes
the section_offsets in this particular case ought to be able to compensate ...

Bye,
Ulrich

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

Re: MIPS dwarf2 location lists

[Daniel Jacobowitz]

On Thu, Feb 18, 2010 at 05:47:32PM +0100, Ulrich Weigand wrote:
> On Cell, the GDB addresses encode the SPU ID for addresses within
> SPU contexts attached to the process.  In the target format (which
> is also used in object files, including DWARF addresses etc.), an
> address is a plain SPU local store address.
> 
> When gdbarch_integer_to_address is called, we attach the SPU ID
> into the high word of the 64-bit CORE_ADDR; for example, an SPU
> local store address of 0x3f000 in SPU context #7 gets encoded
> into a CORE_ADDR value of 0x800000070003f000.
> 
> gdbarch_integer_to_address is the right place to do this, because
> we want the transformation to also happen for addresses entered
> as literals on the command line, for example.

As an aside, I've been thinking about this model.  We have grown a lot
of support for multiple address spaces since this was implemented.
Is that something we can use instead of overloading CORE_ADDR bits?
It'd be a big project; I think values would need to carry an address
space around.  Anyway, back to our topic.

> But this means that we need to distinguish very clearly between
> *addresses* and *offsets*.  A transformation like the above must
> only be done on base addresses; offsets are simply treated as
> integers that are added onto an already transformed base address,
> and must not themselves be transformed as well.
> 
> However, it seems that there is still one choice that isn't
> fully specified: should an offset be treated as *signed* or
> *unsigned* integer?  The DWARF standard is silent on this;
> maybe it assumes this doesn't matter as address arithmetic
> on these offsets is supposed to be performed within the DWARF
> address size precision? 

That's my guess.  We really should be doing CORE_ADDR arithmetic in an
architecture-specific precision.  If we did that, the question of
whether the CORE_ADDR is sign or zero extended would become an
implementation detail.

> Masking high bits off will actually break Cell, because it would
> cause the SPU ID to be simply ignored in comparisons.  In the past,
> the only places where addresses were truncated according to
> gdbarch_addr_bit did so to output an address in human- or machine-
> readable string form (as user output, or remote/monitor protocol
> elements).  At these points, address truncation in a sense serves
> as the reverse of the gdbarch_integer_to_address transform; this
> is exactly what's needed on Cell.

Oh, great.  So we can't sign extend, zero extend, or mask.  And
wraparound offsets are going to completely confuse the situation.

My best idea is to separate addresses and offsets - as you've
suggested, and I think Mark suggested this week also - and define
offsets as sign extended.  I'm not entirely confident that will work,
though, because when we get an offset we don't know if it's supposed
to wrap around or not.  If you have a binary linked at zero (because
it's position-independent, say), and it's loaded at 0x90000000, then
is that a positive or negative offset?  On a 32-bit platform, with
64-bit CORE_ADDR, if you define that as a positive offset you'll break
MIPS.  If you define it as a negative offset you'll break x86 and
SPU (it'll decrement the SPU number in the address).  Obviously I'd
have to pick a different example for it to make sense on SPU, since
they have a tiny local store.

> As to build_section_addr_info_from_objfile, I'm wondering what problem the
> masking was supposed to address:
> 
>       sap->other[i].addr = (bfd_get_section_vma (objfile->obfd, sec)
>                             + objfile->section_offsets->offsets[i]) & mask;
> 
> It looks like this happens if an object is loaded below its original load
> address (I guess this can happen if a prelinked object is loaded somewhere
> else), and thus the section_offsets need to be negative, which may not
> work as intended as CORE_ADDR is unsigned.  It seems that whoever computes
> the section_offsets in this particular case ought to be able to compensate ...

FWIW, the patch was here:

http://sourceware.org/ml/gdb-patches/2009-11/msg00173.html

-- 
Daniel Jacobowitz
CodeSourcery

Re: MIPS dwarf2 location lists

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Thu, Feb 18, 2010 at 05:47:32PM +0100, Ulrich Weigand wrote:
> > On Cell, the GDB addresses encode the SPU ID for addresses within
> > SPU contexts attached to the process.  In the target format (which
> > is also used in object files, including DWARF addresses etc.), an
> > address is a plain SPU local store address.
> > 
> > When gdbarch_integer_to_address is called, we attach the SPU ID
> > into the high word of the 64-bit CORE_ADDR; for example, an SPU
> > local store address of 0x3f000 in SPU context #7 gets encoded
> > into a CORE_ADDR value of 0x800000070003f000.
> > 
> > gdbarch_integer_to_address is the right place to do this, because
> > we want the transformation to also happen for addresses entered
> > as literals on the command line, for example.
> 
> As an aside, I've been thinking about this model.  We have grown a lot
> of support for multiple address spaces since this was implemented.
> Is that something we can use instead of overloading CORE_ADDR bits?
> It'd be a big project; I think values would need to carry an address
> space around.

Yes, this would be nice to fix.  One problem is that we'd not just
need multiple address spaces, but also multiple program spaces --
each SPU context has its own object file(s) loaded.  And even in
current GDB, we still have fundamentally just one program space
per inferior; rooting out the current_program_space global will
be a nontrivial effort.

Values (at least lval_memory values) would certainly need to carry
an address space.  For values of pointer type, we'd also need the
address space of the object pointed to -- this may be different from
the one where the pointer itself resides.

> My best idea is to separate addresses and offsets - as you've
> suggested, and I think Mark suggested this week also - and define
> offsets as sign extended.  I'm not entirely confident that will work,
> though, because when we get an offset we don't know if it's supposed
> to wrap around or not.  If you have a binary linked at zero (because
> it's position-independent, say), and it's loaded at 0x90000000, then
> is that a positive or negative offset?  On a 32-bit platform, with
> 64-bit CORE_ADDR, if you define that as a positive offset you'll break
> MIPS.  If you define it as a negative offset you'll break x86 and
> SPU (it'll decrement the SPU number in the address).  Obviously I'd
> have to pick a different example for it to make sense on SPU, since
> they have a tiny local store.

Actually, I think this can be made to work.  Basic rule needs to be
that an offset is always computed as the difference between two addresses,
where addresses have undergone the appropriate platform-specific treatment.

So in your example, link address zero would be CORE_ADDR 0 on both i386
and mips (note that even zero would actually become something like
0x8000000700000000 on spu!), but the load address 0x90000000 would be
CORE_ADDR 0x90000000 on i386 but 0xffffffff90000000 on mips.  Subtracting
the link address would thus result in the appropriate offset for the
platform.

Or, to look at it differently, if we compute the offset to move A to B
as (B - A), adding that offset to A will always result in B again, as
long as both the subtraction and the addition are performed on plain
CORE_ADDR values.  As long as addresses A and B are always computed
like addresses on the platform need to be, we should be fine.

(There is the theoretical case that if we apply the offset (B - A) to
some address that is itself offset, like A + x, we could get wrong
overflow.  But this can really only occur if a single objfile is
wrapped around the top of memory -- I don't think this is really
possible.)


> > As to build_section_addr_info_from_objfile, I'm wondering what problem the
> > masking was supposed to address:
> > 
> >       sap->other[i].addr = (bfd_get_section_vma (objfile->obfd, sec)
> >                             + objfile->section_offsets->offsets[i]) & mask;
> > 
> > It looks like this happens if an object is loaded below its original load
> > address (I guess this can happen if a prelinked object is loaded somewhere
> > else), and thus the section_offsets need to be negative, which may not
> > work as intended as CORE_ADDR is unsigned.  It seems that whoever computes
> > the section_offsets in this particular case ought to be able to compensate ...
> 
> FWIW, the patch was here:
> 
> http://sourceware.org/ml/gdb-patches/2009-11/msg00173.html

If the offsets were computed as outlined above, it seems to me this should
always work.  It's not quite clear from the patch description where this
is assumption is violated ...

Bye,
Ulrich

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

[patch, rfc] Re: MIPS dwarf2 location lists

[Ulrich Weigand]

Hi Dan,

> Your patch here:
> 
> Subject: [rfc] Fix address vs. offset handling in DWARF-2 location lists
> http://sourceware.org/ml/gdb-patches/2009-07/msg00378.html
> 
> stopped us from using dwarf2_read_address for the offsets in location
> lists.  The comment there talks specifically about MIPS:
> 
>   /* 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.  Make sure we invoke gdbarch_integer_to_address()
>      for those architectures which require it.
> 
> This comment does apply to the calls you removed.  GCC typically
> generates base_address == 0 and puts the whole address in the
> offsets.  Therefore they must be sign extended, and it's
> gdbarch_integer_to_address which does that.  So now we're getting
> zero-extended addresses, and they don't match anything.


I've had another look at fixing this problem, and noted a completely
different bug:

      /* A base-address-selection entry.  */
      if (low == base_mask)
        {
          base_address = dwarf2_read_address (gdbarch,
                                              loc_ptr, buf_end, addr_size);
          loc_ptr += addr_size;
          continue;
        }

This is actually wrong, as it neglects to apply the base relocation offset
of the objfile.

But once we fix this, calling gdbarch_integer_to_address is in fact wrong
again on Cell/B.E., because the relocation offset already includes the
SPU ID (on the Cell, SPU ELF files are "relocated" to an address that
contains the SPU ID) -- adding it a second time is wrong.

On the other hand, the DWARF standard says that location lists ought to
be handled exactly like range lists, and *those* are handled in dwarf2read.c
without any attempt to use gdbarch_integer_to_address.  Instead, the
routine simply consults the BFD's sign_extended_vma flag, and reads the
address as signed or unsigned integer accordling, and then adds the base
relocation offset.  This looks to me as a straighforward, correct way to
handle Cell/B.E., MIPS, and all the "normal" platforms ...

The following patch changes dwarf2loc.c to handle things in exactly the
same way, without using dwarf2_read_address.  (There is a second use of
dwarf2_read_address which I also removed, as TLS offsets are in fact not
addresses but plain integers.  The only remaining uses of dwarf2_read_address
are in dwarf2expr.c itself.)

Tested on Cell/B.E. with no regressions.
What do you think?  Does it work for MIPS?

Bye,
Ulrich

ChangeLog:

	* dwarf2expr.c (dwarf2_read_address): Make static.
	* dwarf2expr.h (dwarf2_read_address): Remove prototype.
	* dwarf2loc.c (find_location_expression): Add relocation offset
	to base-address-selection entry base addresses.  Read addresses
	(and offsets) as signed/unsigned integers, depending on the
	BFD's sign_extend_vma flag.  Do not call dwarf2_read_address.
	(locexpr_describe_location): Read TLS offset as unsigned
	integer.  Do not call dwarf2_read_address.


Index: gdb/dwarf2expr.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2expr.c,v
retrieving revision 1.40
diff -u -p -r1.40 dwarf2expr.c
--- gdb/dwarf2expr.c	20 Jan 2010 18:06:15 -0000	1.40
+++ gdb/dwarf2expr.c	3 Mar 2010 19:24:54 -0000
@@ -245,7 +245,7 @@ read_sleb128 (gdb_byte *buf, gdb_byte *b
 /* Read an address of size ADDR_SIZE from BUF, and verify that it
    doesn't extend past BUF_END.  */
 
-CORE_ADDR
+static CORE_ADDR
 dwarf2_read_address (struct gdbarch *gdbarch, gdb_byte *buf,
 		     gdb_byte *buf_end, int addr_size)
 {
Index: gdb/dwarf2expr.h
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2expr.h,v
retrieving revision 1.20
diff -u -p -r1.20 dwarf2expr.h
--- gdb/dwarf2expr.h	1 Jan 2010 07:31:30 -0000	1.20
+++ gdb/dwarf2expr.h	3 Mar 2010 19:24:54 -0000
@@ -198,7 +198,5 @@ int dwarf_expr_fetch_in_stack_memory (st
 
 gdb_byte *read_uleb128 (gdb_byte *buf, gdb_byte *buf_end, ULONGEST * r);
 gdb_byte *read_sleb128 (gdb_byte *buf, gdb_byte *buf_end, LONGEST * r);
-CORE_ADDR dwarf2_read_address (struct gdbarch *gdbarch, gdb_byte *buf,
-			       gdb_byte *buf_end, int addr_size);
 
 #endif /* dwarf2expr.h */
Index: gdb/dwarf2loc.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2loc.c,v
retrieving revision 1.73
diff -u -p -r1.73 dwarf2loc.c
--- gdb/dwarf2loc.c	26 Feb 2010 12:48:18 -0000	1.73
+++ gdb/dwarf2loc.c	3 Mar 2010 19:24:55 -0000
@@ -65,6 +65,7 @@ find_location_expression (struct dwarf2_
   struct gdbarch *gdbarch = get_objfile_arch (objfile);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   unsigned int addr_size = dwarf2_per_cu_addr_size (baton->per_cu);
+  int signed_addr_p = bfd_get_sign_extend_vma (objfile->obfd);
   CORE_ADDR base_mask = ~(~(CORE_ADDR)1 << (addr_size * 8 - 1));
   /* Adjust base_address for relocatable objects.  */
   CORE_ADDR base_offset = ANOFFSET (objfile->section_offsets,
@@ -79,21 +80,25 @@ find_location_expression (struct dwarf2_
       if (buf_end - loc_ptr < 2 * addr_size)
 	error (_("find_location_expression: Corrupted DWARF expression."));
 
-      low = extract_unsigned_integer (loc_ptr, addr_size, byte_order);
+      if (signed_addr_p)
+	low = extract_signed_integer (loc_ptr, addr_size, byte_order);
+      else
+	low = extract_unsigned_integer (loc_ptr, addr_size, byte_order);
+      loc_ptr += addr_size;
+
+      if (signed_addr_p)
+	high = extract_signed_integer (loc_ptr, addr_size, byte_order);
+      else
+	high = extract_unsigned_integer (loc_ptr, addr_size, byte_order);
       loc_ptr += addr_size;
 
       /* A base-address-selection entry.  */
-      if (low == base_mask)
+      if ((low & base_mask) == base_mask)
 	{
-	  base_address = dwarf2_read_address (gdbarch,
-					      loc_ptr, buf_end, addr_size);
-	  loc_ptr += addr_size;
+	  base_address = high + base_offset;
 	  continue;
 	}
 
-      high = extract_unsigned_integer (loc_ptr, addr_size, byte_order);
-      loc_ptr += addr_size;
-
       /* An end-of-list entry.  */
       if (low == 0 && high == 0)
 	return NULL;
@@ -775,14 +780,13 @@ locexpr_describe_location (struct symbol
       {
 	struct objfile *objfile = dwarf2_per_cu_objfile (dlbaton->per_cu);
 	struct gdbarch *gdbarch = get_objfile_arch (objfile);
-	CORE_ADDR offset = dwarf2_read_address (gdbarch,
-						&dlbaton->data[1],
-						&dlbaton->data[dlbaton->size - 1],
-						addr_size);
+	enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+	ULONGEST offset = extract_unsigned_integer (&dlbaton->data[1],
+						    addr_size, byte_order);
 	fprintf_filtered (stream, 
 			  "a thread-local variable at offset %s in the "
 			  "thread-local storage for `%s'",
-			  paddress (gdbarch, offset), objfile->name);
+			  phex_nz (offset, addr_size), objfile->name);
 	return 1;
       }
   

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

Re: [patch, rfc] Re: MIPS dwarf2 location lists

[Ulrich Weigand]

> 	* dwarf2expr.c (dwarf2_read_address): Make static.
> 	* dwarf2expr.h (dwarf2_read_address): Remove prototype.
> 	* dwarf2loc.c (find_location_expression): Add relocation offset
> 	to base-address-selection entry base addresses.  Read addresses
> 	(and offsets) as signed/unsigned integers, depending on the
> 	BFD's sign_extend_vma flag.  Do not call dwarf2_read_address.
> 	(locexpr_describe_location): Read TLS offset as unsigned
> 	integer.  Do not call dwarf2_read_address.

Ping.  Any thoughts on whether we should go with this?

Bye,
Ulrich

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

Re: [patch, rfc] Re: MIPS dwarf2 location lists

[Joel Brobecker]

> > 	* dwarf2expr.c (dwarf2_read_address): Make static.
> > 	* dwarf2expr.h (dwarf2_read_address): Remove prototype.
> > 	* dwarf2loc.c (find_location_expression): Add relocation offset
> > 	to base-address-selection entry base addresses.  Read addresses
> > 	(and offsets) as signed/unsigned integers, depending on the
> > 	BFD's sign_extend_vma flag.  Do not call dwarf2_read_address.
> > 	(locexpr_describe_location): Read TLS offset as unsigned
> > 	integer.  Do not call dwarf2_read_address.
> 
> Ping.  Any thoughts on whether we should go with this?

Can't really say much about the patch itself; but FWIW, I tested it
on mips-irix, and saw no regression.

-- 
Joel

[patch, rfc, v2] MIPS dwarf2 location lists

[Ulrich Weigand]

Joel Brobecker wrote:

> > > 	* dwarf2expr.c (dwarf2_read_address): Make static.
> > > 	* dwarf2expr.h (dwarf2_read_address): Remove prototype.
> > > 	* dwarf2loc.c (find_location_expression): Add relocation offset
> > > 	to base-address-selection entry base addresses.  Read addresses
> > > 	(and offsets) as signed/unsigned integers, depending on the
> > > 	BFD's sign_extend_vma flag.  Do not call dwarf2_read_address.
> > > 	(locexpr_describe_location): Read TLS offset as unsigned
> > > 	integer.  Do not call dwarf2_read_address.
> > 
> > Ping.  Any thoughts on whether we should go with this?
> 
> Can't really say much about the patch itself; but FWIW, I tested it
> on mips-irix, and saw no regression.

Thanks for running the test!  And sorry for taking so long to get back
to this patch ...   Dan agreed (in an off-line mail) that the patch
makes sense to him, so I'm planning to go forward with it.

Due to Tom's recent dwarf2 changes, a couple of modifications were
required.  An updated patch is appended below.

Retested with no regressions on i386-linux.
I'm planning on committing the patch next week.

Thanks,
Ulrich

ChangeLog:

	* dwarf2expr.c (dwarf2_read_address): Make static.
	* dwarf2expr.h (dwarf2_read_address): Remove prototype.
	* dwarf2loc.c (find_location_expression): Add relocation offset
	to base-address-selection entry base addresses.  Read addresses
	(and offsets) as signed/unsigned integers, depending on the
	BFD's sign_extend_vma flag.  Do not call dwarf2_read_address.
	(loclist_describe_location): Likewise.
	(disassemble_dwarf_expression): Read DW_OP_addr operand as
	unsigned integer.  Do not call dwarf2_read_address.
	(locexpr_describe_location): Likewise for DW_OP_GNU_push_tls_address.


Index: gdb/dwarf2expr.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2expr.c,v
retrieving revision 1.46
diff -u -p -r1.46 dwarf2expr.c
--- gdb/dwarf2expr.c	7 Jun 2010 19:55:33 -0000	1.46
+++ gdb/dwarf2expr.c	11 Jun 2010 13:19:13 -0000
@@ -263,7 +263,7 @@ read_sleb128 (const gdb_byte *buf, const
 /* Read an address of size ADDR_SIZE from BUF, and verify that it
    doesn't extend past BUF_END.  */
 
-CORE_ADDR
+static CORE_ADDR
 dwarf2_read_address (struct gdbarch *gdbarch, const gdb_byte *buf,
 		     const gdb_byte *buf_end, int addr_size)
 {
Index: gdb/dwarf2expr.h
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2expr.h,v
retrieving revision 1.25
diff -u -p -r1.25 dwarf2expr.h
--- gdb/dwarf2expr.h	7 Jun 2010 19:55:33 -0000	1.25
+++ gdb/dwarf2expr.h	11 Jun 2010 13:19:13 -0000
@@ -204,8 +204,6 @@ const gdb_byte *read_uleb128 (const gdb_
 			      ULONGEST * r);
 const gdb_byte *read_sleb128 (const gdb_byte *buf, const gdb_byte *buf_end,
 			      LONGEST * r);
-CORE_ADDR dwarf2_read_address (struct gdbarch *gdbarch, const gdb_byte *buf,
-			       const gdb_byte *buf_end, int addr_size);
 
 const char *dwarf_stack_op_name (unsigned int, int);
 
Index: gdb/dwarf2loc.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2loc.c,v
retrieving revision 1.88
diff -u -p -r1.88 dwarf2loc.c
--- gdb/dwarf2loc.c	7 Jun 2010 19:55:33 -0000	1.88
+++ gdb/dwarf2loc.c	11 Jun 2010 13:19:13 -0000
@@ -67,6 +67,7 @@ find_location_expression (struct dwarf2_
   struct gdbarch *gdbarch = get_objfile_arch (objfile);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   unsigned int addr_size = dwarf2_per_cu_addr_size (baton->per_cu);
+  int signed_addr_p = bfd_get_sign_extend_vma (objfile->obfd);
   CORE_ADDR base_mask = ~(~(CORE_ADDR)1 << (addr_size * 8 - 1));
   /* Adjust base_address for relocatable objects.  */
   CORE_ADDR base_offset = ANOFFSET (objfile->section_offsets,
@@ -81,21 +82,25 @@ find_location_expression (struct dwarf2_
       if (buf_end - loc_ptr < 2 * addr_size)
 	error (_("find_location_expression: Corrupted DWARF expression."));
 
-      low = extract_unsigned_integer (loc_ptr, addr_size, byte_order);
+      if (signed_addr_p)
+	low = extract_signed_integer (loc_ptr, addr_size, byte_order);
+      else
+	low = extract_unsigned_integer (loc_ptr, addr_size, byte_order);
+      loc_ptr += addr_size;
+
+      if (signed_addr_p)
+	high = extract_signed_integer (loc_ptr, addr_size, byte_order);
+      else
+	high = extract_unsigned_integer (loc_ptr, addr_size, byte_order);
       loc_ptr += addr_size;
 
       /* A base-address-selection entry.  */
-      if (low == base_mask)
+      if ((low & base_mask) == base_mask)
 	{
-	  base_address = dwarf2_read_address (gdbarch,
-					      loc_ptr, buf_end, addr_size);
-	  loc_ptr += addr_size;
+	  base_address = high + base_offset;
 	  continue;
 	}
 
-      high = extract_unsigned_integer (loc_ptr, addr_size, byte_order);
-      loc_ptr += addr_size;
-
       /* An end-of-list entry.  */
       if (low == 0 && high == 0)
 	return NULL;
@@ -1468,15 +1473,14 @@ locexpr_describe_location_piece (struct 
 	   && data[1 + addr_size] == DW_OP_GNU_push_tls_address
 	   && piece_end_p (data + 2 + addr_size, end))
     {
-      CORE_ADDR offset = dwarf2_read_address (gdbarch,
-					      data + 1,
-					      end,
-					      addr_size);
+      ULONGEST offset;
+      offset = extract_unsigned_integer (data + 1, addr_size,
+					 gdbarch_byte_order (gdbarch));
 
       fprintf_filtered (stream, 
-			_("a thread-local variable at offset %s "
+			_("a thread-local variable at offset 0x%s "
 			  "in the thread-local storage for `%s'"),
-			paddress (gdbarch, offset), objfile->name);
+			phex_nz (offset, addr_size), objfile->name);
 
       data += 1 + addr_size + 1;
     }
@@ -1513,7 +1517,6 @@ disassemble_dwarf_expression (struct ui_
 	     || (data[0] != DW_OP_piece && data[0] != DW_OP_bit_piece)))
     {
       enum dwarf_location_atom op = *data++;
-      CORE_ADDR addr;
       ULONGEST ul;
       LONGEST l;
       const char *name;
@@ -1528,9 +1531,10 @@ disassemble_dwarf_expression (struct ui_
       switch (op)
 	{
 	case DW_OP_addr:
-	  addr = dwarf2_read_address (arch, data, end, addr_size);
+	  ul = extract_unsigned_integer (data, addr_size,
+					 gdbarch_byte_order (arch));
 	  data += addr_size;
-	  fprintf_filtered (stream, " %s", paddress (arch, addr));
+	  fprintf_filtered (stream, " 0x%s", phex_nz (ul, addr_size));
 	  break;
 
 	case DW_OP_const1u:
@@ -1938,6 +1942,7 @@ loclist_describe_location (struct symbol
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   unsigned int addr_size = dwarf2_per_cu_addr_size (dlbaton->per_cu);
   int offset_size = dwarf2_per_cu_offset_size (dlbaton->per_cu);
+  int signed_addr_p = bfd_get_sign_extend_vma (objfile->obfd);
   CORE_ADDR base_mask = ~(~(CORE_ADDR)1 << (addr_size * 8 - 1));
   /* Adjust base_address for relocatable objects.  */
   CORE_ADDR base_offset = ANOFFSET (objfile->section_offsets,
@@ -1956,23 +1961,27 @@ loclist_describe_location (struct symbol
 	error (_("Corrupted DWARF expression for symbol \"%s\"."),
 	       SYMBOL_PRINT_NAME (symbol));
 
-      low = extract_unsigned_integer (loc_ptr, addr_size, byte_order);
+      if (signed_addr_p)
+	low = extract_signed_integer (loc_ptr, addr_size, byte_order);
+      else
+	low = extract_unsigned_integer (loc_ptr, addr_size, byte_order);
+      loc_ptr += addr_size;
+
+      if (signed_addr_p)
+	high = extract_signed_integer (loc_ptr, addr_size, byte_order);
+      else
+	high = extract_unsigned_integer (loc_ptr, addr_size, byte_order);
       loc_ptr += addr_size;
 
       /* A base-address-selection entry.  */
-      if (low == base_mask)
+      if ((low & base_mask) == base_mask)
 	{
-	  base_address = dwarf2_read_address (gdbarch,
-					      loc_ptr, buf_end, addr_size);
+	  base_address = high + base_offset;
 	  fprintf_filtered (stream, _("  Base address %s"),
 			    paddress (gdbarch, base_address));
-	  loc_ptr += addr_size;
 	  continue;
 	}
 
-      high = extract_unsigned_integer (loc_ptr, addr_size, byte_order);
-      loc_ptr += addr_size;
-
       /* An end-of-list entry.  */
       if (low == 0 && high == 0)
 	break;



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

Re: [patch, rfc, v2] MIPS dwarf2 location lists

[Ulrich Weigand]

> 	* dwarf2loc.c (find_location_expression): Add relocation offset
> 	to base-address-selection entry base addresses.  Read addresses
> 	(and offsets) as signed/unsigned integers, depending on the
> 	BFD's sign_extend_vma flag.  Do not call dwarf2_read_address.
> 	(loclist_describe_location): Likewise.
> 	(disassemble_dwarf_expression): Read DW_OP_addr operand as
> 	unsigned integer.  Do not call dwarf2_read_address.
> 	(locexpr_describe_location): Likewise for DW_OP_GNU_push_tls_address.

I've now checked this in, but omitted this part:

> 	* dwarf2expr.c (dwarf2_read_address): Make static.
> 	* dwarf2expr.h (dwarf2_read_address): Remove prototype.

because we now do have another user of dwarf2_read_address (in the
new dwarf->ax translator).

Bye,
Ulrich

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