[BOLT][AArch64] Handle data at the beginning of a function when disassembling and building CFG.

[treapster]

Summary

This patch adds getFirstInstructionOffset method for BinaryFunction which is used to properly handle cases where data is at zero offset in a function. The main change is that we add basic block at first instruction offset when disassembling, which prevents assertion failures in buildCFG.

[yota9]

Overall LGTM

bolt/test/AArch64/data-at-0-offset.c

4

Which warning is supposed to be here? Please update with more specific string

[yota9]

bolt/test/AArch64/data-at-0-offset.c

2

[treapster]

Seems like clang used on buildkite doesn't mark .space directive as data, changed test to use repeated .byte instead

[treapster]

Add more specific warning

[treapster]

bolt/test/AArch64/data-at-0-offset.c

4

Well, without my patch it just fails with assertion llvm-project/bolt/lib/Core/BinaryFunction.cpp:1982: bool llvm::bolt::BinaryFunction::buildCFG(llvm::bolt::MCPlusBuilder::AllocatorIdTy): Assertion 'PrevBB && "no previous basic block for a fall through"' failed. But just in case i now added check for disassembling failure.

[yota9]

Please fix the test

[treapster]

(Presumably) Fix test

[treapster]

In D127111#3563843, @yota9 wrote:

Please fix the test

Done

[yota9]

LGTM

[rafauler]

Thanks for reviewing @yota9 and thanks for your contribution @treapster. Do you need me to commit this?

[treapster]

@rafauler, yes, it'll be cool if you commit. My author string is Denis Revunov <[e-mail]>. Thanks in advance!

bolt/include/bolt/Core/BinaryFunction.h

public:
   /// Return instruction at a given offset in the function. Valid before
   /// CFG is constructed or while instruction offsets are available in CFG.
   MCInst *getInstructionAtOffset(uint64_t Offset);
 
   const MCInst *getInstructionAtOffset(uint64_t Offset) const {
     return const_cast<BinaryFunction *>(this)->getInstructionAtOffset(Offset);
   }
 
+  /// Return offset for the first instruction. If there is data at the
+  /// beginning of a function then offset of the first instruction could
+  /// be different from 0
+  uint64_t getFirstInstructionOffset() const {
+    if (Instructions.empty())
+      return 0;
+    return Instructions.begin()->first;
+  }
+
   /// Return jump table that covers a given \p Address in memory.
   JumpTable *getJumpTableContainingAddress(uint64_t Address) {
     auto JTI = JumpTables.upper_bound(Address);
     if (JTI == JumpTables.begin())
       return nullptr;
     --JTI;
     if (JTI->first + JTI->second->getSize() > Address)
       return JTI->second;

bolt/lib/Core/BinaryFunction.cpp

add_instruction:
     }
 
     addInstruction(Offset, std::move(Instruction));
   }
 
   // Reset symbolizer for the disassembler.
   BC.SymbolicDisAsm->setSymbolizer(nullptr);
 
+  if (uint64_t Offset = getFirstInstructionOffset())
+    Labels[Offset] = BC.Ctx->createNamedTempSymbol();
+
   clearList(Relocations);
 
   if (!IsSimple) {
     clearList(Instructions);
     return false;
   }
 
   updateState(State::Disassembled);
assert(!BC.HasRelocations &&
            "cannot process file with non-simple function in relocs mode");
     return false;
   }
 
   if (CurrentState != State::Disassembled)
     return false;
 
   assert(BasicBlocks.empty() && "basic block list should be empty");
-  assert((Labels.find(0) != Labels.end()) &&
+  assert((Labels.find(getFirstInstructionOffset()) != Labels.end()) &&
          "first instruction should always have a label");
 
   // Create basic blocks in the original layout order:
   //
   //  * Every instruction with associated label marks
   //    the beginning of a basic block.
   //  * Conditional instruction marks the end of a basic block,
   //    except when the following instruction is an
if (!InsertBB) {
           auto L = BC.scopeLock();
           Label = BC.Ctx->createNamedTempSymbol("FT");
         }
         InsertBB = addBasicBlock(
             Offset, Label, opts::PreserveBlocksAlignment && IsLastInstrNop);
         updateOffset(LastInstrOffset);
       }
     }
-    if (Offset == 0) {
-      // Add associated CFI pseudos in the first offset (0)
-      addCFIPlaceholders(0, InsertBB);
+    if (Offset == getFirstInstructionOffset()) {
+      // Add associated CFI pseudos in the first offset
+      addCFIPlaceholders(Offset, InsertBB);
     }
 
     const bool IsBlockEnd = MIB->isTerminator(Instr);
     IsLastInstrNop = MIB->isNoop(Instr);
     if (!IsLastInstrNop)
       LastInstrOffset = Offset;
     InsertBB->addInstruction(std::move(Instr));
 

bolt/lib/Core/Exceptions.cpp

bool CFIReaderWriter::fillCFIInfoFor(BinaryFunction &Function) const {
   // Ignore zero-length FDE ranges.
   if (I == FDEs.end() || !I->second->getAddressRange())
     return true;
 
   const FDE &CurFDE = *I->second;
   Optional<uint64_t> LSDA = CurFDE.getLSDAAddress();
   Function.setLSDAAddress(LSDA ? *LSDA : 0);
 
-  uint64_t Offset = 0;
+  uint64_t Offset = Function.getFirstInstructionOffset();
   uint64_t CodeAlignment = CurFDE.getLinkedCIE()->getCodeAlignmentFactor();
   uint64_t DataAlignment = CurFDE.getLinkedCIE()->getDataAlignmentFactor();
   if (CurFDE.getLinkedCIE()->getPersonalityAddress()) {
     Function.setPersonalityFunction(
         *CurFDE.getLinkedCIE()->getPersonalityAddress());
     Function.setPersonalityEncoding(
         *CurFDE.getLinkedCIE()->getPersonalityEncoding());
   }

bolt/test/AArch64/data-at-0-offset.c

+// RUN: %clang %cflags -O2 -fPIE -Wl,-q -pie  %s -o %t.exe
+// RUN: llvm-bolt %t.exe -o %t.bolt 2>&1 | FileCheck %s
+// CHECK-NOT: BOLT-WARNING: unable to disassemble instruction at offset
+
+void extra_space() {
+  asm volatile(".rept 256\n"
+               "    .byte 0xff\n"
+               ".endr\n");
+  return;
+}
+
+int main(int argc, char **argv) {
+  void (*fn)(void);
+  fn = extra_space + 256;
+  fn();
+  return 0;
+}