Bundle libedit-compatible readline replacement

[serge-sans-paille]

Summary

Fix https://bugs.llvm.org/show_bug.cgi?id=43830 while avoiding polluting the global Python namespace.

This both reverts r357277 to rebundle a version of Python's readline module based on libedit.

However, this patch also provides two improvements over the previous implementation:

1. use PyMem_RawMalloc instead of PyMem_Malloc, as expected by PyOS_Readline (prevents to segfault upon exit of interactive session)
2. patch the readline module upon embedded interpreter loading, instead of patching it globally, which should prevent any side effect on other modules/packages
3. only activate the patched module if libedit is actually linked in lldb

[mgorny]

lldb/source/Plugins/ScriptInterpreter/Python/PythonReadline.cpp

9

If libedit is disabled, shouldn't you disable building the whole module altogether? I suppose then the standard Python module is just going to work.

[labath]

Thanks for doing this. I really like how you've implemented this. Just some small stylistic comments inline.

lldb/source/Plugins/ScriptInterpreter/Python/PythonReadline.cpp

2

Should this also be #if !APPLE? IIUC, this is not an issue on apple platforms, and the real readline module is going to be more functional than our mocked up version...

30–33

It looks like this isn't needed then.

83

and this

lldb/source/Plugins/ScriptInterpreter/Python/PythonReadline.h

2–3

Fix header.

14–16

How about if this file just exposes a single function like WorkAroundLibeditIncompatibilityIfNeeded? Then this function can be just a no-op if no work is needed, and there's no need for messy ifdefs anywhere...

[JDevlieghere]

+1 on everything Pavel said :-)

[serge-sans-paille]

lldb/source/Plugins/ScriptInterpreter/Python/PythonReadline.h

14–16

I somehow achieved the same by centralizing the messy ifdefs in one explicit name :-)

[labath]

LGTM. Thanks for doing this.

lldb/source/Plugins/ScriptInterpreter/Python/PythonReadline.cpp

18

Maybe also link the python bug/PR here? IIUC, this is something that would be better fixed on the python side...

lldb/source/Plugins/ScriptInterpreter/Python/PythonReadline.h

14–16

Ok, that works for me.

[labath]

1. use PyMem_RawMalloc instead of PyMem_Malloc, as expected by PyOS_Readline (prevents to segfault upon exit of interactive session)

It looks like this is failing on python2, because it has no RawMalloc function https://docs.python.org/2.7/c-api/memory.html. I guess this bit should be #ifdef PY2 ?

[serge-sans-paille]

In D69793#1733672, @labath wrote:

1. use PyMem_RawMalloc instead of PyMem_Malloc, as expected by PyOS_Readline (prevents to segfault upon exit of interactive session)

It looks like this is failing on python2, because it has no RawMalloc function https://docs.python.org/2.7/c-api/memory.html. I guess this bit should be #ifdef PY2 ?

Should be fixed by 590498829d8c0d4f4f673569949fa3850485c9c, at least this one make it work for both py2 and py3 on my laptop.

[labath]

In D69793#1733774, @serge-sans-paille wrote:

In D69793#1733672, @labath wrote:

1. use PyMem_RawMalloc instead of PyMem_Malloc, as expected by PyOS_Readline (prevents to segfault upon exit of interactive session)

It looks like this is failing on python2, because it has no RawMalloc function https://docs.python.org/2.7/c-api/memory.html. I guess this bit should be #ifdef PY2 ?

Should be fixed by 590498829d8c0d4f4f673569949fa3850485c9c, at least this one make it work for both py2 and py3 on my laptop.

Works for me. Thanks.

[mgorny]

Ok, this is breaking build on NetBSD:

http://lab.llvm.org:8014/builders/netbsd-amd64/builds/92/steps/ninja%20build%20local/logs/stdio

Header location is not the only problem, the code fails to build after fixing that.

FWICS, the previous module was built only for subset of systems:

-# build the Python readline suppression module only on Linux
-if("${CMAKE_SYSTEM_NAME}" MATCHES "Linux" OR "${CMAKE_SYSTEM_NAME}" STREQUAL "GNU" OR "${CMAKE_SYSTEM_NAME}" STREQUAL "kFreeBSD")
-   add_subdirectory(readline)
-endif()

I suppose the new one needs to be adjusted accordingly.

[mgorny]

Made D69846 for that.

lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt

 if(NOT LLDB_PYTHON_RELATIVE_PATH)
   message(FATAL_ERROR "LLDB_PYTHON_RELATIVE_PATH is not set.")
 endif()
 add_definitions(-DLLDB_PYTHON_RELATIVE_LIBDIR="${LLDB_PYTHON_RELATIVE_PATH}")
 
 add_lldb_library(lldbPluginScriptInterpreterPython PLUGIN
   PythonDataObjects.cpp
+  PythonReadline.cpp
   ScriptInterpreterPython.cpp
 
   LINK_LIBS
     lldbBreakpoint
     lldbCore
     lldbDataFormatters
     lldbHost
     lldbInterpreter
     lldbTarget
     ${PYTHON_LIBRARY}
 
   LINK_COMPONENTS
     Support
   )

lldb/source/Plugins/ScriptInterpreter/Python/PythonReadline.h

