[all-commits] [llvm/llvm-project] 070a1d: [lldb] Remove nothreadallow from SWIG's __str__ wr...

[Raphael Isemann via All-commits]

  Branch: refs/heads/master
  Home:   https://github.com/llvm/llvm-project
  Commit: 070a1d562b303e564e40d831f6dc125793e19b38
      https://github.com/llvm/llvm-project/commit/070a1d562b303e564e40d831f6dc125793e19b38
  Author: Raphael Isemann <teemperor at gmail.com>
  Date:   2020-09-28 (Mon, 28 Sep 2020)

  Changed paths:
    M lldb/bindings/macros.swig
    A lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/Makefile
    A lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/TestDataFormatterLibcxxDeque.py
    A lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/main.cpp

  Log Message:
  -----------
  [lldb] Remove nothreadallow from SWIG's __str__ wrappers to work around a Python>=3.7 crash

Usually when we enter a SWIG wrapper function from Python, SWIG automatically
adds a `Py_BEGIN_ALLOW_THREADS`/`Py_END_ALLOW_THREADS` around the call to the SB
API C++ function. This will ensure that Python's GIL is released when we enter
LLDB and locked again when we return to the wrapper code.

D51569 changed this behaviour but only for the generated `__str__` wrappers. The
added `nothreadallow` disables the injection of the GIL release/re-acquire code
and the GIL is now kept locked when entering LLDB and is expected to be still
locked when returning from the LLDB implementation. The main reason for that was
that back when D51569 landed the wrapper itself created a Python string. These
days it just creates a std::string and SWIG itself takes care of getting the GIL
and creating the Python string from the std::string, so that workaround isn't
necessary anymore.

This patch just removes `nothreadallow` so that our `__str__` functions now
behave like all other wrapper functions in that they release the GIL when
calling into the SB API implementation.

The motivation here is actually to work around another potential bug in LLDB.
When one calls into the LLDB SB API while holding the GIL and that call causes
LLDB to interpret some Python script via `ScriptInterpreterPython`, then the GIL
will be unlocked when the control flow returns from the SB API. In the case of
the `__str__` wrapper this would cause that the next call to a Python function
requiring the GIL would fail (as SWIG will not try to reacquire the GIL as it
isn't aware that LLDB removed it).

The reason for this unexpected GIL release seems to be a workaround for recent
Python versions:
```
    // The only case we should go further and acquire the GIL: it is unlocked.
    if (PyGILState_Check())
      return;
```

The early-exit here causes `InitializePythonRAII::m_was_already_initialized` to
be always false and that causes that `InitializePythonRAII`'s destructor always
directly unlocks the GIL via `PyEval_SaveThread`. I'm investigating how to
properly fix this bug in a follow up patch, but for now this straightforward
patch seems to be enough to unblock my other patches (and it also has the
benefit of removing this workaround).

The test for this is just a simple test for `std::deque` which has a synthetic
child provider implemented as a Python script. Inspecting the deque object will
cause `expect_expr` to create a string error message by calling
`str(deque_object)`. Printing the ValueObject causes the Python script for the
synthetic children to execute which then triggers the bug described above where
the GIL ends up being unlocked.

Reviewed By: JDevlieghere

Differential Revision: https://reviews.llvm.org/D88302