This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Responding on this thread to Andy Wingo's comments
(see https://sourceware.org/ml/gdb-patches/2015-03/msg00037.html):
Doug & Phil, please comment.

On Mon, Mar 2, 2015 at 5:28 AM, Andy Wingo <wingo@igalia.com> wrote:
> Hi,
>
> I have a local patch that allows me to unwind frames in Guile, loosely
> modelled off of Alexander's patch that does the same in Python.  I've
> been working on getting an unwinder for V8 that uses this interface, but
> it is challenging for a number of reasons that seem more to do with GDB
> than with the extension interface.
>
> First I have some comments about the Python patch; apologies for not
> threading, as I wasn't on the list before.  I think the interface is a
> bit strange.  To sum up, the interface looks like this:
>
>     # private helper
>     def execute_sniffers(sniffer_info):
>         for sniffer in enabled_sniffers:
>             unwind_info = sniffer(sniffer_info)
>             if unwind_info:
>                 return unwind_info
>
> "sniffer_info" is a new data type, a sniffer info object, which exposes
> only a read_register() API.  The return is a (REGISTERS, FRAME_ID)
> tuple, or None if the sniffer didn't handle the frame.  REGISTERS and
> FRAME_ID are both built out of tuples and lists.
>
> To me it seems that this interface does not lend itself to good error
> reporting.  An API that instead built a specialized unwind_info object
> and called set_frame_id(), set_register() etc on it would simplify many
> things.
>
> The return value type is also not very extensible.  If it were an
> object, it would be more future-proof.  For example, what about the
> stop_reason ?

The rationale for the current interface is that the object returned
by a Python sniffer is immediately consumed, so it's not worth
providing any complicated machinery for it. I wasn't thinking about
future-proofing much.

So here's the new proposal for the Python API, hopefully in
line with what you have in mind for Guile:

If a sniffer is able to unwind a frame, it should return an instance of
gdb.sniffer.UnwindInfo class, which has the following methods:
* UnwindInfo(registers)
  Constructor. `registers' is a tuple of (register_number, register_value)
  2-tuples for the registers that can be unwound.
* frame_id_build_wild(SP)
  frame_id_build(SP, PC)
  frame_id_build_special(SP, PC, SPECIAL)
  Sets frame ID by calling the corresponding GDB function. It is an error
  to return UnwindInfo object before one of these methods is called (a
  sniffer should return None if it cannot unwind a frame)
* set_register(register_number, register_value)
  Adds a 2-tuple to the list of unwound registers. Not sure this is needed.

Do we agree on this?

> Exposing register numbers in the API does not makes sense to me, as most
> users won't know those register numbers.  It shouldn't be necessary to
> open up GDB's C source to implement an unwinder.  Instead why not
> specify registers as strings, as elsewhere
> (e.g. gdb.Frame.read_register)?
My concern is that name lookups are expensive, and 'SnifferInfo.read_register'
will be called by each Python sniffer at least once. Then it makes sense to
have UnwindInfo use register numbers, too, for consistency.
In https://sourceware.org/ml/gdb-patches/2015-02/msg00329.html
I am proposing a tradeoff: add
`gdb.Architecture.register_name_to_number' method.
On the Python side, register number values can then be initialized
during architecture-specific sniffer state initialization.

> The sniffer_info object is unfortunate -- it's a frame, but without
> frame methods.  You can't get its architecture from python, for
> example, or get the next frame.  More about that later.
I guess you know by now that it is not a frame. The interface
reflects that.

> In your patch, the sniffer_info object could outlive the call to the
> sniffer, and so AFAICT it needs to be nulled out afterwards, as its
> internal frame_info pointer will become invalid.
Yes, Doug has mentioned that already, and I forgot to do that. Will do
in the next revision.

> In the read_register() function, I believe you can use
> get_frame_register_value instead of deprecated_frame_register_read.
You can't, get frame_register_value wiil assert because the frame
has no frame ID yet.

>                           *   *   *
> As a general comment, separation of frame decorators from unwinders
> causes duplicate work.  For example in V8 I will need to find the
> v8::internal::Code* object that corresponds to the frame in order to
> unwind the frame, but I will also need it for getting the line number.
> I can't cache it in a sensible way because GC could free or move the
> code.  (I suppose I could attach to GC-related breakpoints and
> invalidate a GDB-side cache, but that makes the GDB extension more
> brittle.)

> I don't know if there is a good solution here, though.
>                           *   *   *
>
> I have a similar interface in Scheme.  The Scheme unwinder constructs an
> "ephemeral frame object", which wraps this data:
>
>     /* Data associated with a ephemeral frame.  */
>     struct uwscm_ephemeral_frame
>     {
>       /* The frame being unwound, used for the read-register interface.  */
>       struct frame_info *this_frame;
>
>       /* The architecture of the frame, here for convenience.  */
>       struct gdbarch *gdbarch;
>
>       /* The frame_id for the ephemeral frame; initially unset.  */
>       struct frame_id frame_id;
>
>       /* Nonzero if the frame_id has been set.  */
>       int has_frame_id;
>
>       /* A list of (REGNUM . VALUE) pairs, indicating register values for the
>          ephemeral frame.  */
>       SCM registers;
>     };
>
> (Why a new object type?  Because all <gdb:frame> objects have a
> frame_id, and this one does not, and it turns out it's a deep invariant
> that frames have identifiers.)
>
> The list of unwinders is run in order over the ephemeral frame, and the
> first one that calls (set-ephemeral-frame-id! frame sp [ip [special]])
> on the frame takes responsibility of unwinding the frame.  You can read
> registers with ephemeral-frame-read-register, and set their unwound
> values with ephemeral-frame-write-register!.  Simple stuff, and I'll
> post later once I get the V8 unwinder working.
>
> Unfortunately, the unwind callback is really squirrely -- you can't do
> much there.
>
>   * You can't call the Guile lookup-symbol function within an unwind
>     handler, because the Guile wrapper wants to default the "block"
>     argument from the selected frame, and there is no selected frame.
>
>   * You can't value-call, which is not unexpected in general, but the
>     reason is unexpected: because call_function_by_hand calls
>     get_current_arch and that doesn't work
>
>   * You can't call get_current_arch, directly or indirectly, because it
>     causes unbounded recursion:
>
>       #3  0x00000000006a65f2 in frame_unwind_try_unwinder (this_frame=this_frame@entry=0x5f6af10, this_cache=this_cache@entry=0x5f6af28, unwinder=0x409fe30) at /home/wingo/src/binutils-gdb/+2.2/../gdb/frame-unwind.c:126
>       #4  0x00000000006a696f in frame_unwind_find_by_frame (this_frame=this_frame@entry=0x5f6af10, this_cache=this_cache@entry=0x5f6af28) at /home/wingo/src/binutils-gdb/+2.2/../gdb/frame-unwind.c:157
>       #5  0x00000000006a32cb in get_prev_frame_if_no_cycle (fi=0x5f6af10) at /home/wingo/src/binutils-gdb/+2.2/../gdb/frame.c:454
>       #6  0x00000000006a32cb in get_prev_frame_if_no_cycle (this_frame=this_frame@entry=0x5f6ae40) at /home/wingo/src/binutils-gdb/+2.2/../gdb/frame.c:1780
>       #7  0x00000000006a53a9 in get_prev_frame_always (this_frame=0x5f6ae40) at /home/wingo/src/binutils-gdb/+2.2/../gdb/frame.c:1954
>       #8  0x00000000006a53a9 in get_prev_frame_always (this_frame=this_frame@entry=0x5f6ae40) at /home/wingo/src/binutils-gdb/+2.2/../gdb/frame.c:1971
>       #9  0x00000000006a5ab1 in get_prev_frame (this_frame=this_frame@entry=0x5f6ae40) at /home/wingo/src/binutils-gdb/+2.2/../gdb/frame.c:2212
>       #10 0x00000000006a5d8c in unwind_to_current_frame (ui_out=<optimized out>, args=args@entry=0x5f6ae40) at /home/wingo/src/binutils-gdb/+2.2/../gdb/frame.c:1447
>       #11 0x00000000005cf63c in catch_exceptions_with_msg (func_uiout=<optimized out>, func=func@entry=0x6a5d80 <unwind_to_current_frame>, func_args=func_args@entry=0x5f6ae40, gdberrmsg=gdberrmsg@entry=0x0, mask=mask@entry=RETURN_MASK_ERROR)
>           at /home/wingo/src/binutils-gdb/+2.2/../gdb/exceptions.c:189
>       #12 0x00000000005cf75a in catch_exceptions (uiout=<optimized out>, func=func@entry=0x6a5d80 <unwind_to_current_frame>, func_args=func_args@entry=0x5f6ae40, mask=mask@entry=RETURN_MASK_ERROR) at /home/wingo/src/binutils-gdb/+2.2/../gdb/exceptions.c:169
>       #13 0x00000000006a33e0 in get_current_frame () at /home/wingo/src/binutils-gdb/+2.2/../gdb/frame.c:1486
>       #14 0x00000000006a3fe7 in get_selected_frame (message=message@entry=0x0) at /home/wingo/src/binutils-gdb/+2.2/../gdb/frame.c:1541
>       #15 0x00000000005e7a27 in get_current_arch () at /home/wingo/src/binutils-gdb/+2.2/../gdb/arch-utils.c:784
>
>     Perhaps this is only the case for the most inner frame?  Anyway this
>     is the reason that many other things fail.
>
>   * You can't read user regs from an ephemeral frame for some reason:
>
>       /home/wingo/src/binutils-gdb/+2.2/../gdb/regcache.c:779: internal-error: regcache_cooked_read_value: Assertion `regnum < regcache->descr->nr_cooked_registers' failed.
>       #9  0x00000000005732b5 in regcache_cooked_read_value (regcache=0xd25cc0, regnum=221) at /home/wingo/src/binutils-gdb/+2.2/../gdb/regcache.c:779
>       #10 0x0000000000684c28 in sentinel_frame_prev_register (this_frame=0x6c7c350, this_prologue_cache=<optimized out>, regnum=<optimized out>) at /home/wingo/src/binutils-gdb/+2.2/../gdb/sentinel-frame.c:52
>       #11 0x00000000006a4408 in frame_unwind_register_value (frame=0x6c7c350, regnum=221) at /home/wingo/src/binutils-gdb/+2.2/../gdb/frame.c:1111
>       #12 0x00000000006a468f in frame_register_unwind (frame=<optimized out>, regnum=<optimized out>, optimizedp=0x7fffffffcde8, unavailablep=0x7fffffffcdec, lvalp=0x7fffffffcdf0, addrp=0x7fffffffcdf8, realnump=0x7fffffffcdf4, bufferp=0x7fffffffce40 "l\305\001\367\377\177") at /home/wingo/src/binutils-gdb/+2.2/../gdb/frame.c:1016
>       #13 0x00000000006a4892 in frame_register (frame=<optimized out>, regnum=<optimized out>, optimizedp=<optimized out>, unavailablep=<optimized out>, lvalp=<optimized out>, addrp=<optimized out>, realnump=<optimized out>, bufferp=<optimized out>)
>           at /home/wingo/src/binutils-gdb/+2.2/../gdb/frame.c:1057
>
> And so on.  From what I can tell, all of this is because there is no
> selected frame.  I recognize that this situation reflects reality in
> some way -- we're still building the selected frame -- but is there any
> way that we could have GDB be in a more "normal" state while the unwind
> callback is running?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]