This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Add Debugger & ScriptedMetadata reference to Platform::CreateInstance
ClosedPublic

Authored by mib on Dec 2 2022, 11:48 PM.

Details

Summary

This patch is preparatory work for Scripted Platform support and does
multiple things:

First, it introduces new options for the platform select command and
SBPlatform::Create API, to hold a reference to the debugger object,
the name of the python script managing the Scripted Platform and a
structured data dictionary that the user can use to pass arbitrary data.

Then, it updates the various Create and GetOrCreate methods for
the Platform and PlatformList classes to pass down the new parameter
to the Platform::CreateInstance callbacks.

Finally, it updates every callback to reflect these changes.

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

Diff Detail

Event Timeline

mib created this revision.Dec 2 2022, 11:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2022, 11:48 PM
Herald added a subscriber: emaste. · View Herald Transcript
mib requested review of this revision.Dec 2 2022, 11:48 PM
mib updated this revision to Diff 480160.Dec 5 2022, 10:47 AM

Update ScriptedMetadata header location

The idea of the patch seems fine to me. One thing that I thought about is creating a PlatformSpec struct that can contain all the needed configuration for CreateInstance/GetOrCreate. What do you think of this?

lldb/source/API/SBDebugger.cpp
1501–1503

You could add a small comment indicating what parameter the nullptr is standing in for.

lldb/source/API/SBPlatform.cpp
298–322

Here too

lldb/source/Interpreter/OptionGroupPlatform.cpp
13

Is it OptionGroupPythonClassWithDict.h you want to include here? It looks more like you'd want to include ScriptedMetadata.h

lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
514

Add a small comment here for nullptr as well.

lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
973

Here too.

lldb/source/Target/Platform.cpp
167–200

Yipee! :)

2043–2046

Here too

2083–2084

Here too.

lldb/source/Target/Process.cpp
2934–2935

Here too.

lldb/source/Target/Target.cpp
1506–1509

Here

lldb/source/Target/TargetList.cpp
187–189

Here

222–225

Here

235

H

268

H

You need documentation for what the name & scripted metadata do here somewhere. In SBPlatform.i so that it goes into the SB API docs is one good place. Also maybe in the Platform.h where it gets passed to Create or something.

Other than that, LGTM...

lldb/source/API/SBPlatform.cpp
305

Do you need to check script_name != nullptr here as well as checking the dict?

mib marked 15 inline comments as done.Dec 10 2022, 4:02 AM
mib updated this revision to Diff 481842.Dec 10 2022, 4:24 AM
mib added a reviewer: bulbazord.

Addressed @jingham @bulbazord comments:

  • Add docstring to swig interface
  • Add comment on nullptr arguments

@bulbazord Having a PlatformSpec object is a good idea but I don't think it's necessary quite yet since there are only 2 arguments passed to the Platform::CreateInstance

This revision is now accepted and ready to land.Dec 12 2022, 10:45 AM
labath added inline comments.Dec 15 2022, 1:52 PM
lldb/include/lldb/Interpreter/OptionGroupPlatform.h
73

These nested groups are fairly unusual? Couldn't CommandObjectPlatformSelect just have two OptionGroup members?

labath added inline comments.Dec 15 2022, 1:57 PM
lldb/include/lldb/Interpreter/OptionGroupPlatform.h
73

I see that this is referenced in OptionGroupPlatform::CreatePlatformWithOptions. Maybe that could be fixed by moving that functions somewhere else (making it a free function) and having it take the two groups as arguments? Creating a platforms seems like too high-level of a task for an option group anyway...

mib added inline comments.Dec 16 2022, 7:57 AM
lldb/include/lldb/Interpreter/OptionGroupPlatform.h
73

This special option group is used by many other scripted objects (Scripted Process, Breakpoint Callbacks, Stop Hooks, ...) since their basically an affordance to take a script name and a dictionary from the command line. I think it totally makes sense to use it here.

mib updated this revision to Diff 487989.Jan 10 2023, 2:12 PM

Update unittests

This seems very intrusive: I don't think every plugin should have to be aware of this metadata that only one plugin needs. Is there a way to avoid this? For example, could we initialize a generic scripted platform first, with no backing Python class, and then have that set after the fact? My concern here is how the scripted process/platform is slowly creeping into all the generic code. Maybe that will require bigger design changes or maybe it's entirely unavoidable to achieve what we need, but I'd like to make sure we've given fair consideration to potential alternatives.

(Requesting changes so this shows up in my review queue)

In addition to that, it breaks tests with ubsan. :)

I raised a similar concern in one of the other patches. I'm of the opinion that it's not worth pretending like the scripted processes (platforms, etc.) are plugins. I don't think that it can be achieved without some sort of a leakage, and I don't think it makes sense given that the part that's really interesting to have pluggable is being able to plug a different scripting language into the scripted process. I don't exactly have an opinion on this metadata argument, but it definitely looks sub-optimal.

Given this uncertainty, and the ubsan problem, maybe we can revert this for the time being?

lldb/unittests/Platform/PlatformTest.cpp
90

m_debugger_sp is never initialized. I guess this kinda works because the test does not actually dereference it, but UBSAN lights it up straight away.