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

[Doug Evans <dje at google dot com>]

Alexander Smundak writes:
 > > In python, let's use EphemeralFrame instead of SnifferInfo.
 > 
 > I'd rather have InspectedFrame than EphemeralFrame,
 > but it's fine with me if it expedites the review.

Andy suggested PendingFrame.
Let's go with that.
It is a frame, but not a "real" one yet.

 > > 3) We need to allow sniffers to record anything they
 > > want in gdb.UnwindInfo/<gdb:unwind-info>. In Python
 > > I'm guessing one can just add extra attributes to the
 > > object or subclass. In Guile I guess one could use
 > > an external container (weakly?) indexed by the <gdb:unwind_info>
 > > object. In both cases the documentation should
 > > make recommendations to the reader.
 > > [If it does and I missed it apologies.]
 > 
 > It isn't a big issue for the JVM unwinder to do the lookups
 > twice (first in unwinder, then in the frame filter), but it might
 > be expensive in other languages. Not sure there is a need for
 > multiple attributes -- wouldn't a single attribute whose value is
 > an object suffice?

I was looking through all the internal unwinders in gdb.
Some of them cache more than just registers.
The extension languages don't export the stop_reason, this_id
or prev_register API calls, but some day they might,
and I want to make sure that we don't make it difficult
to do so later. But (3) was written early, I should have
edited it out so disregard.

 > > 5) The docs don't make any mention of target endianness
 > > in the saved register values. They probably should.
 > Register values passed are gdb.Value instances, so I thought
 > that if they are created using the official interface (get inspected
 > frame's register, call its `cast' method to cast it to inferior's memory
 > pointer. then call `dereference' we wolud be fine.

We can't control how the user will construct the value.
We can only provide guidance/rules on what will work.
The value has to be the same size as the register,
but we don't say that anywhere either (we should).

 > > 6) I noticed several routines for building frame ids in Python.
 > > Both Python and Guile support keyword based arguments to functions.
 > > Can we just have one frame id builder function and each take
 > > sp and pc (and special if we need to) as keyword arguments?
 > > E.g. (make-unwind-info ephem-frame #:sp sp #:pc pc)
 > > But see (7) below.
 > >
 > > 7) If we ever need to use frame ids in the extension language
 > > IWBN if they were their own object, in which case we might have
 > > (make-unwind-info ephem-frame #:frame-id frame-id)
 > > but then I'm wondering if there should be an
 > > unwind-info-set-frame-id! function and move sp,pc there.
 > >
 > > IOW change make-unwind-info to this so that the various
 > > ways of creating a frame idea are don't complicate make-unwind-info.
 > > [I realize one might want to not let make-unwind-info return
 > > an object without a frame id, but I don't see this as a problem.
 > > uwscm_sniffer could verify a frame id is present.]
 > >
 > > (let ((unwind-info (make-unwind-info ephem-frame)))
 > >   (unwind-info-set-frame-id! #:sp sp #:pc pc)
 > >   (unwind-info-add-saved-register! unwind-info "rip" rip)
 > >   ...)
 > >
 > > And for python:
 > >
 > > I'm not sure where to put the UnwindInfo creator (factory method).
 > > Do we need one?  Can we just create them via normal object construction?
 > We can (I believe penultimate patch had UnwindInfo constructor and frame_id
 > methods). The advantage of the factory method is that it's impossible to create
 > an UnwindInfo which lacks frame ID.

The constructor could take a frame ID too.

One problem I have is that while frame IDs are just sp,pc,special
today who knows what they will have tomorrow. Hopefully we could
just use special, but I wouldn't assume that.
If we separate out frame ID construction from UnwindInfo construction
then we can grow frame IDs without having to complicate UnwindInfo.
The complexity lives where it should live: in the FrameID constructor.

 > > unwind_info = gdb.UnwindInfo(ephem_frame)
 > > unwind_info.set_frame_id(...)
 > > unwind_info.set_previous_frame_register(...)
 > >
 > > This could all be done in pure Python (I think), and then
 > > pyuw_sniffer could process the object looking for expected
 > > members(attributes) with expected types (and throw an error
 > > if there's a problem).
 > This way the error checking happens on return. If we have several
 > unwinders, it will not be obvious which one sniffer the frame and
 > returned bad UnwindInfo.

Good point, but there's a general problem here: E.g., What if
the sniffer created a frame id with sp and pc swapped?
Or what if the pc got errantly elided?

I recognize the goal of detecting errors at construction time,
but in this case I think you're only solving one piece of a
larger problem. If you feel it's important in this case I
don't make making the frame id a required argument to the
UnwindInfo constructor, but let's pass it an object of type FrameID.

I have similar problems with pretty-printers btw: sometimes
I don't always know which printer got selected.
We need to provide ways to debug these kinds of problems.

For debugging purposes IWBN if the result had a backlink to
its creator. Then any error messages could reference the creator.
Thoughts?

Also, I was thinking of using "set debug foo" options, but those are for
maintainers and these aren't gdb bugs. OTOH "set debug pretty-printer"
is appealing. It's harder for unwinders because we need a unique name
for extension language provided unwinders: we don't want the option
to also turn on debugging of gdb's internal unwinders (it's likely
just noise to the user).

set debug python unwinder?
set python debug unwinder?

I kinda like leaving "set debug foo" for internal gdb usage,
and we do already have set python foo, so "set python debug unwinder"
is what I'm proposing.
I thought of names not containing the word python/guile
but in this case we know which extension language we're working in.
The code that looped over all unwinders could check this parameter
and if set print the name of winning unwinder before it returns.
There's no need to complicate the current patch with this
additional functionality though, we can add it as a follow-on
patch if you want.

 > > At the least let's combine unwind_info_with_id, frame_id_build_special,
 > > and frame_id_build_wild into one function that takes keyword
 > > arguments for each of sp, pc, special.
 > This will loose the association with the underlying GDB API,
 > but I am fine with this.

Cool.