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

[Philip Reames]

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


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

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

================
Comment at: include/llvm/CodeGen/FaultMaps.h:136
@@ +135,3 @@
+    uint64_t getFunctionAddr() const {
+      return read<uint64_t>(P + FunctionAddrOffset, E);
+    }
----------------
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.)

================
Comment at: include/llvm/CodeGen/FaultMaps.h:150
@@ +149,3 @@
+
+    FunctionInfoAccessor getNextFunctionInfo() const {
+      size_t MySize = sizeof(uint64_t) + // FunctionAddress
----------------
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.  

================
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;
----------------
I think you can probably get away with these being inferred which would make the code cleaner.  

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

================
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"));
----------------
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.

================
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;
----------------
The output should probably indicate there isn't a FaultMap table if that's the case.  

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

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1249
@@ +1248,3 @@
+
+  StringRef FaultMapContents;
+  FaultMapSection.getValue().getContents(FaultMapContents);
----------------
I don't believe StringRef has any backing storage.  This looks highly suspicious to say the least.  Should this be an std::string instead?

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1252
@@ +1251,3 @@
+  const uint8_t *Begin =
+      reinterpret_cast<const uint8_t *>(FaultMapContents.data());
+  const uint8_t *End = Begin + FaultMapContents.size();
----------------
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

================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1255
@@ +1254,3 @@
+
+  FaultMapParser<support::little> FMP(Begin, End);
+
----------------
Why can't you just use begin(), end() on StringRef?

http://reviews.llvm.org/D10491

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