[PATCHv4] gdb: Add default frame methods to gdbarch

Message ID 20180908231921.GJ22193@embecosm.com
State New, archived
Headers

Commit Message

Andrew Burgess Sept. 8, 2018, 11:19 p.m. UTC
  Simon,

Thanks for the review.

* Simon Marchi <simon.marchi@ericsson.com> [2018-09-08 23:06:00 +0100]:

> On 2018-09-07 10:26 PM, Andrew Burgess wrote:
> > This is a new version of the patch series that I original submitted
> > here:
> > 
> >     https://sourceware.org/ml/gdb-patches/2018-03/msg00165.html
> > 
> > That patch did get some feedback, which led to some revisions, the
> > final version was posted here:
> > 
> >     https://sourceware.org/ml/gdb-patches/2018-06/msg00090.html
> > 
> > However, this didn't get any feedback :-(
> 
> Sorry about that, my bad :(.  For v2 and subsequent versions, I tend to let
> the original reviewer do the follow-up.  I forgot that this person
> was me.
> 
> > In this new version I have changed my approach.  Rather than a series
> > of patches, each supplying a new default gdbarch method and converting
> > all targets, here I provide one patch that supplies just the new
> > gdbarch methods, and converts no targets.
> > 
> > No target should change once this patch is merged, as previously all
> > the targets had to supply these methods themselves anyway.
> > 
> > Once this patch is merged then I'll supply a series of patches based
> > off of:
> > 
> >    https://sourceware.org/ml/gdb-patches/2018-06/msg00092.html
> >    https://sourceware.org/ml/gdb-patches/2018-06/msg00091.html
> >    https://sourceware.org/ml/gdb-patches/2018-06/msg00093.html
> >    https://sourceware.org/ml/gdb-patches/2018-06/msg00094.html
> > 
> > that converts one target at a time to use the default gdbarch methods
> > (where that is appropriate).  I don't want to waste my time putting
> > those patches together if I still can't get this patch in, but
> > hopefully the above links should show you what my general intention
> > is.
> > 
> > All feedback welcome.
> > 
> > Thanks,
> > Andrew
> > 
> > ---
> > 
> > Supply default gdbarch methods for gdbarch_dummy_id,
> > gdbarch_unwind_pc, and gdbarch_unwind_sp.  This patch doesn't actually
> > convert any targets to use these methods, and so, there will be no
> > user visible changes after this commit.
> > 
> > gdb/ChangeLog:
> > 
> > 	* gdb/dummy-frame.c (default_dummy_id): Defined new function.
> > 	* gdb/dummy-frame.h (default_dummy_id): Declare new function.
> > 	* gdb/frame-unwind.c (default_unwind_pc): Define new function.
> > 	(default_unwind_sp): Define new function.
> > 	* gdb/frame-unwind.h (default_unwind_pc): Declare new function.
> > 	(default_unwind_sp): Declare new function.
> > 	* gdb/frame.c (frame_unwind_pc): Assume gdbarch_unwind_pc is
> > 	available.
> > 	(get_frame_sp): Assume that gdbarch_unwind_sp is available.
> > 	* gdb/gdbarch.c: Regenerate.
> > 	* gdb/gdbarch.h: Regenerate.
> > 	* gdb/gdbarch.sh: Update definition of dummy_id, unwind_pc, and
> > 	unwind_sp.  Add additional header files to be included in
> > 	generated file.
> > ---
> >  gdb/ChangeLog      |  17 +++++++
> >  gdb/dummy-frame.c  |  16 +++++++
> >  gdb/dummy-frame.h  |   6 +++
> >  gdb/frame-unwind.c |  31 +++++++++++++
> >  gdb/frame-unwind.h |  12 +++++
> >  gdb/frame.c        | 132 ++++++++++++++++++++++++-----------------------------
> >  gdb/gdbarch.c      |  41 ++++-------------
> >  gdb/gdbarch.h      |   6 ---
> >  gdb/gdbarch.sh     |   8 ++--
> >  9 files changed, 154 insertions(+), 115 deletions(-)
> > 
> > diff --git a/gdb/dummy-frame.c b/gdb/dummy-frame.c
> > index c6f874a3b19..6bb128233d7 100644
> > --- a/gdb/dummy-frame.c
> > +++ b/gdb/dummy-frame.c
> > @@ -385,6 +385,22 @@ const struct frame_unwind dummy_frame_unwind =
> >    dummy_frame_sniffer,
> >  };
> >  
> > +/* Default implementation of gdbarch_dummy_id.  Generate frame_id for
> > +   THIS_FRAME assuming that it is a dummy frame.  A dummy frame is created
> > +   before an inferior call, the frame_id returned here must match the base
> > +   address returned by gdbarch_push_dummy_call and the frame's pc must
> > +   match the dummy frames breakpoint address.  */
> 
> Use /* See dummy-frame.h.  */
> 
> I think that additional info you have would be good in gdbarch.sh, to document
> gdbarch_dummy_id.
> 
> > +
> > +struct frame_id
> > +default_dummy_id (struct gdbarch *gdbarch, struct frame_info *this_frame)
> > +{
> > +  CORE_ADDR sp, pc;
> > +
> > +  sp = get_frame_sp (this_frame);
> > +  pc = get_frame_pc (this_frame);
> > +  return frame_id_build (sp, pc);
> > +}
> > +
> >  static void
> >  fprint_dummy_frames (struct ui_file *file)
> >  {
> > diff --git a/gdb/dummy-frame.h b/gdb/dummy-frame.h
> > index 407f398404e..9eaec3492bf 100644
> > --- a/gdb/dummy-frame.h
> > +++ b/gdb/dummy-frame.h
> > @@ -73,4 +73,10 @@ extern void register_dummy_frame_dtor (frame_id dummy_id,
> >  extern int find_dummy_frame_dtor (dummy_frame_dtor_ftype *dtor,
> >  				  void *dtor_data);
> >  
> > +/* Default implementation of gdbarch_dummy_id.  Generate a dummy frame_id
> > +   for THIS_FRAME assuming that the frame is a dummy frame.  */
> > +
> > +extern struct frame_id default_dummy_id (struct gdbarch *gdbarch,
> > +					 struct frame_info *this_frame);
> > +
> >  #endif /* !defined (DUMMY_FRAME_H)  */
> > diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
> > index e6e63539ad7..df2e5ddc6bd 100644
> > --- a/gdb/frame-unwind.c
> > +++ b/gdb/frame-unwind.c
> > @@ -193,6 +193,37 @@ default_frame_unwind_stop_reason (struct frame_info *this_frame,
> >      return UNWIND_NO_REASON;
> >  }
> >  
> > +/* Default unwind of the PC.  */
> 
> Use /* See frame-unwind.h.  */
> 
> > +
> > +CORE_ADDR
> > +default_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame)
> > +{
> > +  struct type *type;
> > +  int pc_regnum;
> > +  CORE_ADDR addr;
> > +  struct value *value;
> > +
> > +  pc_regnum = gdbarch_pc_regnum (gdbarch);
> > +  value = frame_unwind_register_value (next_frame, pc_regnum);
> > +  type = builtin_type (gdbarch)->builtin_func_ptr;
> > +  addr = extract_typed_address (value_contents_all (value), type);
> > +  addr = gdbarch_addr_bits_remove (gdbarch, addr);
> 
> It's up to you, but I'd declare the variables when assigning them (same for
> all the new functions in this patch).
> 
> > +
> > +  release_value (value);
> 
> I'm not too familiar with the lifetime of struct value objects, but why is this
> needed?

For why, I'm not really sure.

If we follow the code through then any call to frame_unwind_register,
frame_register_unwind, or frame_unwind_register_unsigned will do the
same release_value.  This patch never set out to change the behaviour
of existing code, just to remove duplication, so on those grounds
alone, in this patch at least I'd want the call to stay.

There is one clue in frame_register_unwind, where we have this code:

    /* Dispose of the new value.  This prevents watchpoints from
       trying to watch the saved frame pointer.  */
    release_value (value);

Pretty cryptic, I'm not really sure how we could end up watching that
value, so I don't really understand that.  (This was added in commit
669fac235d5e about 10 years ago).

My understanding of value lifetime is that generally values live in a
single all_values list at various points we'll call value_set_mark,
run some code that might generate some values, and then call
value_delete_to_mark to delete all values as we know they are no
longer needed.

Values can be moved off the global all_values list, for example into
the value history, which means they will not be deleted by the call to
value_delete_to_mark.

Other than, calling this release_value here will help keep the
all_values list shorter (and the cryptic comment above) I don't see
why we couldn't just leave the $pc value on the all_values list, and
clean it up at the next call to value_delete_to_mark....

... however, like I said, I'd prefer this patch be just a refactor,
unless the code is obviously wrong I'd rather keep the call in.

All your other comments are addressed in the version below.

Thanks again,
Andrew

---

gdb: Add default frame methods to gdbarch

Supply default gdbarch methods for gdbarch_dummy_id,
gdbarch_unwind_pc, and gdbarch_unwind_sp.  This patch doesn't actually
convert any targets to use these methods, and so, there will be no
user visible changes after this commit.

gdb/ChangeLog:

	* gdb/dummy-frame.c (default_dummy_id): Defined new function.
	* gdb/dummy-frame.h (default_dummy_id): Declare new function.
	* gdb/frame-unwind.c (default_unwind_pc): Define new function.
	(default_unwind_sp): Define new function.
	* gdb/frame-unwind.h (default_unwind_pc): Declare new function.
	(default_unwind_sp): Declare new function.
	* gdb/frame.c (frame_unwind_pc): Assume gdbarch_unwind_pc is
	available.
	(get_frame_sp): Assume that gdbarch_unwind_sp is available.
	* gdb/gdbarch.c: Regenerate.
	* gdb/gdbarch.h: Regenerate.
	* gdb/gdbarch.sh: Update definition of dummy_id, unwind_pc, and
	unwind_sp.  Add additional header files to be included in
	generated file.
---
 gdb/ChangeLog      |  17 +++++++
 gdb/dummy-frame.c  |  12 +++++
 gdb/dummy-frame.h  |   6 +++
 gdb/frame-unwind.c |  26 +++++++++++
 gdb/frame-unwind.h |  12 +++++
 gdb/frame.c        | 132 ++++++++++++++++++++++++-----------------------------
 gdb/gdbarch.c      |  41 ++++-------------
 gdb/gdbarch.h      |   6 ---
 gdb/gdbarch.sh     |  13 ++++--
 9 files changed, 150 insertions(+), 115 deletions(-)
  

Comments

Simon Marchi Sept. 8, 2018, 11:52 p.m. UTC | #1
On 2018-09-09 00:19, Andrew Burgess wrote:
>> > +
>> > +  release_value (value);
>> 
>> I'm not too familiar with the lifetime of struct value objects, but 
>> why is this
>> needed?
> 
> For why, I'm not really sure.
> 
> If we follow the code through then any call to frame_unwind_register,
> frame_register_unwind, or frame_unwind_register_unsigned will do the
> same release_value.  This patch never set out to change the behaviour
> of existing code, just to remove duplication, so on those grounds
> alone, in this patch at least I'd want the call to stay.
> 
> There is one clue in frame_register_unwind, where we have this code:
> 
>     /* Dispose of the new value.  This prevents watchpoints from
>        trying to watch the saved frame pointer.  */
>     release_value (value);

Ok, that's interesting.  My hypothesis is that when GDB needs to watch 
an expression (a + b + c), it probably looks at the all_values chain to 
know all the intermediary values (that represent a memory location or a 
register) that were read from the target to evaluate that expression.  
It then puts some hardware or software watches on these memory locations 
or registers.  So if the values chain contains a value representing 
something we don't really watch to watch (such as fp or sp, according to 
the comment), GDB would watch it, even though it doesn't really affect 
the result of the expression.  That's just my guess.

