[rfc] Decode "is a variable with complex or multiple locations" (PR 8399)

[rfc] Decode "is a variable with complex or multiple locations" (PR 8399)

[Jan Kratochvil]

On Wed, 16 Sep 2009 22:11:32 +0200, Roland McGrath wrote:
> If this message went away, it would reduce the incidence of murderous rage
> among certain GDB users.  Even just printing the hex file offset of the DIE
> whose location attribute is in question would be ten times better.  A dump
> of the DWARF location expression that applies at the PC that I asked about
> (i.e. current frame's context for "info addr", given one for "info scope")
> would be almost civilized.

Wrote a patch for it but now I am not sure it is meaningful at all.

For a variable with readelf output:
    001072b5 00000000005157c9 00000000005157e9 (DW_OP_fbreg: -20)
    001072b5 00000000005157e9 00000000005157f2 (DW_OP_breg7: 60)
    001072b5 00000000005157f2 0000000000515835 (DW_OP_fbreg: -20)
    001072b5 <End of list>

the attached patch now prints:

(gdb) info address val
Symbol "val" is a variable with multiple locations, for PC inclusive - exclusive:
0x5157c9 - 0x5157e9: stored relative to frame base at offset -20 
0x5157e9 - 0x5157f2: stored relative to register rsp at offset 60
0x5157f2 - 0x515835: stored relative to frame base at offset -20 
and optimized out in ranges not covered above.


(a) While it does what `info address' should do I think you (+me) wished more
    just the single line valid for the current PC.

    This would do `info scope *$pc' but that one prints _all_ the visible
    variables with no way how to request just one variable.

    As the attached patch mostly just reimplements already available readelf
    one should probably change `info address' to print it only for the current
    PC address.  But for async mode one should expect some LOCATION parameter
    (an optional new second parameter?).

    One IMO expects info on `struct value' (with its lvalue address incl.
    registers) than on `struct dwarf2_locexpr_baton' (DWARF block content).

(b) It prints it in a human readable format so more complex expressions are still
    not decoded.

    GDB should probably reuse binutils/dwarf.c:decode_location_expression().

(b1) http://sources.redhat.com/ml/gdb/2003-07/msg00242.html discussion had no
     conclusion where the code should be placed (probably
     bfd/file-no-in-libbfd.a).

(c) "a variable in register rsi" vs. "DW_OP_reg4":

(c1) Extending the readelf<->gdb shared code to print register names (4->rsi)
     may not be acceptable for arch-independent readelf.

(c2) "in register SOMENAME" is more user friendly than "DW_OP_reg42".
     But too complex expression cannot be printed in a user friendly way.

(c3) Is it OK to break backward compatibility by printing "DW_OP_reg*" (either
     numerically or with register-names)?


Regression tested on {x86_64,x86_64-m32}-fedora11-unknown-linux-gnu.


Regards,
Jan


gdb/
2009-09-20  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix PR 8399.
	* dwarf2loc.c (traverse_location_expression_func_t): New.
	(find_location_expression): Move its traversal code to ...
	(traverse_location_expression): ... a new function.
	(struct find_location_expression, find_location_expression_func): New.
	(locexpr_describe_location): Move its printing code to ...
	(describe_dlbaton): ... a new function.  Extend it for DW_OP_fbreg and
	DW_OP_breg*.  Drop its returned int type.
	(struct loclist_describe_location, loclist_describe_location_func): New.
	(loclist_describe_location): Implement location list printing.

--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -41,17 +41,27 @@
 #include "gdb_string.h"
 #include "gdb_assert.h"
 
-/* A helper function for dealing with location lists.  Given a
-   symbol baton (BATON) and a pc value (PC), find the appropriate
-   location expression, set *LOCEXPR_LENGTH, and return a pointer
-   to the beginning of the expression.  Returns NULL on failure.
-
-   For now, only return the first matching location expression; there
-   can be more than one in the list.  */
+/* Callback function type for traverse_location_expression.  It gets called for
+   the range LOW (inclusive) to HIGH (exclusive) being covered by the DWARF
+   block at BASE with DWARF block length LENGTH.  FUNC_DATA is an arbitrary
+   pointer passed from the caller.  Callback function returns non-zero if the
+   traversal should be terminated.  */
+
+typedef int (*traverse_location_expression_func_t) (CORE_ADDR low,
+						    CORE_ADDR high,
+						    gdb_byte *base,
+						    size_t length,
+						    void *func_data);
+
+/* Given a symbol baton (BATON), traverse al the available location expressions
+   and call FUNC with FUNC_DATA arbitrary pointer for each
+   such location expression.  Return non-zero if any of the FUNC calls returned
+   non-zero.  */
 
-static gdb_byte *
-find_location_expression (struct dwarf2_loclist_baton *baton,
-			  size_t *locexpr_length, CORE_ADDR pc)
+static int
+traverse_location_expression (struct dwarf2_loclist_baton *baton,
+			      traverse_location_expression_func_t func,
+			      void *func_data)
 {
   CORE_ADDR low, high;
   gdb_byte *loc_ptr, *buf_end;
@@ -91,7 +101,7 @@ find_location_expression (struct dwarf2_loclist_baton *baton,
 
       /* An end-of-list entry.  */
       if (low == 0 && high == 0)
-	return NULL;
+	return 0;
 
       /* Otherwise, a location expression entry.  */
       low += base_address;
@@ -100,14 +110,66 @@ find_location_expression (struct dwarf2_loclist_baton *baton,
       length = extract_unsigned_integer (loc_ptr, 2, byte_order);
       loc_ptr += 2;
 
-      if (pc >= low && pc < high)
-	{
-	  *locexpr_length = length;
-	  return loc_ptr;
-	}
+      if (func (low, high, loc_ptr, length, func_data))
+	return 1;
 
       loc_ptr += length;
     }
+  /* NOTREACHED */
+}
+
+/* Storage for data from the caller for find_location_expression_func.  */
+
+struct find_location_expression
+  {
+    CORE_ADDR pc;
+    gdb_byte *retval;
+    size_t locexpr_length;
+  };
+
+/* Helper callback for find_location_expression.  */
+
+static int
+find_location_expression_func (CORE_ADDR low, CORE_ADDR high, gdb_byte *base,
+			       size_t length, void *func_data)
+{
+  struct find_location_expression *data = func_data;
+
+  if (data->pc >= low && data->pc < high)
+    {
+      data->retval = base;
+      data->locexpr_length = length;
+
+      /* Terminate the traversal.  */
+      return 1;
+    }
+
+  /* Continue the traversal.  */
+  return 0;
+}
+
+/* A helper function for dealing with location lists.  Given a
+   symbol baton (BATON) and a pc value (PC), find the appropriate
+   location expression, set *LOCEXPR_LENGTH, and return a pointer
+   to the beginning of the expression.  Returns NULL on failure.
+
+   For now, only return the first matching location expression; there
+   can be more than one in the list.  */
+
+static gdb_byte *
+find_location_expression (struct dwarf2_loclist_baton *baton,
+			  size_t *locexpr_length, CORE_ADDR pc)
+{
+  struct find_location_expression data;
+
+  data.pc = pc;
+
+  if (!traverse_location_expression (baton, find_location_expression_func,
+                                     &data))
+    return NULL;
+
+  *locexpr_length = data.locexpr_length;
+  return data.retval;
 }
 
 /* This is the baton used when performing dwarf2 expression
@@ -682,12 +744,11 @@ locexpr_read_needs_frame (struct symbol *symbol)
 				      dlbaton->per_cu);
 }
 
-/* Print a natural-language description of SYMBOL to STREAM.  */
-static int
-locexpr_describe_location (struct symbol *symbol, struct ui_file *stream)
+/* Print a natural-language description of DLBATON to STREAM.  */
+
+static void
+describe_dlbaton (struct dwarf2_locexpr_baton *dlbaton, struct ui_file *stream)
 {
-  /* FIXME: be more extensive.  */
-  struct dwarf2_locexpr_baton *dlbaton = SYMBOL_LOCATION_BATON (symbol);
   int addr_size = dwarf2_per_cu_addr_size (dlbaton->per_cu);
 
   if (dlbaton->size == 1
@@ -701,7 +762,46 @@ locexpr_describe_location (struct symbol *symbol, struct ui_file *stream)
       fprintf_filtered (stream,
 			"a variable in register %s",
 			gdbarch_register_name (gdbarch, regno));
-      return 1;
+      return;
+    }
+
+  if (dlbaton->size > 1 && dlbaton->data[0] == DW_OP_fbreg)
+    {
+      LONGEST frame_offset;
+      gdb_byte *buf_end;
+
+      buf_end = read_sleb128 (&dlbaton->data[1], &dlbaton->data[dlbaton->size],
+			      &frame_offset);
+      if (buf_end == &dlbaton->data[dlbaton->size])
+	{
+	  fprintf_filtered (stream, _("stored relative to frame base "
+				      "at offset %s"),
+			    plongest (frame_offset));
+	  return;
+	}
+    }
+
+  if (dlbaton->size > 1
+      && (dlbaton->data[0] >= DW_OP_breg0 && dlbaton->data[0] <= DW_OP_breg31))
+    {
+      LONGEST frame_offset;
+      gdb_byte *buf_end;
+
+      buf_end = read_sleb128 (&dlbaton->data[1], &dlbaton->data[dlbaton->size],
+			      &frame_offset);
+      if (buf_end == &dlbaton->data[dlbaton->size])
+	{
+	  struct objfile *objfile = dwarf2_per_cu_objfile (dlbaton->per_cu);
+	  struct gdbarch *gdbarch = get_objfile_arch (objfile);
+	  int regno = gdbarch_dwarf2_reg_to_regnum (gdbarch,
+					    dlbaton->data[0] - DW_OP_breg0);
+
+	  fprintf_filtered (stream, _("stored relative to register %s "
+				      "at offset %s"),
+			    gdbarch_register_name (gdbarch, regno),
+			    plongest (frame_offset));
+	  return;
+	}
     }
 
   /* The location expression for a TLS variable looks like this (on a
@@ -730,15 +830,25 @@ locexpr_describe_location (struct symbol *symbol, struct ui_file *stream)
 			  "a thread-local variable at offset %s in the "
 			  "thread-local storage for `%s'",
 			  paddress (gdbarch, offset), objfile->name);
