From: Simon Marchi <simon.marchi@polymtl.ca>
To: Andrew Burgess <andrew.burgess@embecosm.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCHv5] gdb: Add default frame methods to gdbarch
Date: Wed, 19 Dec 2018 18:37:00 -0000 [thread overview]
Message-ID: <198ef7c34561a818fdd4792477dbf135@polymtl.ca> (raw)
In-Reply-To: <20181219183248.GA3456@embecosm.com>
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
prev parent reply other threads:[~2018-12-19 18:37 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-08 21:44 Andrew Burgess
2018-12-12 14:28 ` PING: " Andrew Burgess
2018-12-14 22:22 ` Simon Marchi
2018-12-19 18:32 ` Andrew Burgess
2018-12-19 18:37 ` Simon Marchi [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=198ef7c34561a818fdd4792477dbf135@polymtl.ca \
--to=simon.marchi@polymtl.ca \
--cc=andrew.burgess@embecosm.com \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).