[BOLT-AArch64] Fix -icf, -use-old-text and -update-debug-sections

Summary:
Refactor MCInst comparison code to support target-dependent
functionality. This was necessary because AArch64 uses MCTargetExprs
that only the AArch64 backend knows how to unpack it and compare. Also
fix a bug where a relocation against a constant island would make BOLT
create a fixed reference against a code location in a similar way to
read-only data, so when we asked to -use-old-text, the code would break
for this particular HHVM function
(_ZN5folly2io4zlib18defaultZlibOptionsEv) because the reference now
contains invalid data, since the original .text was overwritten. Finally,
fix a bug with -update-debug-sections on AArch64 where the update
loop wasn't expecting a function with zero basic blocks, which can
happen on AArch64 because some functions contain just a constant
island.

(cherry picked from FBD7679244)

bolt/BinaryFunction.cpp

@@ -902,7 +902,6 @@ IndirectBranchType BinaryFunction::processIndirectBranch(MCInst &Instruction,
 
 MCSymbol *BinaryFunction::getOrCreateLocalLabel(uint64_t Address,
                                                 bool CreatePastEnd) {
-  MCSymbol *Result;
   // Check if there's already a registered label.
   auto Offset = Address - getAddress();
 
@@ -919,12 +918,16 @@ MCSymbol *BinaryFunction::getOrCreateLocalLabel(uint64_t Address,
   }
 
   auto LI = Labels.find(Offset);
-  if (LI == Labels.end()) {
-    Result = BC.Ctx->createTempSymbol();
-    Labels[Offset] = Result;
-  } else {
-    Result = LI->second;
+  if (LI != Labels.end())
+    return LI->second;
+
+  // For AArch64, check if this address is part of a constant island.
+  if (MCSymbol *IslandSym = getOrCreateIslandAccess(Address).first) {
+    return IslandSym;
   }
+
+  MCSymbol *Result = BC.Ctx->createTempSymbol();
+  Labels[Offset] = Result;
   return Result;
 }
 

bolt/BinaryFunction.h

@@ -475,7 +475,7 @@ private:
       }
     }
 
-    return MCPlus::equals(InstA, InstB, Comp);
+    return BC.MIB->equals(InstA, InstB, Comp);
   }
 
   /// Recompute landing pad information for the function and all its blocks.

bolt/MCPlus.h

@@ -107,89 +107,6 @@ inline unsigned getNumPrimeOperands(const MCInst &Inst) {
   return Inst.getNumOperands();
 }
 
-template <class Compare>
-inline bool equals(const MCExpr &A, const MCExpr &B, Compare Comp) {
-  if (A.getKind() != B.getKind())
-    return false;
-
-  switch (A.getKind()) {
-  case MCExpr::Constant: {
-    const auto &ConstA = cast<MCConstantExpr>(A);
-    const auto &ConstB = cast<MCConstantExpr>(B);
-    return ConstA.getValue() == ConstB.getValue();
-  }
-
-  case MCExpr::SymbolRef: {
-    const MCSymbolRefExpr &SymbolA = cast<MCSymbolRefExpr>(A);
-    const MCSymbolRefExpr &SymbolB = cast<MCSymbolRefExpr>(B);
-    return SymbolA.getKind() == SymbolB.getKind() &&
-           Comp(&SymbolA.getSymbol(), &SymbolB.getSymbol());
-  }
-
-  case MCExpr::Unary: {
-    const auto &UnaryA = cast<MCUnaryExpr>(A);
-    const auto &UnaryB = cast<MCUnaryExpr>(B);
-    return UnaryA.getOpcode() == UnaryB.getOpcode() &&
-           equals(*UnaryA.getSubExpr(), *UnaryB.getSubExpr(), Comp);
-  }
-
-  case MCExpr::Binary: {
-    const auto &BinaryA = cast<MCBinaryExpr>(A);
-    const auto &BinaryB = cast<MCBinaryExpr>(B);
-    return BinaryA.getOpcode() == BinaryB.getOpcode() &&
-           equals(*BinaryA.getLHS(), *BinaryB.getLHS(), Comp) &&
-           equals(*BinaryA.getRHS(), *BinaryB.getRHS(), Comp);
-  }
-
-  case MCExpr::Target: {
-    llvm_unreachable("target-specific expressions are unsupported");
-  }
-  }
-
-  llvm_unreachable("Invalid expression kind!");
-}
-
-template <class Compare>
-inline bool equals(const MCOperand &A, const MCOperand &B, Compare Comp) {
-  if (A.isReg()) {
-    if (!B.isReg())
-      return false;
-    return A.getReg() == B.getReg();
-  } else if (A.isImm()) {
-    if (!B.isImm())
-      return false;
-    return A.getImm() == B.getImm();
-  } else if (A.isFPImm()) {
-    if (!B.isFPImm())
-      return false;
-    return A.getFPImm() == B.getFPImm();
-  } else if (A.isExpr()) {
-    if (!B.isExpr())
-      return false;
-    return equals(*A.getExpr(), *B.getExpr(), Comp);
-  } else {
-    llvm_unreachable("unexpected operand kind");
-    return false;
-  }
-}
-
-template <class Compare>
-inline bool equals(const MCInst &A, const MCInst &B, Compare Comp) {
-  if (A.getOpcode() != B.getOpcode())
-    return false;
-
-  unsigned NumOperands = MCPlus::getNumPrimeOperands(A);
-  if (NumOperands != MCPlus::getNumPrimeOperands(B))
-    return false;
-
-  for (unsigned Index = 0; Index < NumOperands; ++Index) {
-    if (!equals(A.getOperand(Index), B.getOperand(Index), Comp))
-      return false;
-  }
-
-  return true;
-}
-
 } // namespace MCPlus
 
 } // namespace bolt

