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

[Dean Michael Berris via llvm-commits]

dberris added a comment.

PTAL @dblaikie



================
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;
----------------
dblaikie wrote:
> 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).
Ah, I get it now. I'm using llvm::DataExtractor now.

Refactoring this further seems pre-mature, given that this is already an implementation detail. When supporting other formats, it would be something to think about, I agree.


================
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));
----------------
dblaikie wrote:
> 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?)
Actually, nope -- because users of this class could choose to ignore the errors (i.e. treat it as if there was no available instrumentation map). It's just that for this sub-command, it wouldn't work if extraction actually failed. :)


https://reviews.llvm.org/D21987