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

[Andy Wingo <wingo@igalia.com>]

Howdy :)

Reordering some of the replies.

On Wed 04 Mar 2015 03:36, Alexander Smundak <asmundak@google.com> writes:

>> 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.
>
> Checking the type and size of the a register in the implementation of
> the `set_register' method depends only on the current architecture,
> not on what's available in the sniffer_info.

There is no current architecture because there may be no selected frame.

In any case the issue is the arch of the frame being unwound, which the
API implies might be different from the target architecture.  I am not
sure how this can be the case; can a GDB person chime in?

So when your user does:

>     previous_frame = UnwindInfo(((AMD64_RBP_NUM, fp), (AMD64_RIP_NUM,
> pc), (AMD64_RSP_NUM, sp)))

you can't correctly detect errors because you don't know the
architecture.

Incidentally if you're doing this from python then what I would want as
a user is a traceback for the error when it occurs.  That's easier to do
if you don't lump multiple things in one call.  For that reason in the
Guile interface you would have, for your example:

  (ephemeral-frame-set-id! frame sp pc)
  (ephemeral-frame-set-register! frame "rbp" fp)
  (ephemeral-frame-set-register! frame "rip" ip)
  (ephemeral-frame-set-register! frame "rsp" sp)

But, to each folk their stroke.

>>>> [W]hy not specify registers as strings, as elsewhere
>>>> (e.g. gdb.Frame.read_register)?
>
> The function mapping a register name to the number,
> `user_reg_map_name_to_regnum', compares given register name
> with all known register names, one at a time. I thought Python interpreter
> is somewhat smarter than that and use dictionaries for lookups. Even if
> it is not, there is no need to pile up inefficiency.
> Actually, it's not that difficult to be able to pass either a register number,
> or register name.

The one-at-a-time nature of user_reg_map_name_to_regnum is a detail and
not essential.

However I think we have argued this enough and I won't say any more
about it, being as I don't have LGTM power over these things :-))

>>>> In the read_register() function, I believe you can use
>>>> get_frame_register_value instead of deprecated_frame_register_read.
>
> Here's traceback I get when I try to call value_of_register:

I suggested "get_frame_register_value", not "value_of_register".
Confusion is understandable though with all these similarly-named,
similarly-purposed functions...

>> Alexander, did you not run into nasty crashes while doing random Python
>> things inside your unwind handler?
> I used to have random crashes before I learned to call `ensure_python_env'
> to establish Python runtime environment before suing Python API, but after
> that, no. Do you have an example that you can share?

Sure; I figured out what was going on yesterday.  With your test cases,
is the innermost frame handled by the unwinder?  I ran into lots of
problems there, because -- and I'm going to have to bullet-point this as
it gets complicated --

  * An unwinder is called on the innermost frame.

  * Many actions, for example looking up a symbol without specifying a
    block. will request the selected frame.

  * get_selected_frame() sees there is no selected frame, and goes to
    get_current_frame() and will select that.

  * get_current_frame creates a sentinel frame and unwinds that to
    produce the innermost frame.

  * After unwinding saved registers from the sentinel, frame.c finishes
    constructing the innermost frame by doing a compute_frame_id() on
    the frame.

  * compute_frame_id() goes to compute the unwinder for the innermost
    frame, in order to call the this_id() method, which leads us back to
    the beginning.

You may not have seen this if your test cases do not have an
"interesting" frame as the innermost frame.

I have a fix for this, but it's a bit deep.  I'll post my patch today.
Happily, with a combination of unwinders and frame filters, I am able to
get a correct, pretty backtrace:

  #0  0x00000d3c5b0661a1 in TestCase () at /hack/v8/test/mjsunit/debug-step-4-in-frame.js:94
  #1  0x00000d3c5b06a3d3 in  () at /hack/v8/test/mjsunit/debug-step-4-in-frame.js:112
  #2  0x00000d3c5b02c620 in [internal frame] ()
  #3  0x00000d3c5b014d31 in [entry frame] ()
  #4  0x0000000000b4e949 in v8::internal::Invoke(...) at ../src/execution.cc:128
  #5  0x0000000000b4ed23 in v8::internal::Execution::Call(...) at ../src/execution.cc:179
  #6  0x0000000000a3f813 in v8::Script::Run() at ../src/api.cc:1514
  #7  0x0000000000a149fa in v8::Shell::ExecuteString(...) at ../src/d8.cc:281
  #8  0x0000000000a194eb in v8::SourceGroup::Execute(...) at ../src/d8.cc:1213
  #9  0x0000000000a1a128 in v8::Shell::RunMain(...) at ../src/d8.cc:1448
  #10 0x0000000000a1efdc in v8::Shell::Main(...) at ../src/d8.cc:1721
  #11 0x0000000000a1f143 in main(...) at ../src/d8.cc:1757

Before, the backtrace was garbled, incorrect, without names, and wasn't
able to unwind out of the compiled JS code.  Wheeee :-))

Andy