[lldb] Make Python initialization atomic

[JDevlieghere]

Summary

We got a few crash reports that showed LLDB initializing Python on two separate threads. Make sure Python is initialized exactly once.

rdar://87287005

[mib]

LGTM

[labath]

I don't think that atomic<bool> is what you want here. In the case of a race, the "loser" will immediately continue to use python as if it was initialized, even though the winner has not finished the initialization. You most likely need call_once semantics, blocking all threads until the initialization completes.

That said, I think think it would be better to do this initialization in the Initialize static function. Out of general cleanliness, but with a particular with a view towards the SIGINT patch. That way the initialization functions happens in a predictable and single-threaded context (as you can see, threads are hard), hopefully at a point where nobody will care that we're mucking with the SIGINT handlers.

[JDevlieghere]

In D117601#3253663, @labath wrote:

I don't think that atomic<bool> is what you want here. In the case of a race, the "loser" will immediately continue to use python as if it was initialized, even though the winner has not finished the initialization. You most likely need call_once semantics, blocking all threads until the initialization completes.

That said, I think think it would be better to do this initialization in the Initialize static function. Out of general cleanliness, but with a particular with a view towards the SIGINT patch. That way the initialization functions happens in a predictable and single-threaded context (as you can see, threads are hard), hopefully at a point where nobody will care that we're mucking with the SIGINT handlers.

Makes sense. I’ll update the patch tomorrow.

[JDevlieghere]

Do private initialization exactly once in ScriptInterpreterPython::Initialize

[JDevlieghere]

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

3154–3176

I just moved this into the existing anonymous namespace above.

[labath]

LG, thanks

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

 #endif
 
 static ScriptInterpreterPythonImpl *GetPythonInterpreter(Debugger &debugger) {
   ScriptInterpreter *script_interpreter =
       debugger.GetScriptInterpreter(true, lldb::eScriptLanguagePython);
   return static_cast<ScriptInterpreterPythonImpl *>(script_interpreter);
 }
 
-static bool g_initialized = false;
-
 namespace {
 
 // Initializing Python is not a straightforward process.  We cannot control
 // what external code may have done before getting to this point in LLDB,
 // including potentially having already initialized Python, so we need to do a
 // lot of work to ensure that the existing state of the system is maintained
 // across our initialization.  We do this by using an RAII pattern where we
 // save off initial state at the beginning, and restore it at the end
#endif
 
     // InitThreads acquires the GIL if it hasn't been called before.
     PyEval_InitThreads();
   }
 
   PyGILState_STATE m_gil_state = PyGILState_UNLOCKED;
   bool m_was_already_initialized = false;
 };
+
+#if LLDB_USE_PYTHON_SET_INTERRUPT
+/// Saves the current signal handler for the specified signal and restores
+/// it at the end of the current scope.
+struct RestoreSignalHandlerScope {
+  /// The signal handler.
+  struct sigaction m_prev_handler;
+  int m_signal_code;
+  RestoreSignalHandlerScope(int signal_code) : m_signal_code(signal_code) {
+    // Initialize sigaction to their default state.
+    std::memset(&m_prev_handler, 0, sizeof(m_prev_handler));
+    // Don't install a new handler, just read back the old one.
+    struct sigaction *new_handler = nullptr;
+    int signal_err = ::sigaction(m_signal_code, new_handler, &m_prev_handler);
+    lldbassert(signal_err == 0 && "sigaction failed to read handler");
+  }
+  ~RestoreSignalHandlerScope() {
+    int signal_err = ::sigaction(m_signal_code, &m_prev_handler, nullptr);
+    lldbassert(signal_err == 0 && "sigaction failed to restore old handler");
+  }
+};
+#endif
 } // namespace
 
 void ScriptInterpreterPython::ComputePythonDirForApple(
     llvm::SmallVectorImpl<char> &path) {
   auto style = llvm::sys::path::Style::posix;
 
   llvm::StringRef path_ref(path.begin(), path.size());
   auto rbegin = llvm::sys::path::rbegin(path_ref, style);

 }
 
 llvm::StringRef ScriptInterpreterPython::GetPluginDescriptionStatic() {
   return "Embedded Python interpreter";
 }
 
 void ScriptInterpreterPython::Initialize() {
   static llvm::once_flag g_once_flag;
-
   llvm::call_once(g_once_flag, []() {
     PluginManager::RegisterPlugin(GetPluginNameStatic(),
                                   GetPluginDescriptionStatic(),
                                   lldb::eScriptLanguagePython,
                                   ScriptInterpreterPythonImpl::CreateInstance);
+    ScriptInterpreterPythonImpl::Initialize();
   });
 }
 
 void ScriptInterpreterPython::Terminate() {}
 
 ScriptInterpreterPythonImpl::Locker::Locker(
     ScriptInterpreterPythonImpl *py_interpreter, uint16_t on_entry,
     uint16_t on_leave, FileSP in, FileSP out, FileSP err)
