[patch, rfc] Re: MIPS dwarf2 location lists

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

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