[PATCHv5] gdb: Add default frame methods to gdbarch

  This is another iteration of a patch that I last posted here:

  https://sourceware.org/ml/gdb-patches/2018-09/msg00127.html

previous versions can be found here:

  https://sourceware.org/ml/gdb-patches/2018-06/msg00090.html
  https://sourceware.org/ml/gdb-patches/2018-03/msg00306.html
  https://sourceware.org/ml/gdb-patches/2018-03/msg00165.html

I believe v4 actually got approved, but, when I went to merge it I
realised I'd made a mistake!

This new version is mostly the same as v4, but the default_unwind_pc
method is simpler, but will, I think, still cover almost all targets.

As far as testing, I don't believe that any target will be using these
default methods after this initial commit, however, I did a build with
all targets enabled, and it built fine.

Out of tree I've tested this on the RISC-V target, with no regressions.

Is this OK to apply?

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.

The implementations for default_dummy_id and default_unwind_sp are
fairly straight forward, these just take on the pattern used by most
targets.  Once these default methods are in place then most targets
will be able to switch over.

The implementation for default_unwind_pc is also fairly straight
forward, but maybe needs some explanation.

This patch has gone through a number of iterations:

  https://sourceware.org/ml/gdb-patches/2018-03/msg00165.html
  https://sourceware.org/ml/gdb-patches/2018-03/msg00306.html
  https://sourceware.org/ml/gdb-patches/2018-06/msg00090.html
  https://sourceware.org/ml/gdb-patches/2018-09/msg00127.html

and the implementation of default_unwind_pc has changed over this
time.  Originally, I took an implementation like this:

    CORE_ADDR
    default_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame)
    {
      int pc_regnum = gdbarch_pc_regnum (gdbarch);
      return frame_unwind_register_unsigned (next_frame, pc_regnum);
    }

This is basically a clone of default_unwind_sp, but using $pc.  It was
pointed out that we could potentially do better, and in version 2 the
implementation became:

    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);
      release_value (value);
      value_free (value);
      return addr;
    }

The idea was to try split out some of the steps of unwinding the $pc,
steps that are on some (or many) targets no-ops, and so allow targets
that do override these methods, to make use of default_unwind_pc.

This implementation remained in place for version 2, 3, and 4.

However, I realised that I'd made a mistake, most targets simply use
frame_unwind_register_unsigned to unwind the $pc, and this throws an
error if the register value is optimized out or unavailable.  My new
proposed implementation doesn't do this, I was going to end up
breaking many targets.

I considered duplicating the code from frame_unwind_register_unsigned
that throws the errors into my new default_unwind_pc, however, this
felt really overly complex.  So, what I instead went with was to
simply revert back to using frame_unwind_register_unsigned.  Almost
all existing targets already use this. Some of the ones that don't can
be converted to, which means almost all targets could end up using the
default.

One addition I have made over the version 1 implementation is to add a
call to gdbarch_addr_bits_remove.  For most targets this is a no-op,
but for a handful, having this call in place will mean that they can
use the default method.  After all this, the new default_unwind_pc now
looks like this:

    CORE_ADDR
    default_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame)
    {
      int pc_regnum = gdbarch_pc_regnum (gdbarch);
      CORE_ADDR pc = frame_unwind_register_unsigned (next_frame, pc_regnum);
      pc = gdbarch_addr_bits_remove (gdbarch, pc);
      return pc;
    }

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 |  20 ++++++++
 gdb/frame-unwind.h |  12 +++++
 gdb/frame.c        | 132 ++++++++++++++++++++++++-----------------------------
 gdb/gdbarch.c      |  41 ++++-------------
 gdb/gdbarch.h      |   6 ---
 gdb/gdbarch.sh     |  13 ++++--
 9 files changed, 144 insertions(+), 115 deletions(-)
  

Ping!

Thanks,
Andrew

* Andrew Burgess <andrew.burgess@embecosm.com> [2018-11-08 21:43:57 +0000]:

