Close DynamicLibraries in reverse order they were opened.

Frederich Munch · Frederich Munch · commit ad1258001276 · 2017-06-05T16:26:58.000Z
Summary: Matches C++ destruction ordering better and fixes possible problems of loaded libraries having inter-dependencies. Reviewers: efriedma, v.g.vassilev, chapuni Reviewed By: efriedma Subscribers: mgorny, llvm-commits Differential Revision: https://reviews.llvm.org/D33652 llvm-svn: 304720
diff --git a/llvm/lib/Support/Unix/DynamicLibrary.inc b/llvm/lib/Support/Unix/DynamicLibrary.inc
@@ -15,7 +15,8 @@
 #include <dlfcn.h>
 
 DynamicLibrary::HandleSet::~HandleSet() {
-  for (void *Handle : Handles)
+  // Close the libraries in reverse order.
+  for (void *Handle : llvm::reverse(Handles))
     ::dlclose(Handle);
   if (Process)
     ::dlclose(Process);
diff --git a/llvm/lib/Support/Windows/DynamicLibrary.inc b/llvm/lib/Support/Windows/DynamicLibrary.inc
@@ -23,7 +23,7 @@
 
 
 DynamicLibrary::HandleSet::~HandleSet() {
-  for (void *Handle : Handles)
+  for (void *Handle : llvm::reverse(Handles))
     FreeLibrary(HMODULE(Handle));
 
   // 'Process' should not be released on Windows.
diff --git a/llvm/unittests/Support/DynamicLibrary/CMakeLists.txt b/llvm/unittests/Support/DynamicLibrary/CMakeLists.txt
@@ -4,16 +4,21 @@ add_llvm_unittest(DynamicLibraryTests DynamicLibraryTest.cpp)
 
 export_executable_symbols(DynamicLibraryTests)
 
-add_library(PipSqueak SHARED PipSqueak.cxx)
+function(dynlib_add_module NAME)
+  add_library(${NAME} SHARED PipSqueak.cxx)
 
-set_output_directory(PipSqueak
-  BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
-  LIBRARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
-  )
+  set_output_directory(${NAME}
+    BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
+    LIBRARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
+    )
 
-set_target_properties(PipSqueak
-  PROPERTIES PREFIX ""
-  SUFFIX ".so"
-  )
+  set_target_properties(${NAME}
+    PROPERTIES PREFIX ""
+    SUFFIX ".so"
+    )
 
-add_dependencies(DynamicLibraryTests PipSqueak)
+  add_dependencies(DynamicLibraryTests ${NAME})
+endfunction(dynlib_add_module)
+
+dynlib_add_module(PipSqueak)
+dynlib_add_module(SecondLib)
diff --git a/llvm/unittests/Support/DynamicLibrary/DynamicLibraryTest.cpp b/llvm/unittests/Support/DynamicLibrary/DynamicLibraryTest.cpp
@@ -15,26 +15,26 @@
 #include "gtest/gtest.h"
 
 #include "PipSqueak.h"
-#include <string>
 
 using namespace llvm;
 using namespace llvm::sys;
 
 extern "C" PIPSQUEAK_EXPORT const char *TestA() { return "ProcessCall"; }
 
-std::string LibPath() {
+std::string LibPath(const std::string Name = "PipSqueak") {
   const std::vector<testing::internal::string>& Argvs = testing::internal::GetArgvs();
   const char *Argv0 = Argvs.size() > 0 ? Argvs[0].c_str() : "DynamicLibraryTests";
   void *Ptr = (void*)(intptr_t)TestA;
   std::string Path = fs::getMainExecutable(Argv0, Ptr);
   llvm::SmallString<256> Buf(path::parent_path(Path));
-  path::append(Buf, "PipSqueak.so");
+  path::append(Buf, (Name+".so").c_str());
   return Buf.str();
 }
 
 #if defined(_WIN32) || (defined(HAVE_DLFCN_H) && defined(HAVE_DLOPEN))
 
 typedef void (*SetStrings)(std::string &GStr, std::string &LStr);
+typedef void (*TestOrder)(std::vector<std::string> &V);
 typedef const char *(*GetString)();
 
 template <class T> static T FuncPtr(void *Ptr) {
@@ -100,26 +100,59 @@ TEST(DynamicLibrary, Overload) {
 }
 
 TEST(DynamicLibrary, Shutdown) {
-  std::string A, B;
+  std::string A("PipSqueak"), B, C("SecondLib");
+  std::vector<std::string> Order;
   {
     std::string Err;
     llvm_shutdown_obj Shutdown;
     DynamicLibrary DL =
-        DynamicLibrary::getPermanentLibrary(LibPath().c_str(), &Err);
+        DynamicLibrary::getPermanentLibrary(LibPath(A).c_str(), &Err);
     EXPECT_TRUE(DL.isValid());
     EXPECT_TRUE(Err.empty());
 
-    SetStrings SS = FuncPtr<SetStrings>(
+    SetStrings SS_0 = FuncPtr<SetStrings>(
+        DynamicLibrary::SearchForAddressOfSymbol("SetStrings"));
+    EXPECT_TRUE(SS_0 != nullptr);
+
+    SS_0(A, B);
+    EXPECT_EQ(B, "Local::Local(PipSqueak)");
+
+    TestOrder TO_0 = FuncPtr<TestOrder>(
+        DynamicLibrary::SearchForAddressOfSymbol("TestOrder"));
+    EXPECT_TRUE(TO_0 != nullptr);
+    
+    DynamicLibrary DL2 =
+        DynamicLibrary::getPermanentLibrary(LibPath(C).c_str(), &Err);
+    EXPECT_TRUE(DL2.isValid());
+    EXPECT_TRUE(Err.empty());
+
+    // Should find latest version of symbols in SecondLib
+    SetStrings SS_1 = FuncPtr<SetStrings>(
         DynamicLibrary::SearchForAddressOfSymbol("SetStrings"));
-    EXPECT_TRUE(SS != nullptr);
+    EXPECT_TRUE(SS_1 != nullptr);
+    EXPECT_TRUE(SS_0 != SS_1);
 
-    SS(A, B);
-    EXPECT_EQ(B, "Local::Local");
+    TestOrder TO_1 = FuncPtr<TestOrder>(
+        DynamicLibrary::SearchForAddressOfSymbol("TestOrder"));
+    EXPECT_TRUE(TO_1 != nullptr);
+    EXPECT_TRUE(TO_0 != TO_1);
+
+    B.clear();
+    SS_1(C, B);
+    EXPECT_EQ(B, "Local::Local(SecondLib)");
+
+    TO_0(Order);
+    TO_1(Order);
   }
   EXPECT_EQ(A, "Global::~Global");
   EXPECT_EQ(B, "Local::~Local");
   EXPECT_TRUE(FuncPtr<SetStrings>(DynamicLibrary::SearchForAddressOfSymbol(
                   "SetStrings")) == nullptr);
+
+  // Test unload/destruction ordering
+  EXPECT_EQ(Order.size(), 2UL);
+  EXPECT_EQ(Order.front(), "SecondLib");
+  EXPECT_EQ(Order.back(), "PipSqueak");
 }
 
 #else
diff --git a/llvm/unittests/Support/DynamicLibrary/PipSqueak.cxx b/llvm/unittests/Support/DynamicLibrary/PipSqueak.cxx
@@ -9,38 +9,40 @@
 
 #include "PipSqueak.h"
 
-#if defined(_WIN32) && !defined(__GNUC__)
-// Disable warnings from inclusion of xlocale & exception
-#pragma warning(push)
-#pragma warning(disable: 4530)
-#pragma warning(disable: 4577)
-#include <string>
-#pragma warning(pop)
-#else
-#include <string>
-#endif
-
 struct Global {
   std::string *Str;
-  Global() : Str(nullptr) {}
+  std::vector<std::string> *Vec;
+  Global() : Str(nullptr), Vec(nullptr) {}
   ~Global() {
-    if (Str)
+    if (Str) {
+      if (Vec)
+        Vec->push_back(*Str);
       *Str = "Global::~Global";
+    }
   }
 };
 
+static Global Glb;
+
 struct Local {
   std::string &Str;
-  Local(std::string &S) : Str(S) { Str = "Local::Local"; }
+  Local(std::string &S) : Str(S) {
+    Str = "Local::Local";
+    if (Glb.Str && !Glb.Str->empty())
+      Str += std::string("(") + *Glb.Str + std::string(")");
+  }
   ~Local() { Str = "Local::~Local"; }
 };
 
-static Global Glb;
 
 extern "C" PIPSQUEAK_EXPORT void SetStrings(std::string &GStr,
                                             std::string &LStr) {
-  static Local Lcl(LStr);
   Glb.Str = &GStr;
+  static Local Lcl(LStr);
+}
+
+extern "C" PIPSQUEAK_EXPORT void TestOrder(std::vector<std::string> &V) {
+  Glb.Vec = &V;
 }
 
 extern "C" PIPSQUEAK_EXPORT const char *TestA() { return "LibCall"; }
diff --git a/llvm/unittests/Support/DynamicLibrary/PipSqueak.h b/llvm/unittests/Support/DynamicLibrary/PipSqueak.h
@@ -10,6 +10,19 @@
 #ifndef LLVM_PIPSQUEAK_H
 #define LLVM_PIPSQUEAK_H
 
+#if defined(_WIN32) && !defined(__GNUC__)
+// Disable warnings from inclusion of xlocale & exception
+#pragma warning(push)
+#pragma warning(disable: 4530)
+#pragma warning(disable: 4577)
+#include <string>
+#include <vector>
+#pragma warning(pop)
+#else
+#include <string>
+#include <vector>
+#endif
+
 #ifdef _WIN32
 #define PIPSQUEAK_EXPORT __declspec(dllexport)
 #else
