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

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Tue Oct 18 10:14:13 PDT 2016

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?)


More information about the llvm-commits mailing list