[FaultMaps] Add a parser for the __llvm__faultmaps section.

[sanjoy]

Summary

The parser is exercised by llvm-objdump using -print-fault-maps. As is
probably obvious, the code itself was "heavily inspired" by
http://reviews.llvm.org/D10434.

[reames]

The actual parser part is LGTM w/minor comments addressed. I don't feel comfortable signing off on the objdump parts.

include/llvm/CodeGen/FaultMaps.h

74

Add some comments about compatibility guarantees. It's easier to preempt such things now than introduce them later.

118

Minor style suggestion:
If you predeclared both inner classes, you could put the definition for the FunctionInfoAccessor first. When trying to understand the format, that's the piece you need first.

136

I'm not a huge fan of having the types of each field repeated both in the offset code and reader code. Too much room for mismatches. I don't really have a good suggestion on how to do this otherwise though.

(What you have is fine, just airing a general frustration.)

150

I really don't like this style of iterator, but it's hard to avoid if you want to do a flyweight parser in this style with variably sized entries.

199

I think you can probably get away with these being inferred which would make the code cleaner.

212

A clearer structure would be:
for(int i = 0; i < NumFunctions; i++) {

if (i == 0) FFI = getFirstFunctionInfo();
else FFI = getNextFunctionInfo();
OS << FFI;

}

tools/llvm-objdump/llvm-objdump.cpp

158

The general option naming convention above would seem to call for a name like "-fault-map-section"

Description wise, maybe "Display contents of faultmap section"? I'm trying to get at the fact this is not a hex dump, but not be overly verbose.

1235

The output should probably indicate there isn't a FaultMap table if that's the case.

1236

Is this specific to ELF? Or should it work with any format of object file?

1249

I don't believe StringRef has any backing storage. This looks highly suspicious to say the least. Should this be an std::string instead?

1252

reinterpret_cast?! Couldn't this at least be a static_cast?

const char* -> const uint8_t* doesn't seem like it should need a reinterpret_cast

1255

Why can't you just use begin(), end() on StringRef?

[sanjoy]

include/llvm/CodeGen/FaultMaps.h

74

Done.

118

Done.

199

Do you mean Endianness? If so I tried that and it didn't work.

212

Makes sense, fixed.

tools/llvm-objdump/llvm-objdump.cpp

158

Fixed.

1235

Fixed.

1236

Actually, this worked only with Mach-O. I've added support (and a test) for ELF, and changed printFaultMaps to print an error for other file formats (currently only COFF).

1249

This looks intentional -- as far as I can tell, the sections return a reference to memory they own.

1255

That's a good idea, using bytes_begin() and bytes_end() lets me avoid casting altogether.

[sanjoy]

• Address Philip's review.

[sanjoy]

Hey Rafael,

Can you please take a look at the llvm-objdump portion of this patch?

Thanks,
Sanjoy

[• rafael]

I have no objections (just small comments), but lhames should probably be the one to give the final OK.

test/CodeGen/X86/implicit-null-check.ll

2

Do you really need to use llc? Can't you use llvm-mc or even check in a binary?

3

These are both little endian. Your code is templated on endiannes, so you should probably include a big endian test.

[JosephTremoulet]

LGTM, just some minor cosmetic feedback.

include/llvm/CodeGen/FaultMaps.h

75–76

I think you have a typo or missing word(s) here (was it supposed to read "version of LLVM that includes it"?).

117

I had the same reaction as Philip on seeing the uintnn_t types replicated in getFoo vs the FooOffset calculation. Did you consider introducing typedefs for each field (e.g. typedef uint64_t FunctionAddrType) so that the offset calculation and read template argument could each be in terms of the typedef, and there'd be just one place mapping each field to its type?

124

Should "idx" be "Idx" instead?

132–135

This could be size_t NextFunctionInfoOffset = FunctionFaultInfosOffset + getNumFaultingPCs() * FunctionFaultInfoSize; to avoid needing to duplicate the whole field list here.

