Page MenuHomePhabricator

Add the ability to write 'target stop-hooks' in Python

Authored by jingham on Sep 22 2020, 4:46 PM.



Previously, you could only have a list of command-line commands as the reaction to a stop hook.

This patch adds the ability to define the stop hooks directly in Python, which allows you to do logging and add logic to the stop hook more easily.

Diff Detail

Event Timeline

jingham created this revision.Sep 22 2020, 4:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2020, 4:46 PM
jingham requested review of this revision.Sep 22 2020, 4:46 PM
jingham updated this revision to Diff 293603.Sep 22 2020, 5:37 PM

Added docs to the python-reference.rst

kastiglione added inline comments.

missing space after def

I'm very excited about this feature. Great job on the documentation, both in the help output as for the website. Do you have a potential use case in mind that we could add to the examples?


Any reason for the double empty lines?


There needs to be a newline here for this to be rendered as code.


Could this function return an Expected<StructuredData::GenericSP> instead?


Can we drop this altogether? We can always get the address of the reference at the call site.


can this be a const reference? Maybe even a StringRef? Similar question for the methods below.


Please make these Doxygen comments (///) and put them on the line above the variable.




And you might as well fix that here too :-)


I think we should make this a little enum that has the values command and scripted.


Is this used?


So many early returns in this patch 😍


It looks like we will leak this if script_interp is null. One solution for that would be to wrap it in a unique_ptr and release it when passing it to CreateScriptedStopHook. If we do the m_extra_args assignment just before that call that also guarantees that the variable remains null until then.

jingham updated this revision to Diff 293902.Sep 23 2020, 5:48 PM
jingham marked 3 inline comments as done.

Addressed review comments.

jingham marked 9 inline comments as done.Sep 23 2020, 5:51 PM

Addressed review comments.




There are a bunch of instances of objects created to insert scripting into various lldb callbacks around in lldb. It might make sense to convert them all to Expected's, but I wouldn't want to do it piecemeal.

Adding a new one of these is also a bit cargo-culty (another issue we should probably clean up as a separate bit of work) and so making the same things look different is not doing the next person who adds one any favors.

These are also functions that are not going to get called promiscuously, but really from one "make me one of these" places, so they aren't crying out for protection against calling them w/o checking for errors.

But anyway, IMO if we're going to start restructuring the pattern for setting these callback objects up, we should do that as a separate commit and do it for all of them.


const is fine. Since all I plan to do is pass this string to a function that takes a const std::string, it doesn't make much sense to make it a StringRef.


I didn't quite do that.

The comment about "We own..." doesn't seem to me to be a doc comment. It isn't something that a user of this stop hook class would need to know. It's just an answer to a question somebody reading the code closely might ask about why we don't have to have something keeping this m_extra_args alive. I did add a doc comment for this field, but kept the side comment as is.


Nope. Vestigial from an older draft of the StopHook API's.


I did this more simple-mindedly by just checking for the script interpreter before we do any other work.

JDevlieghere accepted this revision.Sep 25 2020, 8:34 AM
JDevlieghere added inline comments.

If I counted correctly there are 2 others: CreateScriptedThreadPlan which takes a std::string& as an error and CreateScriptedBreakpointResolver which doesn't seem to do error handling at all. So I think we should try to refactor all three to return Expecteds. I agree we should do that in a separate patch.


It's private anyway, so I think the boundaries are pretty fluid. But I don't feel strongly about it.

This revision is now accepted and ready to land.Sep 25 2020, 8:34 AM
This revision was landed with ongoing or failed builds.Sep 25 2020, 3:45 PM
This revision was automatically updated to reflect the committed changes.
jingham marked 5 inline comments as done.

Hi Jim, this change broke my Fedora 33 Linux (x86) box. Do you think we can get a quick fix or revert this?

