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

[teemperor]

Summary

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:

#if (PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION >= 7) || (PY_MAJOR_VERSION > 3)  
    // The only case we should go further and acquire the GIL: it is unlocked.  
    if (PyGILState_Check())                                                     
      return;                                                                   
#endif

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.

[teemperor]

If anyone knows another SB object where calling str on it would execute a Python script (and that is simpler than an synthetic child provider), then let me know.

[JDevlieghere]

Thanks for the thorough explanation. LGTM.

lldb/bindings/macros.swig

 %define STRING_EXTENSION_LEVEL(Class, Level)
 %extend {
-  %nothreadallow;
   std::string lldb:: ## Class ## ::__str__(){
     lldb::SBStream stream;
     $self->GetDescription (stream, Level);
     const char *desc = stream.GetData();
     size_t desc_len = stream.GetSize();
     if (desc_len > 0 && (desc[desc_len-1] == '\n' || desc[desc_len-1] == '\r')) {
       --desc_len;
     }
     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);
     const char *desc = stream.GetData();
     size_t desc_len = stream.GetSize();
     if (desc_len > 0 && (desc[desc_len-1] == '\n' || desc[desc_len-1] == '\r')) {
       --desc_len;
     }
     return std::string(desc, desc_len);
   }
-  %clearnothreadallow;
 }
 %enddef

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/Makefile

+CXX_SOURCES := main.cpp
+
+USE_LIBCPP := 1
+
+include Makefile.rules

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/TestDataFormatterLibcxxDeque.py

+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")
+        ])

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/deque/main.cpp

+#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
+}