> Pretty cryptic, I'm not really sure how we could end up watching that
> value, so I don't really understand that.  (This was added in commit
> 669fac235d5e about 10 years ago).
> 
> My understanding of value lifetime is that generally values live in a
> single all_values list at various points we'll call value_set_mark,
> run some code that might generate some values, and then call
> value_delete_to_mark to delete all values as we know they are no
> longer needed.
> 
> Values can be moved off the global all_values list, for example into
> the value history, which means they will not be deleted by the call to
> value_delete_to_mark.

That's what I understand too.

> Other than, calling this release_value here will help keep the
> all_values list shorter (and the cryptic comment above) I don't see
> why we couldn't just leave the $pc value on the all_values list, and
> clean it up at the next call to value_delete_to_mark....
> 
> ... however, like I said, I'd prefer this patch be just a refactor,
> unless the code is obviously wrong I'd rather keep the call in.

I am fine with that (unless someone who really knows this comes up with 
a better explanation).  Since it just causes the value we don't need 
anymore to be deleted immediatly, it should be harmless.

> All your other comments are addressed in the version below.

This version LGTM.  Can you wait 2-3 days before pushing to see if 
somebody has something to add?

Simon
  
Tom Tromey Sept. 9, 2018, 2:54 a.m. UTC | #2
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> Ok, that's interesting.  My hypothesis is that when GDB needs to watch
Simon> an expression (a + b + c), it probably looks at the all_values chain
Simon> to know all the intermediary values (that represent a memory location
Simon> or a register) that were read from the target to evaluate that
Simon> expression.  It then puts some hardware or software watches on these
Simon> memory locations or registers.  So if the values chain contains a
Simon> value representing something we don't really watch to watch (such as
Simon> fp or sp, according to the comment), GDB would watch it, even though
Simon> it doesn't really affect the result of the expression.  That's just my
Simon> guess.

