[Lldb-commits] [lldb] 070a1d5 - [lldb] Remove nothreadallow from SWIG's __str__ wrappers to work around a Python>=3.7 crash

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-09-28T10:10:34+02:00
New Revision: 070a1d562b303e564e40d831f6dc125793e19b38

URL: https://github.com/llvm/llvm-project/commit/070a1d562b303e564e40d831f6dc125793e19b38
DIFF: https://github.com/llvm/llvm-project/commit/070a1d562b303e564e40d831f6dc125793e19b38.diff

LOG: [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

Added: 
    lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/Makefile
    lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/TestDataFormatterLibcxxDeque.py
    lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/main.cpp

Modified: 
    lldb/bindings/macros.swig

Removed: 
    


################################################################################
diff  --git a/lldb/bindings/macros.swig b/lldb/bindings/macros.swig
index 0387f27f3cb9..6793a11c5bd9 100644
--- a/lldb/bindings/macros.swig
+++ b/lldb/bindings/macros.swig
@@ -1,6 +1,5 @@
 %define STRING_EXTENSION_LEVEL(Class, Level)
 %extend {
-  %nothreadallow;
   std::string lldb:: ## Class ## ::__str__(){
     lldb::SBStream stream;
     $self->GetDescription (stream, Level);
@@ -11,13 +10,11 @@
     }
     return std::string(desc, desc_len);
   }
-  %clearnothreadallow;
 }
 %enddef
 
 %define STRING_EXTENSION(Class)
 %extend {
-  %nothreadallow;
   std::string lldb:: ## Class ## ::__str__(){
     lldb::SBStream stream;
     $self->GetDescription (stream);
@@ -28,6 +25,5 @@
     }
     return std::string(desc, desc_len);
   }
-  %clearnothreadallow;
 }
 %enddef

diff  --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/Makefile b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/Makefile
new file mode 100644
index 000000000000..c5df567e01a2
--- /dev/null
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/Makefile
@@ -0,0 +1,5 @@
+CXX_SOURCES := main.cpp
+
+USE_LIBCPP := 1
+
+include Makefile.rules

diff  --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/TestDataFormatterLibcxxDeque.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/TestDataFormatterLibcxxDeque.py
new file mode 100644
index 000000000000..b9949288c989
--- /dev/null
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/TestDataFormatterLibcxxDeque.py
@@ -0,0 +1,25 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class LibcxxDequeDataFormatterTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @add_test_categories(["libc++"])
+    def test(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(self, "break here",
+                                          lldb.SBFileSpec("main.cpp"))
+
+        self.expect_expr("empty", result_children=[])
+        self.expect_expr("deque_1", result_children=[
+            ValueCheck(name="[0]", value="1"),
+        ])
+        self.expect_expr("deque_3", result_children=[
+            ValueCheck(name="[0]", value="3"),
+            ValueCheck(name="[1]", value="1"),
+            ValueCheck(name="[2]", value="2")
+        ])

diff  --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/main.cpp b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/main.cpp
new file mode 100644
index 000000000000..43c3f374a0f9
--- /dev/null
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/main.cpp
@@ -0,0 +1,8 @@
+#include <deque>
+
+int main() {
+  std::deque<int> empty;
+  std::deque<int> deque_1 = {1};
+  std::deque<int> deque_3 = {3, 1, 2};
+  return empty.size() + deque_1.front() + deque_3.front(); // break here
+}