FAIL: lldb-shell :: Commands/command-stop-hook-output.test (69796 of 70913)
******************** TEST 'lldb-shell :: Commands/command-stop-hook-output.test' FAILED ********************
: 'RUN: at line 1';   /tmp/_update_lc/r/bin/clang --target=specify-a-target-or-use-a-_host-substitution --target=x86_64-unknown-linux-gnu -pthread -fmodules-cache-path=/tmp/_update_lc/r/lldb-test-build.noindex/module-cache-clang/lldb-shell -g /home/dave/ro_s/lp/lldb/test/Shell/Commands/Inputs/main.c -o /tmp/_update_lc/r/tools/lldb/test/Commands/Output/command-stop-hook-output.test.tmp
: 'RUN: at line 2';   /tmp/_update_lc/r/bin/lldb --no-lldbinit -S /tmp/_update_lc/r/tools/lldb/test/Shell/lit-lldb-init /tmp/_update_lc/r/tools/lldb/test/Commands/Output/command-stop-hook-output.test.tmp -O 'command script import /home/dave/ro_s/lp/lldb/test/Shell/Commands/Inputs/' -s /home/dave/ro_s/lp/lldb/test/Shell/Commands/command-stop-hook-output.test -o exit | /tmp/_update_lc/r/bin/FileCheck /home/dave/ro_s/lp/lldb/test/Shell/Commands/command-stop-hook-output.test
Exit Code: 1

Command Output (stderr):
clang-12: warning: argument unused during compilation: '-fmodules-cache-path=/tmp/_update_lc/r/lldb-test-build.noindex/module-cache-clang/lldb-shell' [-Wunused-command-line-argument]
error: module importing failed: This script interpreter does not support importing modules.
/home/dave/ro_s/lp/lldb/test/Shell/Commands/command-stop-hook-output.test:5:16: error: CHECK-LABEL: expected string not found in input
# CHECK-LABEL: b main
<stdin>:1:1: note: scanning from here
(lldb) command source -s 0 '/tmp/_update_lc/r/tools/lldb/test/Shell/lit-lldb-init'
<stdin>:1:8: note: possible intended match here
(lldb) command source -s 0 '/tmp/_update_lc/r/tools/lldb/test/Shell/lit-lldb-init'

Input file: <stdin>
Check file: /home/dave/ro_s/lp/lldb/test/Shell/Commands/command-stop-hook-output.test

-dump-input=help explains the following input dump.

Input was:
           1: (lldb) command source -s 0 '/tmp/_update_lc/r/tools/lldb/test/Shell/lit-lldb-init'
label:5'0     X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
label:5'1            ?                                                                           possible intended match
           2: Executing commands in '/tmp/_update_lc/r/tools/lldb/test/Shell/lit-lldb-init'.
label:5'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           3: (lldb) # LLDB init file for the LIT tests.
label:5'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           4: (lldb) settings set symbols.enable-external-lookup false
label:5'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           5: (lldb) settings set plugin.process.gdb-remote.packet-timeout 60
label:5'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           6: (lldb) settings set interpreter.echo-comment-commands false
label:5'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Failed Tests (1):
  lldb-shell :: Commands/command-stop-hook-output.test

Testing Time: 66.80s
  Unsupported      : 10853
  Passed           : 59957
  Expectedly Failed:   102
  Failed           :     1
FAILED: CMakeFiles/check-all

This also doesn't work on my macOS system:

FAIL: LLDB (/Users/teemperor/1llvm/rel/bin/clang-x86_64) :: test_stop_hooks_scripted_return_false (TestStopHookScripted.TestStopHooks)
FAIL: test_stop_hooks_scripted_return_false (TestStopHookScripted.TestStopHooks)
   Test that the returning False from a stop hook works
Traceback (most recent call last):
  File "/Users/teemperor/1llvm/llvm-project/lldb/test/API/commands/target/stop-hooks/", line 54, in test_stop_hooks_scripted_return_false
  File "/Users/teemperor/1llvm/llvm-project/lldb/test/API/commands/target/stop-hooks/", line 91, in do_test_auto_continue
    self.assertEqual("main", func_name, "Didn't stop at the expected function.")
AssertionError: 'main' != 'step_out_of_me'
- main+ step_out_of_me : Didn't stop at the expected function.