Yes, that's correct.

Simon> I am fine with that (unless someone who really knows this comes up
Simon> with a better explanation).  Since it just causes the value we don't
Simon> need anymore to be deleted immediatly, it should be harmless.

I agree.

Tom
  

Patch

diff --git a/gdb/dummy-frame.c b/gdb/dummy-frame.c
index c6f874a3b19..8e380ae1c87 100644
--- a/gdb/dummy-frame.c
+++ b/gdb/dummy-frame.c
@@ -385,6 +385,18 @@  const struct frame_unwind dummy_frame_unwind =
   dummy_frame_sniffer,
 };
 
+/* See dummy-frame.h.  */
+
+struct frame_id
+default_dummy_id (struct gdbarch *gdbarch, struct frame_info *this_frame)
+{
+  CORE_ADDR sp, pc;
+
+  sp = get_frame_sp (this_frame);
+  pc = get_frame_pc (this_frame);
+  return frame_id_build (sp, pc);
+}
+
 static void
 fprint_dummy_frames (struct ui_file *file)
 {
diff --git a/gdb/dummy-frame.h b/gdb/dummy-frame.h
index 407f398404e..9eaec3492bf 100644
--- a/gdb/dummy-frame.h
+++ b/gdb/dummy-frame.h
@@ -73,4 +73,10 @@  extern void register_dummy_frame_dtor (frame_id dummy_id,
 extern int find_dummy_frame_dtor (dummy_frame_dtor_ftype *dtor,
 				  void *dtor_data);
 
+/* Default implementation of gdbarch_dummy_id.  Generate a dummy frame_id
+   for THIS_FRAME assuming that the frame is a dummy frame.  */
+
+extern struct frame_id default_dummy_id (struct gdbarch *gdbarch,
+					 struct frame_info *this_frame);
+
 #endif /* !defined (DUMMY_FRAME_H)  */
diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
index e6e63539ad7..618ceee3551 100644
--- a/gdb/frame-unwind.c
+++ b/gdb/frame-unwind.c
@@ -193,6 +193,32 @@  default_frame_unwind_stop_reason (struct frame_info *this_frame,
     return UNWIND_NO_REASON;
 }
 
+/* See frame-unwind.h.  */
+
+CORE_ADDR
+default_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame)
+{
+  int pc_regnum = gdbarch_pc_regnum (gdbarch);
+  struct value *value = frame_unwind_register_value (next_frame, pc_regnum);
+  struct type *type = builtin_type (gdbarch)->builtin_func_ptr;
+  CORE_ADDR addr = extract_typed_address (value_contents_all (value), type);
+  addr = gdbarch_addr_bits_remove (gdbarch, addr);
+
+  release_value (value);
+  return addr;
+}
+
+/* See frame-unwind.h.  */
+
+CORE_ADDR
+default_unwind_sp (struct gdbarch *gdbarch, struct frame_info *next_frame)
+{
+  int sp_regnum;
+
+  sp_regnum = gdbarch_sp_regnum (gdbarch);
+  return frame_unwind_register_unsigned (next_frame, sp_regnum);
+}
+
 /* Helper functions for value-based register unwinding.  These return
    a (possibly lazy) value of the appropriate type.  */
 
diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h
index af220f72a01..3ca3fdfe727 100644
--- a/gdb/frame-unwind.h
+++ b/gdb/frame-unwind.h
@@ -70,6 +70,18 @@  enum unwind_stop_reason
   default_frame_unwind_stop_reason (struct frame_info *this_frame,
 				    void **this_cache);
 
+/* A default unwind_pc callback that simply unwinds the register identified
+   by GDBARCH_PC_REGNUM.  */
+
+extern CORE_ADDR default_unwind_pc (struct gdbarch *gdbarch,
+				    struct frame_info *next_frame);
+
+/* A default unwind_sp callback that simply unwinds the register identified
+   by GDBARCH_SP_REGNUM.  */
+
+extern CORE_ADDR default_unwind_sp (struct gdbarch *gdbarch,
+				    struct frame_info *next_frame);
+
 /* Assuming the frame chain: (outer) prev <-> this <-> next (inner);
    use THIS frame, and through it the NEXT frame's register unwind
    method, to determine the frame ID of THIS frame.
diff --git a/gdb/frame.c b/gdb/frame.c
index 0dc2cb7e716..2ade6ea1c7c 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -872,76 +872,71 @@  frame_unwind_pc (struct frame_info *this_frame)
 {
   if (this_frame->prev_pc.status == CC_UNKNOWN)
     {
-      if (gdbarch_unwind_pc_p (frame_unwind_arch (this_frame)))
+      struct gdbarch *prev_gdbarch;
+      CORE_ADDR pc = 0;
+      int pc_p = 0;
+
+      /* The right way.  The `pure' way.  The one true way.  This
+	 method depends solely on the register-unwind code to
+	 determine the value of registers in THIS frame, and hence
+	 the value of this frame's PC (resume address).  A typical
+	 implementation is no more than:
+
+	 frame_unwind_register (this_frame, ISA_PC_REGNUM, buf);
+	 return extract_unsigned_integer (buf, size of ISA_PC_REGNUM);
+
+	 Note: this method is very heavily dependent on a correct
+	 register-unwind implementation, it pays to fix that
+	 method first; this method is frame type agnostic, since
+	 it only deals with register values, it works with any
+	 frame.  This is all in stark contrast to the old
+	 FRAME_SAVED_PC which would try to directly handle all the
+	 different ways that a PC could be unwound.  */
+      prev_gdbarch = frame_unwind_arch (this_frame);
+
+      TRY
 	{
-	  struct gdbarch *prev_gdbarch;
-	  CORE_ADDR pc = 0;
-	  int pc_p = 0;
-
-	  /* The right way.  The `pure' way.  The one true way.  This
-	     method depends solely on the register-unwind code to
-	     determine the value of registers in THIS frame, and hence
-	     the value of this frame's PC (resume address).  A typical
-	     implementation is no more than:
-	   
-	     frame_unwind_register (this_frame, ISA_PC_REGNUM, buf);
-	     return extract_unsigned_integer (buf, size of ISA_PC_REGNUM);
-
-	     Note: this method is very heavily dependent on a correct
-	     register-unwind implementation, it pays to fix that
-	     method first; this method is frame type agnostic, since
-	     it only deals with register values, it works with any
-	     frame.  This is all in stark contrast to the old
-	     FRAME_SAVED_PC which would try to directly handle all the
-	     different ways that a PC could be unwound.  */
-	  prev_gdbarch = frame_unwind_arch (this_frame);
-
-	  TRY
+	  pc = gdbarch_unwind_pc (prev_gdbarch, this_frame);
+	  pc_p = 1;
+	}
+      CATCH (ex, RETURN_MASK_ERROR)
+	{
+	  if (ex.error == NOT_AVAILABLE_ERROR)
 	    {
-	      pc = gdbarch_unwind_pc (prev_gdbarch, this_frame);
-	      pc_p = 1;
+	      this_frame->prev_pc.status = CC_UNAVAILABLE;
+
+	      if (frame_debug)
+		fprintf_unfiltered (gdb_stdlog,
+				    "{ frame_unwind_pc (this_frame=%d)"
+				    " -> <unavailable> }\n",
+				    this_frame->level);
 	    }
-	  CATCH (ex, RETURN_MASK_ERROR)
+	  else if (ex.error == OPTIMIZED_OUT_ERROR)
 	    {
-	      if (ex.error == NOT_AVAILABLE_ERROR)
-		{
-		  this_frame->prev_pc.status = CC_UNAVAILABLE;
-
-		  if (frame_debug)
-		    fprintf_unfiltered (gdb_stdlog,
-					"{ frame_unwind_pc (this_frame=%d)"
-					" -> <unavailable> }\n",
-					this_frame->level);
-		}
-	      else if (ex.error == OPTIMIZED_OUT_ERROR)
-		{
-		  this_frame->prev_pc.status = CC_NOT_SAVED;
-
-		  if (frame_debug)
-		    fprintf_unfiltered (gdb_stdlog,
-					"{ frame_unwind_pc (this_frame=%d)"
-					" -> <not saved> }\n",
-					this_frame->level);
-		}
-	      else
-		throw_exception (ex);
-	    }
-	  END_CATCH
+	      this_frame->prev_pc.status = CC_NOT_SAVED;
 
-	  if (pc_p)
-	    {
-	      this_frame->prev_pc.value = pc;
-	      this_frame->prev_pc.status = CC_VALUE;
 	      if (frame_debug)
 		fprintf_unfiltered (gdb_stdlog,
-				    "{ frame_unwind_pc (this_frame=%d) "
-				    "-> %s }\n",
-				    this_frame->level,
-				    hex_string (this_frame->prev_pc.value));
+				    "{ frame_unwind_pc (this_frame=%d)"
+				    " -> <not saved> }\n",
+				    this_frame->level);
 	    }
+	  else
+	    throw_exception (ex);
+	}
+      END_CATCH
+
+      if (pc_p)
+	{
+	  this_frame->prev_pc.value = pc;
+	  this_frame->prev_pc.status = CC_VALUE;
+	  if (frame_debug)
+	    fprintf_unfiltered (gdb_stdlog,
+				"{ frame_unwind_pc (this_frame=%d) "
+				"-> %s }\n",
+				this_frame->level,
+				hex_string (this_frame->prev_pc.value));
 	}
