Page MenuHomePhabricator

Bundle libedit-compatible readline replacement
ClosedPublic

Authored by serge-sans-paille on Mon, Nov 4, 2:34 AM.

Details

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

Diff Detail

Event Timeline

mgorny added inline comments.Mon, Nov 4, 2:44 AM
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.

serge-sans-paille edited the summary of this revision. (Show Details)
serge-sans-paille marked an inline comment as done.
labath added a comment.Mon, Nov 4, 8:21 AM

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...

+1 on everything Pavel said :-)

serge-sans-paille marked 4 inline comments as done.
serge-sans-paille marked 2 inline comments as done.
serge-sans-paille added inline comments.
lldb/source/Plugins/ScriptInterpreter/Python/PythonReadline.h
14–16

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

labath accepted this revision.Mon, Nov 4, 11:15 PM

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.

This revision is now accepted and ready to land.Mon, Nov 4, 11:15 PM
serge-sans-paille marked an inline comment as done.
serge-sans-paille marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.
labath added a comment.Tue, Nov 5, 2:45 AM
  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 ?

  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 added a comment.Tue, Nov 5, 5:31 AM
  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 added a comment.Tue, Nov 5, 6:36 AM

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 added a comment.Tue, Nov 5, 7:32 AM

Made D69846 for that.