bolt/MCPlusBuilder.cpp

@@ -27,6 +27,96 @@ using namespace llvm;
 using namespace bolt;
 using namespace MCPlus;
 
+bool MCPlusBuilder::equals(const MCInst &A, const MCInst &B,
+                           CompFuncTy Comp) const {
+  if (A.getOpcode() != B.getOpcode())
+    return false;
+
+  unsigned NumOperands = MCPlus::getNumPrimeOperands(A);
+  if (NumOperands != MCPlus::getNumPrimeOperands(B))
+    return false;
+
+  for (unsigned Index = 0; Index < NumOperands; ++Index) {
+    if (!equals(A.getOperand(Index), B.getOperand(Index), Comp))
+      return false;
+  }
+
+  return true;
+}
+
+bool MCPlusBuilder::equals(const MCOperand &A, const MCOperand &B,
+                           CompFuncTy Comp) const {
+  if (A.isReg()) {
+    if (!B.isReg())
+      return false;
+    return A.getReg() == B.getReg();
+  } else if (A.isImm()) {
+    if (!B.isImm())
+      return false;
+    return A.getImm() == B.getImm();
+  } else if (A.isFPImm()) {
+    if (!B.isFPImm())
+      return false;
+    return A.getFPImm() == B.getFPImm();
+  } else if (A.isExpr()) {
+    if (!B.isExpr())
+      return false;
+    return equals(*A.getExpr(), *B.getExpr(), Comp);
+  } else {
+    llvm_unreachable("unexpected operand kind");
+    return false;
+  }
+}
+
+bool MCPlusBuilder::equals(const MCExpr &A, const MCExpr &B,
+                           CompFuncTy Comp) const {
+  if (A.getKind() != B.getKind())
+    return false;
+
+  switch (A.getKind()) {
+  case MCExpr::Constant: {
+    const auto &ConstA = cast<MCConstantExpr>(A);
+    const auto &ConstB = cast<MCConstantExpr>(B);
+    return ConstA.getValue() == ConstB.getValue();
+  }
+
+  case MCExpr::SymbolRef: {
+    const MCSymbolRefExpr &SymbolA = cast<MCSymbolRefExpr>(A);
+    const MCSymbolRefExpr &SymbolB = cast<MCSymbolRefExpr>(B);
+    return SymbolA.getKind() == SymbolB.getKind() &&
+           Comp(&SymbolA.getSymbol(), &SymbolB.getSymbol());
+  }
+
+  case MCExpr::Unary: {
+    const auto &UnaryA = cast<MCUnaryExpr>(A);
+    const auto &UnaryB = cast<MCUnaryExpr>(B);
+    return UnaryA.getOpcode() == UnaryB.getOpcode() &&
+           equals(*UnaryA.getSubExpr(), *UnaryB.getSubExpr(), Comp);
+  }
+
+  case MCExpr::Binary: {
+    const auto &BinaryA = cast<MCBinaryExpr>(A);
+    const auto &BinaryB = cast<MCBinaryExpr>(B);
+    return BinaryA.getOpcode() == BinaryB.getOpcode() &&
+           equals(*BinaryA.getLHS(), *BinaryB.getLHS(), Comp) &&
+           equals(*BinaryA.getRHS(), *BinaryB.getRHS(), Comp);
+  }
+
+  case MCExpr::Target: {
+    const auto &TargetExprA = cast<MCTargetExpr>(A);
+    const auto &TargetExprB = cast<MCTargetExpr>(B);
+    return equals(TargetExprA, TargetExprB, Comp);
+  }
+  }
+
+  llvm_unreachable("Invalid expression kind!");
+}
+
+bool MCPlusBuilder::equals(const MCTargetExpr &A, const MCTargetExpr &B,
+                           CompFuncTy Comp) const {
+    llvm_unreachable("target-specific expressions are unsupported");
+}
+
 Optional<MCLandingPad> MCPlusBuilder::getEHInfo(const MCInst &Inst) const {
   if (!isCall(Inst))
     return NoneType();

bolt/MCPlusBuilder.h

@@ -293,6 +293,18 @@ public:
     Allocator.Reset();
   }
 
