From bcdfcbcb1d8694c0f0630e3c326cbda437b24c14 Mon Sep 17 00:00:00 2001
From: Lang Hames <lhames@gmail.com>
Date: Wed, 26 Sep 2018 18:50:01 +0000
Subject: [PATCH] [ORC] Change the field order of ThreadSafeModule to ensure
 the Module is destroyed before its ThreadSharedContext.

Destroying the context first is an error if this ThreadSafeModule is the only
owner of its underlying context.

Add a unit test for ThreadSafeModule/ThreadSafeContext to catch this and other
basic usage issues.

llvm-svn: 343129
---
 .../ExecutionEngine/Orc/ThreadSafeModule.h    | 15 ++--
 .../ExecutionEngine/Orc/CMakeLists.txt        |  1 +
 .../Orc/ThreadSafeModuleTest.cpp              | 84 +++++++++++++++++++
 3 files changed, 92 insertions(+), 8 deletions(-)
 create mode 100644 llvm/unittests/ExecutionEngine/Orc/ThreadSafeModuleTest.cpp

diff --git a/llvm/include/llvm/ExecutionEngine/Orc/ThreadSafeModule.h b/llvm/include/llvm/ExecutionEngine/Orc/ThreadSafeModule.h
index d979c8dda1a8..c48af2ecde98 100644
--- a/llvm/include/llvm/ExecutionEngine/Orc/ThreadSafeModule.h
+++ b/llvm/include/llvm/ExecutionEngine/Orc/ThreadSafeModule.h
@@ -16,6 +16,7 @@
 
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
+#include "llvm/Support/Compiler.h"
 
 #include <functional>
 #include <memory>
@@ -40,7 +41,7 @@ private:
 public:
 
   // RAII based lock for ThreadSafeContext.
-  class Lock {
+  class LLVM_NODISCARD Lock {
   private:
     using UnderlyingLock = std::lock_guard<std::recursive_mutex>;
   public:
@@ -88,13 +89,11 @@ public:
   /// Construct a ThreadSafeModule from a unique_ptr<Module> and a
   /// unique_ptr<LLVMContext>. This creates a new ThreadSafeContext from the
   /// given context.
-  ThreadSafeModule(std::unique_ptr<Module> M,
-                   std::unique_ptr<LLVMContext> Ctx)
-    : M(std::move(M)), TSCtx(std::move(Ctx)) {}
+  ThreadSafeModule(std::unique_ptr<Module> M, std::unique_ptr<LLVMContext> Ctx)
+      : TSCtx(std::move(Ctx)), M(std::move(M)) {}
 
-  ThreadSafeModule(std::unique_ptr<Module> M,
-                   ThreadSafeContext TSCtx)
-    : M(std::move(M)), TSCtx(std::move(TSCtx)) {}
+  ThreadSafeModule(std::unique_ptr<Module> M, ThreadSafeContext TSCtx)
+      : TSCtx(std::move(TSCtx)), M(std::move(M)) {}
 
   Module* getModule() { return M.get(); }
 
@@ -109,8 +108,8 @@ public:
   }
 
 private:
-  std::unique_ptr<Module> M;
   ThreadSafeContext TSCtx;
+  std::unique_ptr<Module> M;
 };
 
 using GVPredicate = std::function<bool(const GlobalValue&)>;
diff --git a/llvm/unittests/ExecutionEngine/Orc/CMakeLists.txt b/llvm/unittests/ExecutionEngine/Orc/CMakeLists.txt
index c18c9361cb00..3a47bfa20bf3 100644
--- a/llvm/unittests/ExecutionEngine/Orc/CMakeLists.txt
+++ b/llvm/unittests/ExecutionEngine/Orc/CMakeLists.txt
@@ -26,6 +26,7 @@ add_llvm_unittest(OrcJITTests
   RTDyldObjectLinkingLayerTest.cpp
   RTDyldObjectLinkingLayer2Test.cpp
   SymbolStringPoolTest.cpp
+  ThreadSafeModuleTest.cpp
   )
 
 target_link_libraries(OrcJITTests PRIVATE ${ORC_JIT_TEST_LIBS})
