This is an archive of the discontinued LLVM Phabricator instance.

Differential D55689

[llvm-xray] Store offset pointers in temporaries
ClosedPublic

Authored by phosek on Dec 13 2018, 8:11 PM.

Details

Reviewers
dberris
MaskRay
Commits
rG27e2f2014aa6: [llvm-xray] Store offset pointers in temporaries
rL349129: [llvm-xray] Store offset pointers in temporaries
Summary

DataExtractor::getU64 modifies the OffsetPtr which also pass to
RelocateOrElse which breaks on Windows. This addresses the issue
introduced in r349120.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Dec 13 2018, 8:11 PM
Herald added subscribers: llvm-commits, hiraditya. · View Herald TranscriptDec 13 2018, 8:11 PM
dberris accepted this revision.Dec 13 2018, 9:34 PM

LGTM

This revision is now accepted and ready to land.Dec 13 2018, 9:34 PM
MaskRay accepted this revision.Dec 13 2018, 9:57 PM

This was undefined behavior before C++17 and indeterminate since C++17...

Closed by commit rL349129: [llvm-xray] Store offset pointers in temporaries (authored by phosek). · Explain WhyDec 13 2018, 9:59 PM
This revision was automatically updated to reflect the committed changes.
stephanemoore added a subscriber: stephanemoore.Dec 14 2018, 2:58 PM
stephanemoore added inline comments.
llvm/trunk/lib/XRay/InstrumentationMap.cpp
141

Did you mean AddrPtr rather than AddrOff?

143

Did you mean FuncPtr rather than FuncOff?

Revision Contents

PathSize
llvm/
trunk/
lib/
XRay/
6 lines

Diff 178176

llvm/trunk/lib/XRay/InstrumentationMap.cpp

Show First 20 Lines • Show All 131 Lines • ▼ Show 20 LinesloadObj(StringRef Filename, object::OwningBinary<object::ObjectFile> &ObjFile,
uint64_t CurFn = 0; uint64_t CurFn = 0;
for (; C != Contents.bytes_end(); C += ELF64SledEntrySize) { for (; C != Contents.bytes_end(); C += ELF64SledEntrySize) {
DataExtractor Extractor( DataExtractor Extractor(
StringRef(reinterpret_cast<const char *>(C), ELF64SledEntrySize), true, StringRef(reinterpret_cast<const char *>(C), ELF64SledEntrySize), true,
8); 8);
Sleds.push_back({}); Sleds.push_back({});
auto &Entry = Sleds.back(); auto &Entry = Sleds.back();
uint32_t OffsetPtr = 0; uint32_t OffsetPtr = 0;
Entry.Address = RelocateOrElse(OffsetPtr, Extractor.getU64(&OffsetPtr)); uint32_t AddrPtr = OffsetPtr;
Entry.Function = RelocateOrElse(OffsetPtr, Extractor.getU64(&OffsetPtr)); Entry.Address = RelocateOrElse(AddrOff, Extractor.getU64(&OffsetPtr));
stephanemooreUnsubmitted
Not Done
ReplyInline Actions

Did you mean AddrPtr rather than AddrOff?

stephanemoore: Did you mean `AddrPtr` rather than `AddrOff`?
uint32_t FuncPtr = OffsetPtr;
Entry.Function = RelocateOrElse(FuncOff, Extractor.getU64(&OffsetPtr));
stephanemooreUnsubmitted
Not Done
ReplyInline Actions

Did you mean FuncPtr rather than FuncOff?

stephanemoore: Did you mean `FuncPtr` rather than `FuncOff`?
auto Kind = Extractor.getU8(&OffsetPtr); auto Kind = Extractor.getU8(&OffsetPtr);
static constexpr SledEntry::FunctionKinds Kinds[] = { static constexpr SledEntry::FunctionKinds Kinds[] = {
SledEntry::FunctionKinds::ENTRY, SledEntry::FunctionKinds::EXIT, SledEntry::FunctionKinds::ENTRY, SledEntry::FunctionKinds::EXIT,
SledEntry::FunctionKinds::TAIL, SledEntry::FunctionKinds::TAIL,
SledEntry::FunctionKinds::LOG_ARGS_ENTER, SledEntry::FunctionKinds::LOG_ARGS_ENTER,
SledEntry::FunctionKinds::CUSTOM_EVENT}; SledEntry::FunctionKinds::CUSTOM_EVENT};
if (Kind >= sizeof(Kinds)) if (Kind >= sizeof(Kinds))
return errorCodeToError( return errorCodeToError(
▲ Show 20 Lines • Show All 89 Lines • Show Last 20 Lines