[PATCH] [FaultMaps] Add a parser for the __llvm__faultmaps section.
Sanjoy Das
sanjoy at playingwithpointers.com
Wed Jun 17 16:00:29 PDT 2015
================
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/
More information about the llvm-commits
mailing list