[PATCH] D137984: [BOLT] Fix state of MCSymbols in lowering pass

[Rafael Auler via Phabricator via llvm-commits]

rafauler created this revision.
rafauler added a reviewer: bolt.
Herald added subscribers: treapster, ayermolo, kristof.beyls.
Herald added a reviewer: Amir.
Herald added a reviewer: maksfb.
Herald added a project: All.
rafauler requested review of this revision.
Herald added subscribers: llvm-commits, yota9.
Herald added a project: LLVM.

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, change our
lowering 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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137984

Files:
  bolt/lib/Passes/BinaryPasses.cpp


Index: bolt/lib/Passes/BinaryPasses.cpp
===================================================================
--- bolt/lib/Passes/BinaryPasses.cpp
+++ bolt/lib/Passes/BinaryPasses.cpp
@@ -590,6 +590,43 @@
   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;
 
@@ -623,6 +660,14 @@
         // 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));


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D137984.475274.patch
Type: text/x-patch
Size: 2506 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20221114/8cbf622f/attachment.bin>