+//===-- PythonReadline.h ----------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_PYTHONREADLINE_H
+#define LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_PYTHONREADLINE_H
+
+#if !defined(LLDB_DISABLE_LIBEDIT) && !defined(__APPLE__)
+// NOTE: Since Python may define some pre-processor definitions which affect the
+// standard headers on some systems, you must include Python.h before any
+// standard headers are included.
+#include "Python.h"
+
+// no need to hack into Python's readline module if libedit isn't used.
+//
+#define LLDB_USE_LIBEDIT_READLINE_COMPAT_MODULE 1
+
+extern "C" PyMODINIT_FUNC initlldb_readline(void);
+
+#endif
+
+#endif // LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_PYTHONREADLINE_H

lldb/source/Plugins/ScriptInterpreter/Python/PythonReadline.cpp

+#include "PythonReadline.h"
+
+#ifdef LLDB_USE_LIBEDIT_READLINE_COMPAT_MODULE
+
+#include <stdio.h>
+
+#include <editline/readline.h>
+
+// Simple implementation of the Python readline module using libedit.
+// In the event that libedit is excluded from the build, this turns
+// back into a null implementation that blocks the module from pulling
+// in the GNU readline shared lib, which causes linkage confusion when
+// both readline and libedit's readline compatibility symbols collide.
+//
+// Currently it only installs a PyOS_ReadlineFunctionPointer, without
+// implementing any of the readline module methods. This is meant to
+// work around LLVM pr18841 to avoid seg faults in the stock Python
+// readline.so linked against GNU readline.
+//
+// Bug on the cpython side: https://bugs.python.org/issue38634
+
+PyDoc_STRVAR(moduleDocumentation,
+             "Simple readline module implementation based on libedit.");
+
+#if PY_MAJOR_VERSION >= 3
+static struct PyModuleDef readline_module = {
+    PyModuleDef_HEAD_INIT, // m_base
+    "lldb_editline",       // m_name
+    moduleDocumentation,   // m_doc
+    -1,                    // m_size
+    nullptr,               // m_methods
+    nullptr,               // m_reload
+    nullptr,               // m_traverse
+    nullptr,               // m_clear
+    nullptr,               // m_free
+};
+#else
+static struct PyMethodDef moduleMethods[] = {{nullptr, nullptr, 0, nullptr}};
+#endif
+
+static char *
+#if PY_MAJOR_VERSION >= 3
+simple_readline(FILE *stdin, FILE *stdout, const char *prompt)
+#else
+simple_readline(FILE *stdin, FILE *stdout, char *prompt)
+#endif
+{
+  rl_instream = stdin;
+  rl_outstream = stdout;
+  char *line = readline(prompt);
+  if (!line) {
+    char *ret = (char *)PyMem_RawMalloc(1);
+    if (ret != NULL)
+      *ret = '\0';
+    return ret;
+  }
+  if (*line)
+    add_history(line);
+  int n = strlen(line);
+  char *ret = (char *)PyMem_RawMalloc(n + 2);
+  if (ret) {
+    strncpy(ret, line, n);
+    free(line);
+    ret[n] = '\n';
+    ret[n + 1] = '\0';
+  }
+  return ret;
+}
+
+PyMODINIT_FUNC initlldb_readline(void) {
+  PyOS_ReadlineFunctionPointer = simple_readline;
+
+#if PY_MAJOR_VERSION >= 3
+  return PyModule_Create(&readline_module);
+#else
+  Py_InitModule4("lldb_readline", moduleMethods, moduleDocumentation,
+                 static_cast<PyObject *>(NULL), PYTHON_API_VERSION);
+#endif
+}
+#endif

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

 // Python is disabled in this build
 
 #else
 
 // LLDB Python header must be included first
 #include "lldb-python.h"
 
 #include "PythonDataObjects.h"
+#include "PythonReadline.h"
 #include "ScriptInterpreterPythonImpl.h"
 
 #include "lldb/API/SBFrame.h"
 #include "lldb/API/SBValue.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Breakpoint/WatchpointOptions.h"
 #include "lldb/Core/Communication.h"
 #include "lldb/Core/Debugger.h"
public:
   InitializePythonRAII()
       : m_gil_state(PyGILState_UNLOCKED), m_was_already_initialized(false) {
     // Python will muck with STDIN terminal state, so save off any current TTY
     // settings so we can restore them.
     m_stdin_tty_state.Save(STDIN_FILENO, false);
 
     InitializePythonHome();
 
+#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 != NULL; p++) {
+      if (strcmp(p->name, "readline") == 0) {
+        p->initfunc = initlldb_readline;
+        break;
+      }
+    }
+    if (!ReadlinePatched) {
+      PyImport_AppendInittab("readline", initlldb_readline);
+      ReadlinePatched = true;
+    }
+#endif
+
     // Register _lldb as a built-in module.
     PyImport_AppendInittab("_lldb", LLDBSwigPyInit);
 
 // Python < 3.2 and Python >= 3.2 reversed the ordering requirements for
 // calling `Py_Initialize` and `PyEval_InitThreads`.  < 3.2 requires that you
 // call `PyEval_InitThreads` first, and >= 3.2 requires that you call it last.
 #if (PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION >= 2) || (PY_MAJOR_VERSION > 3)
     Py_InitializeEx(0);