This is an archive of the discontinued LLVM Phabricator instance.

[lldb/test] Update TestScriptedProcess to use skinny corefiles
ClosedPublic

Authored by mib on Oct 18 2021, 6:19 PM.

Details

Summary

This patch changes the ScriptedProcess test to use a stack-only skinny
corefile as a backing store.

The corefile is saved as a temporary file at the beginning of the test,
and a second target is created for the ScriptedProcess. To do so, we use
the SBAPI from the ScriptedProcess' python script to interact with the
corefile process.

This patch also makes some small adjustments to the other ScriptedProcess
scripts to resolve some inconsistencies and removes the raw memory dump
that was previously checked in.

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib requested review of this revision.Oct 18 2021, 6:19 PM
mib created this revision.
mib updated this revision to Diff 380664.Oct 19 2021, 5:54 AM
mib updated this revision to Diff 380819.Oct 19 2021, 4:36 PM

Fix test.

mib updated this revision to Diff 380825.Oct 19 2021, 4:49 PM
mib updated this revision to Diff 381020.Oct 20 2021, 10:51 AM

Add self argument to scripted_process base class methods.

JDevlieghere added inline comments.Oct 22 2021, 1:33 PM
lldb/examples/python/scripted_process/stack_core_scripted_process.py
1 ↗(On Diff #381020)

This should live next to the test. I don't see a point of shipping this to users.

lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
91–99

This function does more than creating a stack core file: it deletes the current target and replaces it with the stack core file one. I would split this up in two functions, and maybe pass in the temporary file so you can do

with tempfile.NamedTemporaryFile() as file:

in the caller to make it clear what the scope of the temporary file is. Currently, I assume it gets garbage collected after this function exits?

121–137

We should really find a better way to do this. Can you remind me why we can't pass that in the structured data? Other than for you test, I assume we rarely want a process launch for a scripted process, so maybe the default should be inverted.

mib marked 3 inline comments as done.Nov 9 2021, 5:23 PM
mib added inline comments.
lldb/examples/python/scripted_process/stack_core_scripted_process.py
1 ↗(On Diff #381020)

Do you mean the python script or the import statement ? In both case, I don't think this we ship these to the users.

lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
121–137

This is an environment variable used to decide whether we should launch the process right after importing the python script in lldb or leave it to the user' s discretion.

This is also used to launch the process from the script interpreter using the SBAPI, to test launching the scripted process in both a synchronous and asynchronous mode.

It can also come quite handy for debugging and troubleshooting scripted processes.

mib updated this revision to Diff 386021.Nov 9 2021, 5:24 PM
mib marked 2 inline comments as done.

Address @JDevlieghere comments and skip test for non x86_64 archs

JDevlieghere added inline comments.Nov 9 2021, 10:05 PM
lldb/examples/python/scripted_process/stack_core_scripted_process.py
1 ↗(On Diff #381020)

I mean that stack_core_scripted_process.py shouldn't be under lldb/examples but should live in the test directory where it's used.

mib updated this revision to Diff 386176.EditedNov 10 2021, 8:13 AM

Move stack_core_scripted_process.py from examples to test

mib marked an inline comment as done.Nov 10 2021, 8:13 AM

Some of the formatting in the Python tests seems a little off (can you run it through something like yapf?). Other than that this LGTM with the inline comments addressed.

lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
101

You'll also need skipIfOutOfTreeDebugserver because skinny corefiles require debugserver support.

119

You'll want to unset this after your test so it doesn't affect another test:

def cleanup():
    del os.environ["SKIP_SCRIPTED_PROCESS_LAUNCH"]
self.addTearDownHook(cleanup)
JDevlieghere accepted this revision.Nov 10 2021, 8:35 AM
This revision is now accepted and ready to land.Nov 10 2021, 8:35 AM
This revision was landed with ongoing or failed builds.Nov 10 2021, 8:44 AM
This revision was automatically updated to reflect the committed changes.