This is an archive of the discontinued LLVM Phabricator instance.

Python Split [2/2] - Content change
AbandonedPublic

Authored by zturner on Feb 27 2015, 1:17 PM.

Details

Summary

This patch can only be successfully applied to a repo that already has [1/2] applied. This is the core meat of the change.

See D7946 for more context and information about this patch.

Diff Detail

Event Timeline

zturner updated this revision to Diff 20886.Feb 27 2015, 1:17 PM
zturner retitled this revision from to Python Split [2/2] - Content change.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added a subscriber: Unknown Object (MLST).

Bah. It looks like I need to fix the formatting. I didn't notice it when it was all one gigantic changelist, but now that it's not, I noticed that "git clang-format" reformatted the entire patch. Hold off on this until I get that straightened out.

zturner updated this revision to Diff 20918.Feb 27 2015, 5:09 PM

Attempt 2, formatting issues should be fixed.

Bump for part 2

zturner updated this revision to Diff 21209.Mar 4 2015, 9:54 AM

Rebase against ToT

zturner updated this revision to Diff 21211.Mar 4 2015, 10:13 AM

Added some missing headers after the rebase.

zturner updated this revision to Diff 21224.Mar 4 2015, 2:01 PM

Updates for part 2 of the patch. This also updates the Xcode workspace. After applying D7956 and D7957 there will be link errors in argdumper, but everything else appears to work.

I almost fixed the argdumper linker errors, but there's a problem.

OperatingSystemPython is linked into lldb-core, so when argdumper links against lldb-core.a, it tries to link OperatingSystemPython, which then requires we bring in everything else. But we can't link API into lldb-core, so it doesn't work.

Is it possible to have OperatingSystemPython not be part of lldb-core, but instead be part of lldb-core-python or something? This way argdumper could just link against lldb-core.a but not lldb-core-python.a, and lldb could link against both.

I don't know how to set this up in Xcode though.

zturner updated this revision to Diff 21314.Mar 5 2015, 2:12 PM

Rebase against ToT. See part 1 for more information. This patch and part 1 are coupled, please make sure to apply them at the same time.

clayborg edited edge metadata.Mar 5 2015, 2:36 PM

So seems like part of the work required for this patch to work is to change any references in code from PythonList, PythonString, PythonInteger, and PythonDictionary to use classes from lldb/Core/StructuredData.h.

The means we probably need to have OperatingSystemPython rely only upon virtual functions in ScriptInterpreter and have those functions return stuff from StructuredData instead of python variants:

PythonDictionary dictionary(m_interpreter->OSPlugin_RegisterInfo(m_python_object_sp));

Will need to become:

StructuredData::Dictionary dictionary(m_interpreter->OSPlugin_RegisterInfo(m_python_object_sp));

We then need to modify the Python callbacks that return PythonList, PythonString, PythonInteger, and PythonDictionary objects, to convert them into StructuredData::Array, StructuredData::String, StructuredData::Integer and StructuredData::Dictionary respectively.

clayborg requested changes to this revision.Mar 5 2015, 2:45 PM
clayborg edited edge metadata.

ProcessGDBRemote::ParsePythonTargetDefinition() will also need to be updated to use StructuredData objects and the conversion from Python to StructuredData objects will need to be done in ScriptInterpreterPython.

So:

lldb::ScriptInterpreterObjectSP target_definition_sp (interpreter->GetDynamicSettings(module_object_sp,
                                                                                      &GetTarget(),
                                                                                      "gdb-server-target-definition",
                                                                                      error));

PythonDictionary target_dict(target_definition_sp);

Will need to become:

lldb::StructuredDataObjectSP target_definition_sp (interpreter->GetDynamicSettings(module_object_sp,
                                                                                      &GetTarget(),
                                                                                      "gdb-server-target-definition",
                                                                                      error));

StructuredData::Dictionary *dict = target_definition_sp->GetAsDictionary();
if (dict)
{
}

So Python* data types should _only_ exist in ScriptInterpreterPython code and should always be converted to StructuredData types before being given out to any code within LLDB.

This revision now requires changes to proceed.Mar 5 2015, 2:45 PM

There is one more issue which I was unable to solve, which is that
ScriptInterpreter::Initialize() calls
ScriptInterpreterPython::Initialize(). So anyone linking against
liblldb-core.a needs to pull in the python stuff too, which isn't always
possible. This works fine in the CMake build, but I wasn't able to figure
out how to make it work in the Xcode build. I could get argdumper to
build, but not lldb-server. If I tried to have lldb-server depend on the
python stuff under API\Bindings\Python, then I got errors about not being
able to find SB classes, I guess because of something to do with framework?

SBCommandInterpreter::InitializeSWIG () seems like a good place to do this. This could be renamed to be InitializeScriptInterpreters() and then you can do this if and only if python is enabled.

SBDebugger::Initialize () is the main function that must be called prior to using anything in the lldb::SB* layer, and it currently looks like:

void
SBDebugger::Initialize ()
{
    Log *log(GetLogIfAllCategoriesSet (LIBLLDB_LOG_API));

    if (log)
        log->Printf ("SBDebugger::Initialize ()");

    SBCommandInterpreter::InitializeSWIG ();

    Debugger::Initialize(LoadPlugin);
}

You might need to call Debugger::Initialize() first so all plug-ins registers themselves, and then call SBCommandInterpreter::InitializeScriptInterpreters() after to ensure everything is setup as the ScriptInterpreterPython will need to register itself with the ScriptInterpreter if it already isn't doing so.

Makes sense, I will work on this today. Thanks

ScriptInterpreter will need to keep a map of language to static create functions (ScriptInterpreterPython::CreateInstance()) so it can create the interpreter when needed. So the SBCommandInterpreter::InitializeScriptInterpreter() would:
1 - initialize all supported languages (ScriptInterpreterPython::Initialize())
2 - call ScriptInterpreter::Initialize() which would finalize the current script interpreters and select a default one for execution by checking the setting:

(lldb) settings show script-lang
script-lang (enum) = python

The default language is gettable using Debugger::GetScriptLanguage().

I think this part needs to be done as a separate patch. Makes it easier to
review and understand the impact of. So I will do this part first
(basically delete ScriptInterpreterObject and move everything to
StructuredData) and commit that, then rebase this on top of that and go
from there.

zturner abandoned this revision.Oct 15 2015, 1:51 PM