[lldb][NFC] Defer python init until ScriptInterpreter is created

[danobi]

Previously python was initialized during static registration of the plugin. This causes the python interpreter to run even if python support is explicitly disabled thru:

SBDebugger::SetScriptLanguage(ScriptLanguage::eScriptLanguageNone)

This commit defers python initialization until a ScriptInterpreterPython instance is created.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[danobi]

Also see: bpftrace/bpftrace#3359

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Daniel Xu (danobi)

Changes

Previously python was initialized during static registration of the plugin. This causes the python interpreter to run even if python support is explicitly disabled thru:

SBDebugger::SetScriptLanguage(ScriptLanguage::eScriptLanguageNone)

This commit defers python initialization until a ScriptInterpreterPython instance is created.

---

Full diff: https://github.com/llvm/llvm-project/pull/105757.diff

1 Files Affected:

• (modified) lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp (+4-1)

diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
index 335c482f8495ad..bc4e7485067048 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -352,7 +352,6 @@ void ScriptInterpreterPython::Initialize() {
                                   GetPluginDescriptionStatic(),
                                   lldb::eScriptLanguagePython,
                                   ScriptInterpreterPythonImpl::CreateInstance);
-    ScriptInterpreterPythonImpl::Initialize();
   });
 }
 
@@ -429,6 +428,10 @@ ScriptInterpreterPythonImpl::ScriptInterpreterPythonImpl(Debugger &debugger)
       m_active_io_handler(eIOHandlerNone), m_session_is_active(false),
       m_pty_secondary_is_open(false), m_valid_session(true), m_lock_count(0),
       m_command_thread_state(nullptr) {
+  static llvm::once_flag g_once_flag;
+  llvm::call_once(g_once_flag, []() {
+    ScriptInterpreterPythonImpl::Initialize();
+  });
 
   m_dictionary_name.append("_dict");
   StreamString run_string;

[danobi]

Test failure looks unrelated. I see it in other PRs as well.

[JDevlieghere]

There's a few things I don't really like about this patch:

• It's very specific to a single plugin.
• It sidesteps the plugin's Initialize/Terminate paradigm. It happens to work because ScriptInterpreterPython::Terminate is a NOOP (see the comment for ScriptInterpreterPythonImpl::Terminate on why). It's not that we don't have other places where things persist after termination, but as we've seen, that can cause problems, for example in unit testing.
• It relies on changing the scripting language early enough to make sure the plugin doesn't get loaded. If the code changes and tries to use the plugin before you have the opportunity to call SetScriptLanguage you're back to square one.

If it's important to disable certain plugins at runtime, I wonder if we shouldn't do this in a more structured way.

• We could make the initialization of all plugins lazy. If we're always going through the plugin interface, this should work. The question is if this is beneficial.
• We could extend SBDebugger::Initialize with a list of plugins to disable. I expect this to be somewhat contentious though, as there are certain plugins that depend on other plugins. We had a long discussion about this when I wrote a patch to do this at compile time, which would have the same limitations as doing it at runtime.
• A combination or hybrid of the two, where you can tell which plugins to load lazily and which ones not to load.

Can you elaborate on why it's important that the PythonScriptInterepter doesn't get initialized? Maybe that can help guide the solution instead of working back from the current patch.

[danobi]

Hi @JDevlieghere , thanks for taking a look at this.

I'm not very familiar with lldb in general so I can't comment much on alternatives. Lazy loading does some fairly reasonable to me but I don't know what kind of code changes that'd require.

The motivation behind this change starts with bpftrace/bpftrace#3329. Basically we got a bug report from a user saying they're getting python errors when running bpftrace. bpftrace does not use python at runtime anywhere, so it was quite surprising. It turns out that lldb was running python standard library code. And the environment bpftrace was running in (appimage) does not have any python support, so no standard library available.

For context, bpftrace uses liblldb to parse DWARF. We used to directly use libdw but it was kinda hard to work with. liblldb has a very nice high level API that we found useful for velocity. We never have (and probably never will) use python bindings to lldb.
The reason we can't just compile out python bindings is b/c we need to build with whatever distros give us. And my understanding is that distros are more or less forced to build with bindings on so that lldb (the cli tool) will function.

You're right that this patch only addresses a subset of the real goal - the real goal being we want to disable all plugins at runtime. We don't really have a need for plugins. It'd probably help with performance for other similar liblldb users as well.

Hopefully this essay makes the goal more clear. Please lemme know how we should proceed. I'd be happy to help out if it's not too complicated.

[danobi]

Hi @JDevlieghere, we hit this issue again recently. We're wondering if we have a path forward for landing this fix.

[labath]

I'm also not very thrilled by this. I think your use case would be very hard to support in the long term. From the sound of things, you're using a fairly limited subset of the lldb api (SBModule, SBCompileUnit, SBType?), but even so, i think we can't guarantee that neither of these APIs will ever cause a script interpreter to be loaded (lazily).

In your place, I'd look into other alternatives. I'm not sure what "level" you want to view the debug info at, but llvm has another DWARF parser which is more suitable for library usage (llvm/DebugInfo/DWARF). There's also the "logical view" library (llvm/DebugInfo/LogicalView) which should give you a higher level view of the information, though I have no actual experience with it.

[danobi]

Hi @labath, sorry for late reply, I must've missed a notification :(

That makes sense to me - perhaps LLDB is not the right dependency for us to take.