[RFC] Using values to handle unwinding

[RFC] Using values to handle unwinding

[Daniel Jacobowitz]

This is the patch I referenced yesterday on gdb-patches; the
background is here:
  http://sourceware.org/ml/gdb-patches/2007-10/msg00432.html

I've replaced prev_register with a new method, with this signature:

typedef struct value * (frame_prev_register_value_ftype)
  (struct frame_info *this_frame, void **this_prologue_cache,
   int prev_regnum);

It returns a GDB value object describing the register value.  It may
be a lazy reference to memory, a lazy reference to the value of a
register in THIS frame, or a non-lvalue (constant or address).  The
first part of the patch adds support for lazy registers, which are not
fetched until they are needed.  When any frame other than the sentinel
frame sees that a register in the PREV frame is not saved or constant,
it returns a lazy reference to the register in THIS frame.

There are three key differences / motivations:

  - Uses GDB's value infrastructure.  Values carry around the
    location information which previously took four extra arguments
    to every unwinder.  And this lets us use our existing value
    manipulation routines as necessary.

  - Uses the same frame that non-unwinder architecture methods use.
    For instance, in my ARM CPSR unwinding patch I had a routine
    "arm_frame_is_thumb".  It was used during software single
    stepping, where we wanted the state of the current frame, and
    during unwinding, where we wanted the state of THIS frame
    (i.e. the one being unwound / having its ID established).

  - Simplifies getting other registers from the PREV frame.
    For instance, if one register is computed based on the
    value of another.  Previously you had to inline code
    to do this or call the same prev_register routine again
    with the same NEXT frame and THIS cache; awkward if more
    than one function was involved (see previous reason).

The patch at the bottom of my message implements this.  It's complete
and tested on x86_64-linux as far as it goes.  It applies to
HEAD plus my gdbarch_convert_register_p patch from this morning.

This patch leaves the existing prev_register method.  If we want to
adopt this new approach then I would recommend not doing that at all:
just change the type of the prev_register field, break all targets
until they are updated, and fix them.  We can do it on a branch and
merge the whole branch at once, or do it quickly in HEAD.  Converting
the DWARF-2 reader took me only an hour or two, which included writing
the helper routines.

I'm happy with the more concise code in the dwarf2 unwinder now; I
think this will overall shrink and simplify GDB.  And having THIS
frame removed the need for a separate this_id function in the dwarf2
signal frame unwinder.

All comments appreciated!  Really.  I have spent a lot of time
thinking about this, both now and in previous years, and I still think
it's a good idea.  But if it isn't I don't want to convert the other
67 unwinders and find out later.

-- 
Daniel Jacobowitz
CodeSourcery

Index: src/gdb/ada-lang.c
===================================================================
--- src.orig/gdb/ada-lang.c	2007-10-17 09:24:11.000000000 -0400
+++ src/gdb/ada-lang.c	2007-10-17 10:03:32.000000000 -0400
@@ -1930,7 +1930,7 @@ ada_value_primitive_packed_val (struct v
       v = allocate_value (type);
       bytes = (unsigned char *) (valaddr + offset);
     }
