command script import

Subprojects LLDB
1

Hi all,
the proposed patch is a follow-up to the Python script defining an "import" command.
This one patch introduces a new "command script import" with equivalent semantics, but implemented in C++ through ScriptInterpreterPython and not cluttering the root namespace, as Jim was suggesting.
Also as an addition, modules imported through "command script import" can define an __lldb_init_module(SBDebugger,session_dict) function that is called after import-ing the new module. The included test case (I put it in python_api/import, but if this is not the best location, I can easily fix that) also provides an example of how to use the feature.

Any feedback and ideas for improvements are, as usual, most welcome.

Sincerely,
- Enrico Granata

importcmd.diff (30.9 KB)

2

Enrico, this looks great. The only quibble I have is that you do a bunch of path manipulation by hand here that should be done through FileSpec. For instance, calling your SplitPathInDirAndFile, and then running through Python to expand ~ isn't necessary, you should be able to just create a FileSpec, passing in "resolve_path = true" and then grab the file name and the directory name, or grab the full path. If there is something wrong with the FileSpec implementation that's keeping you from using it this way, we should fix that. Also, instead of pulling off the .pyc or .py extension by hand it would be good to add this functionality to FileSpec, since that is also generally useful. Something like GetFileNameExtension and GetFilenameRemovingExtension or whatever...

Jim

3

Hi all,

hopefully the third time is the charm :slight_smile:

The only quibble I have is that you do a bunch of path manipulation by hand here that should be done through FileSpec.

Also, instead of pulling off the .pyc or .py extension by hand it would be good to add this functionality to FileSpec, since that is also generally useful.

I have modified the implementation following Jim’s advice, and now I use FileSpec directly. I have also added a method named GetFilenameExtension() to said class.

As to the test case, no, it does not belong in the python_api subdirectory. You can create a
functionalities/command_script subdir to house the import dir.

It looks like you had already created functionalities/command_script. I have created an import subdirectory there and moved my test case files there.

Thanks for the feedback on this patch.

Sincerely,

importcmd.diff (30.8 KB)

4

Sorry, a few more quibbles.

Why not add FileSpec::GetFilenameStrippingExtension or something like that as well, since that's what you really use? That seems like another useful function. Don't take out the extension one, that also seems good.

What a pain that you can't import a Python module w/o adding its containing directory to the Python Path! There must be some reason why, but sigh...

Why do you have to do this bit:

+ FILE *tmp_fh = (python_interpreter->m_dbg_stdout ? python_interpreter->m_dbg_stdout : stdout);

5

Hi all,

Sorry, a few more quibbles.

Why not add FileSpec::GetFilenameStrippingExtension or something like that as well, since that’s what you really use? That seems like another useful function. Don’t take out the extension one, that also seems good.

I added that in the attached patch file.

What a pain that you can’t import a Python module w/o adding its containing directory to the Python Path! There must be some reason why, but sigh…

I guess Python is trying to be smart in handling dependencies (that way, if you import foo, and foo needs to import bar, as long as bar is in the path you need not worry about its location on disk). Unfortunately, it looks like there is no trivial way to override that.

Why do you have to do this bit:

  • FILE *tmp_fh = (python_interpreter->m_dbg_stdout ? python_interpreter->m_dbg_stdout : stdout);
  • {
  • Locker py_lock(python_interpreter, tmp_fh);

That seems non-obvious and at least merits a comment.

This is part of the original implementation of the interaction between LLDB and Python. Basically, Python is single-threaded and LLDB tries to acquire the lock before sending commands. I am not sure about the reason why a file handle to which to write some text is required (informative for the user? trying to enforce a wait that the optimizer will not strip?), but this behavior is consistently replicated throughout ScriptInterpreterPython, so I guess the original coder of that class had her motives.
A good point would be to move the code that calculates tmp_fh into the Locker class itself. IMHO that would be best served by writing a separate patch because there are so many methods doing exactly that code snippet (I can deal with that once we’re done working on this patch if you want).

Finally since there seem to be several reasons why LoadScriptingModule could fail in this function, it would be better if it took an Error & parameter and filled that out appropriately.

There it is.
I have also taken the time to check against double import. If we try to double import a same module, Python will report no error, and yet the second import silently does nothing. Now I do check for that condition, and the new command does report an error on double import (this would not be harmful in itself, but because we would end up calling __lldb_module_init twice, there might be unwelcome side effects in doing that).

Jim

Thanks,

  • Enrico Granata

importcmd.diff (34.4 KB)

6

That looks good, check it in. If you have the time to centralize the code that locks what needs locking for Python, that would be great. This sounds like the sort of thing that might not stay the same over time, so it would be good to have it in one place.

Jim