This is an archive of the discontinued LLVM Phabricator instance.

Python split [1/2] - File renames
Needs ReviewPublic

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

Details

Reviewers
vharron
clayborg
Summary

The solution I have for making sure the history stays in tact is to commit in 2 separate revisions. One that only renames files, and another that only modifies the files after they've been renamed. Although the file renames will show up as adds / deletes in this web interface, they should be renames on the server. I can verify locally that git log --follow shows history for every file.

So, in summary - this change doesn't actually need to be reviewed, it's just here in case you want to apply these patches locally in succession to test the patch. The deleted and added files have identical content.

Diff Detail

Event Timeline

zturner updated this revision to Diff 20884.Feb 27 2015, 1:15 PM
zturner retitled this revision from to Python split [1/2] - File renames.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added a subscriber: Unknown Object (MLST).
clayborg requested changes to this revision.Feb 27 2015, 1:21 PM
clayborg edited edge metadata.

We probably need to move the ScriptInterpreter folder into the "lldb/source/API" directory:

For general script interpreter files that aren't tied to a language:

lldb/source/API/ScriptInterpreter

Then all python stuff should go into:

lldb/source/API/ScriptInterpreter/Python

This pertains to my comments in your other Python related changes.

This revision now requires changes to proceed.Feb 27 2015, 1:21 PM

Is there any situation where using an embedded interpreter makes sense when API is not built? I can't think of one, just making sure. If there's not, then it seems like a logical place for it.

Also do we need to move the entire ScriptInterpreter folder, or just the language-specific folders? If we move the whole folder, then if you don't compile API, you won't even have definitions for the base class. And people frequently need this for example to get a pointer to the script interpreter from the command interpreter. So I'm thinking we should only move the language specific folder, and leave ScriptInterpreter available even when API is not built. Thoughts?

A little more clarification: it would be great if we can make the script interpreters link agains only the public API. So this is another vote for moving the script interpreter stuff down into "lldb/source/API/ScriptInterpreter".

Right now we have the restriction that The lldb::SB classes can only by code that is in the lldb/API folder. For that reason we have these callbacks in ScriptIntpreterPython that start with "LLDBSwigPython" and they currently can't use the lldb::SB classes because of where the ScriptInterpreterPython.cpp file is currently located: "lldb/source/Interpreter". So if we move the ScriptInterpreterPython.cpp into "source/API" we can move these callbacks to actually use the lldb::SB classes so this function:

extern "C" bool LLDBSwigPythonBreakpointCallbackFunction(const char *python_function_name, const char *session_dictionary_name,
                                                         const lldb::StackFrameSP &sb_frame, const lldb::BreakpointLocationSP &sb_bp_loc);

Would become:

bool LLDBSwigPythonBreakpointCallbackFunction(const char *python_function_name, const char *session_dictionary_name, const lldb::SBFrame &sb_frame, const lldb::SBBreakpointLocation &sb_bp_loc);

Then we can actually build the script interpreters as actual bonified plug-ins that are loaded by LLDB when it starts up since they will only link against the public API.

If we go the plug-in route, then we can avoid moving stuff into "lldb/source/API" and just make a new "lldb/plugins/ScriptInterpeters/Python" folder and they just becomes clients of our public lldb.so API.

It sounds like the first approach (lldb/API/ScriptInterpreter) is
preferred. I will try to get that working and upload when I have something.

It really can't because SWIG links against the lldb::SB API.

zturner updated this revision to Diff 20917.Feb 27 2015, 5:08 PM
zturner edited edge metadata.

Attempt 2 at file renames. This makes source\API\Bindings\Python and a corresponding include folder. I have this building and linking on Windows, python works etc. I'll update the other patch with the updated second part. The formatting issue should be fixed as well, so it should be easy to see what actually changed.

I haven't tried this on Linux yet, plan to do so. But want to see if this is the right high-level organization that can make it work with the Xcode build first.

Bumping this to the top since I updated it last Friday. I don't like to leave large file renames pending for too long because merges involving file renames is always ugly.

zturner updated this revision to Diff 21208.Mar 4 2015, 9:53 AM
zturner edited edge metadata.

Rebase against ToT

clayborg requested changes to this revision.Mar 4 2015, 11:00 AM
clayborg edited edge metadata.

