[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

581

static

[rafauler]

bolt/lib/Passes/BinaryPasses.cpp

581

this is under an anonymous namespace

[tschuett]

bolt/lib/Passes/BinaryPasses.cpp

581

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

581

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/include/bolt/Passes/BinaryPasses.h

 public:
   explicit LowerAnnotations(const cl::opt<bool> &PrintPass)
       : BinaryFunctionPass(PrintPass) {}
 
   const char *getName() const override { return "lower-annotations"; }
   void runOnFunctions(BinaryContext &BC) override;
 };
 
+/// Clean the state of the MC representation before sending it to emission
+class CleanMCState : public BinaryFunctionPass {
+public:
+  explicit CleanMCState(const cl::opt<bool> &PrintPass)
+      : BinaryFunctionPass(PrintPass) {}
+
+  const char *getName() const override { return "clean-mc-state"; }
+  void runOnFunctions(BinaryContext &BC) override;
+};
+
 /// An optimization to simplify conditional tail calls by removing
 /// unnecessary branches.
 ///
 /// This optimization considers both of the following cases:
 ///
 /// foo: ...
 ///      jcc L1   original
 ///      ...

bolt/lib/Passes/BinaryPasses.cpp

 }
 
 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 (BinaryBasicBlock *const BB : FF) {
         // First convert GnuArgsSize annotations into CFIs. This may change
         // instr pointers, so do it before recording ptrs for preserved
         // annotations
         if (BF.usesGnuArgsSize()) {
           for (auto II = BB->begin(); II != BB->end(); ++II) {
void LowerAnnotations::runOnFunctions(BinaryContext &BC) {
   // Release all memory taken by annotations
   BC.MIB->freeAnnotations();
 
   // Reinsert preserved annotations we need during code emission.
   for (const std::pair<MCInst *, uint32_t> &Item : PreservedOffsetAnnotations)
     BC.MIB->setOffset(*Item.first, Item.second);
 }
 
+// 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 CleanMCState::runOnFunctions(BinaryContext &BC) {
+  MCContext &Ctx = *BC.Ctx;
+  for (const auto &SymMapEntry : Ctx.getSymbols()) {
+    const MCSymbol *S = SymMapEntry.second;
+    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";
+    });
+  }
+}
+
 // This peephole fixes jump instructions that jump to another basic
 // block with a single jump instruction, e.g.
 //
 // B0: ...
 //     jmp  B1   (or jcc B1)
 //
 // B1: jmp  B2
 //

bolt/lib/Rewrite/BinaryPassManager.cpp

void BinaryFunctionPassManager::runAllPasses(BinaryContext &BC) {
   // In non-relocation mode, mark functions that do not fit into their original
   // space as non-simple if we have to (e.g. for correct debug info update).
   // NOTE: this pass depends on finalized code.
   if (!BC.HasRelocations)
     Manager.registerPass(std::make_unique<CheckLargeFunctions>(NeverPrint));
 
   Manager.registerPass(std::make_unique<LowerAnnotations>(NeverPrint));
 
+  // Check for dirty state of MCSymbols caused by running calculateEmittedSize
+  // in parallel and restore them
+  Manager.registerPass(std::make_unique<CleanMCState>(NeverPrint));
+
   Manager.runPasses();
 }
 
 } // namespace bolt
 } // namespace llvm