-      else
-	internal_error (__FILE__, __LINE__, _("No unwind_pc method"));
     }
 
   if (this_frame->prev_pc.status == CC_VALUE)
@@ -2782,18 +2777,9 @@  get_frame_sp (struct frame_info *this_frame)
 {
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
 
-  /* Normality - an architecture that provides a way of obtaining any
-     frame inner-most address.  */
-  if (gdbarch_unwind_sp_p (gdbarch))
-    /* NOTE drow/2008-06-28: gdbarch_unwind_sp could be converted to
-       operate on THIS_FRAME now.  */
-    return gdbarch_unwind_sp (gdbarch, this_frame->next);
-  /* Now things are really are grim.  Hope that the value returned by
-     the gdbarch_sp_regnum register is meaningful.  */
-  if (gdbarch_sp_regnum (gdbarch) >= 0)
-    return get_frame_register_unsigned (this_frame,
-					gdbarch_sp_regnum (gdbarch));
-  internal_error (__FILE__, __LINE__, _("Missing unwind SP method"));
+  /* NOTE drow/2008-06-28: gdbarch_unwind_sp could be converted to
+     operate on THIS_FRAME now.  */
+  return gdbarch_unwind_sp (gdbarch, this_frame->next);
 }
 
 /* Return the reason why we can't unwind past FRAME.  */
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index e2abf263b3a..f65af109b2e 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -48,6 +48,8 @@ 
 #include "regcache.h"
 #include "objfiles.h"
 #include "auxv.h"
