This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Commands] Add command options for ScriptedProcess to ProcessLaunch
ClosedPublic

Authored by mib on Jan 29 2021, 5:13 PM.

Details

Reviewers
JDevlieghere
jingham
jasonmolenda
labath
Group Reviewers
Restricted Project
Summary

[lldb/Commands] Add command options for ScriptedProcess to ProcessLaunch

This patch adds a new command options to the CommandObjectProcessLaunch
for scripted processes.

Among the options, the user need to specify the class name managing the
scripted process. The user can also use a key-value dictionary holding
arbitrary data that will be passed to the managing class.

This patch also adds getters and setters to SBLaunchInfo for the
class name managing the scripted process and the dictionary.

rdar://65508855

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

Diff Detail

Event Timeline

mib created this revision.Jan 29 2021, 5:13 PM
mib requested review of this revision.Jan 29 2021, 5:13 PM
vsk added a subscriber: vsk.Feb 1 2021, 3:45 PM
vsk added inline comments.
lldb/include/lldb/Host/ProcessLaunchInfo.h
188

Might be best to use a std::string here, to avoid tying the lifetime of m_scripted_process_class_name to the lifetime of the string stored in m_class_options.

I'm not quite sure why we need the extra m_scripted_process boolean. Seems like you could key off whether we had a non-empty class_name. Is there any case where you would want to boolean set w/o having a class name yet?

lldb/include/lldb/Host/ProcessLaunchInfo.h
188

Agreed. You seldom want to use a StringRef as an ivar, since it doesn't manage lifetimes at all.

mib updated this revision to Diff 321647.Feb 4 2021, 8:48 PM
mib marked 2 inline comments as done.
mib edited the summary of this revision. (Show Details)

Address @jingham and @vsk comments.

JDevlieghere accepted this revision.Feb 5 2021, 9:16 AM

LGTM with a few nits.

lldb/include/lldb/Host/ProcessLaunchInfo.h
187

Can you add a Doxygen comment explaining what they're used for?

lldb/source/Host/common/ProcessLaunchInfo.cpp
178

Let's call clear for consistency with m_plugin_name which is also a string.

This revision is now accepted and ready to land.Feb 5 2021, 9:16 AM
mib updated this revision to Diff 322995.Feb 11 2021, 7:03 AM
mib marked 2 inline comments as done.

Address @JDevlieghere comments.

mib updated this revision to Diff 323911.Feb 16 2021, 1:15 AM
mib edited the summary of this revision. (Show Details)

Add getters and setters for scripted process managing class name and dictionary in SBLaunchInfo.

labath added a comment.Nov 9 2021, 6:25 AM

Sorry for grave digging, but my compiler just became smart enough to start complaining about this.

lldb/source/API/SBLaunchInfo.cpp
392 ↗(On Diff #323911)

you're dereferencing a null pointer here.

mib added a comment.Nov 9 2021, 6:32 AM

Sorry for grave digging, but my compiler just became smart enough to start complaining about this.

Haha, not at all! This wasn't called when I implemented it, but I stumbled into it in following patches and fixed it in D112109.

The fix hasn't landed yet because I'm still trying to understand why would the python-initialized SBStructuredData be pointing the corrupted memory when trying accessing it in C++, instead of deserializing it into a std::string, and serializing it back, which is more a workaround that an actual fix.