[BOLT] Fix trap value for non-X86

[treapster]

Summary

The trap value used by BOLT was assumed to be single-byte instruction. It made some functions unaligned on AArch64(e.g exceptions-instrumentation test with AArch64 instrumentation) and caused emission failures. Fix that by changing fill value to target-specific StringRef. Since AArch64 instrumentation is not merged yet, here i use mark-funcs option to trigger the bug and provide a testcase.

[treapster]

clang-format

[rafauler]

LGTM. Thanks!

bolt/lib/Core/BinaryEmitter.cpp

383

Note: I'm guessing this is not accounted for in LongJmp.cpp.. this will be one source of mismatched layout between what LongJmp thinks the layout will be and what the layout really is, leading to missing trampolines in some cases.

This is a point in favor of maybe trying to insert the trampolines in JITLink maybe, as syncing LongJmp's understanding of the layout with what actually is happening in BOLT's emitter is a hard task.

bolt/include/bolt/Core/MCPlusBuilder.h

public:
   }
 
   /// Returns true if First/Second is a AUIPC/JALR call pair.
   virtual bool isRISCVCall(const MCInst &First, const MCInst &Second) const {
     llvm_unreachable("not implemented");
     return false;
   }
 
-  /// If non-zero, this is used to fill the executable space with instructions
-  /// that will trap. Defaults to 0.
-  virtual unsigned getTrapFillValue() const { return 0; }
+  /// Used to fill the executable space with instructions
+  /// that will trap.
+  virtual StringRef getTrapFillValue() const {
+    llvm_unreachable("not implemented");
+    return StringRef();
+  }
 
   /// Interface and basic functionality of a MCInstMatcher. The idea is to make
   /// it easy to match one or more MCInsts against a tree-like pattern and
   /// extract the fragment operands. Example:
   ///
   ///   auto IndJmpMatcher =
   ///       matchIndJmp(matchAdd(matchAnyOperand(), matchAnyOperand()));
   ///   if (!IndJmpMatcher->match(...))

bolt/lib/Core/BinaryEmitter.cpp

