[PATCH] D21987: [XRay] Implement `llvm-xray extract`, start of the llvm-xray tool

[David Blaikie via llvm-commits]

dblaikie added inline comments.


================
Comment at: tools/llvm-xray/xray-extract.cc:141-144
+    std::aligned_storage<sizeof(ELF64SledEntry)>::type AlignedSled;
+    memcpy(&AlignedSled, C, sizeof(ELF64SledEntry));
+    const auto &ELF64Entry = *reinterpret_cast<ELF64SledEntry *>(&AlignedSled);
+    auto Kind = SledEntry::FunctionKinds::ENTRY;
----------------
I'm still a bit concerned about doing this via memory mapping (what if we are on a different platform that happens to add some extra padding between fields, etc?)

So I'd suggest using DataExtractor or similar (then you don't have to worry about alignment either) techniques/devices/tools, probably?

Maybe it just seems like this function is harder to read than it is in the code review, but might consider breaking it up - perhaps taking all the error handling at the beginning and turning it into one utility function, so this function (LoadBinaryInstrELF) can focus on the parsing, etc (or break out this parsing into another function, etc - or even both).


================
Comment at: tools/llvm-xray/xray-extract.cc:262-266
+    return joinErrors(make_error<StringError>(
+                          Twine("Cannot extract instrumentation map from '") +
+                              ExtractInput + "'",
+                          std::make_error_code(std::errc::protocol_error)),
+                      std::move(Err));
----------------
Would it make sense to sink this code into InstrumentationMapExtractor's ctor? (it has all the information - it knows it's trying to extract an instrumentation map, and the name of the input - so it seems it should be responsible for creating this message, maybe?)


https://reviews.llvm.org/D21987