Re: [RFC] [PATCH] Provide the ability to write the frame unwinder in Python

[Andy Wingo <wingo@igalia.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