[PATCHv5] gdb: Add default frame methods to gdbarch

[Andrew Burgess]

* 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