-  else if (value_lazy (obj))
+  else if (VALUE_LVAL (v) == lval_memory && value_lazy (obj))
     {
       v = value_at (type,
                     VALUE_ADDRESS (obj) + value_offset (obj) + offset);
Index: src/gdb/findvar.c
===================================================================
--- src.orig/gdb/findvar.c	2007-10-17 09:24:11.000000000 -0400
+++ src/gdb/findvar.c	2007-10-17 11:18:48.000000000 -0400
@@ -281,6 +281,30 @@ value_of_register (int regnum, struct fr
   return reg_val;
 }
 
+/* Return a `value' with the contents of (virtual or cooked) register
+   REGNUM as found in the specified FRAME.  The register's type is
+   determined by register_type().  The value is not fetched.  */
+
+struct value *
+value_of_register_lazy (struct frame_info *frame, int regnum)
+{
+  struct gdbarch *gdbarch = get_frame_arch (frame);
+  struct value *reg_val;
+
+  gdb_assert (regnum < (gdbarch_num_regs (gdbarch)
+			+ gdbarch_num_pseudo_regs (gdbarch)));
+
+  /* We should have a valid (i.e. non-sentinel) frame.  */
+  gdb_assert (frame_id_p (get_frame_id (frame)));
+
+  reg_val = allocate_value (register_type (gdbarch, regnum));
+  VALUE_LVAL (reg_val) = lval_register;
+  VALUE_REGNUM (reg_val) = regnum;
+  VALUE_FRAME_ID (reg_val) = get_frame_id (frame);
+  set_value_lazy (reg_val, 1);
+  return reg_val;
+}
+
 /* Given a pointer of type TYPE in target form in BUF, return the
    address it represents.  */
 CORE_ADDR
@@ -695,7 +719,7 @@ locate_var_value (struct symbol *var, st
   if (lazy_value == 0)
     error (_("Address of \"%s\" is unknown."), SYMBOL_PRINT_NAME (var));
 
-  if (value_lazy (lazy_value)
+  if ((VALUE_LVAL (lazy_value) == lval_memory && value_lazy (lazy_value))
       || TYPE_CODE (type) == TYPE_CODE_FUNC)
     {
       struct value *val;
Index: src/gdb/valarith.c
===================================================================
--- src.orig/gdb/valarith.c	2007-10-17 09:24:11.000000000 -0400
+++ src/gdb/valarith.c	2007-10-17 10:03:32.000000000 -0400
@@ -266,7 +266,7 @@ value_subscripted_rvalue (struct value *
     error (_("no such vector element"));
 
   v = allocate_value (elt_type);
-  if (value_lazy (array))
+  if (VALUE_LVAL (array) == lval_memory && value_lazy (array))
     set_value_lazy (v, 1);
   else
     memcpy (value_contents_writeable (v),
Index: src/gdb/valops.c
===================================================================
--- src.orig/gdb/valops.c	2007-10-17 09:24:11.000000000 -0400
+++ src/gdb/valops.c	2007-10-17 10:49:23.000000000 -0400
@@ -519,12 +519,102 @@ value_at_lazy (struct type *type, CORE_A
 int
 value_fetch_lazy (struct value *val)
 {
-  CORE_ADDR addr = VALUE_ADDRESS (val) + value_offset (val);
-  int length = TYPE_LENGTH (value_enclosing_type (val));
+  if (VALUE_LVAL (val) == lval_memory)
+    {
+      CORE_ADDR addr = VALUE_ADDRESS (val) + value_offset (val);
+      int length = TYPE_LENGTH (value_enclosing_type (val));
+
+      struct type *type = value_type (val);
+      if (length)
+	read_memory (addr, value_contents_all_raw (val), length);
+    }
+  else if (VALUE_LVAL (val) == lval_register)
+    {
+      struct frame_info *frame;
+      int regnum;
+      struct type *type = check_typedef (value_type (val));
+      struct value *new_val = val, *mark = value_mark ();
+
+      /* Offsets are not supported here; lazy register values must
+	 refer to the entire register.  */
+      gdb_assert (value_offset (val) == 0);
+
+      while (VALUE_LVAL (new_val) == lval_register
+	     && value_lazy (new_val))
+	{
+	  frame = frame_find_by_id (VALUE_FRAME_ID (new_val));
+	  regnum = VALUE_REGNUM (new_val);
+
+	  gdb_assert (frame != NULL);
+
+	  /* Convertible register routines are used for multi-register
+	     values and for interpretation in different types
+	     (e.g. float or int from a double register).  Lazy
+	     register values should have the register's natural type,
+	     so they do not apply.  */
+	  gdb_assert (!gdbarch_convert_register_p (get_frame_arch (frame),
+						   regnum, type));
+
+	  new_val = get_frame_register_value (frame, regnum);
+	}
+
+      /* If it's still lazy (for instance, a saved register on the
+	 stack), fetch it.  */
+      if (value_lazy (new_val))
+	value_fetch_lazy (new_val);
+
+      /* If the register was not saved, mark it unavailable.  */
+      if (value_optimized_out (new_val))
+	set_value_optimized_out (val, 1);
+      else
+	memcpy (value_contents_raw (val), value_contents (new_val),
+		TYPE_LENGTH (type));
 
-  struct type *type = value_type (val);
-  if (length)
-    read_memory (addr, value_contents_all_raw (val), length);
+      if (frame_debug)
+	{
+	  frame = frame_find_by_id (VALUE_FRAME_ID (val));
+	  regnum = VALUE_REGNUM (val);
+
+	  fprintf_unfiltered (gdb_stdlog, "\
+{ value_fetch_lazy (frame=%d,regnum=%d(%s),...) ",
+			      frame_relative_level (frame), regnum,
+			      frame_map_regnum_to_name (frame, regnum));
+
+	  fprintf_unfiltered (gdb_stdlog, "->");
+	  if (value_optimized_out (new_val))
+	    fprintf_unfiltered (gdb_stdlog, " optimized out");
+	  else
+	    {
+	      int i;
+	      const gdb_byte *buf = value_contents (new_val);
+
+	      if (VALUE_LVAL (new_val) == lval_register)
+		fprintf_unfiltered (gdb_stdlog, " register=%d",
+				    VALUE_REGNUM (new_val));
+	      else if (VALUE_LVAL (new_val) == lval_memory)
+		fprintf_unfiltered (gdb_stdlog, " address=0x%s",
+				    paddr_nz (VALUE_ADDRESS (new_val)));
+	      else
+		fprintf_unfiltered (gdb_stdlog, " computed");
+
+	      fprintf_unfiltered (gdb_stdlog, " bytes=");
+	      fprintf_unfiltered (gdb_stdlog, "[");
+	      for (i = 0;
+		   i < register_size (get_frame_arch (frame), regnum);
+		   i++)
+		fprintf_unfiltered (gdb_stdlog, "%02x", buf[i]);
+	      fprintf_unfiltered (gdb_stdlog, "]");
+	    }
+
+	  fprintf_unfiltered (gdb_stdlog, " }\n");
+	}
+
+      /* Dispose of the intermediate values.  This prevents
+	 watchpoints from trying to watch the saved frame pointer.  */
+      value_free_to_mark (mark);
+    }
+  else
+    internal_error (__FILE__, __LINE__, "Unexpected lazy value type.");
 
   set_value_lazy (val, 0);
   return 0;
@@ -1319,7 +1409,7 @@ search_struct_field (char *name, struct 
 	      VALUE_ADDRESS (v2) = VALUE_ADDRESS (arg1);
 	      VALUE_FRAME_ID (v2) = VALUE_FRAME_ID (arg1);
 	      set_value_offset (v2, value_offset (arg1) + boffset);
-	      if (value_lazy (arg1))
+	      if (VALUE_LVAL (arg1) == lval_memory && value_lazy (arg1))
 		set_value_lazy (v2, 1);
 	      else
 		memcpy (value_contents_raw (v2),
@@ -2875,7 +2965,7 @@ value_slice (struct value *array, int lo
       TYPE_CODE (slice_type) = TYPE_CODE (array_type);
 
       slice = allocate_value (slice_type);
-      if (value_lazy (array))
+      if (VALUE_LVAL (array) == lval_memory && value_lazy (array))
 	set_value_lazy (slice, 1);
       else
 	memcpy (value_contents_writeable (slice),
Index: src/gdb/value.c
===================================================================
--- src.orig/gdb/value.c	2007-10-17 09:24:11.000000000 -0400
+++ src/gdb/value.c	2007-10-17 10:03:32.000000000 -0400
@@ -136,9 +136,9 @@ struct value
   short regnum;
 
   /* If zero, contents of this value are in the contents field.  If
-     nonzero, contents are in inferior memory at address in the
-     location.address field plus the offset field (and the lval field
-     should be lval_memory).
+     nonzero, contents are in inferior.  If the lval field is lval_memory,
+     the contents are in inferior memory at location.address plus offset.
+     The lval field may also be lval_register.
 
      WARNING: This field is used by the code which handles watchpoints
      (see breakpoint.c) to decide whether a particular value can be
@@ -1344,7 +1344,7 @@ value_primitive_field (struct value *arg
          bases, etc.  */
       v = allocate_value (value_enclosing_type (arg1));
       v->type = type;
-      if (value_lazy (arg1))
+      if (VALUE_LVAL (arg1) == lval_memory && value_lazy (arg1))
 	set_value_lazy (v, 1);
       else
 	memcpy (value_contents_all_raw (v), value_contents_all_raw (arg1),
@@ -1358,7 +1358,7 @@ value_primitive_field (struct value *arg
       /* Plain old data member */
       offset += TYPE_FIELD_BITPOS (arg_type, fieldno) / 8;
       v = allocate_value (type);
-      if (value_lazy (arg1))
+      if (VALUE_LVAL (arg1) == lval_memory && value_lazy (arg1))
 	set_value_lazy (v, 1);
       else
 	memcpy (value_contents_raw (v),
Index: src/gdb/value.h
===================================================================
--- src.orig/gdb/value.h	2007-10-17 09:24:11.000000000 -0400
+++ src/gdb/value.h	2007-10-17 11:18:48.000000000 -0400
@@ -135,9 +135,9 @@ extern int value_embedded_offset (struct
 extern void set_value_embedded_offset (struct value *value, int val);
 
 /* If zero, contents of this value are in the contents field.  If
-   nonzero, contents are in inferior memory at address in the
-   location.address field plus the offset field (and the lval field
-   should be lval_memory).
+   nonzero, contents are in inferior.  If the lval field is lval_memory,
+   the contents are in inferior memory at location.address plus offset.
+   The lval field may also be lval_register.
 
    WARNING: This field is used by the code which handles watchpoints
    (see breakpoint.c) to decide whether a particular value can be
@@ -299,6 +299,8 @@ extern struct value *value_of_variable (
 
 extern struct value *value_of_register (int regnum, struct frame_info *frame);
 
+struct value *value_of_register_lazy (struct frame_info *frame, int regnum);
+
 extern int symbol_read_needs_frame (struct symbol *);
 
 extern struct value *read_var_value (struct symbol *var,
Index: src/gdb/frame.c
===================================================================
--- src.orig/gdb/frame.c	2007-10-17 10:11:26.000000000 -0400
+++ src/gdb/frame.c	2007-10-17 10:53:39.000000000 -0400
@@ -113,7 +113,7 @@ struct frame_info
 
 /* Flag to control debugging.  */
 
-static int frame_debug;
+int frame_debug;
 static void
 show_frame_debug (struct ui_file *file, int from_tty,
 		  struct cmd_list_element *c, const char *value)
@@ -258,7 +258,10 @@ get_frame_id (struct frame_info *fi)
 	fi->unwind = frame_unwind_find_by_frame (fi->next,
 						 &fi->prologue_cache);
       /* Find THIS frame's ID.  */
-      fi->unwind->this_id (fi->next, &fi->prologue_cache, &fi->this_id.value);
+      if (fi->unwind->prev_register_value != NULL)
+	fi->unwind->this_id (fi, &fi->prologue_cache, &fi->this_id.value);
+      else
+	fi->unwind->this_id (fi->next, &fi->prologue_cache, &fi->this_id.value);
       fi->this_id.p = 1;
       if (frame_debug)
 	{
@@ -427,15 +430,7 @@ frame_pc_unwind (struct frame_info *this
   if (!this_frame->prev_pc.p)
     {
       CORE_ADDR pc;
-      if (this_frame->unwind == NULL)
-	this_frame->unwind
-	  = frame_unwind_find_by_frame (this_frame->next,
-					&this_frame->prologue_cache);
-      if (this_frame->unwind->prev_pc != NULL)
-	/* A per-frame unwinder, prefer it.  */
-	pc = this_frame->unwind->prev_pc (this_frame->next,
-					  &this_frame->prologue_cache);
-      else if (gdbarch_unwind_pc_p (get_frame_arch (this_frame)))
+      if (gdbarch_unwind_pc_p (get_frame_arch (this_frame)))
 	{
 	  /* The right way.  The `pure' way.  The one true way.  This
 	     method depends solely on the register-unwind code to
@@ -552,15 +547,7 @@ frame_register_unwind (struct frame_info
 		       int *optimizedp, enum lval_type *lvalp,
 		       CORE_ADDR *addrp, int *realnump, gdb_byte *bufferp)
 {
-  struct frame_unwind_cache *cache;
-
-  if (frame_debug)
-    {
-      fprintf_unfiltered (gdb_stdlog, "\
-{ frame_register_unwind (frame=%d,regnum=%d(%s),...) ",
-			  frame->level, regnum,
-			  frame_map_regnum_to_name (frame, regnum));
-    }
+  struct value *value;
 
   /* Require all but BUFFERP to be valid.  A NULL BUFFERP indicates
      that the value proper does not need to be fetched.  */
@@ -570,43 +557,23 @@ frame_register_unwind (struct frame_info
   gdb_assert (realnump != NULL);
   /* gdb_assert (bufferp != NULL); */
 
-  /* NOTE: cagney/2002-11-27: A program trying to unwind a NULL frame
-     is broken.  There is always a frame.  If there, for some reason,
-     isn't a frame, there is some pretty busted code as it should have
-     detected the problem before calling here.  */
-  gdb_assert (frame != NULL);
-
-  /* Find the unwinder.  */
-  if (frame->unwind == NULL)
-    frame->unwind = frame_unwind_find_by_frame (frame->next,
-						&frame->prologue_cache);
+  value = frame_unwind_register_value (frame, regnum);
 
-  /* Ask this frame to unwind its register.  See comment in
-     "frame-unwind.h" for why NEXT frame and this unwind cache are
-     passed in.  */
-  frame->unwind->prev_register (frame->next, &frame->prologue_cache, regnum,
-				optimizedp, lvalp, addrp, realnump, bufferp);
+  gdb_assert (value != NULL);
 
-  if (frame_debug)
-    {
-      fprintf_unfiltered (gdb_stdlog, "->");
-      fprintf_unfiltered (gdb_stdlog, " *optimizedp=%d", (*optimizedp));
-      fprintf_unfiltered (gdb_stdlog, " *lvalp=%d", (int) (*lvalp));
-      fprintf_unfiltered (gdb_stdlog, " *addrp=0x%s", paddr_nz ((*addrp)));
-      fprintf_unfiltered (gdb_stdlog, " *bufferp=");
-      if (bufferp == NULL)
-	fprintf_unfiltered (gdb_stdlog, "<NULL>");
-      else
-	{
-	  int i;
-	  const unsigned char *buf = bufferp;
-	  fprintf_unfiltered (gdb_stdlog, "[");
-	  for (i = 0; i < register_size (get_frame_arch (frame), regnum); i++)
-	    fprintf_unfiltered (gdb_stdlog, "%02x", buf[i]);
-	  fprintf_unfiltered (gdb_stdlog, "]");
-	}
-      fprintf_unfiltered (gdb_stdlog, " }\n");
-    }
+  *optimizedp = value_optimized_out (value);
+  *lvalp = VALUE_LVAL (value);
+  *addrp = VALUE_ADDRESS (value);
+  *realnump = VALUE_REGNUM (value);
+
+  if (bufferp)
+    memcpy (bufferp, value_contents_all (value),
+	    TYPE_LENGTH (value_type (value)));
+
+  /* Dispose of the new value.  This prevents watchpoints from
+     trying to watch the saved frame pointer.  */
+  release_value (value);
+  value_free (value);
 }
 
 void
@@ -647,6 +614,111 @@ get_frame_register (struct frame_info *f
   frame_unwind_register (frame->next, regnum, buf);
 }
 
+struct value *
+frame_unwind_register_value (struct frame_info *frame, int regnum)
+{
+  struct value *value;
+
+  if (frame_debug)
+    {
+      fprintf_unfiltered (gdb_stdlog, "\
+{ frame_unwind_register_value (frame=%d,regnum=%d(%s),...) ",
+			  frame->level, regnum,
+			  frame_map_regnum_to_name (frame, regnum));
+    }
+
+  gdb_assert (frame != NULL);
+
+  /* Find the unwinder.  */
+  if (frame->unwind == NULL)
+    frame->unwind = frame_unwind_find_by_frame (frame->next,
+						&frame->prologue_cache);
+
+  /* Ask this frame to unwind its register.  */
+  if (frame->unwind->prev_register_value != NULL)
+    value = frame->unwind->prev_register_value (frame, &frame->prologue_cache,
+						regnum);
+  else
+    {
+      gdb_byte buffer[MAX_REGISTER_SIZE];
+      int optimized;
+      CORE_ADDR addr;
+      int realnum;
+      enum lval_type lval;
+
+      gdb_assert (frame->unwind->prev_register != NULL);
+
+      /* We can not create a lazy value, because if the result is not_lval
+	 we have no way to fetch the bytes later.  */
+      frame->unwind->prev_register (frame->next, &frame->prologue_cache, regnum,
+				    &optimized, &lval, &addr, &realnum, buffer);
+
+      if (optimized)
+	{
+	  value = value_zero (register_type (get_frame_arch (frame), regnum),
+			      not_lval);
+	  set_value_optimized_out (value, 1);
+	}
+      else if (lval == lval_memory)
+	value = value_at_lazy (register_type (get_frame_arch (frame), regnum),
+			       addr);
+      else if (lval == lval_register)
+	value = value_of_register_lazy (frame, realnum);
+      else
+	{
+	  gdb_assert (lval == not_lval);
+
+	  value = value_zero (register_type (get_frame_arch (frame), regnum),
+			      not_lval);
+	  VALUE_LVAL (value) = not_lval;
+	  memcpy (value_contents_writeable (value), buffer,
+		  TYPE_LENGTH (value_type (value)));
+	}
+    }
+
+  if (frame_debug)
+    {
+      fprintf_unfiltered (gdb_stdlog, "->");
+      if (value_optimized_out (value))
+	fprintf_unfiltered (gdb_stdlog, " optimized out");
+      else
+	{
+	  if (VALUE_LVAL (value) == lval_register)
+	    fprintf_unfiltered (gdb_stdlog, " register=%d",
+				VALUE_REGNUM (value));
+	  else if (VALUE_LVAL (value) == lval_memory)
+	    fprintf_unfiltered (gdb_stdlog, " address=0x%s",
+				paddr_nz (VALUE_ADDRESS (value)));
+	  else
+	    fprintf_unfiltered (gdb_stdlog, " computed");
+
+	  if (value_lazy (value))
+	    fprintf_unfiltered (gdb_stdlog, " lazy");
+	  else
+	    {
+	      int i;
+	      const gdb_byte *buf = value_contents (value);
+
+	      fprintf_unfiltered (gdb_stdlog, " bytes=");
+	      fprintf_unfiltered (gdb_stdlog, "[");
+	      for (i = 0; i < register_size (get_frame_arch (frame), regnum); i++)
+		fprintf_unfiltered (gdb_stdlog, "%02x", buf[i]);
+	      fprintf_unfiltered (gdb_stdlog, "]");
+	    }
+	}
+
+      fprintf_unfiltered (gdb_stdlog, " }\n");
+    }
+
+  return value;
+}
+
+struct value *
+get_frame_register_value (struct frame_info *frame, int regnum)
+{
+  return frame_unwind_register_value (frame->next, regnum);
+}
+
 LONGEST
 frame_unwind_register_signed (struct frame_info *frame, int regnum)
 {
@@ -1566,7 +1638,12 @@ get_frame_base_address (struct frame_inf
   /* Sneaky: If the low-level unwind and high-level base code share a
      common unwinder, let them share the prologue cache.  */
   if (fi->base->unwind == fi->unwind)
-    return fi->base->this_base (fi->next, &fi->prologue_cache);
+    {
+      if (fi->unwind->prev_register_value != NULL)
+	return fi->base->this_base (fi, &fi->prologue_cache);
+      else
+	return fi->base->this_base (fi->next, &fi->prologue_cache);
+    }
   return fi->base->this_base (fi->next, &fi->base_cache);
 }
 
@@ -1582,10 +1659,13 @@ get_frame_locals_address (struct frame_i
   /* Sneaky: If the low-level unwind and high-level base code share a
      common unwinder, let them share the prologue cache.  */
   if (fi->base->unwind == fi->unwind)
-    cache = &fi->prologue_cache;
-  else
-    cache = &fi->base_cache;
-  return fi->base->this_locals (fi->next, cache);
+    {
+      if (fi->unwind->prev_register_value != NULL)
+	return fi->base->this_locals (fi, &fi->prologue_cache);
+      else
+	return fi->base->this_locals (fi->next, &fi->prologue_cache);
+    }
+  return fi->base->this_locals (fi->next, &fi->base_cache);
 }
 
 CORE_ADDR
@@ -1600,10 +1680,13 @@ get_frame_args_address (struct frame_inf
   /* Sneaky: If the low-level unwind and high-level base code share a
      common unwinder, let them share the prologue cache.  */
   if (fi->base->unwind == fi->unwind)
-    cache = &fi->prologue_cache;
-  else
-    cache = &fi->base_cache;
-  return fi->base->this_args (fi->next, cache);
+    {
+      if (fi->unwind->prev_register_value != NULL)
+	return fi->base->this_args (fi, &fi->prologue_cache);
+      else
+	return fi->base->this_args (fi->next, &fi->prologue_cache);
+    }
+  return fi->base->this_args (fi->next, &fi->base_cache);
 }
 
 /* Level of the selected frame: 0 for innermost, 1 for its caller, ...
Index: src/gdb/frame.h
===================================================================
--- src.orig/gdb/frame.h	2007-10-17 10:11:26.000000000 -0400
+++ src/gdb/frame.h	2007-10-17 10:43:15.000000000 -0400
@@ -144,6 +144,10 @@ struct frame_id
 /* For convenience.  All fields are zero.  */
 extern const struct frame_id null_frame_id;
 
+/* Flag to control debugging.  */
+
+extern int frame_debug;
+
 /* Construct a frame ID.  The first parameter is the frame's constant
    stack address (typically the outer-bound), and the second the
    frame's constant code address (typically the entry point).
@@ -459,13 +463,19 @@ extern void frame_register_unwind (struc
 /* Fetch a register from this, or unwind a register from the next
    frame.  Note that the get_frame methods are wrappers to
    frame->next->unwind.  They all [potentially] throw an error if the
-   fetch fails.  */
+   fetch fails.  The value methods never return NULL, but usually
+   do return a lazy value.  */
 
 extern void frame_unwind_register (struct frame_info *frame,
 				   int regnum, gdb_byte *buf);
 extern void get_frame_register (struct frame_info *frame,
 				int regnum, gdb_byte *buf);
 
+struct value *frame_unwind_register_value (struct frame_info *frame,
+					   int regnum);
+struct value *get_frame_register_value (struct frame_info *frame,
+					int regnum);
+
 extern LONGEST frame_unwind_register_signed (struct frame_info *frame,
 					     int regnum);
 extern LONGEST get_frame_register_signed (struct frame_info *frame,
Index: src/gdb/frame-unwind.h
===================================================================
--- src.orig/gdb/frame-unwind.h	2007-10-17 10:11:26.000000000 -0400
+++ src/gdb/frame-unwind.h	2007-10-17 11:19:45.000000000 -0400
@@ -26,6 +26,7 @@ struct frame_id;
 struct frame_unwind;
 struct gdbarch;
 struct regcache;
+struct value;
 
 #include "frame.h"		/* For enum frame_type.  */
 
@@ -53,6 +54,10 @@ typedef int (frame_sniffer_ftype) (const
    use the NEXT frame, and its register unwind method, to determine
    the frame ID of THIS frame.
 
+   NOTE!  If this unwinder has a prev_register_value method, then
+   the frame argument will be THIS frame instead of NEXT frame!
+   FIXME: This is transitional.
+
    A frame ID provides an invariant that can be used to re-identify an
    instance of a frame.  It is a combination of the frame's `base' and
    the frame's function's code address.
@@ -76,7 +81,10 @@ typedef void (frame_this_id_ftype) (stru
 				    void **this_prologue_cache,
 				    struct frame_id *this_id);
 
-/* Assuming the frame chain: (outer) prev <-> this <-> next (inner);
+/* The use of prev_register_value is recommended instead of
+   prev_register.
+
+   Assuming the frame chain: (outer) prev <-> this <-> next (inner);
    use the NEXT frame, and its register unwind method, to unwind THIS
    frame's registers (returning the value of the specified register
    REGNUM in the previous frame).
@@ -117,11 +125,37 @@ typedef void (frame_prev_register_ftype)
 					  int *realnump, gdb_byte *valuep);
 
 /* Assuming the frame chain: (outer) prev <-> this <-> next (inner);
-   use the NEXT frame, and its register unwind method, to return the PREV
-   frame's program-counter.  */
+   use THIS frame, and implicitly the NEXT frame's register unwind
+   method, to unwind THIS frame's registers (returning the value of
+   the specified register REGNUM in the previous frame).
+
+   Traditionally, THIS frame's registers were unwound by examining
+   THIS frame's function's prologue and identifying which registers
+   that prolog code saved on the stack.
 
-typedef CORE_ADDR (frame_prev_pc_ftype) (struct frame_info *next_frame,
-					 void **this_prologue_cache);
+   Example: An examination of THIS frame's prologue reveals that, on
+   entry, it saves the PC(+12), SP(+8), and R1(+4) registers
+   (decrementing the SP by 12).  Consequently, the value of the PC
+   register in the previous frame is found in memory at SP+12, and
+   THIS frame's SP can be obtained by unwinding the NEXT frame's SP.
+
+   This version takes THIS_FRAME, unlike the older prev_register
+   interface (above).  It can find the values of registers in THIS
+   frame by calling get_frame_register (THIS_FRAME), and reinvoke
+   itself to find other registers in the PREVIOUS frame by calling
+   frame_unwind_register (THIS_FRAME).
+
+   The result is a GDB value object describing the register value.  It
+   may be a lazy reference to memory, a lazy reference to the value of
+   a register in THIS frame, or a non-lvalue.
+
+   THIS_PROLOGUE_CACHE can be used to share any prolog analysis data
+   with the other unwind methods.  Memory for that cache should be
+   allocated using FRAME_OBSTACK_ZALLOC().  */
+
+typedef struct value * (frame_prev_register_value_ftype)
+  (struct frame_info *this_frame, void **this_prologue_cache,
+   int prev_regnum);
 
 /* Deallocate extra memory associated with the frame cache if any.  */
 
@@ -139,8 +173,8 @@ struct frame_unwind
   frame_prev_register_ftype *prev_register;
   const struct frame_data *unwind_data;
   frame_sniffer_ftype *sniffer;
-  frame_prev_pc_ftype *prev_pc;
   frame_dealloc_cache_ftype *dealloc_cache;
+  frame_prev_register_value_ftype *prev_register_value;
 };
 
 /* Register a frame unwinder, _prepending_ it to the front of the
@@ -171,4 +205,32 @@ extern void frame_unwind_append_sniffer 
 extern const struct frame_unwind *frame_unwind_find_by_frame (struct frame_info *next_frame,
 							      void **this_cache);
 
+/* Helper functions for value-based register unwinding.  These return
+   a (possibly lazy) value of the appropriate type.  */
+
+/* Return a value which indicates that REGNUM from FRAME is not
+   available.  */
+
+struct value *frame_unwind_got_optimized (struct frame_info *frame,
+					  int regnum);
+
+/* Return a value which indicates that REGNUM from FRAME is saved
+   in memory at ADDR.  */
+
+struct value *frame_unwind_got_memory (struct frame_info *frame, int regnum,
+				       CORE_ADDR addr);
+
+/* Return a value which indicates that REGNUM from FRAME has a
+   known constant (computed) value of VAL.  */
+
+struct value *frame_unwind_got_constant (struct frame_info *frame, int regnum,
+					 ULONGEST val);
+
+/* Return a value which indicates that REGNUM from FRAME has a
+   known constant (computed) value of ADDR.  Convert the CORE_ADDR
+   to a target address if necessary.  */
+
+struct value *frame_unwind_got_address (struct frame_info *frame, int regnum,
+					CORE_ADDR addr);
+
 #endif
Index: src/gdb/frame-base.h
===================================================================
--- src.orig/gdb/frame-base.h	2007-10-17 10:11:26.000000000 -0400
+++ src/gdb/frame-base.h	2007-10-17 10:11:33.000000000 -0400
@@ -27,6 +27,13 @@ struct frame_base;
 struct gdbarch;
 struct regcache;
 
+/* NOTE!  If this frame base shares a cache with the prologue
+   unwinder, and that prologue unwinder has a prev_register_value
+   method, then the frame argument to all of these methods will be
+   THIS frame instead of NEXT frame!
+
+   FIXME: This is transitional.  */
+
 /* Assuming the frame chain: (outer) prev <-> this <-> next (inner);
    and that this is a `normal frame'; use the NEXT frame, and its
    register unwind method, to determine the address of THIS frame's
Index: src/gdb/sentinel-frame.c
===================================================================
--- src.orig/gdb/sentinel-frame.c	2007-10-17 10:11:26.000000000 -0400
+++ src/gdb/sentinel-frame.c	2007-10-17 11:19:31.000000000 -0400
@@ -42,34 +42,31 @@ sentinel_frame_cache (struct regcache *r
 
 /* Here the register value is taken direct from the register cache.  */
 
-static void
-sentinel_frame_prev_register (struct frame_info *next_frame,
-			      void **this_prologue_cache,
-			      int regnum, int *optimized,
-			      enum lval_type *lvalp, CORE_ADDR *addrp,
-			      int *realnum, gdb_byte *bufferp)
+static struct value *
+sentinel_frame_prev_register_value (struct frame_info *this_frame,
+				    void **this_prologue_cache,
+				    int regnum)
 {
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
   struct frame_unwind_cache *cache = *this_prologue_cache;
-  /* Describe the register's location.  A reg-frame maps all registers
-     onto the corresponding hardware register.  */
-  *optimized = 0;
-  *lvalp = lval_register;
-  *addrp = register_offset_hack (current_gdbarch, regnum);
-  *realnum = regnum;
-
-  /* If needed, find and return the value of the register.  */
-  if (bufferp != NULL)
-    {
-      /* Return the actual value.  */
-      /* Use the regcache_cooked_read() method so that it, on the fly,
-         constructs either a raw or pseudo register from the raw
-         register cache.  */
-      regcache_cooked_read (cache->regcache, regnum, bufferp);
-    }
+  struct value *value;
+
+  /* Return the actual value.  */
+  value = allocate_value (register_type (gdbarch, regnum));
+  VALUE_LVAL (value) = lval_register;
+  VALUE_REGNUM (value) = regnum;
+  VALUE_FRAME_ID (value) = get_frame_id (this_frame);
+
+  /* Use the regcache_cooked_read() method so that it, on the fly,
+     constructs either a raw or pseudo register from the raw
+     register cache.  */
+  regcache_cooked_read (cache->regcache, regnum, value_contents_raw (value));
+
+  return value;
 }
 
 static void
-sentinel_frame_this_id (struct frame_info *next_frame,
+sentinel_frame_this_id (struct frame_info *this_frame,
 			void **this_prologue_cache,
 			struct frame_id *this_id)
 {
@@ -79,22 +76,15 @@ sentinel_frame_this_id (struct frame_inf
   internal_error (__FILE__, __LINE__, _("sentinel_frame_this_id called"));
 }
 
-static CORE_ADDR
-sentinel_frame_prev_pc (struct frame_info *next_frame,
-			void **this_prologue_cache)
-{
-  struct gdbarch *gdbarch = get_frame_arch (next_frame);
-  return gdbarch_unwind_pc (gdbarch, next_frame);
-}
-
 const struct frame_unwind sentinel_frame_unwinder =
 {
   SENTINEL_FRAME,
   sentinel_frame_this_id,
-  sentinel_frame_prev_register,
+  NULL, /* prev_register */
   NULL, /* unwind_data */
   NULL, /* sniffer */
-  sentinel_frame_prev_pc,
+  NULL, /* dealloc_cache */
+  sentinel_frame_prev_register_value
 };
 
 const struct frame_unwind *const sentinel_frame_unwind = &sentinel_frame_unwinder;
Index: src/gdb/ia64-tdep.c
===================================================================
--- src.orig/gdb/ia64-tdep.c	2007-10-17 10:55:11.000000000 -0400
+++ src/gdb/ia64-tdep.c	2007-10-17 10:55:13.000000000 -0400
@@ -2794,7 +2794,6 @@ static const struct frame_unwind ia64_li
   ia64_libunwind_frame_prev_register,
   NULL,
   NULL,
-  NULL,
   libunwind_frame_dealloc_cache
 };
 
Index: src/gdb/libunwind-frame.c
===================================================================
--- src.orig/gdb/libunwind-frame.c	2007-10-17 10:54:57.000000000 -0400
+++ src/gdb/libunwind-frame.c	2007-10-17 10:55:03.000000000 -0400
@@ -213,7 +213,6 @@ static const struct frame_unwind libunwi
   libunwind_frame_prev_register,
   NULL,
   NULL,
-  NULL,
   libunwind_frame_dealloc_cache
 };
 
Index: src/gdb/dwarf2-frame.c
===================================================================
--- src.orig/gdb/dwarf2-frame.c	2007-10-17 11:18:48.000000000 -0400
+++ src/gdb/dwarf2-frame.c	2007-10-17 11:19:45.000000000 -0400
@@ -230,15 +230,15 @@ dwarf2_frame_state_free (void *p)
 static CORE_ADDR
 read_reg (void *baton, int reg)
 {
-  struct frame_info *next_frame = (struct frame_info *) baton;
-  struct gdbarch *gdbarch = get_frame_arch (next_frame);
+  struct frame_info *this_frame = (struct frame_info *) baton;
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
   int regnum;
   gdb_byte *buf;
 
   regnum = gdbarch_dwarf2_reg_to_regnum (gdbarch, reg);
 
   buf = alloca (register_size (gdbarch, regnum));
-  frame_unwind_register (next_frame, regnum, buf);
+  get_frame_register (this_frame, regnum, buf);
 
   /* Convert the register to an integer.  This returns a LONGEST
      rather than a CORE_ADDR, but unpack_pointer does the same thing
@@ -270,13 +270,13 @@ no_get_tls_address (void *baton, CORE_AD
 
 static CORE_ADDR
 execute_stack_op (gdb_byte *exp, ULONGEST len,
-		  struct frame_info *next_frame, CORE_ADDR initial)
+		  struct frame_info *this_frame, CORE_ADDR initial)
 {
   struct dwarf_expr_context *ctx;
   CORE_ADDR result;
 
   ctx = new_dwarf_expr_context ();
-  ctx->baton = next_frame;
+  ctx->baton = this_frame;
   ctx->read_reg = read_reg;
   ctx->read_mem = read_mem;
   ctx->get_frame_base = no_get_frame_base;
@@ -287,7 +287,7 @@ execute_stack_op (gdb_byte *exp, ULONGES
   result = dwarf_expr_fetch (ctx, 0);
 
   if (ctx->in_reg)
-    result = read_reg (next_frame, result);
+    result = read_reg (this_frame, result);
 
   free_dwarf_expr_context (ctx);
 
@@ -297,12 +297,12 @@ execute_stack_op (gdb_byte *exp, ULONGES
 
 static void
 execute_cfa_program (gdb_byte *insn_ptr, gdb_byte *insn_end,
-		     struct frame_info *next_frame,
+		     struct frame_info *this_frame,
 		     struct dwarf2_frame_state *fs, int eh_frame_p)
 {
-  CORE_ADDR pc = frame_pc_unwind (next_frame);
+  CORE_ADDR pc = get_frame_pc (this_frame);
   int bytes_read;
-  struct gdbarch *gdbarch = get_frame_arch (next_frame);
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
 
   while (insn_ptr < insn_end && fs->pc <= pc)
     {
@@ -549,8 +549,7 @@ bad CFI data; mismatched DW_CFA_restore_
 		 Incidentally that's what GCC does too in its
 		 unwinder.  */
 	      {
-		struct gdbarch *gdbarch = get_frame_arch (next_frame);
-		int size = register_size(gdbarch, 0);
+		int size = register_size (gdbarch, 0);
 		dwarf2_frame_state_alloc_regs (&fs->regs, 32);
 		for (reg = 8; reg < 16; reg++)
 		  {
@@ -618,7 +617,7 @@ struct dwarf2_frame_ops
 static void
 dwarf2_frame_default_init_reg (struct gdbarch *gdbarch, int regnum,
 			       struct dwarf2_frame_state_reg *reg,
-			       struct frame_info *next_frame)
+			       struct frame_info *this_frame)
 {
   /* If we have a register that acts as a program counter, mark it as
      a destination for the return address.  If we have a register that
@@ -683,11 +682,15 @@ dwarf2_frame_set_init_reg (struct gdbarc
 static void
 dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum,
 		       struct dwarf2_frame_state_reg *reg,
-		       struct frame_info *next_frame)
+		       struct frame_info *this_frame)
 {
   struct dwarf2_frame_ops *ops = gdbarch_data (gdbarch, dwarf2_frame_data);
 
-  ops->init_reg (gdbarch, regnum, reg, next_frame);
+  /* FIXME: Most definitions of the init_reg callback still take an
+     argument named next_frame instead of this_frame.  Only one,
+     in sparc-tdep.c, uses its argument; that one has been updated.
+     The others just need the name changed.  */
+  ops->init_reg (gdbarch, regnum, reg, this_frame);
 }
 
 /* Set the architecture-specific signal trampoline recognition
@@ -804,10 +807,10 @@ struct dwarf2_frame_cache
 };
 
 static struct dwarf2_frame_cache *
-dwarf2_frame_cache (struct frame_info *next_frame, void **this_cache)
+dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
 {
   struct cleanup *old_chain;
-  struct gdbarch *gdbarch = get_frame_arch (next_frame);
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
   const int num_regs = gdbarch_num_regs (gdbarch)
 		       + gdbarch_num_pseudo_regs (gdbarch);
   struct dwarf2_frame_cache *cache;
@@ -828,9 +831,9 @@ dwarf2_frame_cache (struct frame_info *n
 
   /* Unwind the PC.
 
-     Note that if NEXT_FRAME is never supposed to return (i.e. a call
+     Note that if the next frame is never supposed to return (i.e. a call
      to abort), the compiler might optimize away the instruction at
-     NEXT_FRAME's return address.  As a result the return address will
+     its return address.  As a result the return address will
      point at some random instruction, and the CFI for that
      instruction is probably worthless to us.  GCC's unwinder solves
      this problem by substracting 1 from the return address to get an
@@ -841,7 +844,7 @@ dwarf2_frame_cache (struct frame_info *n
      frame_unwind_address_in_block does just this.  It's not clear how
      reliable the method is though; there is the potential for the
      register state pre-call being different to that on return.  */
-  fs->pc = frame_unwind_address_in_block (next_frame, NORMAL_FRAME);
+  fs->pc = get_frame_address_in_block (this_frame);
 
   /* Find the correct FDE.  */
   fde = dwarf2_frame_find_fde (&fs->pc);
@@ -857,21 +860,21 @@ dwarf2_frame_cache (struct frame_info *n
 
   /* First decode all the insns in the CIE.  */
   execute_cfa_program (fde->cie->initial_instructions,
-		       fde->cie->end, next_frame, fs, fde->eh_frame_p);
+		       fde->cie->end, this_frame, fs, fde->eh_frame_p);
 
   /* Save the initialized register set.  */
   fs->initial = fs->regs;
   fs->initial.reg = dwarf2_frame_state_copy_regs (&fs->regs);
 
   /* Then decode the insns in the FDE up to our target PC.  */
-  execute_cfa_program (fde->instructions, fde->end, next_frame, fs,
+  execute_cfa_program (fde->instructions, fde->end, this_frame, fs,
 		       fde->eh_frame_p);
 
   /* Caclulate the CFA.  */
   switch (fs->cfa_how)
     {
     case CFA_REG_OFFSET:
-      cache->cfa = read_reg (next_frame, fs->cfa_reg);
+      cache->cfa = read_reg (this_frame, fs->cfa_reg);
       if (fs->armcc_cfa_offsets_reversed)
 	cache->cfa -= fs->cfa_offset;
       else
@@ -880,7 +883,7 @@ dwarf2_frame_cache (struct frame_info *n
 
     case CFA_EXP:
       cache->cfa =
-	execute_stack_op (fs->cfa_exp, fs->cfa_exp_len, next_frame, 0);
+	execute_stack_op (fs->cfa_exp, fs->cfa_exp_len, this_frame, 0);
       break;
 
     default:
@@ -892,7 +895,7 @@ dwarf2_frame_cache (struct frame_info *n
     int regnum;
 
     for (regnum = 0; regnum < num_regs; regnum++)
-      dwarf2_frame_init_reg (gdbarch, regnum, &cache->reg[regnum], next_frame);
+      dwarf2_frame_init_reg (gdbarch, regnum, &cache->reg[regnum], this_frame);
   }
 
   /* Go through the DWARF2 CFI generated table and save its register
@@ -995,118 +998,77 @@ incomplete CFI data; unspecified registe
 }
 
 static void
-dwarf2_frame_this_id (struct frame_info *next_frame, void **this_cache,
+dwarf2_frame_this_id (struct frame_info *this_frame, void **this_cache,
 		      struct frame_id *this_id)
 {
   struct dwarf2_frame_cache *cache =
-    dwarf2_frame_cache (next_frame, this_cache);
+    dwarf2_frame_cache (this_frame, this_cache);
 
   if (cache->undefined_retaddr)
     return;
 
-  (*this_id) = frame_id_build (cache->cfa,
-			       frame_func_unwind (next_frame, NORMAL_FRAME));
+  (*this_id) = frame_id_build (cache->cfa, get_frame_func (this_frame));
 }
 
-static void
-dwarf2_signal_frame_this_id (struct frame_info *next_frame, void **this_cache,
-			     struct frame_id *this_id)
-{
-  struct dwarf2_frame_cache *cache =
-    dwarf2_frame_cache (next_frame, this_cache);
-
-  if (cache->undefined_retaddr)
-    return;
-
-  (*this_id) = frame_id_build (cache->cfa,
-			       frame_func_unwind (next_frame, SIGTRAMP_FRAME));
-}
-
-static void
-dwarf2_frame_prev_register (struct frame_info *next_frame, void **this_cache,
-			    int regnum, int *optimizedp,
-			    enum lval_type *lvalp, CORE_ADDR *addrp,
-			    int *realnump, gdb_byte *valuep)
+static struct value *
+dwarf2_frame_prev_register_value (struct frame_info *this_frame,
+				  void **this_cache, int regnum)
 {
-  struct gdbarch *gdbarch = get_frame_arch (next_frame);
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
   struct dwarf2_frame_cache *cache =
-    dwarf2_frame_cache (next_frame, this_cache);
+    dwarf2_frame_cache (this_frame, this_cache);
+  CORE_ADDR addr;
+  int realnum;
 
   switch (cache->reg[regnum].how)
     {
     case DWARF2_FRAME_REG_UNDEFINED:
       /* If CFI explicitly specified that the value isn't defined,
 	 mark it as optimized away; the value isn't available.  */
-      *optimizedp = 1;
-      *lvalp = not_lval;
-      *addrp = 0;
-      *realnump = -1;
+      return frame_unwind_got_optimized (this_frame, regnum);
+#if 0
+      /* FIXME: There's no easy way to do this now so we need to figure
+	 out where it mattered.  A strategic error when attempting to
+	 access the contents of an optimized-out value should turn
+	 it up.  This patch causes no failures, so either it has
+	 been fixed or the testsuite does not catch it.
+
+         I suspect making frame_pop ignore unavailable registers
+         would do the trick.  */
       if (valuep)
 	{
 	  /* In some cases, for example %eflags on the i386, we have
 	     to provide a sane value, even though this register wasn't
-	     saved.  Assume we can get it from NEXT_FRAME.  */
-	  frame_unwind_register (next_frame, regnum, valuep);
+	     saved.  Assume we can get it from the next frame.  */
+	  get_frame_register (this_frame, regnum, valuep);
 	}
       break;
+#endif
 
     case DWARF2_FRAME_REG_SAVED_OFFSET:
-      *optimizedp = 0;
-      *lvalp = lval_memory;
-      *addrp = cache->cfa + cache->reg[regnum].loc.offset;
-      *realnump = -1;
-      if (valuep)
-	{
-	  /* Read the value in from memory.  */
-	  read_memory (*addrp, valuep, register_size (gdbarch, regnum));
-	}
-      break;
+      addr = cache->cfa + cache->reg[regnum].loc.offset;
+      return frame_unwind_got_memory (this_frame, regnum, addr);
 
     case DWARF2_FRAME_REG_SAVED_REG:
-      *optimizedp = 0;
-      *lvalp = lval_register;
-      *addrp = 0;
-      *realnump = gdbarch_dwarf2_reg_to_regnum
-		    (gdbarch, cache->reg[regnum].loc.reg);
-      if (valuep)
-	frame_unwind_register (next_frame, (*realnump), valuep);
-      break;
+      realnum
+	= gdbarch_dwarf2_reg_to_regnum (gdbarch, cache->reg[regnum].loc.reg);
+      return value_of_register_lazy (this_frame, realnum);
 
     case DWARF2_FRAME_REG_SAVED_EXP:
-      *optimizedp = 0;
-      *lvalp = lval_memory;
-      *addrp = execute_stack_op (cache->reg[regnum].loc.exp,
-				 cache->reg[regnum].exp_len,
-				 next_frame, cache->cfa);
-      *realnump = -1;
-      if (valuep)
-	{
-	  /* Read the value in from memory.  */
-	  read_memory (*addrp, valuep, register_size (gdbarch, regnum));
-	}
-      break;
+      addr = execute_stack_op (cache->reg[regnum].loc.exp,
+			       cache->reg[regnum].exp_len,
+			       this_frame, cache->cfa);
+      return frame_unwind_got_memory (this_frame, regnum, addr);
 
     case DWARF2_FRAME_REG_SAVED_VAL_OFFSET:
-      *optimizedp = 0;
-      *lvalp = not_lval;
-      *addrp = 0;
-      *realnump = -1;
-      if (valuep)
-	store_unsigned_integer (valuep, register_size (gdbarch, regnum),
-				cache->cfa + cache->reg[regnum].loc.offset);
-      break;
+      addr = cache->cfa + cache->reg[regnum].loc.offset;
+      return frame_unwind_got_constant (this_frame, regnum, addr);
 
     case DWARF2_FRAME_REG_SAVED_VAL_EXP:
-      *optimizedp = 0;
-      *lvalp = not_lval;
-      *addrp = 0;
-      *realnump = -1;
-      if (valuep)
-	store_unsigned_integer (valuep, register_size (gdbarch, regnum),
-				execute_stack_op (cache->reg[regnum].loc.exp,
-						  cache->reg[regnum].exp_len,
-						  next_frame, cache->cfa));
-      break;
+      addr = execute_stack_op (cache->reg[regnum].loc.exp,
+			       cache->reg[regnum].exp_len,
+			       this_frame, cache->cfa);
+      return frame_unwind_got_constant (this_frame, regnum, addr);
 
     case DWARF2_FRAME_REG_UNSPECIFIED:
       /* GCC, in its infinite wisdom decided to not provide unwind
@@ -1116,57 +1078,24 @@ dwarf2_frame_prev_register (struct frame
 	 "undefined").  Code above issues a complaint about this.
 	 Here just fudge the books, assume GCC, and that the value is
 	 more inner on the stack.  */
-      *optimizedp = 0;
-      *lvalp = lval_register;
-      *addrp = 0;
-      *realnump = regnum;
-      if (valuep)
-	frame_unwind_register (next_frame, (*realnump), valuep);
-      break;
+      return value_of_register_lazy (this_frame, regnum);
 
     case DWARF2_FRAME_REG_SAME_VALUE:
-      *optimizedp = 0;
-      *lvalp = lval_register;
-      *addrp = 0;
-      *realnump = regnum;
-      if (valuep)
-	frame_unwind_register (next_frame, (*realnump), valuep);
-      break;
+      return value_of_register_lazy (this_frame, regnum);
 
     case DWARF2_FRAME_REG_CFA:
-      *optimizedp = 0;
-      *lvalp = not_lval;
-      *addrp = 0;
-      *realnump = -1;
-      if (valuep)
-	pack_long (valuep, register_type (gdbarch, regnum), cache->cfa);
-      break;
+      return frame_unwind_got_address (this_frame, regnum, cache->cfa);
 
     case DWARF2_FRAME_REG_CFA_OFFSET:
-      *optimizedp = 0;
-      *lvalp = not_lval;
-      *addrp = 0;
-      *realnump = -1;
-      if (valuep)
-	pack_long (valuep, register_type (gdbarch, regnum),
-		   cache->cfa + cache->reg[regnum].loc.offset);
-      break;
+      addr = cache->cfa + cache->reg[regnum].loc.offset;
+      return frame_unwind_got_address (this_frame, regnum, addr);
 
     case DWARF2_FRAME_REG_RA_OFFSET:
-      *optimizedp = 0;
-      *lvalp = not_lval;
-      *addrp = 0;
-      *realnump = -1;
-      if (valuep)
-        {
-          CORE_ADDR pc = cache->reg[regnum].loc.offset;
-
-          regnum = gdbarch_dwarf2_reg_to_regnum
-		     (gdbarch, cache->retaddr_reg.loc.reg);
-          pc += frame_unwind_register_unsigned (next_frame, regnum);
-          pack_long (valuep, register_type (gdbarch, regnum), pc);
-        }
-      break;
+      addr = cache->reg[regnum].loc.offset;
+      regnum = gdbarch_dwarf2_reg_to_regnum
+	(gdbarch, cache->retaddr_reg.loc.reg);
+      addr += get_frame_register_unsigned (this_frame, regnum);
+      return frame_unwind_got_address (this_frame, regnum, addr);
 
     default:
       internal_error (__FILE__, __LINE__, _("Unknown register rule."));
@@ -1177,14 +1106,22 @@ static const struct frame_unwind dwarf2_
 {
   NORMAL_FRAME,
   dwarf2_frame_this_id,
-  dwarf2_frame_prev_register
+  NULL,
+  NULL,
+  NULL,
+  NULL,
+  dwarf2_frame_prev_register_value
 };
 
 static const struct frame_unwind dwarf2_signal_frame_unwind =
 {
   SIGTRAMP_FRAME,
-  dwarf2_signal_frame_this_id,
-  dwarf2_frame_prev_register
+  dwarf2_frame_this_id,
+  NULL,
+  NULL,
+  NULL,
+  NULL,
+  dwarf2_frame_prev_register_value
 };
 
 const struct frame_unwind *
@@ -1222,10 +1159,10 @@ dwarf2_frame_sniffer (struct frame_info 
    response to the "info frame" command.  */
 
 static CORE_ADDR
-dwarf2_frame_base_address (struct frame_info *next_frame, void **this_cache)
+dwarf2_frame_base_address (struct frame_info *this_frame, void **this_cache)
 {
   struct dwarf2_frame_cache *cache =
-    dwarf2_frame_cache (next_frame, this_cache);
+    dwarf2_frame_cache (this_frame, this_cache);
 
   return cache->cfa;
 }
Index: src/gdb/frame-unwind.c
===================================================================
--- src.orig/gdb/frame-unwind.c	2007-10-17 11:18:48.000000000 -0400
+++ src/gdb/frame-unwind.c	2007-10-17 11:22:16.000000000 -0400
@@ -20,8 +20,11 @@
 #include "defs.h"
 #include "frame.h"
 #include "frame-unwind.h"
-#include "gdb_assert.h"
 #include "dummy-frame.h"
+#include "value.h"
+#include "regcache.h"
+
+#include "gdb_assert.h"
 #include "gdb_obstack.h"
 
 static struct gdbarch_data *frame_unwind_data;
@@ -107,6 +110,67 @@ frame_unwind_find_by_frame (struct frame
   internal_error (__FILE__, __LINE__, _("frame_unwind_find_by_frame failed"));
 }
 
+/* Helper functions for value-based register unwinding.  These return
+   a (possibly lazy) value of the appropriate type.  */
+
+/* Return a value which indicates that REGNUM from FRAME is not
+   available.  */
+
+struct value *
+frame_unwind_got_optimized (struct frame_info *frame, int regnum)
+{
+  struct gdbarch *gdbarch = get_frame_arch (frame);
+  struct value *reg_val;
+
+  reg_val = value_zero (register_type (gdbarch, regnum), not_lval);
+  set_value_optimized_out (reg_val, 1);
+  return reg_val;
+}
+
+/* Return a value which indicates that REGNUM from FRAME is saved
+   in memory at ADDR.  */
+
+struct value *
+frame_unwind_got_memory (struct frame_info *frame, int regnum, CORE_ADDR addr)
+{
+  struct gdbarch *gdbarch = get_frame_arch (frame);
+
+  return value_at_lazy (register_type (gdbarch, regnum), addr);
+}
+
+/* Return a value which indicates that REGNUM from FRAME has a
+   known constant (computed) value of VAL.  */
+
+struct value *
+frame_unwind_got_constant (struct frame_info *frame, int regnum,
+			   ULONGEST val)
+{
+  struct gdbarch *gdbarch = get_frame_arch (frame);
+  struct value *reg_val;
+
+  reg_val = value_zero (register_type (gdbarch, regnum), not_lval);
+  store_unsigned_integer (value_contents_writeable (reg_val),
+			  register_size (gdbarch, regnum), val);
+  return reg_val;
+}
+
+/* Return a value which indicates that REGNUM from FRAME has a
+   known constant (computed) value of ADDR.  Convert the CORE_ADDR
+   to a target address if necessary.  */
+
+struct value *
+frame_unwind_got_address (struct frame_info *frame, int regnum,
+			  CORE_ADDR addr)
+{
+  struct gdbarch *gdbarch = get_frame_arch (frame);
+  struct value *reg_val;
+
+  reg_val = value_zero (register_type (gdbarch, regnum), not_lval);
+  pack_long (value_contents_writeable (reg_val),
+	     register_type (gdbarch, regnum), addr);
+  return reg_val;
+}
+
 extern initialize_file_ftype _initialize_frame_unwind; /* -Wmissing-prototypes */
 
 void
Index: src/gdb/sparc-tdep.c
===================================================================
--- src.orig/gdb/sparc-tdep.c	2007-10-17 11:18:48.000000000 -0400
+++ src/gdb/sparc-tdep.c	2007-10-17 11:19:45.000000000 -0400
@@ -1191,7 +1191,7 @@ sparc32_stabs_argument_has_addr (struct 
 static int
 sparc32_dwarf2_struct_return_p (struct frame_info *next_frame)
 {
-  CORE_ADDR pc = frame_unwind_address_in_block (next_frame, NORMAL_FRAME);
+  CORE_ADDR pc = get_frame_address_in_block (this_frame, NORMAL_FRAME);
   struct symbol *sym = find_pc_function (pc);
 
   if (sym)
@@ -1202,7 +1202,7 @@ sparc32_dwarf2_struct_return_p (struct f
 static void
 sparc32_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum,
 			       struct dwarf2_frame_state_reg *reg,
-			       struct frame_info *next_frame)
+			       struct frame_info *this_frame)
 {
   int off;
 
@@ -1221,7 +1221,7 @@ sparc32_dwarf2_frame_init_reg (struct gd
     case SPARC32_NPC_REGNUM:
       reg->how = DWARF2_FRAME_REG_RA_OFFSET;
       off = 8;
-      if (sparc32_dwarf2_struct_return_p (next_frame))
+      if (sparc32_dwarf2_struct_return_p (this_frame))
 	off += 4;
       if (regnum == SPARC32_NPC_REGNUM)
 	off += 4;
Index: src/gdb/Makefile.in
===================================================================
--- src.orig/gdb/Makefile.in	2007-10-17 11:20:53.000000000 -0400
+++ src/gdb/Makefile.in	2007-10-17 11:22:01.000000000 -0400
@@ -2040,7 +2040,8 @@ frame.o: frame.c $(defs_h) $(frame_h) $(
 	$(annotate_h) $(language_h) $(frame_unwind_h) $(frame_base_h) \
 	$(command_h) $(gdbcmd_h) $(observer_h) $(objfiles_h) $(exceptions_h)
 frame-unwind.o: frame-unwind.c $(defs_h) $(frame_h) $(frame_unwind_h) \
-	$(gdb_assert_h) $(dummy_frame_h) $(gdb_obstack_h)
+	$(gdb_assert_h) $(dummy_frame_h) $(gdb_obstack_h) $(value_h) \
+	$(regcache_h)
 frv-linux-tdep.o: frv-linux-tdep.c $(defs_h) $(gdbcore_h) $(target_h) \
 	$(frame_h) $(osabi_h) $(regcache_h) $(elf_bfd_h) $(elf_frv_h) \
 	$(frv_tdep_h) $(trad_frame_h) $(frame_unwind_h) $(regset_h) \

Re: [RFC] Using values to handle unwinding

[Daniel Jacobowitz]

There is one current use I know of for the NEXT_FRAME argument to
unwinders, in s390-tdep.c:

      /* If the next frame is a NORMAL_FRAME, this frame *cannot* have frame
         size zero.  This is only possible if the next frame is a sentinel
         frame, a dummy frame, or a signal trampoline frame.  */
      /* FIXME: cagney/2004-05-01: This sanity check shouldn't be
         needed, instead the code should simpliy rely on its
         analysis.  */
      if (get_frame_type (next_frame) == NORMAL_FRAME)
        return 0;

Maybe this means we should either find a generic place to do this sort
of check, or pass both this and next frame, or leave the frame
argument alone after all.  Or add a new frame function, like
"frame_called_normally (this_frame)" which seems to be the question
people are really asking when they write code like the above.

I noticed this while looking at m68k-elf backtraces.  It would be nice
to add a check like the above, either there or somewhere more generic,
because otherwise a garbage stack pointer leads to a near-infinite
backtrace.  Any time that the current frame's PC points to somewhere
GDB has no symbol info, GDB will conclude that there is a frameless
function which only stored its return address on the stack at the
call.  So each word of the stack is popped in turn and becomes a new
PC.  Not very useful!

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Using values to handle unwinding

[Joel Brobecker]

>   - Uses GDB's value infrastructure.  Values carry around the
>     location information which previously took four extra arguments
>     to every unwinder.  And this lets us use our existing value
>     manipulation routines as necessary.

I really like this part. It looks very elegant to me.

>   - Simplifies getting other registers from the PREV frame.

This idea was under discussion on gdb-patches. I said it looked like
a good idea, but Mark expressed some concerns (to which I think you
tried to answer).

> This patch leaves the existing prev_register method.  If we want to
> adopt this new approach then I would recommend not doing that at all:
> just change the type of the prev_register field, break all targets
> until they are updated, and fix them.  We can do it on a branch and
> merge the whole branch at once, or do it quickly in HEAD.  Converting
> the DWARF-2 reader took me only an hour or two, which included writing
> the helper routines.

I don't mind breaking all the targets if this is the way we want
to do it. But either way, whether we use a branch or not, I'd like
us to break the target by breaking the build. This way, managing to
build again means we're done with the conversion.

If I understand the patch correctly, this is going to be the case,
right? You're removing some old routines, and replacing with a
new one that has a different name.

-- 
Joel

Re: [RFC] Using values to handle unwinding

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> There is one current use I know of for the NEXT_FRAME argument to
> unwinders, in s390-tdep.c:
> 
>       /* If the next frame is a NORMAL_FRAME, this frame *cannot* have frame
>          size zero.  This is only possible if the next frame is a sentinel
>          frame, a dummy frame, or a signal trampoline frame.  */
>       /* FIXME: cagney/2004-05-01: This sanity check shouldn't be
>          needed, instead the code should simpliy rely on its
>          analysis.  */
>       if (get_frame_type (next_frame) == NORMAL_FRAME)
>         return 0;
> 
> Maybe this means we should either find a generic place to do this sort
> of check, or pass both this and next frame, or leave the frame
> argument alone after all.  Or add a new frame function, like
> "frame_called_normally (this_frame)" which seems to be the question
> people are really asking when they write code like the above.

Well, we can always just use "get_next_frame (this_frame)" instead
of next_frame.   Getting the next frame is always well-defined.
So I don't think this influences the this_frame vs. next_frame
discussion one way or the other ...

> I noticed this while looking at m68k-elf backtraces.  It would be nice
> to add a check like the above, either there or somewhere more generic,
> because otherwise a garbage stack pointer leads to a near-infinite
> backtrace.  Any time that the current frame's PC points to somewhere
> GDB has no symbol info, GDB will conclude that there is a frameless
> function which only stored its return address on the stack at the
> call.  So each word of the stack is popped in turn and becomes a new
> PC.  Not very useful!

Yes, situations similar to that were what prompted my addition of the
above sanity check (Andrew's comment nonwithstanding :-/).

Bye,
Ulrich

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

Re: [RFC] Using values to handle unwinding

[Daniel Jacobowitz]

On Thu, Oct 18, 2007 at 09:29:51PM -0700, Joel Brobecker wrote:
> I don't mind breaking all the targets if this is the way we want
> to do it. But either way, whether we use a branch or not, I'd like
> us to break the target by breaking the build. This way, managing to
> build again means we're done with the conversion.
> 
> If I understand the patch correctly, this is going to be the case,
> right? You're removing some old routines, and replacing with a
> new one that has a different name.

The version I posted would leave other targets working, so they could
be switched one at a time.  Not the best way to handle a transition,
but it saved me having to convert the other non-dwarf unwinders in
the build I was testing.

If we're going to make a switch I think changing the signature of
unwind->prev_register would be best; that will stop other targets
from compiling (and remove some of the ugly bits of the patch,
too).

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Using values to handle unwinding

[Daniel Jacobowitz]

On Fri, Oct 19, 2007 at 01:41:50PM +0200, Ulrich Weigand wrote:
> >       if (get_frame_type (next_frame) == NORMAL_FRAME)
> >         return 0;

> Well, we can always just use "get_next_frame (this_frame)" instead
> of next_frame.   Getting the next frame is always well-defined.

Right.  get_next_frame fails when the next frame is the sentinel
frame, so it is not always useful if you need a frame that you can
pass to a frame_unwind_* routine.  But it never fails if
frame->next->type would be NORMAL_FRAME.

> > I noticed this while looking at m68k-elf backtraces.  It would be nice
> > to add a check like the above, either there or somewhere more generic,
> > because otherwise a garbage stack pointer leads to a near-infinite
> > backtrace.  Any time that the current frame's PC points to somewhere
> > GDB has no symbol info, GDB will conclude that there is a frameless
> > function which only stored its return address on the stack at the
> > call.  So each word of the stack is popped in turn and becomes a new
> > PC.  Not very useful!
> 
> Yes, situations similar to that were what prompted my addition of the
> above sanity check (Andrew's comment nonwithstanding :-/).

The m68k check turns out to be particularly thorny; this is a problem
with architectures where call pushes onto the stack.  You can't
distinguish that from any other stack push, so all frames are assumed
to have a valid return address as long as the stack is readable.

I'm speculating about a "set backtrace max-uncertainty" and
frame_is_uncertain ()... or "set backtrace aggressive off"...

We shouldn't give up when we have no symbol information if the
architecture claims to know how to handle that case.  This happens
when crashing in a stripped shared library, for instance, and on
i386 GNU/Linux GDB generally manages a useful backtrace anyway.

Anyway, that's unrelated to this patch.  I'll save it for another
day :-)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Using values to handle unwinding

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> It returns a GDB value object describing the register value.  It may
> be a lazy reference to memory, a lazy reference to the value of a
> register in THIS frame, or a non-lvalue (constant or address).  The
> first part of the patch adds support for lazy registers, which are not
> fetched until they are needed.  When any frame other than the sentinel
> frame sees that a register in the PREV frame is not saved or constant,
> it returns a lazy reference to the register in THIS frame.

I think this is an excellent idea.  I'm wondering how this plays with
special conversions needed for certain registers, see e.g. the
discussion we had last December that led to introduction of the
value_from_register gdbarch callback.

For example, while we do have ways of performing special conversions
on the registers themselves, there is no straightforward way to do so
for unwound register contents.  Maybe if we're going to using values
to represent those, we could allow the architecture to perform type-
specific conversions on those (this should e.g. allow me to eliminate
a specical pseudo on the SPU that I'm using to hold the properly
converted unwound stack pointer register contents).

> There are three key differences / motivations:
> 
>   - Uses GDB's value infrastructure.  Values carry around the
>     location information which previously took four extra arguments
>     to every unwinder.  And this lets us use our existing value
>     manipulation routines as necessary.
> 
>   - Uses the same frame that non-unwinder architecture methods use.
>     For instance, in my ARM CPSR unwinding patch I had a routine
>     "arm_frame_is_thumb".  It was used during software single
>     stepping, where we wanted the state of the current frame, and
>     during unwinding, where we wanted the state of THIS frame
>     (i.e. the one being unwound / having its ID established).
> 
>   - Simplifies getting other registers from the PREV frame.
>     For instance, if one register is computed based on the
>     value of another.  Previously you had to inline code
>     to do this or call the same prev_register routine again
>     with the same NEXT frame and THIS cache; awkward if more
>     than one function was involved (see previous reason).

I must admit I also no longer understand the original reason for
using the NEXT_FRAME here.  If THIS_FRAME works, it would certainly
appear to be much more straightforward ...

B.t.w. there's another change in the patch: the elimination of the
prev_pc unwinder method.  Could you explain the reason why this is
now no longer necessary?

Bye,
Ulrich

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

Re: [RFC] Using values to handle unwinding

[Daniel Jacobowitz]

On Fri, Oct 19, 2007 at 02:10:07PM +0200, Ulrich Weigand wrote:
> I think this is an excellent idea.  I'm wondering how this plays with
> special conversions needed for certain registers, see e.g. the
> discussion we had last December that led to introduction of the
> value_from_register gdbarch callback.
> 
> For example, while we do have ways of performing special conversions
> on the registers themselves, there is no straightforward way to do so
> for unwound register contents.  Maybe if we're going to using values
> to represent those, we could allow the architecture to perform type-
> specific conversions on those (this should e.g. allow me to eliminate
> a specical pseudo on the SPU that I'm using to hold the properly
> converted unwound stack pointer register contents).

I see we ended up with both "gdbarch_value_from_register" and
"gdbarch_register_to_value".  That's unfortunately confusing :-(

The patch I posted should make things neither better nor worse,
because reading a variable from a register (hardware or saved in a
frame) will still call value_from_register.  We'll get the raw bytes
by using the value method but call gdbarch_value_from_register and
get_frame_register_bytes for the final value.  So everything works
the same as before.

Perhaps we can adjust value_from_register to convert one struct value
* representing a register's contents to another representing its value
in a particular type?  It's a machine-dependent variant of a cast.

> B.t.w. there's another change in the patch: the elimination of the
> prev_pc unwinder method.  Could you explain the reason why this is
> now no longer necessary?

Sorry, I should have separated that out.  There is only one definition
of prev_pc anywhere.  Here it is:

static CORE_ADDR
sentinel_frame_prev_pc (struct frame_info *next_frame,
                        void **this_prologue_cache)
{
  struct gdbarch *gdbarch = get_frame_arch (next_frame);
  return gdbarch_unwind_pc (gdbarch, next_frame);
}

And here's the call, with comments removed for brevity:

      if (this_frame->unwind->prev_pc != NULL)
        /* A per-frame unwinder, prefer it.  */
        pc = this_frame->unwind->prev_pc (this_frame->next,
                                          &this_frame->prologue_cache);
      else if (gdbarch_unwind_pc_p (get_frame_arch (this_frame)))
        pc = gdbarch_unwind_pc (get_frame_arch (this_frame), this_frame);

So, as you can see, there's no longer any need for the hook in current
targets.

-- 
Daniel Jacobowitz
CodeSourcery

[RFC] Using values to handle unwinding

[Daniel Jacobowitz]

On Wed, Oct 17, 2007 at 12:03:50PM -0400, Daniel Jacobowitz wrote:
> This is the patch I referenced yesterday on gdb-patches; the
> background is here:
>   http://sourceware.org/ml/gdb-patches/2007-10/msg00432.html

And the thread I'm replying to starts here for more:
  http://sourceware.org/ml/gdb/2007-10/msg00124.html

I have finished polishing these patches.  I will post (to gdb-patches,
in a moment) patches for lazy register values, prev_register returning
a value, and passing this_frame instead of next_frame to basically
everything except the gdbarch unwind_pc method.  unwind_dummy_id
becomes just dummy_id, et cetera.

These patches break the build for every unconverted target.  I updated
x86_64-* and i386-*, and all the platform-independent unwinders
(tramp, trad, dummy, dwarf2, sentinel).  Later in this mail are some
detailed conversion instructions.

I also wrote some gdbint documentation, for both new and existing
frame unwinding details.  It's not complete but it's a step in the
right direction.

One addition to the instructions: unless you have to do something
particularly strange to a file, I think a single-line changelog entry
is reasonable.  I want to improve GDB, not the size of its ChangeLog
:-) And writing the changelogs for this patch set took quite a long
time.

I'd love comments on the patches, the overall approach, and how to
proceed.  Ideally, we check this in (breaking many targets), update
each target completely as someone needs that target, and make sure all
targets are updated by the next release of GDB.  I personally use
amd64, i386, arm, mips, m68k, and powerpc; so I'm pretty likely to
update all of those (mechanically).  I'll do the laggards before the
next release too, but not right this minute.  I would appreciate
assistance with other targets :-)

Thoughts?

===

Conversion notes:

Here's how you convert an unwinder.  Start by finding a prev_register
routine.  They're pretty much all named *_prev_register so you can
grep for "_prev_register ".  Update it to have the new signature.
After you're done converting it, be sure to convert the matching
this_id routine too!  The signature of this_id has changed (next_frame
-> this_frame), but in a way that will not cause compilation errors.
So never do a prev_register routine without the matching this_id.
They come in pairs pretty much everywhere.

frame_unwind_append_sniffer has gone away.  It has been replaced by
frame_unwind_append_unwinder, which takes a different signature
(matching the pre-existing frame_unwind_prepend_unwinder).  And both
prepended and appended sniffers now take next_frame, just like
prev_register and this_id.  During execution of the sniffer, the
unwinder being sniffed for is installed in the current frame.  This
lets simple interfaces like get_frame_func and
get_frame_address_in_block behave as expected.  Which means the exact
same routines can be called from the sniffer as from prev_value.

What this means during convertion is that you need to add the
sniffer to the "struct frame_unwind" and call frame_unwind_append_unwinder
instead of frame_unwind_append_sniffer.  The type of the sniffer has
also changed.  Sniffers which just "return 1" (every architecture
has one) can use default_frame_sniffer.

There are not as many frame base unwinders, but keep an eye out for
those too - they have the same problem as this_id, and the frame base
sniffers have to take this_frame also.  Similarly, the init function
in struct tramp_frame now takes this_frame.  I did a lot of searching
files for next_frame.  A few gdbarch methods still use next_frame, but
most functions with an argument named next_frame are due for an
update.

For each affected routine, rename the next_frame argument to this_frame.
Be sure to update the description in the function comment, and any
other comments inside the function.  Check every use of this_frame;
some examples are in the table below.  If it is passed to other
miscellaneous target-specific functions, then you need to convert
those (add them to your worklist, as it were...) and be sure to
update all their callers too.

I found it best to do an entire architecture at a time, e.g. amd64*.c
+ i386*.c all in one go.  I started with amd64-tdep.c.  The only
nonstandard routine was called from amd64_sigtramp_frame_cache:
tdep->sigcontext_addr.  So from there I had to go out and do
all of the amd64 and i386 OS ports and the i386 sigtramp unwinder
too.

You have to be careful when using get_prev_frame in an unwinder
(generally don't do it - the prev frame requires this frame) and
similarly for get_next_frame (it returns NULL instead of the sentinel
frame, so generally don't do this either).  Let me know if you run
into any problems with having the wrong frame and not being able
to change all the affected functions.

For instance, before I converted sniffers to take this_frame, I ran
into i386_linux_sigcontext_addr.  That calls
i386_linux_sigtramp_start, which was particularly tricky because I had
not converted the sigtramp_p routine to take this_frame, which was
itself tricky because I haven't converted sniffers to take this_frame
being sniffed.  So I had to revisit the design at that point.  Until
I did, I deliberately left the call untouched:

  pc = i386_linux_sigtramp_start (next_frame);

Since I'd renamed next_frame -> this_frame in
i386_linux_sigcontext_addr already, this clearly won't compile -
that's a flag to come back and look at it later.  It's not practical
to test all platforms in advance for this sort of conversion,
but the majority of unwinders are in tdep files so we can compile
them easily (--enable-targets=...).  I added a /* FIXME */
while I was working on it, just for good measure, and
periodically scanned my diff for added FIXMEs.

Then some of the files I'd already touched contained additional
unwinders (e.g. amd64obsd_trapframe_sniffer) so I went back
and did those too.

Here's some example conversions:

NEXT_FRAME				THIS_FRAME
----------				----------
frame_unwind_register			get_frame_register
frame_unwind_register_unsigned		get_frame_register_unsigned
frame_unwind_register_signed		get_frame_register_signed
frame_pc_unwind				get_frame_pc
frame_func_unwind			get_frame_func
						(lose the extra argument)
frame_unwind_address_in_block		get_frame_address_in_block
						(lose the extra argument)
get_frame_memory_unsigned		*unchanged*
						(just rename the argument)
safe_frame_unwind_memory		*unchanged*
						(ditto, ignore the name...)

Old register kind			New function
-----------------			------------
not_lval				frame_unwind_got_constant
    [For values no larger than a ULONGEST.
     Calculation of the value may be guarded by if (valuep).
     We always calculate the value in this new interface.  That's
     the majority of calls, so this is not a noticeable loss.]
not_lval				frame_unwind_got_address
    [If the port does complicated address to pointer conversion
     and you have a CORE_ADDR, you'll need this instead.]
lval_register				frame_unwind_got_register
lval_memory				frame_unwind_got_memory


-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Using values to handle unwinding

[Joel Brobecker]

> Thoughts?

I read the first patch (register values) pretty carefully, the second
one (frame unwinding using struct value & using this_frame instead of
next_frame) as carefully as I could, and skimed the other few patches.
Overall, it looks pretty nice. I think it would be very valuable if
others took the time to look at them, I had to spend a lot of time
to understand them, and even now I'm sure there are things I missed.

> I'd love comments on the patches, the overall approach, and how to
> proceed.  Ideally, we check this in (breaking many targets), update
> each target completely as someone needs that target, and make sure all
> targets are updated by the next release of GDB.  I personally use
> amd64, i386, arm, mips, m68k, and powerpc; so I'm pretty likely to
> update all of those (mechanically).  I'll do the laggards before the
> next release too, but not right this minute.  I would appreciate
> assistance with other targets :-)

You said "mechanically" - does it mean you are not able to test some
of the targets you listed?

I can help (including testing) with: hppa, ia64, sparc and powerpc (aix).
I would prefer if you took care of mips if you're able to test that.
In terms of other architectures that no one volunteers to convert, we
can split them among ourselves, and perform mechanical conversions.
We can use a second pair of eyes as our means of testing...

In terms of how to proceed, I don't mind if we temporarily break
some targets.

-- 
Joel

Re: [RFC] Using values to handle unwinding

[Daniel Jacobowitz]

Thanks for looking at them!

On Fri, Apr 04, 2008 at 10:37:10AM -0700, Joel Brobecker wrote:
> > I'd love comments on the patches, the overall approach, and how to
> > proceed.  Ideally, we check this in (breaking many targets), update
> > each target completely as someone needs that target, and make sure all
> > targets are updated by the next release of GDB.  I personally use
> > amd64, i386, arm, mips, m68k, and powerpc; so I'm pretty likely to
> > update all of those (mechanically).  I'll do the laggards before the
> > next release too, but not right this minute.  I would appreciate
> > assistance with other targets :-)
> 
> You said "mechanically" - does it mean you are not able to test some
> of the targets you listed?

I can test a representative triplet.  But there's a wide variety of OS
tdep files with sniffers; the patches I've already posted change about
a dozen targets, only one of which I can readily test.  So a certain
amount of mechanical conversion and proofreading is the best I can
do.

> We can use a second pair of eyes as our means of testing...

Yes indeed :-)

-- 
Daniel Jacobowitz
CodeSourcery