Page MenuHomePhabricator

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

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

Details

Summary

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.
lldb/source/Commands/CommandObjectTarget.cpp
4625

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?

lldb/docs/use/python-reference.rst
843

Any reason for the double empty lines?

863

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

lldb/include/lldb/Interpreter/ScriptInterpreter.h
302

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

lldb/include/lldb/Target/Target.h
1200

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

1202

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

1232

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

1238

///

1248–1249

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

1249–1255

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

lldb/source/Commands/CommandObjectTarget.cpp
4691

Is this used?

lldb/source/Target/Target.cpp
3153

So many early returns in this patch 😍

3273

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.

lldb/docs/use/python-reference.rst
843

Nope.

lldb/include/lldb/Interpreter/ScriptInterpreter.h
302

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.

lldb/include/lldb/Target/Target.h
1202

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.

1232

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.

lldb/source/Commands/CommandObjectTarget.cpp
4691

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

lldb/source/Target/Target.cpp
3273

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.
lldb/include/lldb/Interpreter/ScriptInterpreter.h
302

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.

lldb/include/lldb/Target/Target.h
1232

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 ********************
Script:
--
: '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/stop_hook.py' -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/TestStopHookScripted.py", line 54, in test_stop_hooks_scripted_return_false
    self.do_test_auto_continue(True)
  File "/Users/teemperor/1llvm/llvm-project/lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py", 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.
Config=x86_64-/Users/teemperor/1llvm/rel/bin/clang
----------------------------------------------------------------------