[sanjoy]

• address Joseph's review comments
• add an llvm-mc based test

include/llvm/CodeGen/FaultMaps.h

75–76

Fixed.

117

That's a good idea, done.

124

Fixed.

132–135

Fixed.

test/CodeGen/X86/implicit-null-check.ll

2

Unless you have strong objections, I'd like to keep this test because I think it is valuable to check that the parser can parse the output of -enable-implicit-null-checks. I'll add a second test that works with llvm-mc.

3

Right now the faultmaps section is generated only for x86_64.

[• rafael]

---

rafael wrote:

Do you really need to use llc? Can't you use llvm-mc or even check in a binary?

Unless you have strong objections, I'd like to keep this test because I think it is valuable to check that the parser can parse the output of -enable-implicit-null-checks. I'll add a second test that works with llvm-mc.

In which case you should do

llc ... -o %t.s < %s
llvm-mc ... -o %t.o < %t.s

We really should only use "llc -filetype=obj" when there a code path
that is only ever used with that.

Right now the faultmaps section is generated only for x86_64.

Why the template then?

Cheers,
Rafael

[JosephTremoulet]

LGTM, thanks.

tools/llvm-objdump/llvm-objdump.cpp

1241–1245

What's missing that will need to be implemented to support this on COFF? (Not asking you to make any adjustments to this change; just saw this code and wondered about COFF.)

[sanjoy]

• address Rafaels' comment: change test to not pass -filetype=obj to llc

[sanjoy]

• address Rafael's review: remove templating

[• rafael]

This is fine with me.
lhames?

[sanjoy]

ping!

Does it make sense to check in the parser now, and wait for Lang to comment on the objdump bits? I don't really need the objdump changes locally, they're just for testing.

[• rafael]

This is fine. I am sure any further comment can be addressed in post commit.

include/llvm/CodeGen/FaultMaps.h

 //===------------------- FaultMaps.h - StackMaps ----------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CODEGEN_FAULTMAPS_H
 #define LLVM_CODEGEN_FAULTMAPS_H
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/MC/MCSymbol.h"
+#include "llvm/Support/Endian.h"
+#include "llvm/Support/Format.h"
 
 #include <vector>
 #include <map>
 
 namespace llvm {
 
 class AsmPrinter;
 class MCExpr;
private:
   };
 
   std::map<const MCSymbol *, FunctionFaultInfos, MCSymbolComparator>
       FunctionInfos;
   AsmPrinter &AP;
 
   void emitFunctionInfo(const MCSymbol *FnLabel, const FunctionFaultInfos &FFI);
 };