bool BinaryEmitter::emitFunction(BinaryFunction &Function,
   // Emit padding if requested.
   if (size_t Padding = opts::padFunction(Function)) {
     LLVM_DEBUG(dbgs() << "BOLT-DEBUG: padding function " << Function << " with "
                       << Padding << " bytes\n");
     Streamer.emitFill(Padding, MAI->getTextAlignFillValue());
   }
 
   if (opts::MarkFuncs)
-    Streamer.emitIntValue(BC.MIB->getTrapFillValue(), 1);
+    Streamer.emitBytes(BC.MIB->getTrapFillValue());
 
   // Emit CFI end
   if (Function.hasCFI())
     Streamer.emitCFIEndProc();
 
   MCSymbol *EndSymbol = Function.getFunctionEndLabel(FF.getFragmentNum());
   Streamer.emitLabel(EndSymbol);
 
void BinaryEmitter::emitFunctionBody(BinaryFunction &BF, FunctionFragment &FF,
   if (!FF.empty() && FF.front()->isLandingPad()) {
     assert(!FF.front()->isEntryPoint() &&
            "Landing pad cannot be entry point of function");
     // If the first block of the fragment is a landing pad, it's offset from the
     // start of the area that the corresponding LSDA describes is zero. In this
     // case, the call site entries in that LSDA have 0 as offset to the landing
     // pad, which the runtime interprets as "no handler". To prevent this,
     // insert some padding.
-    Streamer.emitIntValue(BC.MIB->getTrapFillValue(), 1);
+    Streamer.emitBytes(BC.MIB->getTrapFillValue());
   }
 
   // Track the first emitted instruction with debug info.
   bool FirstInstr = true;
   for (BinaryBasicBlock *const BB : FF) {
     if ((opts::AlignBlocks || opts::PreserveBlocksAlignment) &&
         BB->getAlignment() > 1)
       Streamer.emitCodeAlignment(BB->getAlign(), &*BC.STI,

bolt/lib/Rewrite/RewriteInstance.cpp

void RewriteInstance::rewriteFile() {
   if (BC->HasRelocations && opts::TrapOldCode) {
     uint64_t SavedPos = OS.tell();
     // Overwrite function body to make sure we never execute these instructions.
     for (auto &BFI : BC->getBinaryFunctions()) {
       BinaryFunction &BF = BFI.second;
       if (!BF.getFileOffset() || !BF.isEmitted())
         continue;
       OS.seek(BF.getFileOffset());
-      for (unsigned I = 0; I < BF.getMaxSize(); ++I)
-        OS.write((unsigned char)BC->MIB->getTrapFillValue());
+      StringRef TrapInstr = BC->MIB->getTrapFillValue();
+      unsigned NInstr = BF.getMaxSize() / TrapInstr.size();
+      for (unsigned I = 0; I < NInstr; ++I)
+        OS.write(TrapInstr.data(), TrapInstr.size());
     }
     OS.seek(SavedPos);
   }
 
   // Write all allocatable sections - reloc-mode text is written here as well
   for (BinarySection &Section : BC->allocatableSections()) {
     if (!Section.isFinalized() || !Section.getOutputData())
       continue;

bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp

bool shouldRecordCodeRelocation(uint64_t RelType) const override {
     case ELF::R_AARCH64_TLSLE_ADD_TPREL_HI12:
     case ELF::R_AARCH64_TLSLE_ADD_TPREL_LO12_NC:
       return false;
     default:
       llvm_unreachable("Unexpected AArch64 relocation type in code");
     }
   }
 
+  StringRef getTrapFillValue() const override {
+    return StringRef("\0\0\0\0", 4);
+  }
+
   bool createReturn(MCInst &Inst) const override {
     Inst.setOpcode(AArch64::RET);
     Inst.clear();
     Inst.addOperand(MCOperand::createReg(AArch64::LR));
     return true;
   }
 
   InstructionListType materializeAddress(const MCSymbol *Target, MCContext *Ctx,

bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp

bool createUncondBranch(MCInst &Inst, const MCSymbol *TBB,
     Inst.setOpcode(RISCV::JAL);
     Inst.clear();
     Inst.addOperand(MCOperand::createReg(RISCV::X0));
     Inst.addOperand(MCOperand::createExpr(
         MCSymbolRefExpr::create(TBB, MCSymbolRefExpr::VK_None, *Ctx)));
     return true;
   }
 
+  StringRef getTrapFillValue() const override {
+    return StringRef("\0\0\0\0", 4);
+  }
+
   bool analyzeBranch(InstructionIterator Begin, InstructionIterator End,
                      const MCSymbol *&TBB, const MCSymbol *&FBB,
                      MCInst *&CondBranch,
                      MCInst *&UncondBranch) const override {
     auto I = End;
 
     while (I != Begin) {
       --I;

bolt/lib/Target/X86/X86MCPlusBuilder.cpp

bool shouldRecordCodeRelocation(uint64_t RelType) const override {
     case ELF::R_X86_64_TPOFF32:
     case ELF::R_X86_64_GOTTPOFF:
       return false;
     default:
       llvm_unreachable("Unexpected x86 relocation type in code");
     }
   }
 
-  unsigned getTrapFillValue() const override { return 0xCC; }
+  StringRef getTrapFillValue() const override { return StringRef("\314", 1); }
 
   struct IndJmpMatcherFrag1 : MCInstMatcher {
     std::unique_ptr<MCInstMatcher> Base;
     std::unique_ptr<MCInstMatcher> Scale;
     std::unique_ptr<MCInstMatcher> Index;
     std::unique_ptr<MCInstMatcher> Offset;
 
     IndJmpMatcherFrag1(std::unique_ptr<MCInstMatcher> Base,