Subzero: Fixed deadlock with _start but no globals

src/IceASanInstrumentation.cpp

 //===- subzero/src/IceASanInstrumentation.cpp - ASan ------------*- C++ -*-===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
@@ skipping 56 matching lines @@
 bool ASanInstrumentation::isInstrumentable(Cfg *Func) {
   std::string FuncName = Func->getFunctionName().toStringOrEmpty();
   return FuncName == "" ||
          (FuncBlackList.count(FuncName) == 0 && FuncName.find(ASanPrefix) != 0);
 }
 
 // Create redzones around all global variables, ensuring that the initializer
 // types of the redzones and their associated globals match so that they are
 // laid out together in memory.
 void ASanInstrumentation::instrumentGlobals(VariableDeclarationList &Globals) {
-  if (DidProcessGlobals)
+  GlobalsMutex.lock();

[Jim Stichnoth, 2016/07/19 14:04:13]

Can you do this?

  std::unique_lock<std::mutex> _(GlobalsMutex);

Then you can remove the lock() and unlock() calls.

[tlively, 2016/07/19 22:27:04]

Done.

+  if (DidProcessGlobals) {
+    GlobalsMutex.unlock();
     return;
+  }
   VariableDeclarationList NewGlobals;
   // Global holding pointers to all redzones
   auto *RzArray = VariableDeclaration::create(&NewGlobals);
   // Global holding sizes of all redzones
   auto *RzSizes = VariableDeclaration::create(&NewGlobals);
 
   RzArray->setName(Ctx, RzArrayName);
   RzSizes->setName(Ctx, RzSizesName);
   RzArray->setIsConstant(true);
   RzSizes->setIsConstant(true);
@@ skipping 39 matching lines @@
     NewGlobals.push_back(RzLeft);
     NewGlobals.push_back(Global);
     NewGlobals.push_back(RzRight);
     RzGlobalsNum += 2;
   }
 
   // Replace old list of globals, without messing up arena allocators
   Globals.clear();
   Globals.merge(&NewGlobals);
   DidProcessGlobals = true;
-  GlobalsDoneCV.notify_all();
+  GlobalsMutex.unlock();
 
   // Log the new set of globals
   if (BuildDefs::dump() && (getFlags().getVerbose() & IceV_GlobalInit)) {
     OstreamLocker _(Ctx);
     Ctx->getStrDump() << "========= Instrumented Globals =========\n";
     for (VariableDeclaration *Global : Globals) {
       Global->dump(Ctx->getStrDump());
     }
   }
 }
@@ skipping 176 matching lines @@
 
 void ASanInstrumentation::instrumentStart(Cfg *Func) {
   Constant *ShadowMemInit =
       Ctx->getConstantExternSym(Ctx->getGlobalString("__asan_init"));
   constexpr SizeT NumArgs = 3;
   constexpr Variable *Void = nullptr;
   constexpr bool NoTailCall = false;
   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();
+  // wait for globals to be installed
+  while (!Ctx->getGlobalsReceived()) {

[tlively, 2016/07/19 07:59:54]

This spinning could be replaced by a condition variable, but I wanted to make
sure this general direction looked good first.

[Jim Stichnoth, 2016/07/19 14:04:13]

Definitely add a TODO about properly using a condition variable.

Also, it would probably be good to do something like
  std::this_thread::yield();
inside the spin loop.

[tlively, 2016/07/19 22:27:04]

Done.

   }
+  // instrument globals if not already done
+  instrumentGlobals(*Ctx->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)));
 }
 
 // TODO(tlively): make this more efficient with swap idiom
 void ASanInstrumentation::finishFunc(Cfg *) {
   ICE_TLS_GET_FIELD(LocalDtors)->clear();
 }
 
 } // end of namespace Ice