+
+/// A parser for the __llvm_faultmaps section generated by the FaultMaps class
+/// above.  This parser is version locked with with the __llvm_faultmaps section
+/// generated by the version of that includes it.  No guarantees are made with
+/// respect to forward or backward compatibility.
+template <support::endianness Endianness> class FaultMapParser {
+  static const size_t FaultMapVersionOffset = 0;
+  static const size_t Reserved0Offset = FaultMapVersionOffset + sizeof(uint8_t);
+  static const size_t Reserved1Offset = Reserved0Offset + sizeof(uint8_t);
+  static const size_t NumFunctionsOffset = Reserved1Offset + sizeof(uint16_t);
+  static const size_t FunctionInfosOffset =
+      NumFunctionsOffset + sizeof(uint32_t);
+
+  const uint8_t *P;
+  const uint8_t *E;
+
+  template <typename T> static T read(const uint8_t *P, const uint8_t *E) {
+    assert(P + sizeof(T) <= E && "out of bounds read!");
+    return support::endian::read<T, Endianness, 1>(P);
+  }
+
+public:
+  class FunctionFaultInfoAccessor;
+
+  class FunctionInfoAccessor {
+    static const size_t FunctionAddrOffset = 0;
+    static const size_t NumFaultingPCsOffset =
+        FunctionAddrOffset + sizeof(uint64_t);
+    static const size_t ReservedOffset =
+        NumFaultingPCsOffset + sizeof(uint32_t);
+    static const size_t FunctionFaultInfosOffset =
+        ReservedOffset + sizeof(uint32_t);
+    static const size_t FunctionFaultInfoSize = sizeof(uint32_t) * 3;
+
+    const uint8_t *P;
+    const uint8_t *E;
+
+  public:
+    FunctionInfoAccessor() : P(nullptr), E(nullptr) {}
+
+    explicit FunctionInfoAccessor(const uint8_t *P, const uint8_t *E)
+        : P(P), E(E) {}
+
+    uint64_t getFunctionAddr() const {
+      return read<uint64_t>(P + FunctionAddrOffset, E);
+    }
+
+    uint32_t getNumFaultingPCs() const {
+      return read<uint32_t>(P + NumFaultingPCsOffset, E);
+    }
+
+    FunctionFaultInfoAccessor getFunctionFaultInfoAt(uint32_t idx) const {
+      assert(idx < getNumFaultingPCs() && "index out of bounds!");
+      const uint8_t *Begin =
+          P + FunctionFaultInfosOffset + FunctionFaultInfoSize * idx;
+      return FunctionFaultInfoAccessor(Begin, E);
+    }
+
+    FunctionInfoAccessor getNextFunctionInfo() const {
+      size_t MySize = sizeof(uint64_t) + // FunctionAddress
+                      sizeof(uint32_t) + // NumFaultingPCs
+                      sizeof(uint32_t) + // Reserved
+                      getNumFaultingPCs() * FunctionFaultInfoSize;
+
+      const uint8_t *Begin = P + MySize;
+      assert(Begin < E && "out of bounds!");
+      return FunctionInfoAccessor(Begin, E);
+    }
+  };
+
+  class FunctionFaultInfoAccessor {
+    static const size_t FaultKindOffset = 0;
+    static const size_t FaultingPCOffsetOffset =
+        FaultKindOffset + sizeof(uint32_t);
+    static const size_t HandlerPCOffsetOffset =
+        FaultingPCOffsetOffset + sizeof(uint32_t);
+
+    const uint8_t *P;
+    const uint8_t *E;
+
+  public:
+    explicit FunctionFaultInfoAccessor(const uint8_t *P, const uint8_t *E)
+        : P(P), E(E) {}
+
+    uint32_t getFaultKind() const {
+      return read<uint32_t>(P + FaultKindOffset, E);
+    }
+
+    uint32_t getFaultingPCOffset() const {
+      return read<uint32_t>(P + FaultingPCOffsetOffset, E);
+    }
+
+    uint32_t getHandlerPCOffset() const {
+      return read<uint32_t>(P + HandlerPCOffsetOffset, E);
+    }
+  };
+
+  explicit FaultMapParser(const uint8_t *Begin, const uint8_t *End)
+      : P(Begin), E(End) {}
+
+  uint8_t getFaultMapVersion() const {
+    uint8_t Version = read<uint8_t>(P + FaultMapVersionOffset, E);
+    assert(Version == 1 && "only version 1 supported!");
+    return Version;
+  }
+
+  uint32_t getNumFunctions() const {
+    return read<uint32_t>(P + NumFunctionsOffset, E);
+  }
+
+  FunctionInfoAccessor getFirstFunctionInfo() const {
+    const uint8_t *Begin = P + FunctionInfosOffset;
+    return FunctionInfoAccessor(Begin, E);
+  }
+};
+
+template <support::endianness Endianness>
+raw_ostream &operator<<(
+    raw_ostream &OS,
+    const typename FaultMapParser<Endianness>::FunctionFaultInfoAccessor &FFI) {
+  OS << "Fault kind: "
+     << FaultMaps::faultTypeToString((FaultMaps::FaultKind)FFI.getFaultKind())
+     << ", faulting PC offset: " << FFI.getFaultingPCOffset()
+     << ", handling PC offset: " << FFI.getHandlerPCOffset();
+  return OS;
+}
+
+template <support::endianness Endianness>
+raw_ostream &operator<<(
+    raw_ostream &OS,
+    const typename FaultMapParser<Endianness>::FunctionInfoAccessor &FI) {
+  OS << "FunctionAddress: " << format_hex(FI.getFunctionAddr(), 8)
+     << ", NumFaultingPCs: " << FI.getNumFaultingPCs() << "\n";
+  for (unsigned i = 0, e = FI.getNumFaultingPCs(); i != e; ++i)
+    operator<<<Endianness>(OS, FI.getFunctionFaultInfoAt(i)) << "\n";
+  return OS;
+}
+
+template <support::endianness Endianness>
+raw_ostream &operator<<(raw_ostream &OS,
+                        const FaultMapParser<Endianness> &FMP) {
+  OS << "Version: " << format_hex(FMP.getFaultMapVersion(), 2) << "\n";
+  OS << "NumFunctions: " << FMP.getNumFunctions() << "\n";
+
+  if (FMP.getNumFunctions() == 0)
+    return OS;
+
+  typename FaultMapParser<Endianness>::FunctionInfoAccessor FI;
+
+  for (unsigned i = 0, e = FMP.getNumFunctions(); i != e; ++i) {
+    FI = (i == 0) ? FMP.getFirstFunctionInfo() : FI.getNextFunctionInfo();
+    operator<<<Endianness>(OS, FI);
+  }
+
+  return OS;
+}
 }
 
 #endif