+#include "frame-unwind.h"
+#include "dummy-frame.h"
 
 /* Static function declarations */
 
@@ -408,6 +410,7 @@  gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->ecoff_reg_to_regnum = no_op_reg_to_regnum;
   gdbarch->sdb_reg_to_regnum = no_op_reg_to_regnum;
   gdbarch->dwarf2_reg_to_regnum = no_op_reg_to_regnum;
+  gdbarch->dummy_id = default_dummy_id;
   gdbarch->deprecated_fp_regnum = -1;
   gdbarch->call_dummy_location = AT_ENTRY_POINT;
   gdbarch->code_of_frame_writable = default_code_of_frame_writable;
@@ -427,6 +430,8 @@  gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->memory_insert_breakpoint = default_memory_insert_breakpoint;
   gdbarch->memory_remove_breakpoint = default_memory_remove_breakpoint;
   gdbarch->remote_register_number = default_remote_register_number;
+  gdbarch->unwind_pc = default_unwind_pc;
+  gdbarch->unwind_sp = default_unwind_sp;
   gdbarch->stabs_argument_has_addr = default_stabs_argument_has_addr;
   gdbarch->convert_from_func_ptr_addr = convert_from_func_ptr_addr_identity;
   gdbarch->addr_bits_remove = core_addr_identity;
