[BOLT] Fix state of MCSymbols in lowering pass

[rafauler]

Summary

We have mostly harmless data races when running
BinaryContext::calculateEmittedSize() in parallel, while performing
split function pass. However, it is possible to end up in a state
where some MCSymbols are still registered and our clean up
failed. This happens rarely but it does happen, and when it happens,
it is a difficult to diagnose heisenbug. To avoid this, add a new
clean pass to perform a last check on MCSymbols, before they
undergo our final emission pass, to verify that they are in a sane
state. If we fail to do this, we might resolve some symbols to zero
and crash the output binary.

[Amir]

Thank you for fixing this non-determinism!
LGTM sans clang-format issue. I wish there was a way to clear symbol state without doing the series of dyn_casts.

[rafauler]

Fix clang format

[tschuett]

bolt/lib/Passes/BinaryPasses.cpp

599

static

[rafauler]

bolt/lib/Passes/BinaryPasses.cpp

599

this is under an anonymous namespace

[tschuett]

bolt/lib/Passes/BinaryPasses.cpp

599

Exactly. You are supposed to use static instead of anonymous namespaces. It is about readability. If I look at resetMCSymbolState, then I cannot see that it has internal linkage. static makes the job simpler.

[rafauler]

bolt/lib/Passes/BinaryPasses.cpp

599

Oh, I didn't know/remember this rule changed in LLVM coding standards. Sounds good, thanks.

Ref for other readers: https://llvm.org/docs/CodingStandards.html#id58

[rafauler]

Convert anonymous namespace -> static

[rafauler]

Use a simpler strategy suggested by @maksfb, iterating over MCContext
symbol table.

[rafauler]

This new pass typically runs in the order of less than 100ms for very large binaries, so it does not introduce any important overhead to our pipeline.

bolt/lib/Passes/BinaryPasses.cpp

ParallelUtilities::runOnEachFunction(
       SkipFunc, "CheckLargeFunctions");
 }
 
 bool CheckLargeFunctions::shouldOptimize(const BinaryFunction &BF) const {
   // Unlike other passes, allow functions in non-CFG state.
   return BF.isSimple() && !BF.isIgnored();
 }
 
+namespace {
+
+// Check for dirty state in MCSymbol objects that might be a consequence
+// of running calculateEmittedSize() in parallel, during split functions
+// pass. If an inconsistent state is found (symbol already registered or
+// already defined), clean it.
+void resetMCSymbolState(const MCExpr *Expr) {
+  if (const MCUnaryExpr *UE = dyn_cast<MCUnaryExpr>(Expr)) {
+    resetMCSymbolState(UE->getSubExpr());
+    return;
+  }
+  if (const MCBinaryExpr *BE = dyn_cast<MCBinaryExpr>(Expr)) {
+    resetMCSymbolState(BE->getLHS());
+    resetMCSymbolState(BE->getRHS());
+    return;
+  }
+  if (const MCSymbolRefExpr *SRef = dyn_cast<MCSymbolRefExpr>(Expr)) {
+    const MCSymbol &S = SRef->getSymbol();
+    if (S.isDefined()) {
+      LLVM_DEBUG(dbgs() << "BOLT-DEBUG: Symbol \"" << S.getName()
+                        << "\" is already defined\n");
+      const_cast<MCSymbol *>(&S)->setUndefined();
+    }
+    if (S.isRegistered()) {
+      LLVM_DEBUG(dbgs() << "BOLT-DEBUG: Symbol \"" << S.getName()
+                        << "\" is already registered\n");
+      const_cast<MCSymbol *>(&S)->setIsRegistered(false);
+    }
+    LLVM_DEBUG(if (S.isVariable()) {
+      dbgs() << "BOLT-DEBUG: Symbol \"" << S.getName() << "\" is variable\n";
+    });
+    return;
+  }
+}
+
+} // end anonymous namespace
+
 void LowerAnnotations::runOnFunctions(BinaryContext &BC) {
   std::vector<std::pair<MCInst *, uint32_t>> PreservedOffsetAnnotations;
 
   for (auto &It : BC.getBinaryFunctions()) {
     BinaryFunction &BF = It.second;
 
     for (FunctionFragment &FF : BF.getLayout().fragments()) {
       int64_t CurrentGnuArgsSize = 0;
for (FunctionFragment &FF : BF.getLayout().fragments()) {
               II = std::next(InsertII);
             }
           }
         }
 
         // Now record preserved annotations separately and then strip
         // annotations.
         for (auto II = BB->begin(); II != BB->end(); ++II) {
+          // Check for dirty state of MCSymbols caused by running
+          // calculateEmittedSize in parallel and restore them
+          for (unsigned OpI = 0, OpE = II->getNumOperands(); OpI != OpE; ++OpI) {
+            const MCOperand& Operand = II->getOperand(OpI);
+            if (!Operand.isExpr())
+              continue;
+            resetMCSymbolState(Operand.getExpr());
+          }
           if (BF.requiresAddressTranslation() && BC.MIB->getOffset(*II))
             PreservedOffsetAnnotations.emplace_back(&(*II),
                                                     *BC.MIB->getOffset(*II));
           BC.MIB->stripAnnotations(*II);
         }
       }
     }
   }