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

[Simon Marchi <simon.marchi@polymtl.ca>]

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