[PATCH] [FaultMaps] Add a parser for the __llvm__faultmaps section.

[Sanjoy Das]

================
Comment at: include/llvm/CodeGen/FaultMaps.h:75-76
@@ +74,4 @@
+/// 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.
----------------
JosephTremoulet wrote:
> I think you have a typo or missing word(s) here (was it supposed to read "version of LLVM that includes it"?).
Fixed.

================
Comment at: include/llvm/CodeGen/FaultMaps.h:117
@@ +116,3 @@
+    uint64_t getFunctionAddr() const {
+      return read<uint64_t>(P + FunctionAddrOffset, E);
+    }
----------------
JosephTremoulet wrote:
> I had the same reaction as Philip on seeing the uint//nn//_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?
That's a good idea, done.

================
Comment at: include/llvm/CodeGen/FaultMaps.h:124
@@ +123,3 @@
+
+    FunctionFaultInfoAccessor getFunctionFaultInfoAt(uint32_t idx) const {
+      assert(idx < getNumFaultingPCs() && "index out of bounds!");
----------------
JosephTremoulet wrote:
> Should "idx" be "Idx" instead?
Fixed.

================
Comment at: include/llvm/CodeGen/FaultMaps.h:132-135
@@ +131,6 @@
+    FunctionInfoAccessor getNextFunctionInfo() const {
+      size_t MySize = sizeof(uint64_t) + // FunctionAddress
+                      sizeof(uint32_t) + // NumFaultingPCs
+                      sizeof(uint32_t) + // Reserved
+                      getNumFaultingPCs() * FunctionFaultInfoSize;
+
----------------
JosephTremoulet wrote:
> This could be `size_t NextFunctionInfoOffset = FunctionFaultInfosOffset + getNumFaultingPCs() * FunctionFaultInfoSize;` to avoid needing to duplicate the whole field list here.
Fixed.

================
Comment at: test/CodeGen/X86/implicit-null-check.ll:2
@@ -1,1 +1,3 @@
 ; 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
----------------
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`.

================
Comment at: test/CodeGen/X86/implicit-null-check.ll:3
@@ -2,1 +2,3 @@
+; 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
 
----------------
rafael wrote:
> These are both little endian. Your code is templated on endiannes, so you should probably include a big endian test.
Right now the faultmaps section is generated only for x86_64.

http://reviews.llvm.org/D10491

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/