: ScriptInterpreterPython(debugger), m_saved_stdin(), m_saved_stdout(),
       m_saved_stderr(), m_main_module(),
       m_session_dict(PyInitialValue::Invalid),
       m_sys_module_dict(PyInitialValue::Invalid), m_run_one_line_function(),
       m_run_one_line_str_global(),
       m_dictionary_name(m_debugger.GetInstanceName().AsCString()),
       m_active_io_handler(eIOHandlerNone), m_session_is_active(false),
       m_pty_secondary_is_open(false), m_valid_session(true), m_lock_count(0),
       m_command_thread_state(nullptr) {
-  InitializePrivate();
-
   m_scripted_process_interface_up =
       std::make_unique<ScriptedProcessPythonInterface>(*this);
 
   m_dictionary_name.append("_dict");
   StreamString run_string;
   run_string.Printf("%s = dict()", m_dictionary_name.c_str());
 
   Locker locker(this, Locker::AcquireLock, Locker::FreeAcquiredLock);

 std::unique_ptr<ScriptInterpreterLocker>
 ScriptInterpreterPythonImpl::AcquireInterpreterLock() {
   std::unique_ptr<ScriptInterpreterLocker> py_lock(new Locker(
       this, Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN,
       Locker::FreeLock | Locker::TearDownSession));
   return py_lock;
 }
 
-#if LLDB_USE_PYTHON_SET_INTERRUPT
-namespace {
-/// Saves the current signal handler for the specified signal and restores
-/// it at the end of the current scope.
-struct RestoreSignalHandlerScope {
-  /// The signal handler.
-  struct sigaction m_prev_handler;
-  int m_signal_code;
-  RestoreSignalHandlerScope(int signal_code) : m_signal_code(signal_code) {
-    // Initialize sigaction to their default state.
-    std::memset(&m_prev_handler, 0, sizeof(m_prev_handler));
-    // Don't install a new handler, just read back the old one.
-    struct sigaction *new_handler = nullptr;
-    int signal_err = ::sigaction(m_signal_code, new_handler, &m_prev_handler);
-    lldbassert(signal_err == 0 && "sigaction failed to read handler");
-  }
-  ~RestoreSignalHandlerScope() {
-    int signal_err = ::sigaction(m_signal_code, &m_prev_handler, nullptr);
-    lldbassert(signal_err == 0 && "sigaction failed to restore old handler");
-  }
-};
-} // namespace
-#endif
-
-void ScriptInterpreterPythonImpl::InitializePrivate() {
-  if (g_initialized)
-    return;
-
-  g_initialized = true;
-
+void ScriptInterpreterPythonImpl::Initialize() {
   LLDB_SCOPED_TIMER();
 
   // RAII-based initialization which correctly handles multiple-initialization,
   // version- specific differences among Python 2 and Python 3, and saving and
   // restoring various other pieces of state that can get mucked with during
   // initialization.
   InitializePythonRAII initialize_guard;
 

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

public:
 
   static bool BreakpointCallbackFunction(void *baton,
                                          StoppointCallbackContext *context,
                                          lldb::user_id_t break_id,
                                          lldb::user_id_t break_loc_id);
   static bool WatchpointCallbackFunction(void *baton,
                                          StoppointCallbackContext *context,
                                          lldb::user_id_t watch_id);
-  static void InitializePrivate();
+  static void Initialize();
 
   class SynchronicityHandler {
   private:
     lldb::DebuggerSP m_debugger_sp;
     ScriptedCommandSynchronicity m_synch_wanted;
     bool m_old_asynch;
 
   public:

lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp

 #include "lldb/Host/HostInfo.h"
 
 #include "PythonTestSuite.h"
 
 using namespace lldb_private;
 class TestScriptInterpreterPython : public ScriptInterpreterPythonImpl {
 public:
   using ScriptInterpreterPythonImpl::Initialize;
-  using ScriptInterpreterPythonImpl::InitializePrivate;
 };
 
 void PythonTestSuite::SetUp() {
   FileSystem::Initialize();
   HostInfoBase::Initialize();
   // ScriptInterpreterPython::Initialize() depends on HostInfo being
   // initializedso it can compute the python directory etc.
   TestScriptInterpreterPython::Initialize();
-  TestScriptInterpreterPython::InitializePrivate();
 
   // Although we don't care about concurrency for the purposes of running
   // this test suite, Python requires the GIL to be locked even for
   // deallocating memory, which can happen when you call Py_DECREF or
   // Py_INCREF.  So acquire the GIL for the entire duration of this
   // test suite.
   m_gil_state = PyGILState_Ensure();
 }