diff --git a/llvm/unittests/ExecutionEngine/Orc/ThreadSafeModuleTest.cpp b/llvm/unittests/ExecutionEngine/Orc/ThreadSafeModuleTest.cpp
new file mode 100644
index 000000000000..40efd36160e2
--- /dev/null
+++ b/llvm/unittests/ExecutionEngine/Orc/ThreadSafeModuleTest.cpp
@@ -0,0 +1,84 @@
+//===--- ThreadSafeModuleTest.cpp - Test basic use of ThreadSafeModule ----===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ExecutionEngine/Orc/ThreadSafeModule.h"
+#include "gtest/gtest.h"
+
+#include <atomic>
+#include <future>
+#include <thread>
+
+using namespace llvm;
+using namespace llvm::orc;
+
+namespace {
+
+TEST(ThreadSafeModuleTest, ContextWhollyOwnedByOneModule) {
+  // Test that ownership of a context can be transferred to a single
+  // ThreadSafeModule.
+  ThreadSafeContext TSCtx(llvm::make_unique<LLVMContext>());
+  ThreadSafeModule TSM(llvm::make_unique<Module>("M", *TSCtx.getContext()),
+                       std::move(TSCtx));
+}
+
+TEST(ThreadSafeModuleTest, ContextOwnershipSharedByTwoModules) {
+  // Test that ownership of a context can be shared between more than one
+  // ThreadSafeModule.
+  ThreadSafeContext TSCtx(llvm::make_unique<LLVMContext>());
+
+  ThreadSafeModule TSM1(llvm::make_unique<Module>("M1", *TSCtx.getContext()),
+                        TSCtx);
+  ThreadSafeModule TSM2(llvm::make_unique<Module>("M2", *TSCtx.getContext()),
+                        std::move(TSCtx));
+}
+
+TEST(ThreadSafeModuleTest, ContextOwnershipSharedWithClient) {
+  // Test that ownership of a context can be shared with a client-held
+  // ThreadSafeContext so that it can be re-used for new modules.
+  ThreadSafeContext TSCtx(llvm::make_unique<LLVMContext>());
+
+  {
+    // Create and destroy a module.
+    ThreadSafeModule TSM1(llvm::make_unique<Module>("M1", *TSCtx.getContext()),
+                          TSCtx);
+  }
+
+  // Verify that the context is still available for re-use.
+  ThreadSafeModule TSM2(llvm::make_unique<Module>("M2", *TSCtx.getContext()),
+                        std::move(TSCtx));
+}
+
+TEST(ThreadSafeModuleTest, BasicContextLockAPI) {
+  // Test that basic lock API calls work.
+  ThreadSafeContext TSCtx(llvm::make_unique<LLVMContext>());
+  ThreadSafeModule TSM(llvm::make_unique<Module>("M", *TSCtx.getContext()),
+                       TSCtx);
+
+  { auto L = TSCtx.getLock(); }
+
+  { auto L = TSM.getContextLock(); }
+}
+
+TEST(ThreadSafeModuleTest, ContextLockPreservesContext) {
+  // Test that the existence of a context lock preserves the attached
+  // context.
+  // The trick to verify this is a bit of a hack: We attach a Module
+  // (without the ThreadSafeModule wrapper) to the context, then verify
+  // that this Module destructs safely (which it will not if its context
+  // has been destroyed) even though all references to the context have
+  // been thrown away (apart from the lock).
+
+  ThreadSafeContext TSCtx(llvm::make_unique<LLVMContext>());
+  auto L = TSCtx.getLock();
+  auto &Ctx = *TSCtx.getContext();
+  auto M = llvm::make_unique<Module>("M", Ctx);
+  TSCtx = ThreadSafeContext();
+}
+
+} // end anonymous namespace