-	return 1;
+	return;
       }
   
 
   fprintf_filtered (stream,
 		    "a variable with complex or multiple locations (DWARF2)");
-  return 1;
 }
 
+/* Print a natural-language description of SYMBOL to STREAM.  */
+
+static int
+locexpr_describe_location (struct symbol *symbol, struct ui_file *stream)
+{
+  struct dwarf2_locexpr_baton *dlbaton = SYMBOL_LOCATION_BATON (symbol);
+
+  describe_dlbaton (dlbaton, stream);
+
+  return 1;
+}
 
 /* Describe the location of SYMBOL as an agent value in VALUE, generating
    any necessary bytecode in AX.
@@ -810,12 +920,58 @@ loclist_read_needs_frame (struct symbol *symbol)
   return 1;
 }
 
+/* Storage for data from the caller for loclist_describe_location_func.  */
+
+struct loclist_describe_location
+  {
+    struct ui_file *stream;
+    struct dwarf2_per_cu_data *per_cu;
+  };
+
+/* Helper callback for loclist_describe_location.  Print location list item as
+   a separate DWARF block text wrapping it by its validity address range.  */
+
+static int
+loclist_describe_location_func (CORE_ADDR low, CORE_ADDR high, gdb_byte *base,
+				size_t length, void *func_data)
+{
+  struct loclist_describe_location *data = func_data;
+  struct objfile *objfile = dwarf2_per_cu_objfile (data->per_cu);
+  struct gdbarch *arch = get_objfile_arch (objfile);
+  struct dwarf2_locexpr_baton dlbaton;
+
+  fprintf_filtered (data->stream, "%s - %s: ", paddress (arch, low),
+		    paddress (arch, high));
+
+  dlbaton.data = base;
+  dlbaton.size = length;
+  dlbaton.per_cu = data->per_cu;
+  describe_dlbaton (&dlbaton, data->stream);
+
+  fputc_filtered ('\n', data->stream);
+
+  /* Continue the traversal.  */
+  return 0;
+}
+
 /* Print a natural-language description of SYMBOL to STREAM.  */