> This is another iteration of a patch that I last posted here:
> 
>   https://sourceware.org/ml/gdb-patches/2018-09/msg00127.html
> 
> previous versions can be found here:
> 
>   https://sourceware.org/ml/gdb-patches/2018-06/msg00090.html
>   https://sourceware.org/ml/gdb-patches/2018-03/msg00306.html
>   https://sourceware.org/ml/gdb-patches/2018-03/msg00165.html
> 
> I believe v4 actually got approved, but, when I went to merge it I
> realised I'd made a mistake!
> 
> This new version is mostly the same as v4, but the default_unwind_pc
> method is simpler, but will, I think, still cover almost all targets.
> 
> As far as testing, I don't believe that any target will be using these
> default methods after this initial commit, however, I did a build with
> all targets enabled, and it built fine.
> 
> Out of tree I've tested this on the RISC-V target, with no regressions.
> 
> Is this OK to apply?
> 
> 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.
> 
> The implementations for default_dummy_id and default_unwind_sp are
> fairly straight forward, these just take on the pattern used by most
> targets.  Once these default methods are in place then most targets
> will be able to switch over.
> 
> The implementation for default_unwind_pc is also fairly straight
> forward, but maybe needs some explanation.
> 
> This patch has gone through a number of iterations:
> 
>   https://sourceware.org/ml/gdb-patches/2018-03/msg00165.html
>   https://sourceware.org/ml/gdb-patches/2018-03/msg00306.html
>   https://sourceware.org/ml/gdb-patches/2018-06/msg00090.html
>   https://sourceware.org/ml/gdb-patches/2018-09/msg00127.html
> 
> and the implementation of default_unwind_pc has changed over this
> time.  Originally, I took an implementation like this:
> 
>     CORE_ADDR
>     default_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame)
>     {
>       int pc_regnum = gdbarch_pc_regnum (gdbarch);
>       return frame_unwind_register_unsigned (next_frame, pc_regnum);
>     }
> 
> This is basically a clone of default_unwind_sp, but using $pc.  It was
> pointed out that we could potentially do better, and in version 2 the
> implementation became:
> 
>     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);
>       release_value (value);
>       value_free (value);
>       return addr;
>     }
> 
> The idea was to try split out some of the steps of unwinding the $pc,
> steps that are on some (or many) targets no-ops, and so allow targets
> that do override these methods, to make use of default_unwind_pc.
> 
> This implementation remained in place for version 2, 3, and 4.
> 
> However, I realised that I'd made a mistake, most targets simply use
> frame_unwind_register_unsigned to unwind the $pc, and this throws an
> error if the register value is optimized out or unavailable.  My new
> proposed implementation doesn't do this, I was going to end up
> breaking many targets.
> 
> I considered duplicating the code from frame_unwind_register_unsigned
> that throws the errors into my new default_unwind_pc, however, this
> felt really overly complex.  So, what I instead went with was to
> simply revert back to using frame_unwind_register_unsigned.  Almost
> all existing targets already use this. Some of the ones that don't can
> be converted to, which means almost all targets could end up using the
> default.
> 
> One addition I have made over the version 1 implementation is to add a
> call to gdbarch_addr_bits_remove.  For most targets this is a no-op,
> but for a handful, having this call in place will mean that they can
> use the default method.  After all this, the new default_unwind_pc now
> looks like this:
> 
>     CORE_ADDR
>     default_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame)
>     {
>       int pc_regnum = gdbarch_pc_regnum (gdbarch);
>       CORE_ADDR pc = frame_unwind_register_unsigned (next_frame, pc_regnum);
>       pc = gdbarch_addr_bits_remove (gdbarch, pc);
>       return pc;
>     }
> 
> 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 |  20 ++++++++
>  gdb/frame-unwind.h |  12 +++++
>  gdb/frame.c        | 132 ++++++++++++++++++++++++-----------------------------
>  gdb/gdbarch.c      |  41 ++++-------------
>  gdb/gdbarch.h      |   6 ---
>  gdb/gdbarch.sh     |  13 ++++--
>  9 files changed, 144 insertions(+), 115 deletions(-)
> 
> 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..fb0e5e83213 100644
> --- a/gdb/frame-unwind.c
> +++ b/gdb/frame-unwind.c
> @@ -193,6 +193,26 @@ 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);
> +  CORE_ADDR pc = frame_unwind_register_unsigned (next_frame, pc_regnum);
> +  pc = gdbarch_addr_bits_remove (gdbarch, pc);
> +  return pc;
> +}
> +
> +/* See frame-unwind.h.  */
> +
> +CORE_ADDR
> +default_unwind_sp (struct gdbarch *gdbarch, struct frame_info *next_frame)
> +{
> +  int 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 4624eada87b..f98a54a9e0f 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 2cb69610837..e8063922725 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 bbfa8d22058..96afb01c098 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
> @@ -1670,6 +1675,8 @@ cat <<EOF
>  #include "regcache.h"
>  #include "objfiles.h"
>  #include "auxv.h"
> +#include "frame-unwind.h"
> +#include "dummy-frame.h"
>  
>  /* Static function declarations */
>  
> -- 
> 2.14.5
>
  

On 2018-11-08 16:43, Andrew Burgess wrote:
> This is another iteration of a patch that I last posted here:
> 
>   https://sourceware.org/ml/gdb-patches/2018-09/msg00127.html
> 
> previous versions can be found here:
> 
>   https://sourceware.org/ml/gdb-patches/2018-06/msg00090.html
>   https://sourceware.org/ml/gdb-patches/2018-03/msg00306.html
>   https://sourceware.org/ml/gdb-patches/2018-03/msg00165.html
> 
> I believe v4 actually got approved, but, when I went to merge it I
> realised I'd made a mistake!
> 
> This new version is mostly the same as v4, but the default_unwind_pc
> method is simpler, but will, I think, still cover almost all targets.
> 
> As far as testing, I don't believe that any target will be using these
> default methods after this initial commit, however, I did a build with
> all targets enabled, and it built fine.
> 
> Out of tree I've tested this on the RISC-V target, with no regressions.
> 
> Is this OK to apply?
> 
> 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.
> 
> The implementations for default_dummy_id and default_unwind_sp are
> fairly straight forward, these just take on the pattern used by most
> targets.  Once these default methods are in place then most targets
> will be able to switch over.
> 
> The implementation for default_unwind_pc is also fairly straight
> forward, but maybe needs some explanation.
> 
> This patch has gone through a number of iterations:
> 
>   https://sourceware.org/ml/gdb-patches/2018-03/msg00165.html
>   https://sourceware.org/ml/gdb-patches/2018-03/msg00306.html
>   https://sourceware.org/ml/gdb-patches/2018-06/msg00090.html
>   https://sourceware.org/ml/gdb-patches/2018-09/msg00127.html
> 
> and the implementation of default_unwind_pc has changed over this
> time.  Originally, I took an implementation like this:
> 
>     CORE_ADDR
>     default_unwind_pc (struct gdbarch *gdbarch, struct frame_info 
> *next_frame)
>     {
>       int pc_regnum = gdbarch_pc_regnum (gdbarch);
>       return frame_unwind_register_unsigned (next_frame, pc_regnum);
>     }
> 
> This is basically a clone of default_unwind_sp, but using $pc.  It was
> pointed out that we could potentially do better, and in version 2 the
> implementation became:
> 
>     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);
>       release_value (value);
>       value_free (value);
>       return addr;
>     }
> 
> The idea was to try split out some of the steps of unwinding the $pc,
> steps that are on some (or many) targets no-ops, and so allow targets
> that do override these methods, to make use of default_unwind_pc.
> 
> This implementation remained in place for version 2, 3, and 4.
> 
> However, I realised that I'd made a mistake, most targets simply use
> frame_unwind_register_unsigned to unwind the $pc, and this throws an
> error if the register value is optimized out or unavailable.  My new
> proposed implementation doesn't do this, I was going to end up
> breaking many targets.
> 
> I considered duplicating the code from frame_unwind_register_unsigned
> that throws the errors into my new default_unwind_pc, however, this
> felt really overly complex.  So, what I instead went with was to
> simply revert back to using frame_unwind_register_unsigned.  Almost
> all existing targets already use this. Some of the ones that don't can
> be converted to, which means almost all targets could end up using the
> default.
> 
> One addition I have made over the version 1 implementation is to add a
> call to gdbarch_addr_bits_remove.  For most targets this is a no-op,
> but for a handful, having this call in place will mean that they can
> use the default method.  After all this, the new default_unwind_pc now
> looks like this:
> 
>     CORE_ADDR
>     default_unwind_pc (struct gdbarch *gdbarch, struct frame_info 
> *next_frame)
>     {
>       int pc_regnum = gdbarch_pc_regnum (gdbarch);
>       CORE_ADDR pc = frame_unwind_register_unsigned (next_frame, 
> pc_regnum);
>       pc = gdbarch_addr_bits_remove (gdbarch, pc);
>       return pc;
>     }