+  using CompFuncTy = std::function<bool(const MCSymbol *, const MCSymbol *)>;
+
+  bool equals(const MCInst &A, const MCInst &B, CompFuncTy Comp) const;
+
+  bool equals(const MCOperand &A, const MCOperand &B,
+                      CompFuncTy Comp) const;
+
+  bool equals(const MCExpr &A, const MCExpr &B, CompFuncTy Comp) const;
+
+  virtual bool equals(const MCTargetExpr &A, const MCTargetExpr &B,
+                      CompFuncTy Comp) const;
+
   virtual bool isBranch(const MCInst &Inst) const {
     return Analysis->isBranch(Inst);
   }

bolt/Passes/ShrinkWrapping.cpp

@@ -1313,7 +1313,8 @@ void ShrinkWrapping::moveSaveRestores() {
 namespace {
 /// Helper function to identify whether two basic blocks created by splitting
 /// a critical edge have the same contents.
-bool isIdenticalSplitEdgeBB(const BinaryBasicBlock &A,
+bool isIdenticalSplitEdgeBB(const BinaryContext &BC,
+                            const BinaryBasicBlock &A,
                             const BinaryBasicBlock &B) {
   if (A.succ_size() != B.succ_size())
     return false;
@@ -1332,7 +1333,7 @@ bool isIdenticalSplitEdgeBB(const BinaryBasicBlock &A,
   while (I != E && OtherI != OtherE) {
     if (I->getOpcode() != OtherI->getOpcode())
       return false;
-    if (!MCPlus::equals(*I, *OtherI,
+    if (!BC.MIB->equals(*I, *OtherI,
           [](const MCSymbol *A, const MCSymbol *B) { return true; }))
       return false;
     ++I;
@@ -1354,7 +1355,7 @@ bool ShrinkWrapping::foldIdenticalSplitEdges() {
         break;
       if (!RBB.getName().startswith(".LSplitEdge") ||
           !RBB.isValid() ||
-          !isIdenticalSplitEdgeBB(*Iter, RBB))
+          !isIdenticalSplitEdgeBB(BC, *Iter, RBB))
         continue;
       assert(RBB.pred_size() == 1 && "Invalid split edge BB");
       BinaryBasicBlock *Pred = *RBB.pred_begin();

bolt/RewriteInstance.cpp

@@ -2106,8 +2106,8 @@ void RewriteInstance::readRelocations(const SectionRef &Section) {
 
     // Occasionally we may see a reference past the last byte of the function
     // typically as a result of __builtin_unreachable(). Check it here.
-    auto *ReferencedBF =
-      getBinaryFunctionContainingAddress(Address, /*CheckPastEnd*/ true);
+    auto *ReferencedBF = getBinaryFunctionContainingAddress(
+        Address, /*CheckPastEnd*/ true, /*UseMaxSize*/ IsAArch64);
     uint64_t RefFunctionOffset = 0;
     MCSymbol *ReferencedSymbol = nullptr;
     if (ForceRelocation) {
@@ -3059,6 +3059,10 @@ void RewriteInstance::updateOutputValues(const MCAsmLayout &Layout) {
     if (!Function.isSimple() && !BC->HasRelocations)
       continue;
 
+    // AArch64 may have functions that only contains a constant island (no code)
+    if (Function.layout_begin() == Function.layout_end())
+      continue;
+
     BinaryBasicBlock *PrevBB = nullptr;
     for (auto BBI = Function.layout_begin(), BBE = Function.layout_end();
          BBI != BBE; ++BBI) {

bolt/Target/AArch64/AArch64MCPlusBuilder.cpp

@@ -46,6 +46,17 @@ public:
                        const MCRegisterInfo *RegInfo)
     : MCPlusBuilder(Analysis, Info, RegInfo) {}
 
+  bool equals(const MCTargetExpr &A, const MCTargetExpr &B,
+              CompFuncTy Comp) const override {
+    const auto &AArch64ExprA = cast<AArch64MCExpr>(A);
+    const auto &AArch64ExprB = cast<AArch64MCExpr>(B);
+    if (AArch64ExprA.getKind() !=  AArch64ExprB.getKind())
+      return false;
+
+    return MCPlusBuilder::equals(*AArch64ExprA.getSubExpr(),
+                                 *AArch64ExprB.getSubExpr(), Comp);
+  }
+
   bool hasEVEXEncoding(const MCInst &) const override {
     return false;
   }