[lldb] Use (only) PyImport_AppendInittab to patch readline

[JDevlieghere]

The current implementation tries to (1) patch the existing readline module definition if it's already present in the inittab and (2) append our patched readline module to the inittab. The former (1) uses the non-stable Python API and I can't find a situation where this is necessary. We do this work before initialization, so for the readline module to exist, it either needs to be added by Python itself (which doesn't seem to be the case), or someone would have had to have added it without initializing.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

PyImport_AppendInittab will replace an existing module if it's already present in the inittab, so we shouldn't need the code that iterates through the modules. The old code also wasn't setting ReadlinePatched so we end up unconditionally calling PyImport_AppendInittab anyway.

---

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

1 Files Affected:

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

diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
index 5b97fcb5acf58..a04cecac96355 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -98,17 +98,7 @@ struct InitializePythonRAII {
 #ifdef LLDB_USE_LIBEDIT_READLINE_COMPAT_MODULE
       // Python's readline is incompatible with libedit being linked into lldb.
       // Provide a patched version local to the embedded interpreter.
-      bool ReadlinePatched = false;
-      for (auto *p = PyImport_Inittab; p->name != nullptr; p++) {
-        if (strcmp(p->name, "readline") == 0) {
-          p->initfunc = initlldb_readline;
-          break;
-        }
-      }
-      if (!ReadlinePatched) {
-        PyImport_AppendInittab("readline", initlldb_readline);
-        ReadlinePatched = true;
-      }
+      PyImport_AppendInittab("readline", initlldb_readline);
 #endif
 
       // Register _lldb as a built-in module.

[JDevlieghere]

Part of #151617

[JDevlieghere]

This was the reason I had to revert #153119. The pre-commit bot didn't catch it, not because it doesn't use LLDB_USE_LIBEDIT_READLINE_COMPAT_MODULE, but because it uses SWIG 4.0 so it wasn't building with the limited API.

[labath]

It sounds like the goal of the original code was the override/patch a existing readline module definition. Will PyImport_AppendInittab overwrite an existing definition? If it doesn't, the fact that we're calling it might not matter if whoever reads this list picks the first matching item.

Or maybe we don't need to worry about a preexisting readline impl for some reason?

[JDevlieghere]

It sounds like the goal of the original code was the override/patch a existing readline module definition. Will PyImport_AppendInittab overwrite an existing definition?

Yesterday I found online that it did, but when I went to check the cpython implementation, it just memcopy's the new table after the existing one.

If it doesn't, the fact that we're calling it might not matter if whoever reads this list picks the first matching item.
Or maybe we don't need to worry about a preexisting readline impl for some reason?

I built LLDB with my change and the tests pass and script behaved correctly so that may indeed be the case. The most likely explanation would be that there isn't an existing readline implementation in the inittab, which is easy to confirm. The only question then becomes if there's a chance that someone else adds it before we do. I think because we do this before initialization the possibility of that is small, but maybe you can think of a scenario where that could happen?

[JDevlieghere]

Confirmed using an assert that there's no readline module in the inittab when launching lldb or when doing import lldb from Python, even after doing import readline.

[labath]

Okay, let's give that a shot then.