[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 ↗

(On Diff #27795)

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

118 ↗

(On Diff #27795)

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 ↗

(On Diff #27795)

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 ↗

(On Diff #27795)

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 ↗

(On Diff #27795)

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

212 ↗

(On Diff #27795)

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 ↗

(On Diff #27795)

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 ↗

(On Diff #27795)

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

1236 ↗

(On Diff #27795)

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

1249 ↗

(On Diff #27795)

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 ↗

(On Diff #27795)

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 ↗

(On Diff #27795)

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

[sanjoy]

include/llvm/CodeGen/FaultMaps.h

74 ↗

(On Diff #27795)

Done.

118 ↗

(On Diff #27795)

Done.

199 ↗

(On Diff #27795)

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

212 ↗

(On Diff #27795)

Makes sense, fixed.

tools/llvm-objdump/llvm-objdump.cpp

158 ↗

(On Diff #27795)

Fixed.

1235 ↗

(On Diff #27795)

Fixed.

1236 ↗

(On Diff #27795)

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 ↗

(On Diff #27795)

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

1255 ↗

(On Diff #27795)

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 ↗

(On Diff #27886)

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

3 ↗

(On Diff #27886)

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 ↗

(On Diff #27886)

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

117 ↗

(On Diff #27886)

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 ↗

(On Diff #27886)

Should "idx" be "Idx" instead?

132–135 ↗

(On Diff #27886)

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 ↗

(On Diff #27886)

Fixed.

117 ↗

(On Diff #27886)

That's a good idea, done.

124 ↗

(On Diff #27886)

Fixed.

132–135 ↗

(On Diff #27886)

Fixed.

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

2 ↗

(On Diff #27886)

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 ↗

(On Diff #27886)

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 ↗

(On Diff #27975)

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.

llvm/trunk/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 LLVM that includes it.  No guarantees are made
+/// with respect to forward or backward compatibility.
+class FaultMapParser {
+  typedef uint8_t FaultMapVersionType;
+  static const size_t FaultMapVersionOffset = 0;
+
+  typedef uint8_t Reserved0Type;
+  static const size_t Reserved0Offset =
+      FaultMapVersionOffset + sizeof(FaultMapVersionType);
+
+  typedef uint16_t Reserved1Type;
+  static const size_t Reserved1Offset = Reserved0Offset + sizeof(Reserved0Type);
+
+  typedef uint32_t NumFunctionsType;
+  static const size_t NumFunctionsOffset =
+      Reserved1Offset + sizeof(Reserved1Type);
+
+  static const size_t FunctionInfosOffset =
+      NumFunctionsOffset + sizeof(NumFunctionsType);
+
+  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, support::little, 1>(P);
+  }
+
+public:
+  class FunctionFaultInfoAccessor {
+    typedef uint32_t FaultKindType;
+    static const size_t FaultKindOffset = 0;
+
+    typedef uint32_t FaultingPCOffsetType;
+    static const size_t FaultingPCOffsetOffset =
+        FaultKindOffset + sizeof(FaultKindType);
+
+    typedef uint32_t HandlerPCOffsetType;
+    static const size_t HandlerPCOffsetOffset =
+        FaultingPCOffsetOffset + sizeof(FaultingPCOffsetType);
+
+    const uint8_t *P;
+    const uint8_t *E;
+
+  public:
+    static const size_t Size =
+        HandlerPCOffsetOffset + sizeof(HandlerPCOffsetType);
+
+    explicit FunctionFaultInfoAccessor(const uint8_t *P, const uint8_t *E)
+        : P(P), E(E) {}
+
+    FaultKindType getFaultKind() const {
+      return read<FaultKindType>(P + FaultKindOffset, E);
+    }
+
+    FaultingPCOffsetType getFaultingPCOffset() const {
+      return read<FaultingPCOffsetType>(P + FaultingPCOffsetOffset, E);
+    }
+
+    HandlerPCOffsetType getHandlerPCOffset() const {
+      return read<HandlerPCOffsetType>(P + HandlerPCOffsetOffset, E);
+    }
+  };
+
+  class FunctionInfoAccessor {
+    typedef uint64_t FunctionAddrType;
+    static const size_t FunctionAddrOffset = 0;
+
+    typedef uint32_t NumFaultingPCsType;
+    static const size_t NumFaultingPCsOffset =
+        FunctionAddrOffset + sizeof(FunctionAddrType);
+
+    typedef uint32_t ReservedType;
+    static const size_t ReservedOffset =
+        NumFaultingPCsOffset + sizeof(NumFaultingPCsType);
+
+    static const size_t FunctionFaultInfosOffset =
+        ReservedOffset + sizeof(ReservedType);
+
+    static const size_t FunctionInfoHeaderSize = FunctionFaultInfosOffset;
+
+    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) {}
+
+    FunctionAddrType getFunctionAddr() const {
+      return read<FunctionAddrType>(P + FunctionAddrOffset, E);
+    }
+
+    NumFaultingPCsType getNumFaultingPCs() const {
+      return read<NumFaultingPCsType>(P + NumFaultingPCsOffset, E);
+    }
+
+    FunctionFaultInfoAccessor getFunctionFaultInfoAt(uint32_t Index) const {
+      assert(Index < getNumFaultingPCs() && "index out of bounds!");
+      const uint8_t *Begin = P + FunctionFaultInfosOffset +
+                             FunctionFaultInfoAccessor::Size * Index;
+      return FunctionFaultInfoAccessor(Begin, E);
+    }
+
+    FunctionInfoAccessor getNextFunctionInfo() const {
+      size_t MySize = FunctionInfoHeaderSize +
+                      getNumFaultingPCs() * FunctionFaultInfoAccessor::Size;
+
+      const uint8_t *Begin = P + MySize;
+      assert(Begin < E && "out of bounds!");
+      return FunctionInfoAccessor(Begin, E);
+    }
+  };
+
+  explicit FaultMapParser(const uint8_t *Begin, const uint8_t *End)
+      : P(Begin), E(End) {}
+
+  FaultMapVersionType getFaultMapVersion() const {
+    auto Version = read<FaultMapVersionType>(P + FaultMapVersionOffset, E);
+    assert(Version == 1 && "only version 1 supported!");
+    return Version;
+  }
+
+  NumFunctionsType getNumFunctions() const {
+    return read<NumFunctionsType>(P + NumFunctionsOffset, E);
+  }
+
+  FunctionInfoAccessor getFirstFunctionInfo() const {
+    const uint8_t *Begin = P + FunctionInfosOffset;
+    return FunctionInfoAccessor(Begin, E);
+  }
+};
+
+raw_ostream &
+operator<<(raw_ostream &OS,
+           const typename FaultMapParser::FunctionFaultInfoAccessor &);
+
+raw_ostream &operator<<(raw_ostream &OS,
+                        const typename FaultMapParser::FunctionInfoAccessor &);
+
+raw_ostream &operator<<(raw_ostream &OS, const FaultMapParser &);
+
 } // namespace llvm
 
 #endif

llvm/trunk/lib/CodeGen/FaultMaps.cpp

const char *FaultMaps::faultTypeToString(FaultMaps::FaultKind FT) {
   switch (FT) {
   default:
     llvm_unreachable("unhandled fault type!");
 
   case FaultMaps::FaultingLoad:
     return "FaultingLoad";
   }
 }
+
+raw_ostream &llvm::
+operator<<(raw_ostream &OS,
+           const typename FaultMapParser::FunctionFaultInfoAccessor &FFI) {
+  OS << "Fault kind: "
+     << FaultMaps::faultTypeToString((FaultMaps::FaultKind)FFI.getFaultKind())
+     << ", faulting PC offset: " << FFI.getFaultingPCOffset()
+     << ", handling PC offset: " << FFI.getHandlerPCOffset();
+  return OS;
+}
+
+raw_ostream &llvm::
+operator<<(raw_ostream &OS,
+           const typename FaultMapParser::FunctionInfoAccessor &FI) {
+  OS << "FunctionAddress: " << format_hex(FI.getFunctionAddr(), 8)
+     << ", NumFaultingPCs: " << FI.getNumFaultingPCs() << "\n";
+  for (unsigned i = 0, e = FI.getNumFaultingPCs(); i != e; ++i)
+    OS << FI.getFunctionFaultInfoAt(i) << "\n";
+  return OS;
+}
+
+raw_ostream &llvm::operator<<(raw_ostream &OS, const FaultMapParser &FMP) {
+  OS << "Version: " << format_hex(FMP.getFaultMapVersion(), 2) << "\n";
+  OS << "NumFunctions: " << FMP.getNumFunctions() << "\n";
+
+  if (FMP.getNumFunctions() == 0)
+    return OS;
+
+  typename FaultMapParser::FunctionInfoAccessor FI;
+
+  for (unsigned i = 0, e = FMP.getNumFunctions(); i != e; ++i) {
+    FI = (i == 0) ? FMP.getFirstFunctionInfo() : FI.getNextFunctionInfo();
+    OS << FI;
+  }
+
+  return OS;
+}

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

 ; RUN: llc -O3 -mtriple=x86_64-apple-macosx -enable-implicit-null-checks < %s | FileCheck %s
 
+; RUN: llc < %s -mtriple=x86_64-apple-macosx -enable-implicit-null-checks \
+; RUN:    | llvm-mc -triple x86_64-apple-macosx -filetype=obj -o - \
+; RUN:    | llvm-objdump -triple x86_64-apple-macosx -fault-map-section - \
+; RUN:    | FileCheck %s -check-prefix OBJDUMP
+
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -enable-implicit-null-checks \
+; RUN:    | llvm-mc -triple x86_64-unknown-linux-gnu -filetype=obj -o - \
+; RUN:    | llvm-objdump -triple x86_64-unknown-linux-gnu -fault-map-section - \
+; RUN:    | 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
 ; CHECK: retq

 ; 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

llvm/trunk/test/MC/X86/faultmap-section-parsing.s

+// RUN: llvm-mc < %s -triple=x86_64-apple-macosx -filetype=obj -o - | llvm-objdump -fault-map-section - | FileCheck %s
+
+  .section  __LLVM_FAULTMAPS,__llvm_faultmaps
+__LLVM_FaultMaps:
+  .byte 1
+  .byte 0
+  .short  0
+  .long 2
+  .quad 0xFFDEAD
+  .long 1
+  .long 0
+  .long 1
+  .long 100
+  .long 200
+
+  .quad 0xFFDAED
+  .long 1
+  .long 0
+  .long 1
+  .long 400
+  .long 500
+
+// CHECK: FaultMap table:
+// CHECK-NEXT: Version: 0x1
+// CHECK-NEXT: NumFunctions: 2
+// CHECK-NEXT: FunctionAddress: 0xffdead, NumFaultingPCs: 1
+// CHECK-NEXT: Fault kind: FaultingLoad, faulting PC offset: 100, handling PC offset: 200
+// CHECK-NEXT: FunctionAddress: 0xffdaed, NumFaultingPCs: 1
+// CHECK-NEXT: Fault kind: FaultingLoad, faulting PC offset: 400, handling PC offset: 500

llvm/trunk/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 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;
 }