This is an archive of the discontinued LLVM Phabricator instance.

[XRay] [compiler-rt] Implement trampoline and handler for typed xray event tracing.
ClosedPublic

Authored by kpw on Feb 22 2018, 7:09 PM.

Details

Summary

Compiler-rt support first before defining the __xray_typedevent() lowering in
llvm. I'm looking for some early feedback before I touch much more code.

Event Timeline

kpw created this revision.Feb 22 2018, 7:09 PM
Herald added subscribers: Restricted Project, delcypher. · View Herald TranscriptFeb 22 2018, 7:09 PM
kpw updated this revision to Diff 135735.Feb 23 2018, 4:46 PM
  • Jump 20

Great start, @kpw! Thanks. Please see comments below.

include/xray/xray_interface.h
88

I don't think you need the additional const for the declaration.

Also, I suspect 'noexcept' in the extern "C" { ... } section wouldn't make any sense, and would be superfluous.

lib/xray/xray_fdr_logging.cc
328

Add a comment why we need to have this at 3 bytes?

Or, if we encounter that the EventType is > unsigned 24 bits, then we should say something.

Alternatively, we can limit the size of the payload to 16 bits (64Kb) instead of going the full 32 bit length. WDYT?

lib/xray/xray_interface.cc
389

You might want to make sure that the descriptor count doesn't exceed the 24 bits maximum, and provide for returning an invalid number. We can make either 0 or numeric_limits<uint32_t>::max() represent an error condition when that happens. Either we do it here, or do it in the FDR implementation somehow.

lib/xray/xray_x86_64.cc
293–301

Sled versioning is per-sled, so we can use Version 0 to be this implementation. Note that the XRaySledEntry type has a version field that's associated with each sled, so when lowering we fill it out by default with 0s -- in the custom event sleds, we've had to change the version but still be backwards compatible (hence the comment from which this came).

kpw added a comment.Feb 26 2018, 5:47 PM

Thanks for the feedback Dean.

include/xray/xray_interface.h
88

Fair enough about the const.

I wasn't sure if extern "C" only specified linkage. Digging was inconclusive.

https://stackoverflow.com/questions/24362616/does-the-c-standard-mandate-that-c-linkage-functions-are-noexcept

lib/xray/xray_fdr_logging.cc
328

32 bit length payloads seem excessive for our use cases, as do 32 bits worth of differing event types. Constraining the event types seemed more natural because we control the id registration.

There's probably better error handling to do here.

lib/xray/xray_interface.cc
389

Ack. Doing it here seems reasonable.

dberris added inline comments.Feb 26 2018, 6:39 PM
include/xray/xray_interface.h
88

The noexcept doesn't really add much here, and could be distracting, and is not something we can guarantee either. Is there something in particular you're guarding for with putting the noexcept specifier(s) on these functions?

Adding just one comment, after thinking about it a little more.

lib/xray/xray_fdr_logging.cc
328

I agree. Can we live with unsigned 16 bits worth of types and payloads? That would make a lot of this stuff more conservative.

kpw updated this revision to Diff 142297.Apr 12 2018, 4:26 PM
kpw marked 7 inline comments as done.

[XRay] [compiler-rt] Implement trampoline and handler for typed xray event tracing.

Compiler-rt support first before defining the __xray_typedevent() lowering in
llvm.

kpw added a comment.Apr 12 2018, 4:29 PM

These comments were dangling for a while, so I'm posting an update. Currently compiling llvm with a new intrinsic to match. Fingers crossed!

include/xray/xray_interface.h
88

I've removed them. I'm not sure the stack unwinder even works for functions that use the C calling convention. Since I'm not sure we can support exceptions properly, my thinking was that it might be better to put noexcept so that this behavior just becomes std::terminate if anything is thrown. I don't know what you mean that we can't guarantee no exceptions? We've written the implementations without throwing. These are just handler registration, not handler execution.

Sure there could be a std::bad_alloc, but that's common in noexcept functions and noexcept just means exceptions will be transformed into std::terminate.

I don't want to make the file inconsistent and confusing, by adding the specifier selectively, but it may be more clear to add to all the registration functions since we haven't written implementations that can throw.

I don't think compiler optimizations are very relevant here. They mostly come down to

  1. std::move_if_no_except - Not relevant here.
  2. Should the compiler generate code for stack unwinding? - Registering is rare compared to invocation anyway.
kpw added inline comments.Apr 12 2018, 4:41 PM
include/xray/xray_interface.h
87

I should change the return type here as well. Whoops.

dberris accepted this revision.Apr 12 2018, 5:24 PM

LGTM

include/xray/xray_interface.h
88

Thanks -- I think you raise interesting points here that are worth chasing down.

In particular, we should definitely think more about the exception safety of the XRay implementation in general. Especially in the parts where we're calling the event handlers, which internally could be throwing exceptions -- the assembler parts ought to either handle that properly by having some exception-handling built-in.

That might be too hard, or not worth it (depending on the usage scenario for XRay) but we certainly should document the requirements for event handlers (function entry/exit, typed events, custom events, etc.).

Let's keep thinking about this, and incrementally address the exception safety aspect as we go along. I definitely think that noexcept is appropriate in a lot of the XRay implementation (if not all of them) but we should do it in a disciplined, thoughtful manner. :)

This revision is now accepted and ready to land.Apr 12 2018, 5:24 PM
kpw updated this revision to Diff 142307.Apr 12 2018, 6:30 PM

[XRay] [compiler-rt] Implement trampoline and handler for typed xray event tracing.

Compiler-rt support first before defining the __xray_typedevent() lowering in llvm.

kpw added a comment.Apr 17 2018, 9:58 AM

I'm going to land this and the llvm/clang changes and in a separate CL make some improvements as discussed offline for event type registration (handling deduping event descriptors).

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.