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.
Details
Diff Detail
- Repository
- rCRT Compiler Runtime
- Build Status
Buildable 17046 Build 17046: arc lint + arc unit
Event Timeline
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). |
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. | |
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. |
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. |
[XRay] [compiler-rt] Implement trampoline and handler for typed xray event tracing.
Compiler-rt support first before defining the __xray_typedevent() lowering in
llvm.
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
|
include/xray/xray_interface.h | ||
---|---|---|
87 | I should change the return type here as well. Whoops. |
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. :) |
[XRay] [compiler-rt] Implement trampoline and handler for typed xray event tracing.
Compiler-rt support first before defining the __xray_typedevent() lowering in llvm.
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).
I should change the return type here as well. Whoops.