This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC] [PATCH] Provide the ability to write the frame unwinder in Python
- From: Andy Wingo <wingo at igalia dot com>
- To: Alexander Smundak <asmundak at google dot com>
- Cc: Doug Evans <dje at google dot com>, gdb-patches <gdb-patches at sourceware dot org>
- Date: Mon, 16 Mar 2015 12:29:25 +0100
- Subject: Re: [RFC] [PATCH] Provide the ability to write the frame unwinder in Python
- Authentication-results: sourceware.org; auth=none
- References: <CAHQ51u7NUoQ8w9c5mc-Eiz05b1Nub6zqj_Ne7vfgWb5EP9_X8w at mail dot gmail dot com> <21714 dot 40641 dot 510825 dot 30998 at ruffy2 dot mtv dot corp dot google dot com> <CAHQ51u5_ViLaEmv9e43R-wzuWw8dwNkb-2XgCRy5ELQq5FUAWg at mail dot gmail dot com> <54E71694 dot 1080304 at redhat dot com> <CAHQ51u75+9HYAVJXYNQa0gTnQtYKEgmSkyAhAPYp-y4HGtXssg at mail dot gmail dot com> <CAHQ51u6UZ7A47rpGgX0QGeYSTCz1eo_3jWHc=q2ZX3YhqcJ6iQ at mail dot gmail dot com> <87ioei31uj dot fsf at igalia dot com> <CAHQ51u4f+Vx7qXPm-KAAENOceaVogMbDMw6==N_nY+GrLr4Pgg at mail dot gmail dot com> <87d24p19tt dot fsf at igalia dot com> <54FD7DAA dot 7010603 at redhat dot com> <CAHQ51u7sUkGhkmvTaaO_Jo6Jn+kojfiMWHmc2=7OWHThAq6EKw at mail dot gmail dot com> <87twxrncld dot fsf at igalia dot com> <CAHQ51u60nHp1a2DXZ4srvRefyTtge1BUw7-=JuYqChHN_wUGyQ at mail dot gmail dot com>
Hi :)
[-pmuldoon as he has already given an LGTM]
On Wed 11 Mar 2015 19:48, Alexander Smundak <asmundak@google.com> writes:
>> What do you think about merging the SnifferInfo and UnwindInfo objects
>> into an EphemeralFrame object? It would be nice to use the same nouns
>> on the Guile and Python sides.
>
> I prefer the separation of the input (SnifferInfo) and output (UnwindInfo).
> I am surprised someone from Guile side is advocating stateful interface :-)
I know what you mean, but because there should be a link between them --
in that saved register set should be checked for validity against the
architecture of the incoming frame -- then given that, joining them
makes sense to me. Otherwise you'd have to pass the SnifferInfo to the
UnwindInfo constructor, which would be fine but what if we could avoid
it by joining them...
And then there's the question of what do you call the two things --
SnifferInfo is a bad name, for starters. It's a frame, albeit some
ephemeral kind of frame. It's literally a struct frame_info* under the
hood. Currently you can read a register, but you might want to do other
frame things on it -- get_next_frame, get_frame_arch,
frame_relative_level, etc. WDYT about the name EphemeralFrame ?
> In fact, I'd rather avoid having modifiable UnwindInfo at all -- a sniffer can
> return a 2-tuple of (<previous frame registers>, <FrameId instance>),
> where FrameId instance can be created by a class method
> gdb.SnifferInfo.frame_id_build_xxx(). Can we agree on that?
Regarding the return value, I think returning a 2-tuple is a bad idea
for all of the reasons I have previously mentioned and which you have
not responded to :)
* bad error reporting (e.g. gdb.Value width doesn't match register
width, how to map that back to some source location or program
state)
* bad extensibility (e.g. what about stop_reason? what about other
register states?)
* bad ergonomics (is the frame id first, or is it the saved
registers?)
On the other hand to have to give a name the result object is painful
too -- "this is an UnwindInfo and it is composed of blah blah". For
that reason I joined the result with the ephemeral frame object, to
avoid introducing another concept. But checking "did this unwinder
succeed" via "did the unwinder set the frame ID on this ephemeral frame"
is not nice either.
How about let's meet somewhat halfway.
* We rename SnifferInfo to EphemeralFrame.
* Unwinders return UnwindInfo (better name welcome) on success or
None/#f otherwise
* UnwindInfo takes EphemeralFrame as constructor arg
- the EphemeralFrame must be valid
- in practice there is only ever one EphemeralFrame alive because
unwinding is not recursive
* UnwindInfo also takes frame ID as positional constructor args
- setting frame_id is the only thing an unwinder *must* do, so
this makes an invariant "if return value is an UnwindInfo, then
it is valid and has all necessary info"
* UnwindInfo has add_saved_register() API (see discussion with Pedro
here: http://article.gmane.org/gmane.comp.gdb.patches/105538)
* After accepting an UnwindInfo as an unwinder return result,
py-unwinders.c / scm-frame-unwinders.c marks the UnwindInfo as
frozen so that add_saved_register() etc can't alter the state
* continuation of unwinder call also checks that the ephemeral frame
on the unwindinfo is valid
Example of use:
def unwind(frame):
if we_can_handle(frame):
var ret = UnwindInfo(frame, fp, pc)
ret.add_saved_register(r0)
return ret
I will rework the Guile patch along these lines, and then hopefully I am
done reworking :)
Andy