+
 static int
 loclist_describe_location (struct symbol *symbol, struct ui_file *stream)
 {
-  /* FIXME: Could print the entire list of locations.  */
-  fprintf_filtered (stream, "a variable with multiple locations");
+  struct loclist_describe_location data;
+  struct dwarf2_loclist_baton *dlbaton = SYMBOL_LOCATION_BATON (symbol);
+
+  fprintf_filtered (stream, _("a variable with multiple locations, "
+			      "for PC inclusive - exclusive:\n"));
+
+  data.stream = stream;
+  data.per_cu = dlbaton->per_cu;
+
+  traverse_location_expression (dlbaton, loclist_describe_location_func, &data);
+
+  fprintf_filtered (stream, _("and optimized out in ranges not covered above"));
+
   return 1;
 }
 

Re: [rfc] Decode "is a variable with complex or multiple locations" (PR 8399)

[Eli Zaretskii]

> Date: Sun, 20 Sep 2009 22:51:09 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: Roland McGrath <roland@redhat.com>
> 
> the attached patch now prints:
> 
> (gdb) info address val
> Symbol "val" is a variable with multiple locations, for PC inclusive - exclusive:
> 0x5157c9 - 0x5157e9: stored relative to frame base at offset -20 
> 0x5157e9 - 0x5157f2: stored relative to register rsp at offset 60
> 0x5157f2 - 0x515835: stored relative to frame base at offset -20 
> and optimized out in ranges not covered above.

This is a very useful feature, thanks.

> (a) While it does what `info address' should do I think you (+me) wished more
>     just the single line valid for the current PC.
> 
>     This would do `info scope *$pc' but that one prints _all_ the visible
>     variables with no way how to request just one variable.

We could have "info scope *$pc VAR" that does it for a single
variable.

>     As the attached patch mostly just reimplements already available readelf

Does it mean this patch will only work with ELF-based executables, not
with any executable that uses DWARF-2?