@@ -570,7 +575,7 @@  verify_gdbarch (struct gdbarch *gdbarch)
   if (gdbarch->register_name == 0)
     log.puts ("\n\tregister_name");
   /* Skip verify of register_type, has predicate.  */
-  /* Skip verify of dummy_id, has predicate.  */
+  /* Skip verify of dummy_id, invalid_p == 0 */
   /* Skip verify of deprecated_fp_regnum, invalid_p == 0 */
   /* Skip verify of push_dummy_call, has predicate.  */
   /* Skip verify of call_dummy_location, invalid_p == 0 */
@@ -609,8 +614,8 @@  verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of remote_register_number, invalid_p == 0 */
   /* Skip verify of fetch_tls_load_module_address, has predicate.  */
   /* Skip verify of frame_args_skip, invalid_p == 0 */
-  /* Skip verify of unwind_pc, has predicate.  */
-  /* Skip verify of unwind_sp, has predicate.  */
+  /* Skip verify of unwind_pc, invalid_p == 0 */
+  /* Skip verify of unwind_sp, invalid_p == 0 */
   /* Skip verify of frame_num_args, has predicate.  */
   /* Skip verify of frame_align, has predicate.  */
   /* Skip verify of stabs_argument_has_addr, invalid_p == 0 */
@@ -954,9 +959,6 @@  gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   fprintf_unfiltered (file,
                       "gdbarch_dump: dtrace_probe_is_enabled = <%s>\n",
                       host_address_to_string (gdbarch->dtrace_probe_is_enabled));
