Subzero: Fixed deadlock when _start is first function

It was previously the case that instrumentStart in ASanInstrumentation would block until instrumentGlobals had completed. This was because instrumentStart depends on the global redzones having been inserted. However, instrumentGlobals was not called until the first function was popped off the emit queue, and when _start was the first function, it was not placed on the emit queue until after it had been instrumented and lowered. instrumentStart was waiting for instrumentGlobals, which could not happen until instrumentStart completed.

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
R=kschimpf@google.com, stichnot@chromium.org

Review URL: https://codereview.chromium.org/2165493002 .

diff --git a/src/IceASanInstrumentation.cpp b/src/IceASanInstrumentation.cpp
index a02cb6e..fa0da63 100644 (file)
--- a/src/IceASanInstrumentation.cpp
+++ b/src/IceASanInstrumentation.cpp
@@ -74,6 +74,7 @@ bool ASanInstrumentation::isInstrumentable(Cfg *Func) {
 // types of the redzones and their associated globals match so that they are
 // laid out together in memory.
 void ASanInstrumentation::instrumentGlobals(VariableDeclarationList &Globals) {
+  std::unique_lock<std::mutex> _(GlobalsMutex);
   if (DidProcessGlobals)
     return;
   VariableDeclarationList NewGlobals;
@@ -135,7 +136,6 @@ void ASanInstrumentation::instrumentGlobals(VariableDeclarationList &Globals) {
   Globals.clear();
   Globals.merge(&NewGlobals);
   DidProcessGlobals = true;
-  GlobalsDoneCV.notify_all();
 
   // Log the new set of globals
   if (BuildDefs::dump() && (getFlags().getVerbose() & IceV_GlobalInit)) {
@@ -332,13 +332,8 @@ void ASanInstrumentation::instrumentStart(Cfg *Func) {
   auto *Call = InstCall::create(Func, NumArgs, Void, ShadowMemInit, NoTailCall);
   Func->getEntryNode()->getInsts().push_front(Call);
 
-  // wait to get the final count of global redzones
-  if (!DidProcessGlobals) {
-    GlobalsLock.lock();
-    while (!DidProcessGlobals)
-      GlobalsDoneCV.wait(GlobalsLock);
-    GlobalsLock.release();
-  }
+  instrumentGlobals(*getGlobals());
+
   Call->addArg(ConstantInteger32::create(Ctx, IceType_i32, RzGlobalsNum));
   Call->addArg(Ctx->getConstantSym(0, Ctx->getGlobalString(RzArrayName)));
   Call->addArg(Ctx->getConstantSym(0, Ctx->getGlobalString(RzSizesName)));

diff --git a/src/IceASanInstrumentation.h b/src/IceASanInstrumentation.h
index c9095fa..eb75b20 100644 (file)
--- a/src/IceASanInstrumentation.h
+++ b/src/IceASanInstrumentation.h
@@ -23,8 +23,6 @@
 #include "IceGlobalInits.h"
 #include "IceInstrumentation.h"
 
-#include <condition_variable>
-
 namespace Ice {
 
 class ASanInstrumentation : public Instrumentation {
@@ -33,9 +31,7 @@ class ASanInstrumentation : public Instrumentation {
   ASanInstrumentation &operator=(const ASanInstrumentation &) = delete;
 
 public:
-  ASanInstrumentation(GlobalContext *Ctx)
-      : Instrumentation(Ctx), RzNum(0),
-        GlobalsLock(GlobalsMutex, std::defer_lock) {
+  ASanInstrumentation(GlobalContext *Ctx) : Instrumentation(Ctx), RzNum(0) {
     ICE_TLS_INIT_FIELD(LocalDtors);
   }
   void instrumentGlobals(VariableDeclarationList &Globals) override;
@@ -57,8 +53,6 @@ private:
   bool DidProcessGlobals = false;
   SizeT RzGlobalsNum = 0;
   std::mutex GlobalsMutex;
-  std::unique_lock<std::mutex> GlobalsLock;
-  std::condition_variable GlobalsDoneCV;
 };
 } // end of namespace Ice
 

diff --git a/src/IceGlobalContext.h b/src/IceGlobalContext.h
index 2d60f48..e4be36b 100644 (file)
--- a/src/IceGlobalContext.h
+++ b/src/IceGlobalContext.h
@@ -482,6 +482,10 @@ public:
     return LockedPtr<StringPool>(Strings.get(), &StringsLock);
   }
 
+  LockedPtr<VariableDeclarationList> getGlobals() {
+    return LockedPtr<VariableDeclarationList>(&Globals, &InitAllocLock);
+  }
+
   /// Number of function blocks that can be queued before waiting for
   /// translation
   /// threads to consume.
@@ -612,6 +616,8 @@ private:
     LockedPtr<VariableDeclarationList> _(&Globals, &InitAllocLock);
     if (Globls != nullptr) {
       Globals.merge(Globls.get());
+      if (!BuildDefs::minimal() && Instrumentor != nullptr)
+        Instrumentor->setHasSeenGlobals();
     }
   }
 

diff --git a/src/IceInstrumentation.cpp b/src/IceInstrumentation.cpp
index 0cd218a..0709bfc 100644 (file)
--- a/src/IceInstrumentation.cpp
+++ b/src/IceInstrumentation.cpp
@@ -118,4 +118,18 @@ void Instrumentation::instrumentInst(LoweringContext &Context) {
   }
 }
 
+void Instrumentation::setHasSeenGlobals() {
+  {
+    std::unique_lock<std::mutex> _(GlobalsSeenMutex);
+    HasSeenGlobals = true;
+  }
+  GlobalsSeenCV.notify_all();
+}
+
+LockedPtr<VariableDeclarationList> Instrumentation::getGlobals() {
+  std::unique_lock<std::mutex> GlobalsLock(GlobalsSeenMutex);
+  GlobalsSeenCV.wait(GlobalsLock, [this] { return HasSeenGlobals; });
+  return Ctx->getGlobals();
+}
+
 } // end of namespace Ice

diff --git a/src/IceInstrumentation.h b/src/IceInstrumentation.h
index 3297963..2392f96 100644 (file)
--- a/src/IceInstrumentation.h
+++ b/src/IceInstrumentation.h
@@ -30,6 +30,8 @@
 
 #include "IceDefs.h"
 
+#include <condition_variable>
+
 namespace Ice {
 
 class LoweringContext;
@@ -41,11 +43,14 @@ class Instrumentation {
 
 public:
   Instrumentation(GlobalContext *Ctx) : Ctx(Ctx) {}
+  virtual ~Instrumentation() = default;
   virtual void instrumentGlobals(VariableDeclarationList &) {}
   void instrumentFunc(Cfg *Func);
+  void setHasSeenGlobals();
 
 protected:
   virtual void instrumentInst(LoweringContext &Context);
+  LockedPtr<VariableDeclarationList> getGlobals();
 
 private:
   virtual bool isInstrumentable(Cfg *) { return true; }
@@ -78,6 +83,11 @@ private:
 
 protected:
   GlobalContext *Ctx;
+
+private:
+  bool HasSeenGlobals = false;
+  std::mutex GlobalsSeenMutex;
+  std::condition_variable GlobalsSeenCV;
 };
 
 } // end of namespace Ice

diff --git a/tests_lit/asan_tests/no_globals.ll b/tests_lit/asan_tests/no_globals.ll
new file mode 100644 (file)
index 0000000..1a3d7f9
--- /dev/null
+++ b/tests_lit/asan_tests/no_globals.ll
@@ -0,0 +1,19 @@
+; Check that Subzero can instrument _start when there are no globals.
+; Previously Subzero would deadlock when _start was the first function. Also
+; test that instrumenting start does not deadlock waiting for nonexistent
+; global initializers to be lowered.
+
+; REQUIRES: no_minimal_build
+
+; RUN: %p2i -i %s --args -verbose=inst -fsanitize-address \
+; RUN:     | FileCheck --check-prefix=DUMP %s
+
+; RUN: %p2i -i %s --args -verbose=inst -fsanitize-address -threads=0 \
+; RUN:     | FileCheck --check-prefix=DUMP %s
+
+
+define void @_start(i32 %arg) {
+  ret void
+}
+
+; DUMP: __asan_init