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

[Sanjoy Das]

================
Comment at: include/llvm/CodeGen/FaultMaps.h:74
@@ +73,3 @@
+
+template <support::endianness Endianness> class FaultMapParser {
+  static const size_t FaultMapVersionOffset = 0;
----------------
reames wrote:
> Add some comments about compatibility guarantees.  It's easier to preempt such things now than introduce them later.
Done.

================
Comment at: include/llvm/CodeGen/FaultMaps.h:118
@@ +117,3 @@
+
+  class FunctionInfoAccessor {
+    static const size_t FunctionAddrOffset = 0;
----------------
reames wrote:
> 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.  
Done.

================
Comment at: include/llvm/CodeGen/FaultMaps.h:199
@@ +198,3 @@
+  for (unsigned i = 0, e = FI.getNumFaultingPCs(); i != e; ++i)
+    operator<<<Endianness>(OS, FI.getFunctionFaultInfoAt(i)) << "\n";
+  return OS;
----------------
reames wrote:
> I think you can probably get away with these being inferred which would make the code cleaner.  
Do you mean `Endianness`?  If so I tried that and it didn't work.

================
Comment at: include/llvm/CodeGen/FaultMaps.h:212
@@ +211,3 @@
+
+  auto FFI = FMP.getFirstFunctionInfo();
+  for (unsigned i = 1, e = FMP.getNumFunctions(); i != e; ++i) {
----------------
reames wrote:
> A clearer structure would be:
> for(int i = 0; i < NumFunctions; i++) {
>   if (i == 0) FFI = getFirstFunctionInfo();
>   else FFI = getNextFunctionInfo();
>   OS << FFI;
> }
>   
Makes sense, fixed.

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:158
@@ -155,1 +157,3 @@
 
+cl::opt<bool> PrintFaultMaps("print-fault-maps",
+                             cl::desc("Print the faultmap section"));
----------------
reames wrote:
> 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.
Fixed.

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1235
@@ +1234,3 @@
+static void printFaultMaps(const ObjectFile *Obj) {
+  outs() << "FaultMap table:\n";
+  Optional<object::SectionRef> FaultMapSection;
----------------
reames wrote:
> The output should probably indicate there isn't a FaultMap table if that's the case.  
Fixed.

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1236
@@ +1235,3 @@
+  outs() << "FaultMap table:\n";
+  Optional<object::SectionRef> FaultMapSection;
+  for (auto Sec : Obj->sections()) {
----------------
reames wrote:
> Is this specific to ELF?  Or should it work with any format of object file?
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).

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1249
@@ +1248,3 @@
+
+  StringRef FaultMapContents;
+  FaultMapSection.getValue().getContents(FaultMapContents);
----------------
reames wrote:
> I don't believe StringRef has any backing storage.  This looks highly suspicious to say the least.  Should this be an std::string instead?
This looks intentional -- as far as I can tell, the sections return a reference to memory they own.

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1255
@@ +1254,3 @@
+
+  FaultMapParser<support::little> FMP(Begin, End);
+
----------------
reames wrote:
> Why can't you just use begin(), end() on StringRef?
That's a good idea, using `bytes_begin()` and `bytes_end()` lets me avoid casting altogether.

http://reviews.llvm.org/D10491

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