It seems like "lldb/Interpreter/ScriptInterpreter.h" still exists in your tree and you are able to compile because it is still there? It isn't there for me and there are 20 locations that are still trying to include it.

After fixing 100 or so build errors I am giving up on this patch. Please try to make it work for MacOSX. The major issues are:

  • There is no need to create a new "include/lldb/ScriptInterpreter" directory it will just make merges very hard for us and it gains us nothing since only abstract virtual classes should be in there (and a default implementation for None)
  • Where ever there is a #ifndef LLDB_DISABLE_PYTHON, this will need to be abstracted to go through the abstract ScriptInterpreter class for the current language.
  • No one should be including ScriptInterpreterPython.h anywhere, they should just use the current ScriptInterpreter subclass gotten from the interpreter and anything that was being done using a special version of this class should be abstracted through ScriptInterpreter
  • cases like ProcessGDBRemote::ParsePythonTargetDefinition(const FileSpec &target_definition_fspec) should find the script interpreter for the target definition file and call through the abstract ScriptInterpreter class to parse it somehow
This revision now requires changes to proceed.Mar 4 2015, 11:00 AM

It seems like "lldb/Interpreter/ScriptInterpreter.h" still exists in your tree and you are able to compile because it is still there? It isn't there for me and there are 20 locations that are still trying to include it.

After fixing 100 or so build errors I am giving up on this patch. Please try to make it work for MacOSX. The major issues are:

  • There is no need to create a new "include/lldb/ScriptInterpreter" directory it will just make merges very hard for us and it gains us nothing since only abstract virtual classes should be in there (and a default implementation for None)

So leave it in Interpreter? I'm fine with that, I just don't want it to be under the API folder, since it should be accessible even when API is not built.

  • Where ever there is a #ifndef LLDB_DISABLE_PYTHON, this will need to be abstracted to go through the abstract ScriptInterpreter class for the current language.

Agree, but this is a MUCH larger change, and I wanted to take the steps incrementally. That has been the end goal all along, but it's too much to do all at once.

  • No one should be including ScriptInterpreterPython.h anywhere, they should just use the current ScriptInterpreter subclass gotten from the interpreter and anything that was being done using a special version of this class should be abstracted through ScriptInterpreter

Same as before, I agree, but that's a much larger change. This is only intended to be a first step.

  • cases like ProcessGDBRemote::ParsePythonTargetDefinition(const FileSpec &target_definition_fspec) should find the script interpreter for the target definition file and call through the abstract ScriptInterpreter class to parse it somehow

Same as before.

Also, it sounds like the reason the patch didn't work for you and you had to fix a bunch of errors is because you only applied this patch and not the subsequent one, D7957. This patch only renames files, it doesn't change any content. The reason it was done that way is because it's the only way to get the review tool to not conflate the two (even if git detects it as a rename, when you upload the tool will show one mass of deleted content and another mass of (different) added content. The way around this is two patches. One that only renames, and one that only fixes up changes.

In any case, I will try to get it working on Mac.

zturner updated this revision to Diff 21223.Mar 4 2015, 2:00 PM
zturner edited edge metadata.

This takes Greg's suggestions into account. It removes the ScriptInterpreter folder, and moves ScriptInterpreter.h/cpp back to their locations in source/Interpreter and include/lldb/Interpreter.

NOTE: This patch won't work without subsequently applying D7957. You *must* apply D7956 followed by D7957.

Everything compiles, the only thing is I'm getting link errors with argdumper. It's hard to diff / merge Xcode workspace files, so rather than muck it up with what I think works, I'll just mention this in hope of suggestions.

zturner updated this revision to Diff 21312.Mar 5 2015, 2:12 PM
zturner edited edge metadata.

Rebase against ToT. I don't really want this to be blocked much longer. All the compile errors are solved, and this looks like strictly a build system issue at this point. I have updated the Xcode project as much as I can, and I even pulled in another person who is familiar with Xcode to try to work through the linker errors, but we did not much make much progress. I need to get this in within the next couple days.

Please make sure you apply both patches. The reason there were issues last time is because I believe only the first patch was applied. You need both.

granata.enrico resigned from this revision.Oct 15 2015, 1:34 PM
granata.enrico removed a reviewer: granata.enrico.
source/API/Bindings/Python/embedded_interpreter.py