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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

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



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


================
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;
----------------
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?


================
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);
----------------
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.


================
Comment at: lib/xray/xray_x86_64.cc:284-292
+  // In Version 0:
+  //
+  // Feature did not exist.
+  //
+  // In Version 1:
+  //
+  // xray_sled_n:
----------------
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 `0`s -- in the custom event sleds, we've had to change the version but still be backwards compatible (hence the comment from which this came).


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D43668