[BOLT][NFC] Refactor creation of symbol+addend references

[rafauler]

Summary

Put code that creates references to symbol+addend behind MCPlusBuilder.
Will use this later in validate memory references pass.

[yota9]

Thanks for the refactoring!

bolt/lib/Core/BinaryFunction.cpp

1054

Maybe change assert to if + exit ?

[rafauler]

bolt/lib/Core/BinaryFunction.cpp

1054

Are we migrating away from assertions? This is not supposed to fail at all, unless we modify our codebase in weird ways, so I would like to assert here. But I'm not sure how hard are people trying to get rid of assertions.

Another reason this might fail is if somebody is writing a new backend and fails to replace the memory operand of a given instruction that they need to support replacing.

[maksfb]

Add "[BOLT]" to the title. Replace "this code" in the Summary with the description of the code. Otherwise LGTM.

bolt/lib/Core/BinaryFunction.cpp

1054

It it's a validation of internal assumptions, not an assumption about the input, then it should be an assertion.

1054

[rafauler]

Thanks all

bolt/lib/Core/BinaryFunction.cpp

1054

I don't follow this change, assertions (at least in this file) follow lower case messages with no period. Am I missing something?

[maksfb]

bolt/lib/Core/BinaryFunction.cpp

1054

You are right about the existing assertions. The idea is to move toward LLVM standard in messages and assertions, hence the suggestion.

[rafauler]

Updating assertion message

bolt/include/bolt/Core/MCPlusBuilder.h

public:
 
   /// Return true if this instruction uses the specified physical
   /// register either explicitly or implicitly.
   virtual bool hasUseOfPhysReg(const MCInst &MI, unsigned Reg) const;
 
   /// Replace displacement in compound memory operand with given \p Label.
   bool replaceMemOperandDisp(MCInst &Inst, const MCSymbol *Label,
                              MCContext *Ctx) const {
-    return replaceMemOperandDisp(
-        Inst, MCOperand::createExpr(MCSymbolRefExpr::create(Label, *Ctx)));
+    return replaceMemOperandDisp(Inst, Label, 0, Ctx);
+  }
+
+  /// Replace displacement in compound memory operand with given \p Label
+  /// plus addend.
+  bool replaceMemOperandDisp(MCInst &Inst, const MCSymbol *Label,
+                             int64_t Addend, MCContext *Ctx) const {
+    MCInst::iterator MemOpI = getMemOperandDisp(Inst);
+    if (MemOpI == Inst.end())
+      return false;
+    return setOperandToSymbolRef(Inst, MemOpI - Inst.begin(), Label, Addend,
+                                 Ctx, 0);
   }
 
   /// Returns how many bits we have in this instruction to encode a PC-rel
   /// imm.
   virtual int getPCRelEncodingSize(const MCInst &Inst) const {
     llvm_unreachable("not implemented");
     return 0;
   }

bolt/lib/Core/BinaryContext.cpp

BinaryContext::getSubBinaryData(BinaryData *BD) {
   while (End != BinaryDataMap.end() && BD->isAncestorOf(End->second))
     ++End;
   return make_range(Start, End);
 }
 
 std::pair<const MCSymbol *, uint64_t>
 BinaryContext::handleAddressRef(uint64_t Address, BinaryFunction &BF,
                                 bool IsPCRel) {
-  uint64_t Addend = 0;
-
   if (isAArch64()) {
     // Check if this is an access to a constant island and create bookkeeping
     // to keep track of it and emit it later as part of this function.
     if (MCSymbol *IslandSym = BF.getOrCreateIslandAccess(Address))
-      return std::make_pair(IslandSym, Addend);
+      return std::make_pair(IslandSym, 0);
 
     // Detect custom code written in assembly that refers to arbitrary
     // constant islands from other functions. Write this reference so we
     // can pull this constant island and emit it as part of this function
     // too.
     auto IslandIter = AddressToConstantIslandMap.lower_bound(Address);
 
     if (IslandIter != AddressToConstantIslandMap.begin() &&
         (IslandIter == AddressToConstantIslandMap.end() ||
          IslandIter->first > Address))
       --IslandIter;
 
     if (IslandIter != AddressToConstantIslandMap.end()) {
       if (MCSymbol *IslandSym =
               IslandIter->second->getOrCreateProxyIslandAccess(Address, BF)) {
         BF.createIslandDependency(IslandSym, IslandIter->second);
-        return std::make_pair(IslandSym, Addend);
+        return std::make_pair(IslandSym, 0);
       }
     }
   }
 
   // Note that the address does not necessarily have to reside inside
   // a section, it could be an absolute address too.
   ErrorOr<BinarySection &> Section = getSectionForAddress(Address);
   if (Section && Section->isText()) {
     if (BF.containsAddress(Address, /*UseMaxSize=*/isAArch64())) {
       if (Address != BF.getAddress()) {
         // The address could potentially escape. Mark it as another entry
         // point into the function.
         if (opts::Verbosity >= 1) {
           outs() << "BOLT-INFO: potentially escaped address 0x"
                  << Twine::utohexstr(Address) << " in function " << BF << '\n';
         }
         BF.HasInternalLabelReference = true;
         return std::make_pair(
-            BF.addEntryPointAtOffset(Address - BF.getAddress()), Addend);
+            BF.addEntryPointAtOffset(Address - BF.getAddress()), 0);
       }
     } else {
       addInterproceduralReference(&BF, Address);
     }
   }
 
   // With relocations, catch jump table references outside of the basic block
   // containing the indirect jump.
   if (HasRelocations) {
     const MemoryContentsType MemType = analyzeMemoryAt(Address, BF);
     if (MemType == MemoryContentsType::POSSIBLE_PIC_JUMP_TABLE && IsPCRel) {
       const MCSymbol *Symbol =
           getOrCreateJumpTable(BF, Address, JumpTable::JTT_PIC);
 
-      return std::make_pair(Symbol, Addend);
+      return std::make_pair(Symbol, 0);
     }
   }
 
   if (BinaryData *BD = getBinaryDataContainingAddress(Address))
     return std::make_pair(BD->getSymbol(), Address - BD->getAddress());
 
   // TODO: use DWARF info to get size/alignment here?
   MCSymbol *TargetSymbol = getOrCreateGlobalSymbol(Address, "DATAat");
   LLVM_DEBUG(dbgs() << "Created symbol " << TargetSymbol->getName() << '\n');
