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

[Keith via Phabricator via llvm-commits]

kpw added a comment.

Thanks for the feedback Dean.



================
Comment at: include/xray/xray_interface.h:88
+extern uint32_t
+__xray_register_event_type(const char *const event_type) noexcept;
+
----------------
dberris wrote:
> 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.
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


================
Comment at: lib/xray/xray_fdr_logging.cc:328
+  //   - 4 bytes (32-bits) for the length of the data.
+  //   - 3 bytes (24-bits) for the event type.
+  MetadataRecord TypedEvent;
----------------
dberris wrote:
> 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?
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.


================
Comment at: lib/xray/xray_interface.cc:389
+uint32_t __xray_register_event_type(
+    const char *const event_type) noexcept XRAY_NEVER_INSTRUMENT {
+  TypeDescriptorMapType::Handle h(&TypeDescriptorAddressMap, (uptr)event_type);
----------------
dberris wrote:
> 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.
Ack. Doing it here seems reasonable.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D43668