This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Plugins] Update ScriptedProcess Process Plugin
ClosedPublic

Authored by mib on Apr 13 2021, 6:58 AM.

Details

Summary

This patch is an update of D95713 since the commit have been reverted.

It fixes the hangs, crashes and other asynchronous issues when using Scripted Processes.

rdar://65508855

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

Diff Detail

Event Timeline

mib created this revision.Apr 13 2021, 6:58 AM
mib requested review of this revision.Apr 13 2021, 6:58 AM
mib updated this revision to Diff 337184.Apr 13 2021, 9:13 AM

Dead-code removal

JDevlieghere added inline comments.Apr 13 2021, 9:39 AM
lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
93

You can move this into the if on line 98, or even just inline it.

100–103
138

I see this newline between log and LLDB_LOGF across this patch and while I don't mind (it's consistent) I'm curious why it's here. The next line is immediately using log and it other places in this patch where a variable is used directly after there is (usually) no newline.

205

Do we need this loop at all? It looks like nothing is ever setting is_running to true, so effectively this runs just the once? The two nested loops make it pretty hard to follow what the control flow is here.

236
316–345
lldb/source/Plugins/Process/scripted/ScriptedProcess.h
128

Can we add a Doxygen comment (group) that explains what these things are and what they're used for?

mib updated this revision to Diff 337226.Apr 13 2021, 12:09 PM
mib marked 7 inline comments as done.

Address @JDevlieghere feedbacks

JDevlieghere added inline comments.Apr 14 2021, 8:55 AM
lldb/source/Plugins/Process/scripted/ScriptedProcess.h
130–135

The trailing comments are a real pain with the 80 col limit and we're actively moving away from them. Please use /// above the line instead or consider documenting the whole thing with a comment group.

mib updated this revision to Diff 337549.Apr 14 2021, 1:57 PM
mib marked an inline comment as done.

Use Doxygen group comment for ScriptedProcess members.

This revision is now accepted and ready to land.Apr 14 2021, 2:03 PM

A bunch of little comments. I was also curious why you directly set the running private state, but use the async thread to generate the stopped events?

lldb/source/Plugins/Process/CMakeLists.txt
15

Why is this necessary? The code in Plugins/Process/scripted should work for any script interpreter (and should handle the case when there isn't one.)

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
79

If making the scripted process fails because there's no script interpreter, it would be good to get that fact out to the user. Maybe the ScriptedProcess constructor should take a Status object that it's caller can somehow bubble up?

86

If these two conditions aren't true, do you actually want to continue? Seems like you should error out here as well.

171

Does it do any good to broadcast the ShouldExit bit if the Async thread isn't running? If you're following the llvm style, you would do:

if (!m_async_thread.IsJoinable()) {

// Do your logging
return;

}

First, and then all the actual work of the function outside an if. That would the odd look of sending an event that you know is going to a particular thread when the thread isn't running?

262

I don't know if you actually need this, but it seems weird to be setting the PID in WillLaunch. I don't think there's any real process plugin that would know the PID of the launched process in WillLaunch. Maybe it just looks odd, but still is seems worthwhile to follow the behavior of "real" process plugins wherever you can.

301

Maybe these should be asserts. It really shouldn't be possible to get to DoResume for a ScriptedProcess with no interpreter and no script object. Also messages in the generic ScriptProcess code shouldn't refer to Python directly. You don't know that that's the script interpreter language being used here.

316

We call Status objects "error" pretty much everywhere. If we end up changing to use "status" then we should do that wholesale.

323

This seems asymmetric, why do you use the async thread to generate the stopped event, but not the running event?

328

Presumably the Interface knew why Resume failed if it did. Don't erase that information in the error you return here.

337

You checked both m_interpreter & m_script_object_sp above when checking for validity. Do you need to check both here as well.

343

The comment above Process::ReadMemory says processes plugins should not override the method. Why do you need to do this here? If there is a reason, it should be explained.

389

You are inconsistent about what you check for "We didn't manage to make the scripted process at all". Sometimes you just check for the interpreter and sometimes you check that and the m_script_object_sp...

Maybe you should put those checks into a ScriptedProcess method so you do it the same way everywhere.

411

Presumably this is just unimplemented? This is supposed to get the current set of threads, if any of them are in old_thread_list then they get copied to new_thread_list, and then any actually new threads will get added to new_thread_list.

lldb/source/Plugins/Process/scripted/ScriptedProcess.h
24

I'm not sure LaunchInfo is the right name for this class. While it does get constructed by grabbing a couple of fields out of the ProcessLaunchInfo, it's actually used just to construct the ScriptedProcess so it isn't directly about launching the scripted process. Since you do a separate Launch operation, and you pass that a different entity also called LaunchInfo, I think this name is confusing.

119

The name of this bit is odd. It instructs the Async Thread to switch the private process state to eStateStopped. So AsyncContinue is a confusing name.

mib marked 15 inline comments as done.Jul 20 2021, 3:40 AM
mib updated this revision to Diff 360074.Jul 20 2021, 3:48 AM
mib edited the summary of this revision. (Show Details)

Address @jingham feedbacks:

  • Make ScriptedProcess plugin language agnostic
  • Simplify execution control by removing Listener-Broadcaster model
  • Solve non-deterministic hangs following D105698
  • Fix memory read bug.
mib updated this revision to Diff 360779.Jul 22 2021, 5:50 AM

Fix ScriptedProcess::IsAlive crash during process destruction

This revision was landed with ongoing or failed builds.Jul 22 2021, 5:51 AM
This revision was automatically updated to reflect the committed changes.