This is an archive of the discontinued LLVM Phabricator instance.

The great Python split
Needs ReviewPublic

Authored by zturner on Feb 27 2015, 10:44 AM.

Details

Summary

(This CL looks scarier than it really is. It's honestly not that great of a split.)

After the discussion on list, I took it up on myself to make my ideas concrete. Along the way I learned some interesting things.

  1. There are definitely some assumptions baked into places where they shouldn't be about the existence of Python. See some of the Type summary code and data formatters for examples. Or grep for LLDB_DISABLE_PYTHON. This isn't anyone's fault, it's just a consequence of the weak barrier between Python and the rest of LLDB.
  1. It is pretty much impossible right now to link Python into some binaries and not others. A concrete example of what I'm talking about is LLGS. All of our decisions are based on conditionally compiling code based on the value of a global flag, LLDB_DISABLE_PYTHON. If you want python for the host but not the server, this can't really be done without compiling LLDB core libraries twice. If you try not to link Python into lldb-server you will get linker errors because it links against some .a file which has already been compiled without LLDB_DISABLE_PYTHON.
  1. There were some hidden race conditions involving Python initialization. Because python code was split between lldbAPI, Interpreter, and LLDBWrapPython.cpp (which was linked into liblldb), there were some complexity introduced to make sure Python was initialized at the right time.

#2 and #3 are largely solved with this patch. With a few minor exceptions, all python code is now compiled into a single library, lldbPythonInterpreter. The end goal is to get *all* Python code into this library. This will solve #1 as well, and fill in the missing holes needed to finish off #2 (just don't link in lldbPythonInterpreter.a into llgs and that's it).

The overall structure of script interpreter is now as follows:

lldb

_source
_Interpreter
_ScriptInterpreter
_ScriptInterpreter.cpp
_ScriptInterpreterNone.cpp
_PythonInterpreter
_ScriptInterpreterPython.cpp
_PythonDataObjects.cpp
_LLDBWrapPython.cpp
_PythonPointer.h

The idea is that the rest of LLDB should be shielded from the particular language of the interpreter. It seems like that was the original design goal anyway, this just makes it explicit. The advantage of this is that now someone who doesn't want Python can easily just not link against lldbPythonInterpreter. Of course they could do this already by using LLDB_DISABLE_PYTHON, but that doesn't solve the issue of wanting Python sometimes and not others. All uses of LLDB_DISABLE_PYTHON will eventually end up hidden inside of PythonInterpreter.

I know LLGS wants to not link against Python. I believe this patch is a necessary first step. Importantly, it removes the dependency from the CommandInterpreter on Python. But it is not yet sufficient. There are still some references to Python in the TypeSummary stuff and Data formatter code which will cause an implit dependency on Python.

Note: There is no Xcode project here. I know this probably makes it hard to review for people at Apple. I plan to work on updates to the Xcode project today, I just wanted to get this up in the meantime.

Note 2: If you have a diff utility that works across renames it makes looking at this patch much less scary. Most changes were purely mechanical (changing incldue paths). There were only a few that were not, mostly surrounding factory creation of the ScriptInterpreter, and a complete removal of the callback-initialization-via-function-pointer that was used (callbacks are now initialized automatically through static linkage).

Diff Detail

Event Timeline

zturner updated this revision to Diff 20862.Feb 27 2015, 10:44 AM
zturner retitled this revision from to The great Python split.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner updated this object.
zturner added a subscriber: Unknown Object (MLST).
zturner updated this object.Feb 27 2015, 10:48 AM

Also, just to re-iterate since I don't want this point to get lost in the noise, this is mostly a straight move of about 5 source/header files and header fixups. The only places where it's more than just a code re-org are:

  1. ScriptInterpeter::InitializeInterpreter(<huge list of function pointers>) no longer exists. Callbacks are resolved through static linkage
  2. CommandInterpreter used to have a std::unique_ptr<ScriptInterpreter> This is now a global variable in ScriptInterpreter, and that library has ownership of the pointer now.
emaste added a subscriber: emaste.Feb 27 2015, 11:02 AM
clayborg edited edge metadata.Feb 27 2015, 11:09 AM

Overall looks good I would like you to "svn move" the files instead of deleting them. Not sure if this is what you did, but we really need the SVN history on these files to stay intact. Not sure if this is just a limitation of this review site, but it says "file deleted" and "file added" which makes question this. Can you verify that you actually "svn move"ed the files?

I used git, but my limited knowledge of git told me that there was no way I could force it to be a rename, because in the end git does similarity matches and if it can't figure out that two files are similar enough, there's no way to make it know that it was a rename. I'm very likely wrong about this though, so I'll try to make it work. Your concern is warranted though, because when I do git log on the files in my tree, there's no history. So I definitely need to figure it out before comitting.

After fixing the Xcode project file and trying to compiler: argdumper and lldb-server fail to link as they can't find the LLDBSwig stuff:

Undefined symbols for architecture x86_64:
  "_LLDBSwigPythonCallCommand", referenced from:
      lldb_private::ScriptInterpreterPython::RunScriptBasedCommand(char const*, char const*, lldb_private::ScriptedCommandSynchronicity, lldb_private::CommandReturnObject&, lldb_private::Error&, lldb_private::ExecutionContext const&) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSwigPython_GetValueSynthProviderInstance", referenced from:
      lldb_private::ScriptInterpreterPython::GetSyntheticValue(std::__1::shared_ptr<lldb_private::ScriptInterpreterObject> const&) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSWIGPythonRunScriptKeywordValue", referenced from:
      lldb_private::ScriptInterpreterPython::RunScriptFormatKeyword(char const*, lldb_private::ValueObject*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&, lldb_private::Error&) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSwigPython_CalculateNumChildren", referenced from:
      lldb_private::ScriptInterpreterPython::CalculateNumChildren(std::__1::shared_ptr<lldb_private::ScriptInterpreterObject> const&) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSWIGPythonRunScriptKeywordThread", referenced from:
      lldb_private::ScriptInterpreterPython::RunScriptFormatKeyword(char const*, lldb_private::Thread*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&, lldb_private::Error&) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSWIGPythonRunScriptKeywordTarget", referenced from:
      lldb_private::ScriptInterpreterPython::RunScriptFormatKeyword(char const*, lldb_private::Target*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&, lldb_private::Error&) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSWIGPythonRunScriptKeywordProcess", referenced from:
      lldb_private::ScriptInterpreterPython::RunScriptFormatKeyword(char const*, lldb_private::Process*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&, lldb_private::Error&) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSwigPython_UpdateSynthProviderInstance", referenced from:
      lldb_private::ScriptInterpreterPython::UpdateSynthProviderInstance(std::__1::shared_ptr<lldb_private::ScriptInterpreterObject> const&) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSwigPythonCreateSyntheticProvider", referenced from:
      lldb_private::ScriptInterpreterPython::CreateSyntheticScriptedProvider(char const*, lldb_private::SharingPtr<lldb_private::ValueObject>) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSwigPythonCallTypeScript", referenced from:
      lldb_private::ScriptInterpreterPython::GetScriptedSummary(char const*, lldb_private::SharingPtr<lldb_private::ValueObject>, std::__1::shared_ptr<lldb_private::ScriptInterpreterObject>&, lldb_private::TypeSummaryOptions const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSwigPythonWatchpointCallbackFunction", referenced from:
      lldb_private::ScriptInterpreterPython::WatchpointCallbackFunction(void*, lldb_private::StoppointCallbackContext*, unsigned long long) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSwigPythonCreateScriptedThreadPlan", referenced from:
      lldb_private::ScriptInterpreterPython::CreateScriptedThreadPlan(char const*, std::__1::shared_ptr<lldb_private::ThreadPlan>) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSwigPythonBreakpointCallbackFunction", referenced from:
      lldb_private::ScriptInterpreterPython::BreakpointCallbackFunction(void*, lldb_private::StoppointCallbackContext*, unsigned long long, unsigned long long) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSWIGPythonRunScriptKeywordFrame", referenced from:
      lldb_private::ScriptInterpreterPython::RunScriptFormatKeyword(char const*, lldb_private::StackFrame*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&, lldb_private::Error&) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSwigPythonCallModuleInit", referenced from:
      lldb_private::ScriptInterpreterPython::LoadScriptingModule(char const*, bool, bool, lldb_private::Error&, std::__1::shared_ptr<lldb_private::ScriptInterpreterObject>*) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSwigPython_GetIndexOfChildWithName", referenced from:
      lldb_private::ScriptInterpreterPython::GetIndexOfChildWithName(std::__1::shared_ptr<lldb_private::ScriptInterpreterObject> const&, char const*) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSWIGPython_CastPyObjectToSBValue", referenced from:
      lldb_private::ScriptInterpreterPython::GetChildAtIndex(std::__1::shared_ptr<lldb_private::ScriptInterpreterObject> const&, unsigned int) in liblldb-core.a(ScriptInterpreterPython.o)
      lldb_private::ScriptInterpreterPython::GetSyntheticValue(std::__1::shared_ptr<lldb_private::ScriptInterpreterObject> const&) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSWIGPythonCreateOSPlugin", referenced from:
      lldb_private::ScriptInterpreterPython::OSPlugin_CreatePluginObject(char const*, std::__1::shared_ptr<lldb_private::Process>) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSWIGPython_GetDynamicSetting", referenced from:
      lldb_private::ScriptInterpreterPython::GetDynamicSettings(std::__1::shared_ptr<lldb_private::ScriptInterpreterObject>, lldb_private::Target*, char const*, lldb_private::Error&) in liblldb-core.a(ScriptInterpreterPython.o)
  "LLDBSWIGPython_GetValueObjectSPFromSBValue(void*)", referenced from:
      lldb_private::ScriptInterpreterPython::GetChildAtIndex(std::__1::shared_ptr<lldb_private::ScriptInterpreterObject> const&, unsigned int) in liblldb-core.a(ScriptInterpreterPython.o)
      lldb_private::ScriptInterpreterPython::GetSyntheticValue(std::__1::shared_ptr<lldb_private::ScriptInterpreterObject> const&) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSwigPython_GetChildAtIndex", referenced from:
      lldb_private::ScriptInterpreterPython::GetChildAtIndex(std::__1::shared_ptr<lldb_private::ScriptInterpreterObject> const&, unsigned int) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSWIGPythonCallThreadPlan", referenced from:
      lldb_private::ScriptInterpreterPython::ScriptedThreadPlanExplainsStop(std::__1::shared_ptr<lldb_private::ScriptInterpreterObject>, lldb_private::Event*, bool&) in liblldb-core.a(ScriptInterpreterPython.o)
      lldb_private::ScriptInterpreterPython::ScriptedThreadPlanShouldStop(std::__1::shared_ptr<lldb_private::ScriptInterpreterObject>, lldb_private::Event*, bool&) in liblldb-core.a(ScriptInterpreterPython.o)
      lldb_private::ScriptInterpreterPython::ScriptedThreadPlanGetRunState(std::__1::shared_ptr<lldb_private::ScriptInterpreterObject>, bool&) in liblldb-core.a(ScriptInterpreterPython.o)
  "_LLDBSwigPython_MightHaveChildrenSynthProviderInstance", referenced from:
      lldb_private::ScriptInterpreterPython::MightHaveChildrenSynthProviderInstance(std::__1::shared_ptr<lldb_private::ScriptInterpreterObject> const&) in liblldb-core.a(ScriptInterpreterPython.o)
  "_init_lldb", referenced from:
      lldb_private::ScriptInterpreterPython::InitializePrivate() in liblldb-core.a(ScriptInterpreterPython.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Seems the LLDBWrapPython.cpp file is no longer created after this change?

The LLDBWrapPython.cpp was only created for the LLDB.framework on MacOSX, so this causes static linkage to fail.

Oh cool, thanks for trying to fix it. How big of an issue is this? Is it
something that's easy to fix in the Xcode project, or does it mean back to
the drawing board?

So the LLDBWrapPython.cpp is only made along with all of the files inside source/API, which is the public lldb::SB API layer. the LLDB.framework target is what creates these and I am guessing the make/cmake stuff also only makes the swig bindings when this is created. For that reason we can't rely upon ScriptInterpreterPython.cpp being able to link these files. We might be able to if we move your ScriptInterpreter folder into "lldb/source/API"? Then the stuff inside "lldb/source/API" could require that "lldb/source/API/ScriptInterpreter" be linked, but only if we enable python.

So we can make this work if we shuffle things around such that ScriptInterpreterPython.cpp is only built if the "lldb/source//API" stuff is built and we can make LLDBWrapPython.cpp is only created along with the files in "lldb/source/API/ScriptInterpreter/Python"?

Can the Xcode project set up a dependency between the entire
ScriptInterpreterPython project (or target, or library, I'm not sure what
the right term is in Xcode parlance) to the API library? Nothing else
Python-related makes sense to compile either if you haven't built API.

granata.enrico resigned from this revision.Oct 15 2015, 1:33 PM
granata.enrico removed a reviewer: granata.enrico.
jplatte removed a subscriber: jplatte.Sep 10 2019, 5:16 AM
source/ScriptInterpreter/Python/PythonDataObjects.cpp