>     one should probably change `info address' to print it only for the current
>     PC address.

Yes, I agree.

Re: [rfc] Decode "is a variable with complex or multiple locations" (PR 8399)

[Roland McGrath]

> (gdb) info address val
> Symbol "val" is a variable with multiple locations, for PC inclusive - exclusive:
> 0x5157c9 - 0x5157e9: stored relative to frame base at offset -20 
> 0x5157e9 - 0x5157f2: stored relative to register rsp at offset 60
> 0x5157f2 - 0x515835: stored relative to frame base at offset -20 
> and optimized out in ranges not covered above.

My hero!

> (a) While it does what `info address' should do I think you (+me) wished more
>     just the single line valid for the current PC.

Actually I think "info address" as it stands probably ought to print just
the one for the current PC, that seems most consistent with what it does
when it works.  But there should be easy ways to get both the current
location and all the locations for that name in the current scope, and I
don't really care what the command syntax is for either of them.

>     This would do `info scope *$pc' but that one prints _all_ the visible
>     variables with no way how to request just one variable.

To me it seems most usefully concise for "info scope *PC" to give only the
locations that actually apply at PC, most of the time.  (It would be dwimmy
to have "info scope foo.c:23" or "info scope func" to make the constraint
be all PCs that the expression implies as given, but I don't imagine that
is what the natural division of labor makes straightforward to do.)  As
with info addr, clearly one wants both options available somehow.

Perhaps:
	info addr sym		<-- as above, all PCs for sym in current scope
	info addr sym *0x1234	<-- sym (resolved as per "scope *0x1234")
	     	      		    location for 0x1234 only 
	info scope *0x1234	<-- full details about locals in "scope *0x1234"
	info scope *0x1234 *0x1234 <-- those locals' locations at PC=0x1234 only

>     One IMO expects info on `struct value' (with its lvalue address incl.
>     registers) than on `struct dwarf2_locexpr_baton' (DWARF block content).

I don't follow this part.

> (b) It prints it in a human readable format so more complex expressions are still
>     not decoded.

That wins for a huge majority of cases, I'm sure.  The plain expression
dump (as by readelf) would make me more than happy enough (though giving
appropriate $names to the DWARF register operations would be spiffy), but
I'd be satisfied enough for now with just a symfile name + hex DIE offset
to tell me what DWARF to go decode myself. 

> (c) "a variable in register rsi" vs. "DW_OP_reg4":

If you are printing things in DWARF-speak it is not really a big deal one
way or the other to have register names vs numbers, just a nicety.  The
person reading this sort of output is already expected to grok DWARF magic.
Whenever the whole story is explainable in human terms instead of
DWARF-speak, that is clearly the better default feature.  (Of course
someone like me would always like more options to tell me precise DWARF
bits, but I'm funny that way.)


Thanks,
Roland

Re: [rfc] Decode "is a variable with complex or multiple locations" (PR 8399)

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> (b) It prints it in a human readable format so more complex
Jan> expressions are still not decoded.
Jan>     GDB should probably reuse
Jan>     binutils/dwarf.c:decode_location_expression().
Jan> (b1) http://sources.redhat.com/ml/gdb/2003-07/msg00242.html
Jan> discussion had no conclusion where the code should be placed
Jan> (probably bfd/file-no-in-libbfd.a).

Ask on the binutils list.  I'm sure we can arrive at a place to store
it.  We can always make a new library in bfd/, or anywhere.

Jan> (c) "a variable in register rsi" vs. "DW_OP_reg4":

Jan> (c1) Extending the readelf<->gdb shared code to print register
Jan> names (4->rsi) may not be acceptable for arch-independent readelf.

We can add a callback that GDB would use to supply the register name.

Jan> (c2) "in register SOMENAME" is more user friendly than "DW_OP_reg42".
Jan>      But too complex expression cannot be printed in a user friendly way.

I think it is fine to show it in "assembly-like" form using the DW_OP
names, because it is better to expose things for users who do understand
them than to hide them from users who don't.  If we get too many
complaints, we can add an option.q

Jan> (c3) Is it OK to break backward compatibility by printing
Jan> "DW_OP_reg*" (either numerically or with register-names)?

IMO, yes.

Tom

Re: [rfc] Decode "is a variable with complex or multiple locations" (PR 8399)

[Daniel Jacobowitz]

On Fri, Sep 25, 2009 at 03:53:01PM -0600, Tom Tromey wrote:
> Jan> (c) "a variable in register rsi" vs. "DW_OP_reg4":
> 
> Jan> (c1) Extending the readelf<->gdb shared code to print register
> Jan> names (4->rsi) may not be acceptable for arch-independent readelf.
> 
> We can add a callback that GDB would use to supply the register name.

I've always wanted readelf to print these names, anyway.

-- 
Daniel Jacobowitz
CodeSourcery