-  fprintf_unfiltered (file,
-                      "gdbarch_dump: gdbarch_dummy_id_p() = %d\n",
-                      gdbarch_dummy_id_p (gdbarch));
   fprintf_unfiltered (file,
                       "gdbarch_dump: dummy_id = <%s>\n",
                       host_address_to_string (gdbarch->dummy_id));
@@ -1440,15 +1442,9 @@  gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   fprintf_unfiltered (file,
                       "gdbarch_dump: type_align = <%s>\n",
                       host_address_to_string (gdbarch->type_align));
-  fprintf_unfiltered (file,
-                      "gdbarch_dump: gdbarch_unwind_pc_p() = %d\n",
-                      gdbarch_unwind_pc_p (gdbarch));
   fprintf_unfiltered (file,
                       "gdbarch_dump: unwind_pc = <%s>\n",
                       host_address_to_string (gdbarch->unwind_pc));
-  fprintf_unfiltered (file,
-                      "gdbarch_dump: gdbarch_unwind_sp_p() = %d\n",
-                      gdbarch_unwind_sp_p (gdbarch));
   fprintf_unfiltered (file,
                       "gdbarch_dump: unwind_sp = <%s>\n",
                       host_address_to_string (gdbarch->unwind_sp));
@@ -2307,13 +2303,6 @@  set_gdbarch_register_type (struct gdbarch *gdbarch,
   gdbarch->register_type = register_type;
 }
 
