[PATCH] D21987: [XRay] Implement `llvm-xray extract`, start of the llvm-xray tool
Dean Michael Berris via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 18 23:47:39 PDT 2016
dberris added a comment.
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).
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 + "'",
> 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. :)
More information about the llvm-commits