-  return std::make_pair(TargetSymbol, Addend);
+  return std::make_pair(TargetSymbol, 0);
 }
 
 MemoryContentsType BinaryContext::analyzeMemoryAt(uint64_t Address,
                                                   BinaryFunction &BF) {
   if (!isX86())
     return MemoryContentsType::UNKNOWN;
 
   ErrorOr<BinarySection &> Section = getSectionForAddress(Address);

bolt/lib/Core/BinaryFunction.cpp

if (TargetAddress == 0 && opts::Verbosity >= 1) {
     outs() << "BOLT-INFO: PC-relative operand is zero in function " << *this
            << '\n';
   }
 
   const MCSymbol *TargetSymbol;
   uint64_t TargetOffset;
   std::tie(TargetSymbol, TargetOffset) =
       BC.handleAddressRef(TargetAddress, *this, /*IsPCRel*/ true);
-  const MCExpr *Expr =
-      MCSymbolRefExpr::create(TargetSymbol, MCSymbolRefExpr::VK_None, *BC.Ctx);
-  if (TargetOffset) {
-    const MCConstantExpr *Offset =
-        MCConstantExpr::create(TargetOffset, *BC.Ctx);
-    Expr = MCBinaryExpr::createAdd(Expr, Offset, *BC.Ctx);
-  }
-  MIB->replaceMemOperandDisp(Instruction,
-                             MCOperand::createExpr(BC.MIB->getTargetExprFor(
-                                 Instruction, Expr, *BC.Ctx, 0)));
+
+  bool ReplaceSuccess = MIB->replaceMemOperandDisp(
+      Instruction, TargetSymbol, static_cast<int64_t>(TargetOffset), &*BC.Ctx);
+  (void)ReplaceSuccess;
+  assert(ReplaceSuccess && "Failed to replace mem operand with symbol+off.");
 }
 
 MCSymbol *BinaryFunction::handleExternalReference(MCInst &Instruction,
                                                   uint64_t Size,
                                                   uint64_t Offset,
                                                   uint64_t TargetAddress,
                                                   bool &IsCall) {
   auto &MIB = BC.MIB;

bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp

bool evaluateMemOperandTarget(const MCInst &Inst, uint64_t &Target,
     Target = DispValue;
     if (Inst.getOpcode() == AArch64::ADRP)
       Target += Address & ~0xFFFULL;
     else
       Target += Address;
     return true;
   }
 
-  bool replaceMemOperandDisp(MCInst &Inst, MCOperand Operand) const override {
+  MCInst::iterator getMemOperandDisp(MCInst &Inst) const override {
     MCInst::iterator OI = Inst.begin();
     if (isADR(Inst) || isADRP(Inst)) {
       assert(MCPlus::getNumPrimeOperands(Inst) >= 2 &&
              "Unexpected number of operands");
-      ++OI;
-    } else {
-      const MCInstrDesc &MCII = Info->get(Inst.getOpcode());
-      for (unsigned I = 0, E = MCII.getNumOperands(); I != E; ++I) {
-        if (MCII.OpInfo[I].OperandType == MCOI::OPERAND_PCREL)
-          break;
-        ++OI;
-      }
-      assert(OI != Inst.end() && "Literal operand not found");
-    }
+      return ++OI;
+    }
+    return OI;
+
+  bool replaceMemOperandDisp(MCInst &Inst, MCOperand Operand) const override {
+    MCInst::iterator OI = getMemOperandDisp(Inst);
     *OI = Operand;
     return true;
   }
 
   const MCExpr *getTargetExprFor(MCInst &Inst, const MCExpr *Expr,
                                  MCContext &Ctx,
                                  uint64_t RelType) const override {