-int
-gdbarch_dummy_id_p (struct gdbarch *gdbarch)
-{
-  gdb_assert (gdbarch != NULL);
-  return gdbarch->dummy_id != NULL;
-}
-
 struct frame_id
 gdbarch_dummy_id (struct gdbarch *gdbarch, struct frame_info *this_frame)
 {
@@ -3046,13 +3035,6 @@  set_gdbarch_frame_args_skip (struct gdbarch *gdbarch,
   gdbarch->frame_args_skip = frame_args_skip;
 }
 
-int
-gdbarch_unwind_pc_p (struct gdbarch *gdbarch)
-{
-  gdb_assert (gdbarch != NULL);
-  return gdbarch->unwind_pc != NULL;
-}
-
 CORE_ADDR
 gdbarch_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame)
 {
@@ -3070,13 +3052,6 @@  set_gdbarch_unwind_pc (struct gdbarch *gdbarch,
   gdbarch->unwind_pc = unwind_pc;
 }
 
-int
-gdbarch_unwind_sp_p (struct gdbarch *gdbarch)
-{
-  gdb_assert (gdbarch != NULL);
-  return gdbarch->unwind_sp != NULL;
-}
-
 CORE_ADDR
 gdbarch_unwind_sp (struct gdbarch *gdbarch, struct frame_info *next_frame)
 {
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index fc2f1a84a1c..e1a9c009f8a 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -380,8 +380,6 @@  typedef struct type * (gdbarch_register_type_ftype) (struct gdbarch *gdbarch, in
 extern struct type * gdbarch_register_type (struct gdbarch *gdbarch, int reg_nr);
 extern void set_gdbarch_register_type (struct gdbarch *gdbarch, gdbarch_register_type_ftype *register_type);
 
-extern int gdbarch_dummy_id_p (struct gdbarch *gdbarch);
-
 typedef struct frame_id (gdbarch_dummy_id_ftype) (struct gdbarch *gdbarch, struct frame_info *this_frame);
 extern struct frame_id gdbarch_dummy_id (struct gdbarch *gdbarch, struct frame_info *this_frame);
 extern void set_gdbarch_dummy_id (struct gdbarch *gdbarch, gdbarch_dummy_id_ftype *dummy_id);
@@ -622,14 +620,10 @@  extern void set_gdbarch_fetch_tls_load_module_address (struct gdbarch *gdbarch,
 extern CORE_ADDR gdbarch_frame_args_skip (struct gdbarch *gdbarch);
 extern void set_gdbarch_frame_args_skip (struct gdbarch *gdbarch, CORE_ADDR frame_args_skip);
 
-extern int gdbarch_unwind_pc_p (struct gdbarch *gdbarch);
-
 typedef CORE_ADDR (gdbarch_unwind_pc_ftype) (struct gdbarch *gdbarch, struct frame_info *next_frame);
 extern CORE_ADDR gdbarch_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame);
 extern void set_gdbarch_unwind_pc (struct gdbarch *gdbarch, gdbarch_unwind_pc_ftype *unwind_pc);
 
-extern int gdbarch_unwind_sp_p (struct gdbarch *gdbarch);
-
 typedef CORE_ADDR (gdbarch_unwind_sp_ftype) (struct gdbarch *gdbarch, struct frame_info *next_frame);
 extern CORE_ADDR gdbarch_unwind_sp (struct gdbarch *gdbarch, struct frame_info *next_frame);
 extern void set_gdbarch_unwind_sp (struct gdbarch *gdbarch, gdbarch_unwind_sp_ftype *unwind_sp);
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 670ac30c030..0e79ff19cd1 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -480,7 +480,12 @@  m;const char *;register_name;int regnr;regnr;;0
 # use "register_type".
 M;struct type *;register_type;int reg_nr;reg_nr
 
-M;struct frame_id;dummy_id;struct frame_info *this_frame;this_frame
+# Generate a dummy frame_id for THIS_FRAME assuming that the frame is
+# a dummy frame.  A dummy frame is created before an inferior call,
+# the frame_id returned here must  match the base address returned by
+# gdbarch_push_dummy_call and the frame's pc must match the dummy
+# frames breakpoint address.
+m;struct frame_id;dummy_id;struct frame_info *this_frame;this_frame;;default_dummy_id;;0
 # Implement DUMMY_ID and PUSH_DUMMY_CALL, then delete
 # deprecated_fp_regnum.
 v;int;deprecated_fp_regnum;;;-1;-1;;0
@@ -596,8 +601,8 @@  m;int;remote_register_number;int regno;regno;;default_remote_register_number;;0
 F;CORE_ADDR;fetch_tls_load_module_address;struct objfile *objfile;objfile
 #
 v;CORE_ADDR;frame_args_skip;;;0;;;0
-M;CORE_ADDR;unwind_pc;struct frame_info *next_frame;next_frame
-M;CORE_ADDR;unwind_sp;struct frame_info *next_frame;next_frame
+m;CORE_ADDR;unwind_pc;struct frame_info *next_frame;next_frame;;default_unwind_pc;;0
+m;CORE_ADDR;unwind_sp;struct frame_info *next_frame;next_frame;;default_unwind_sp;;0
 # DEPRECATED_FRAME_LOCALS_ADDRESS as been replaced by the per-frame
 # frame-base.  Enable frame-base before frame-unwind.
 F;int;frame_num_args;struct frame_info *frame;frame
@@ -1662,6 +1667,8 @@  cat <<EOF
 #include "regcache.h"
 #include "objfiles.h"
 #include "auxv.h"
+#include "frame-unwind.h"
+#include "dummy-frame.h"
 
 /* Static function declarations */