That is fine with me.

The patch LGTM, except the comment in gdbarch.sh, could we clarify it a 
bit?

> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index bbfa8d22058..96afb01c098 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

double space

> +# gdbarch_push_dummy_call and the frame's pc must match the dummy
> +# frames breakpoint address.

I have a bit of trouble following what must match what.  Do you mean 
that:

- the returned frame_id's stack_addr must match the base address 
returned by gdbarch_push_dummy_call
- the returned frame_id's code_addr must match the frame's breakpoint 
address

I'm not sure what "frame's breakpoint address" mean, I guess it's the 
same as the frame's pc?  i.e. the pc we will jump back to when we go 
back to executing this frame, which in the case of the dummy frame, is 
where we have put a breakpoint?

Simon
  

* Simon Marchi <simon.marchi@polymtl.ca> [2018-12-14 17:22:13 -0500]:

> On 2018-11-08 16:43, Andrew Burgess wrote:
> > This is another iteration of a patch that I last posted here:
> > 
> >   https://sourceware.org/ml/gdb-patches/2018-09/msg00127.html
> > 
> > previous versions can be found here:
> > 
> >   https://sourceware.org/ml/gdb-patches/2018-06/msg00090.html
> >   https://sourceware.org/ml/gdb-patches/2018-03/msg00306.html
> >   https://sourceware.org/ml/gdb-patches/2018-03/msg00165.html
> > 
> > I believe v4 actually got approved, but, when I went to merge it I
> > realised I'd made a mistake!
> > 
> > This new version is mostly the same as v4, but the default_unwind_pc
> > method is simpler, but will, I think, still cover almost all targets.
> > 
> > As far as testing, I don't believe that any target will be using these
> > default methods after this initial commit, however, I did a build with
> > all targets enabled, and it built fine.
> > 
> > Out of tree I've tested this on the RISC-V target, with no regressions.
> > 
> > Is this OK to apply?
> > 
> > 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.
> > 
> > The implementations for default_dummy_id and default_unwind_sp are
> > fairly straight forward, these just take on the pattern used by most
> > targets.  Once these default methods are in place then most targets
> > will be able to switch over.
> > 
> > The implementation for default_unwind_pc is also fairly straight
> > forward, but maybe needs some explanation.
> > 
> > This patch has gone through a number of iterations:
> > 
> >   https://sourceware.org/ml/gdb-patches/2018-03/msg00165.html
> >   https://sourceware.org/ml/gdb-patches/2018-03/msg00306.html
> >   https://sourceware.org/ml/gdb-patches/2018-06/msg00090.html
> >   https://sourceware.org/ml/gdb-patches/2018-09/msg00127.html
> > 
> > and the implementation of default_unwind_pc has changed over this
> > time.  Originally, I took an implementation like this:
> > 
> >     CORE_ADDR
> >     default_unwind_pc (struct gdbarch *gdbarch, struct frame_info
> > *next_frame)
> >     {
> >       int pc_regnum = gdbarch_pc_regnum (gdbarch);
> >       return frame_unwind_register_unsigned (next_frame, pc_regnum);
> >     }
> > 
> > This is basically a clone of default_unwind_sp, but using $pc.  It was
> > pointed out that we could potentially do better, and in version 2 the
> > implementation became:
> > 
> >     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);
> >       release_value (value);
> >       value_free (value);
> >       return addr;
> >     }
> > 
> > The idea was to try split out some of the steps of unwinding the $pc,
> > steps that are on some (or many) targets no-ops, and so allow targets
> > that do override these methods, to make use of default_unwind_pc.
> > 
> > This implementation remained in place for version 2, 3, and 4.
> > 
> > However, I realised that I'd made a mistake, most targets simply use
> > frame_unwind_register_unsigned to unwind the $pc, and this throws an
> > error if the register value is optimized out or unavailable.  My new
> > proposed implementation doesn't do this, I was going to end up
> > breaking many targets.
> > 
> > I considered duplicating the code from frame_unwind_register_unsigned
> > that throws the errors into my new default_unwind_pc, however, this
> > felt really overly complex.  So, what I instead went with was to
> > simply revert back to using frame_unwind_register_unsigned.  Almost
> > all existing targets already use this. Some of the ones that don't can
> > be converted to, which means almost all targets could end up using the
> > default.
> > 
> > One addition I have made over the version 1 implementation is to add a
> > call to gdbarch_addr_bits_remove.  For most targets this is a no-op,
> > but for a handful, having this call in place will mean that they can
> > use the default method.  After all this, the new default_unwind_pc now
> > looks like this:
> > 
> >     CORE_ADDR
> >     default_unwind_pc (struct gdbarch *gdbarch, struct frame_info
> > *next_frame)
> >     {
> >       int pc_regnum = gdbarch_pc_regnum (gdbarch);
> >       CORE_ADDR pc = frame_unwind_register_unsigned (next_frame,
> > pc_regnum);
> >       pc = gdbarch_addr_bits_remove (gdbarch, pc);
> >       return pc;
> >     }
> 
> That is fine with me.
> 
> The patch LGTM, except the comment in gdbarch.sh, could we clarify it a bit?
> 
> > diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> > index bbfa8d22058..96afb01c098 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
> 
> double space
> 
> > +# gdbarch_push_dummy_call and the frame's pc must match the dummy
> > +# frames breakpoint address.
> 
> I have a bit of trouble following what must match what.  Do you mean that:
> 
> - the returned frame_id's stack_addr must match the base address returned by
> gdbarch_push_dummy_call
> - the returned frame_id's code_addr must match the frame's breakpoint
> address
> 
> I'm not sure what "frame's breakpoint address" mean, I guess it's the same
> as the frame's pc?  i.e. the pc we will jump back to when we go back to
> executing this frame, which in the case of the dummy frame, is where we have
> put a breakpoint?

