Don't use an atexit handler for cleaning up process specific temp dir

[zturner]

Summary

For seemingly no reason today, I started getting crashes on exit in CleanupProcessSpecificTempDirectory when I ran certain tests. I determined this wasn't related to a code change, and when I looked in my temp lldb directory, I had close to 1500 process specific temp directories that had never been cleaned up, some dating back over 1 year ago.

Best I can figure out is that in the case of running the test suite, we've installed an atexit handler for the python executable itself. Which means it will call this atexit handler after it has already (presumably) unloaded our extension module, leading to undefined behavior.

Whatever the cause, atexit handlers are dangerous because of the restrictions on what kind of code you can run in them. I moved this to an explicit cleanup on Terminate, and verified that there are no more crashes on exit, and additionally the process specific temp directory is now correctly cleaned up after every test.

I don't know how (or even if) this was ever working on other platforms besides Windows, but you guys should check your temp/lldb directory and see if there are thousands of directories in there.

[amccarth]

source/Host/common/HostInfoBase.cpp

88 ↗

(On Diff #48410)

There's a difference in behavior in that this now happens even if we haven't (yet) created a temp directory. As a side effect, CleanupProcessSpecificLLDBTempDir will first _create_ the temp directory and then immediately delete it, which seems wasteful.

[zturner]

This should make sure it happens if and only if the directory was successfully created earlier during program execution.

[clayborg]

The HostInfoBase::Initialize() and HostInfoBase::Terminate() might be good places for your PDB calls like ::CoUninitialize(); to be placed...

[labath]

So, linux actually manages to invoke the atexit handlers (through some deep magic, no doubt) upon shared library unload, so we are ok here (plus linux likes to nuke /tmp after every reboot). LGTM, with a small RAII request.

source/Host/common/HostInfoBase.cpp

86 ↗

(On Diff #48414)

I think we should move this into the destructor of g_fields. Then you can simply do delete g_fields (or even g_fields.reset()) here.

lldb/trunk/include/lldb/Host/HostInfoBase.h

 {
   private:
     // Static class, unconstructable.
     HostInfoBase() {}
     ~HostInfoBase() {}
 
   public:
     static void Initialize();
+    static void
+    Terminate();
 
     //------------------------------------------------------------------
     /// Returns the number of CPUs on this current host.
     ///
     /// @return
     ///     Number of CPUs on this current host, or zero if the number
     ///     of CPUs can't be determined on this host.
     //------------------------------------------------------------------

lldb/trunk/source/Host/common/HostInfoBase.cpp

 #include <thread>
 #include <mutex> // std::once
 
 using namespace lldb;
 using namespace lldb_private;
 
 namespace
 {
-    void
-    CleanupProcessSpecificLLDBTempDir()
-    {
-        // Get the process specific LLDB temporary directory and delete it.
-        FileSpec tmpdir_file_spec;
-        if (!HostInfo::GetLLDBPath(ePathTypeLLDBTempSystemDir, tmpdir_file_spec))
-            return;
-
-        // Remove the LLDB temporary directory if we have one. Set "recurse" to
-        // true to all files that were created for the LLDB process can be cleaned up.
-        FileSystem::DeleteDirectory(tmpdir_file_spec, true);
-    }
-
     //----------------------------------------------------------------------
     // The HostInfoBaseFields is a work around for windows not supporting
     // static variables correctly in a thread safe way. Really each of the
     // variables in HostInfoBaseFields should live in the functions in which
     // they are used and each one should be static, but the work around is
     // in place to avoid this restriction. Ick.
     //----------------------------------------------------------------------
 
     struct HostInfoBaseFields
     {
+        ~HostInfoBaseFields()
+        {
+            if (m_lldb_process_tmp_dir.Exists())
+            {
+                // Remove the LLDB temporary directory if we have one. Set "recurse" to
+                // true to all files that were created for the LLDB process can be cleaned up.
+                FileSystem::DeleteDirectory(m_lldb_process_tmp_dir, true);
+            }
+        }
+
         uint32_t m_number_cpus;
         std::string m_vendor_string;
         std::string m_os_string;
         std::string m_host_triple;
 
         ArchSpec m_host_arch_32;
         ArchSpec m_host_arch_64;
 

 }
 
 void
 HostInfoBase::Initialize()
 {
     g_fields = new HostInfoBaseFields();
 }
 
+void
+HostInfoBase::Terminate()
+{
+    delete g_fields;
+    g_fields = nullptr;
+}
+
 uint32_t
 HostInfoBase::GetNumberCPUS()
 {
     static std::once_flag g_once_flag;
     std::call_once(g_once_flag,  []() {
         g_fields->m_number_cpus = std::thread::hardware_concurrency();
     });
     return g_fields->m_number_cpus;
HostInfoBase::ComputeProcessTempFileDirectory(FileSpec &file_spec)
     if (!HostInfo::ComputeGlobalTempFileDirectory(temp_file_spec))
         return false;
 
     std::string pid_str{std::to_string(Host::GetCurrentProcessID())};
     temp_file_spec.AppendPathComponent(pid_str);
     if (!FileSystem::MakeDirectory(temp_file_spec, eFilePermissionsDirectoryDefault).Success())
         return false;
 
-    // Make an atexit handler to clean up the process specify LLDB temp dir
-    // and all of its contents.
-    ::atexit(CleanupProcessSpecificLLDBTempDir);
     file_spec.GetDirectory().SetCString(temp_file_spec.GetCString());
     return true;
 }
 
 bool
 HostInfoBase::ComputeTempFileBaseDirectory(FileSpec &file_spec)
 {
     llvm::SmallVector<char, 16> tmpdir;

lldb/trunk/source/Initialization/SystemInitializerCommon.cpp

#if defined(_MSC_VER)
     ProcessWindowsLog::Terminate();
 #endif
 
 #ifndef LLDB_DISABLE_PYTHON
     OperatingSystemPython::Terminate();
 #endif
     OperatingSystemGo::Terminate();
 
+    HostInfo::Terminate();
     Log::Terminate();
 }