This is an archive of the discontinued LLVM Phabricator instance.

Convert ScriptInterpreters to being first-class plugins
AbandonedPublic

Authored by zturner on Jul 22 2015, 2:41 PM.

Details

Summary

This patch converts ScriptInterpreterPython and ScriptInterpreterNone into plugins as discussed on the mailing list and in previous reviews (e.g. D10189)

This has a number of benefits:

  1. Generic code no longer makes assumptions about the type of script interpreter in use.
  2. All interaction between LLDB and Python is localized and isolated to the plugin.
  3. A link-time dependency is removed from all lldb_private libraries to libpython.
  4. lldbAPI no longer needs to do a funny dance where it initializes the interpreter with function pointers, the interpreter plugin can just bind to them directly as extern.

This patch creates a couple of new projects as well as alters the structures of some existing ones. So I will need some help getting this working on MacOSX. I am going to tinker with the Xcode project, but I suspect I will get stuck, so if someone can help me out I would appreciate it.

Diff Detail

Event Timeline

zturner updated this revision to Diff 30403.Jul 22 2015, 2:41 PM
zturner retitled this revision from to Convert ScriptInterpreters to being first-class plugins.
zturner updated this object.
zturner added a reviewer: clayborg.
zturner added a subscriber: lldb-commits.
zturner updated this revision to Diff 30432.Jul 22 2015, 5:15 PM

This gets the Xcode project mostly working. But there is something related to linking the LLDBSwig methods. I don't know how this is setup on Mac, but the code works as-is on all other platforms.

Is there any way to make this work on Mac? It's much better and more logically organized using the way I've done in this patch, so it would be a shame if there's no way to make this work in the Xcode build. Can you take a look Greg?

clayborg edited edge metadata.Jul 22 2015, 5:34 PM
clayborg added a subscriber: clayborg.

I'll try and take a look first thing tomorrow.

Greg

Hi Greg, did you have a chance to look at this yet?

Hi Greg,

I wonder if this has something to do with the fact that the SWIG generation shell script is one of the build phases of LLDB.Framework as you mentioned once before here: http://lists.cs.uiuc.edu/pipermail/lldb-dev/2015-February/006685.html. Is it possible to change this so that the SWIG wrappers are built along with the regular source? It seems to me like LLDBWrapPython.cpp should be compiled into Plugins/ScriptInterpreter/Python

Or, alternatively, manually fiddle with the linker line as a last resort? This might be a better approach for now, because moving the location of LLDBWrapPython.cpp might require changes to the shells cript and python script. Which we can do, but it seems better as a followup CL just so that we can do things in small pieces in order to keep moving forward.

I'm still a little unclear why the problem happens though. If LLDBWrapPython.cpp is being linked into liblldb, and ScriptInterpreterPython is being built as a static library, then where are the linker errors coming from? They're declared extern in the Python plugin, which shouldn't care that they're not visible at link time, because liblldb contains all the definitions. What part am I missing?

zturner updated this revision to Diff 30742.Jul 27 2015, 3:10 PM
zturner edited edge metadata.

Fixes the Xcode build by backing out the portion of the CL which converts function pointers to linker bound calls in the Python plugin. This can be handled as a followup patch.

Mac OSX build completes successfully, so this should be working on all platforms now.

Let me know if there are still any remaining issues I need to work out.

Hi Greg, any comments on this? AFAIK there are no outstanding issues and it should be ok to go in, but as it's kind of large I want to make sure you don't have any remaining concerns before I go in.

Ping. If you don't have time to review this in detail, what do you think about just going in with the change as-is, and I'll keep an eye on things and make sure to be responsive to any breaks, reverting if necessary.

It's all using the plugin system now, which was your primary issue back in June, so unless there's remaining issues that haven't been brought up yet, I think I've addressed everything?

Compiles on Linux, Mac, and Windows, and no test regressions that I can see.

I have time today. I will check this out.

zturner updated this revision to Diff 31055.Jul 30 2015, 11:05 AM

Rebase against ToT

clayborg requested changes to this revision.Jul 30 2015, 11:31 AM
clayborg edited edge metadata.

A few unused things to remove and a reformat of a constructor and this is good to go!

include/lldb/API/SBCommandInterpreter.h
100–102

Remove unused definition

source/API/SBCommandInterpreter.cpp
11

Discard this change

source/Interpreter/CommandInterpreter.cpp
105–122

Reformat. Please don't do the comma at the beginning of a line.

This revision now requires changes to proceed.Jul 30 2015, 11:31 AM

Thanks. Also you reminded me that I need to file a bug to get that
constructor format issue fixed in clang-format.

zturner abandoned this revision.Oct 15 2015, 1:54 PM
source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h