test/CodeGen/X86/implicit-null-check.ll

 ; RUN: llc -O3 -mtriple=x86_64-apple-macosx -enable-implicit-null-checks < %s | FileCheck %s
+; RUN: llc -O3 -mtriple=x86_64-apple-macosx -enable-implicit-null-checks < %s -filetype=obj -o - | llvm-objdump -fault-map-section - | FileCheck %s -check-prefix=OBJDUMP
+; RUN: llc -O3 -mtriple=x86_64-unknown-linux-gnu -enable-implicit-null-checks < %s -filetype=obj -o - | llvm-objdump -fault-map-section - | FileCheck %s -check-prefix=OBJDUMP
 
 define i32 @imp_null_check_load(i32* %x) {
 ; CHECK-LABEL: _imp_null_check_load:
 ; CHECK: Ltmp1:
 ; CHECK: movl (%rdi), %eax
 ; CHECK: retq
 ; CHECK: Ltmp0:
 ; CHECK: movl $42, %eax

 ; Reserved:
 ; CHECK-NEXT: .long 0
 ; Fault[0].Type:
 ; CHECK-NEXT: .long 1
 ; Fault[0].FaultOffset:
 ; CHECK-NEXT: .long Ltmp1-_imp_null_check_load
 ; Fault[0].HandlerOffset:
 ; CHECK-NEXT: .long Ltmp0-_imp_null_check_load
+
+; OBJDUMP: FaultMap table:
+; OBJDUMP-NEXT: Version: 0x1
+; OBJDUMP-NEXT: NumFunctions: 3
+; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1
+; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 5
+; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1
+; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 7
+; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1
+; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 3

tools/llvm-objdump/llvm-objdump.cpp

 // flags.
 //
 // The flags and output of this program should be near identical to those of
 // binutils objdump.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm-objdump.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/Triple.h"
+#include "llvm/CodeGen/FaultMaps.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDisassembler.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstPrinter.h"
 #include "llvm/MC/MCInstrAnalysis.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCObjectFileInfo.h"

 static cl::alias
 PrivateHeadersShort("p", cl::desc("Alias for --private-headers"),
                     cl::aliasopt(PrivateHeaders));
 
 cl::opt<bool>
     llvm::PrintImmHex("print-imm-hex",
                       cl::desc("Use hex format for immediate values"));
 
+cl::opt<bool> PrintFaultMaps("fault-map-section",
+                             cl::desc("Display contents of faultmap section"));
+
 static StringRef ToolName;
 static int ReturnValue = EXIT_SUCCESS;
 
 bool llvm::error(std::error_code EC) {
   if (!EC)
     return false;
 
   outs() << ToolName << ": error reading file: " << EC.message() << ".\n";
if (const MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(o))
     printMachOWeakBindTable(MachO);
   else {
     errs() << "This operation is only currently supported "
               "for Mach-O executable files.\n";
     return;
   }
 }
 
+static void printFaultMaps(const ObjectFile *Obj) {
+  const char *FaultMapSectionName = nullptr;
+
+  if (isa<ELFObjectFileBase>(Obj)) {
+    FaultMapSectionName = ".llvm_faultmaps";
+  } else if (isa<MachOObjectFile>(Obj)) {
+    FaultMapSectionName = "__llvm_faultmaps";
+  } else {
+    errs() << "This operation is only currently supported "
+              "for ELF and Mach-O executable files.\n";
+    return;
+  }
+
+  Optional<object::SectionRef> FaultMapSection;
+
+  for (auto Sec : Obj->sections()) {
+    StringRef Name;
+    Sec.getName(Name);
+    if (Name == FaultMapSectionName) {
+      FaultMapSection = Sec;
+      break;
+    }
+  }
+
+  outs() << "FaultMap table:\n";
+
+  if (!FaultMapSection.hasValue()) {
+    outs() << "<not found>\n";
+    return;
+  }
+
+  StringRef FaultMapContents;
+  if (error(FaultMapSection.getValue().getContents(FaultMapContents))) {
+    errs() << "Could not read the " << FaultMapContents << " section!\n";
+    return;
+  }
+
+  FaultMapParser<support::little> FMP(FaultMapContents.bytes_begin(),
+                                      FaultMapContents.bytes_end());
+
+  outs() << FMP;
+}
+
 static void printPrivateFileHeader(const ObjectFile *o) {
   if (o->isELF()) {
     printELFFileHeader(o);
   } else if (o->isCOFF()) {
     printCOFFFileHeader(o);
   } else if (o->isMachO()) {
     printMachOFileHeader(o);
   }
static void DumpObject(const ObjectFile *o) {
   if (Rebase)
     printRebaseTable(o);
   if (Bind)
     printBindTable(o);
   if (LazyBind)
     printLazyBindTable(o);
   if (WeakBind)
     printWeakBindTable(o);
+  if (PrintFaultMaps)
+    printFaultMaps(o);
 }
 
 /// @brief Dump each object file in \a a;
 static void DumpArchive(const Archive *a) {
   for (Archive::child_iterator i = a->child_begin(), e = a->child_end(); i != e;
        ++i) {
     ErrorOr<std::unique_ptr<Binary>> ChildOrErr = i->getAsBinary();
     if (std::error_code EC = ChildOrErr.getError()) {
if (!Disassemble
       && !(ArchiveHeaders && MachOOpt)
       && !(IndirectSymbols && MachOOpt)
       && !(DataInCode && MachOOpt)
       && !(LinkOptHints && MachOOpt)
       && !(InfoPlist && MachOOpt)
       && !(DylibsUsed && MachOOpt)
       && !(DylibId && MachOOpt)
       && !(ObjcMetaData && MachOOpt)
-      && !(DumpSections.size() != 0 && MachOOpt)) {
+      && !(DumpSections.size() != 0 && MachOOpt)
+      && !PrintFaultMaps) {
     cl::PrintHelpMessage();
     return 2;
   }
 
   std::for_each(InputFilenames.begin(), InputFilenames.end(),
                 DumpInput);
 
   return ReturnValue;
 }