This is an archive of the discontinued LLVM Phabricator instance.

[XRay][compiler-rt] Runtime changes to support custom event logging
ClosedPublic

Authored by dberris on Mar 5 2017, 8:07 PM.

Details

Summary

This change implements support for the custom event logging sleds and
intrinsics at runtime. For now it only supports handling the sleds in
x86_64, with the implementations for other architectures stubbed out to
do nothing.

Depends on D27503, D30018, and D33032.

Diff Detail

Repository
rL LLVM

Event Timeline

dberris created this revision.Mar 5 2017, 8:07 PM
dberris updated this revision to Diff 91275.Mar 10 2017, 12:24 AM
dberris edited the summary of this revision. (Show Details)
dberris added a reviewer: pelikan.

Updated to work with most recent changes in D27503. This is now ready for a look.

timshen added inline comments.Mar 14 2017, 11:24 AM
lib/xray/xray_fdr_logging.cc
508 ↗(On Diff #91275)

Do you want to assert here?

lib/xray/xray_trampoline_x86_64.S
205 ↗(On Diff #91275)

Why stash rbp, if we don't clobber it?

dberris added inline comments.Mar 14 2017, 3:48 PM
lib/xray/xray_fdr_logging.cc
508 ↗(On Diff #91275)

I thought about it, but then figured it would be bad form to be crashing the process if there was a runtime implementation bug. Is it OK to have assertions in runtime implementations?

I thought about using llvm_unreachable(...) but wasn't sure that was available in compiler-rt.

lib/xray/xray_trampoline_x86_64.S
205 ↗(On Diff #91275)

I can't fully explain it, but if I don't stash this (seems to be the frame pointer) then I've run into issues in the past. I can try removing it, and see how that goes.

dberris updated this revision to Diff 98254.May 8 2017, 11:59 PM

Rebase and add a bit more refactoring to share some core parts of the logic between custom event handling and function entry/exit handling in FDR mode.

dberris planned changes to this revision.May 9 2017, 12:02 AM

Still a work in progress, need to add a bit more testing and ensure that we're able to handle the records in the log.

dberris updated this revision to Diff 98411.May 10 2017, 12:42 AM
dberris edited the summary of this revision. (Show Details)

Ready for a look now.

timshen added inline comments.May 10 2017, 4:52 PM
lib/xray/xray_fdr_logging.cc
202 ↗(On Diff #98411)

Running &&

206 ↗(On Diff #98411)

static.

Also, it doesn't have to be a bool. An empty struct or tuple<> will do.

Please also check other places.

235 ↗(On Diff #98411)

s/sizeof(uint64_t)/64/? By definition. :)

257 ↗(On Diff #98411)

Does FDROptions really need to be a unique_ptr? How about just using the value type?

lib/xray/xray_fdr_logging_impl.h
464 ↗(On Diff #98411)

I don't usually see "static inline" around. Usually static defeats the purpose of inline. I suggest to remove static.

lib/xray/xray_trampoline_x86_64.S
205 ↗(On Diff #91275)

Do you have any clue about the rbp? I believe that rbp is caller-saved, therefore if __xray_CustomEvent doesn't use it, it doesn't have to save it.

dberris updated this revision to Diff 98579.May 10 2017, 9:04 PM
dberris marked 5 inline comments as done.

Address comments, some cleanup.

lib/xray/xray_fdr_logging.cc
235 ↗(On Diff #98411)

Actually, sizeof(uint64_t) is 8. However, we're doing this to try and match what the type of the element is. I've made it a local constant instead, to give it a name.

257 ↗(On Diff #98411)

Good point. Doesn't need to be a unique_ptr. Turned it into a global instead, protected with a spinlock.

lib/xray/xray_trampoline_x86_64.S
205 ↗(On Diff #91275)

So I looked this up, and it seems that the frame pointer in x86_64 is usually stored in %rbp -- and if I don't put this on the stack, we run into issues locating the frame pointer when unwinding and debugging. If the frame pointer is potentially omitted, we can't know so we're being safe anyway in this case by stashing it regardless.

Does this make sense?

timshen accepted this revision.May 11 2017, 4:25 PM

Other than a typo, LGTM. I believe that the atomics are correct as well, especially in the assembly - x86 load-acquire is indeed a no-op.

lib/xray/xray_fdr_logging_impl.h
483 ↗(On Diff #98579)

s/Redy/Ready/ :)

lib/xray/xray_trampoline_x86_64.S
205 ↗(On Diff #91275)

Yes! This reminds me of -fomit-frame-pointer.

I think it's good save rbp.

This revision is now accepted and ready to land.May 11 2017, 4:25 PM
dberris updated this revision to Diff 98706.May 11 2017, 6:04 PM
dberris marked an inline comment as done.
  • fixup: typo