Thanks for the feedback.  The original comment was lifted from some
target's dummy_id method.  Your interpretation is correct.  I've tried
to reword it to make the comment clearer, but I'm not totally sure
this is better.  What do you think?

  diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
  index a876a21555e..845d9e27e4c 100755
  --- a/gdb/gdbarch.sh
  +++ b/gdb/gdbarch.sh
  @@ -480,7 +480,15 @@ 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 frame_id that was built
  +# for the inferior call.  Usually this means the returned frame_id's
  +# stack address should match the address returned by
  +# gdbarch_push_dummy_call, and the retuned frame_id's code address
  +# should match the address at which the breakpoint was set in the dummy
  +# frame.
  +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

Thanks,
Andrew
  

On 2018-12-19 13:32, Andrew Burgess wrote:
> * Simon Marchi <simon.marchi@polymtl.ca> [2018-12-14 17:22:13 -0500]:
> 
>> On 2018-11-08 16:43, Andrew Burgess wrote:
>> > This is another iteration of a patch that I last posted here:
>> >
>> >   https://sourceware.org/ml/gdb-patches/2018-09/msg00127.html
>> >
>> > previous versions can be found here:
>> >
>> >   https://sourceware.org/ml/gdb-patches/2018-06/msg00090.html
>> >   https://sourceware.org/ml/gdb-patches/2018-03/msg00306.html
>> >   https://sourceware.org/ml/gdb-patches/2018-03/msg00165.html
>> >
>> > I believe v4 actually got approved, but, when I went to merge it I
>> > realised I'd made a mistake!
>> >
>> > This new version is mostly the same as v4, but the default_unwind_pc
>> > method is simpler, but will, I think, still cover almost all targets.
>> >
>> > As far as testing, I don't believe that any target will be using these
>> > default methods after this initial commit, however, I did a build with
>> > all targets enabled, and it built fine.
>> >
>> > Out of tree I've tested this on the RISC-V target, with no regressions.
>> >
>> > Is this OK to apply?
>> >
>> > 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.
>> >
>> > The implementations for default_dummy_id and default_unwind_sp are
>> > fairly straight forward, these just take on the pattern used by most
>> > targets.  Once these default methods are in place then most targets
>> > will be able to switch over.
>> >
>> > The implementation for default_unwind_pc is also fairly straight
>> > forward, but maybe needs some explanation.
>> >
>> > This patch has gone through a number of iterations:
>> >
>> >   https://sourceware.org/ml/gdb-patches/2018-03/msg00165.html
>> >   https://sourceware.org/ml/gdb-patches/2018-03/msg00306.html
>> >   https://sourceware.org/ml/gdb-patches/2018-06/msg00090.html
>> >   https://sourceware.org/ml/gdb-patches/2018-09/msg00127.html
>> >
>> > and the implementation of default_unwind_pc has changed over this
>> > time.  Originally, I took an implementation like this:
>> >
>> >     CORE_ADDR
>> >     default_unwind_pc (struct gdbarch *gdbarch, struct frame_info
>> > *next_frame)
>> >     {
>> >       int pc_regnum = gdbarch_pc_regnum (gdbarch);
>> >       return frame_unwind_register_unsigned (next_frame, pc_regnum);
>> >     }
>> >
>> > This is basically a clone of default_unwind_sp, but using $pc.  It was
>> > pointed out that we could potentially do better, and in version 2 the
>> > implementation became:
>> >
>> >     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);
>> >       release_value (value);
>> >       value_free (value);
>> >       return addr;
>> >     }
>> >
>> > The idea was to try split out some of the steps of unwinding the $pc,
>> > steps that are on some (or many) targets no-ops, and so allow targets
>> > that do override these methods, to make use of default_unwind_pc.
>> >
>> > This implementation remained in place for version 2, 3, and 4.
>> >
>> > However, I realised that I'd made a mistake, most targets simply use
>> > frame_unwind_register_unsigned to unwind the $pc, and this throws an
>> > error if the register value is optimized out or unavailable.  My new
>> > proposed implementation doesn't do this, I was going to end up
>> > breaking many targets.
>> >
>> > I considered duplicating the code from frame_unwind_register_unsigned
>> > that throws the errors into my new default_unwind_pc, however, this
>> > felt really overly complex.  So, what I instead went with was to
>> > simply revert back to using frame_unwind_register_unsigned.  Almost
>> > all existing targets already use this. Some of the ones that don't can
>> > be converted to, which means almost all targets could end up using the
>> > default.
>> >
>> > One addition I have made over the version 1 implementation is to add a
>> > call to gdbarch_addr_bits_remove.  For most targets this is a no-op,
>> > but for a handful, having this call in place will mean that they can
>> > use the default method.  After all this, the new default_unwind_pc now
>> > looks like this:
>> >
>> >     CORE_ADDR
>> >     default_unwind_pc (struct gdbarch *gdbarch, struct frame_info
>> > *next_frame)
>> >     {
>> >       int pc_regnum = gdbarch_pc_regnum (gdbarch);
>> >       CORE_ADDR pc = frame_unwind_register_unsigned (next_frame,
>> > pc_regnum);
>> >       pc = gdbarch_addr_bits_remove (gdbarch, pc);
>> >       return pc;
>> >     }
>> 
>> That is fine with me.
>> 
>> The patch LGTM, except the comment in gdbarch.sh, could we clarify it 
>> a bit?
>> 
>> > diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
>> > index bbfa8d22058..96afb01c098 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
>> 
>> double space
>> 
>> > +# gdbarch_push_dummy_call and the frame's pc must match the dummy
>> > +# frames breakpoint address.
>> 
>> I have a bit of trouble following what must match what.  Do you mean 
>> that:
>> 
>> - the returned frame_id's stack_addr must match the base address 
>> returned by
>> gdbarch_push_dummy_call
>> - the returned frame_id's code_addr must match the frame's breakpoint
>> address
>> 
>> I'm not sure what "frame's breakpoint address" mean, I guess it's the 
>> same
>> as the frame's pc?  i.e. the pc we will jump back to when we go back 
>> to
>> executing this frame, which in the case of the dummy frame, is where 
>> we have
>> put a breakpoint?
> 
> Thanks for the feedback.  The original comment was lifted from some
> target's dummy_id method.  Your interpretation is correct.  I've tried
> to reword it to make the comment clearer, but I'm not totally sure
> this is better.  What do you think?
> 
>   diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
>   index a876a21555e..845d9e27e4c 100755
>   --- a/gdb/gdbarch.sh
>   +++ b/gdb/gdbarch.sh
>   @@ -480,7 +480,15 @@ 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 frame_id that was built
>   +# for the inferior call.  Usually this means the returned frame_id's
>   +# stack address should match the address returned by
>   +# gdbarch_push_dummy_call, and the retuned frame_id's code address

"retuned"

>   +# should match the address at which the breakpoint was set in the 
> dummy
>   +# frame.
>   +m;struct frame_id;dummy_id;struct frame_info

Thanks, this is fine with me, please push.

Simon