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

[Andy Wingo <wingo@igalia.com>]

Hi Alexander,

Thanks for the reply!

On Mon 02 Mar 2015 23:56, Alexander Smundak <asmundak@google.com> writes:

> 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.

You'll need a link to the sniffer_info in order to be able to give good
errors for set_register, to check that the register exists and that the
value is of the correct type and size.  For that reason, in my first
draft of a Guile interface, the "ephemeral frame" is like your
sniffer_info and unwind_info together.  Perhaps this is a bad idea
though.

I would note as a meta-point that there are going to be some differences
between a Python and a Scheme interface, just for linguistic reasons.
Please consider my feedback as merely a friendly review and not an
obligation in any way :)  In particular, I'm not a GDB developer and
don't have a finely tuned nose for the tao of GDB :)

>> [W]hy not specify registers as strings, as elsewhere
>> (e.g. gdb.Frame.read_register)?
> My concern is that name lookups are expensive

Are they?  I wouldn't think so, no more than anything that happens in
Python.

> 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.

If it were Guile I would leave off the numbers, but hey that's me :)
I'll leave this one to Doug.

>> 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.

Well.  I mean, it's not a frame to Python, but its only state is a
"struct frame_info" pointer, and its only method is also present on
gdb.Frame, so it looks a lot like a frame to me :)

>> 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.

The comment in the source says:

          /* Call `deprecated_frame_register_read' -- calling
             `value_of_register' would an assert in `get_frame_id'
             because our frame is incomplete.  */

Whereas get_frame_register_value looks something like this:

  struct value *
  frame_unwind_register_value (struct frame_info *frame, int regnum)
  {
    /* Find the unwinder.  */
    if (frame->unwind == NULL)
      frame_unwind_find_by_frame (frame, &frame->prologue_cache);
  
    /* Ask this frame to unwind its register.  */
    return frame->unwind->prev_register (frame, &frame->prologue_cache, regnum);
  }
  
  struct value *
  get_frame_register_value (struct frame_info *frame, int regnum)
  {
    return frame_unwind_register_value (frame->next, regnum);
  }

So it doesn't touch THIS_FRAME.

Alexander, did you not run into nasty crashes while doing random Python
things inside your unwind handler?

For completeness, here's a draft of the unwinder I was working on, with
a bunch of helpers elided:

  (define (unwind-v8-frame frame)
    (let* ((isolate (cached-current-isolate))
           (prev-pc (ephemeral-frame-read-register frame "rip"))
           (code (and isolate
                      (lookup-code-for-pc prev-pc isolate))))
      (when code
        (let* ((fp (ephemeral-frame-read-register frame "rbp"))
               (type (if (code-optimized? code)
                         (v8-constant "StackFrame::OPTIMIZED")
                         (v8-constant "StackFrame::JAVA_SCRIPT")))
               (pc-address (compute-standard-frame-pc-address fp))
               (pc (value-dereference pc-address))
               (start-pc (code-instruction-start code))
               (sp (compute-frame-older-sp fp type))
               (fp (compute-standard-frame-older-fp fp)))
          (set-ephemeral-frame-id! frame fp start-pc)
          (ephemeral-frame-write-register! frame "rsp" sp)
          (ephemeral-frame-write-register! frame "rbp" fp)
          (ephemeral-frame-write-register! frame "rip" pc)))))

As you can see it's the set-ephemeral-frame-id! that marks the frame as
unwound.  A pretty weird interface, maybe I'd do better to separate them
again.

Andy