This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Plugins] Add support for ScriptedThread in ScriptedProcess
ClosedPublic

Authored by mib on Aug 5 2021, 10:34 AM.

Details

Summary

This patch introduces the ScriptedThread class with its python
interface.

When used with ScriptedProcess, ScriptedThreaad can provide various
information such as the thread state, stop reason or even its register
context.

This can be used to reconstruct the program stack frames using lldb's unwinder.

rdar://74503836

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

Diff Detail

Event Timeline

mib created this revision.Aug 5 2021, 10:34 AM
mib requested review of this revision.Aug 5 2021, 10:34 AM
JDevlieghere added inline comments.Aug 12 2021, 1:17 PM
lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
295

Same comment as in the other review: no need to allocate a std::string here.

299–310
lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
110
mib updated this revision to Diff 369566.Aug 30 2021, 3:47 PM
mib marked 3 inline comments as done.
  • Address @JDevlieghere feedbacks
  • Use the new ScriptedPythonInterface::Dispatch method in ScriptedThreadPythonInterface
  • Add Test
mib updated this revision to Diff 370261.Sep 2 2021, 6:59 AM

Tighten PythonInterpreter Lock.

Can you explain why we need the memory region? When we're using an SB class from the ScriptedProcess plugin, we need to be very careful to avoid introducing cyclic dependencies. The memory region seems like a pretty "dumb" class (i.e. it's basically just a few ivars with getters and setters) so it's probably fine, but we should have a comment explaining why it's fine.

lldb/include/lldb/API/SBMemoryRegionInfo.h
133 ↗(On Diff #370261)

Why is this necessary?

mib updated this revision to Diff 370300.EditedSep 2 2021, 9:35 AM

Remove code related to D108953

mib added a comment.Sep 2 2021, 9:49 AM

Can you explain why we need the memory region? When we're using an SB class from the ScriptedProcess plugin, we need to be very careful to avoid introducing cyclic dependencies. The memory region seems like a pretty "dumb" class (i.e. it's basically just a few ivars with getters and setters) so it's probably fine, but we should have a comment explaining why it's fine.

I mistakenly merge the changes for this patch and D108953 on my previous diff ... Since your question is concerning that other patch, I'll follow-up there.

JDevlieghere added inline comments.Sep 9 2021, 9:59 AM
lldb/examples/python/scripted_process/scripted_process.py
242

Should eStateStopped be the default?

244

Why's this commented out?

lldb/include/lldb/Interpreter/ScriptedProcessInterface.h
77
lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
106

We're caching the name and the queue, is this an optimization (to avoid calling into Python over and over) or are there assumptions that will break if these are not stable? If so, should that be something we call out on the Python side or alternatively, should we allow this to change and not cache it?

142–145
153–157

Seems like you have this lambda in a lot of places, I wonder if it's worth the trade-off of making this a static function that takes __FUNCTION__ as an extra argument.

159–161

You call SetStopInfo(m_cached_stop_info_sp); on line 211, so we could drop the case for m_cached_stop_info_sp being true?

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedThreadPythonInterface.cpp
129–134

This code is duplicated over and over for objects and dicts. Seems like a templated function could help here. Then you wouldn't have to worry about being more verbose and differentiating the different errors (null vs invalid vs error). Also, is there always an error when !obj or !obj->IsValid() or are there cases where this might print something like "Null or invalid object ()"?

mib marked 7 inline comments as done.Oct 1 2021, 6:25 AM
mib added inline comments.
lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
106

This meant to be an optimization to avoid calling the Python interface but I guess we can rid of that.

mib updated this revision to Diff 376502.Oct 1 2021, 6:26 AM
mib marked an inline comment as done.

Address @JDevlieghere comments:

  • Remove "caching"
  • Refactor error_with_message lambda and StructuredData objects sanity checks.
mib updated this revision to Diff 376504.Oct 1 2021, 6:29 AM

Reformat code.

mib updated this revision to Diff 376507.Oct 1 2021, 6:43 AM

Pass __PRETTY_FUNCTION__ to CheckStructuredDataObject to log the caller function name.

JDevlieghere accepted this revision.Oct 4 2021, 10:41 AM

LGTM with a few nits that don't require re-review if they're fixed or turn out to be not relevant.

lldb/include/lldb/Interpreter/ScriptedInterface.h
43–51

I think I left a similar comment in the past, but since you know whether the object is null or invalid, why drop that information and be less precise in your error message?

lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
187

Still relevant?

206–207

Does this assertion depend on "user-input"? In other words, can this be triggered by not returning any registers from the interface?

This revision is now accepted and ready to land.Oct 4 2021, 10:41 AM
mib marked 2 inline comments as done.Oct 5 2021, 12:02 PM
mib added inline comments.
lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
187

Yes. We might need to implement this when introducing execution control to Scripted Threads.

206–207

Yes, the user has to provide some basic register information to be able to parse it and use it.

mib updated this revision to Diff 377324.Oct 5 2021, 12:03 PM
mib marked 2 inline comments as done.

Address @JDevlieghere last comments.

This revision was landed with ongoing or failed builds.Oct 8 2021, 5:55 AM
This revision was automatically updated to reflect the committed changes.
mib added a comment.Oct 8 2021, 9:05 AM
C:\buildbot\lldb-x64-windows-ninja\llvm-project\lldb\source\Plugins\Process\scripted\ScriptedThread.cpp(144): error C2065: '__PRETTY_FUNCTION__': undeclared identifier

It looks like MSVC doesn't have the __PRETTY_FUNCTION__ macro ... Should be an easy fix.

omjavaid added inline comments.
lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
203

Hi
This fails on arm and aarch64 linux buildbot. please have a look i have temporarily marked it skipped.