Re: [PATCHv4] gdb: Add default frame methods to gdbarch

[Simon Marchi <simon dot marchi at ericsson dot com>]

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?

> +  return addr;
> +}
> +
> +/* Default unwind of the SP.  */

Use /* See frame-unwind.h.  */

Thanks,

Simon