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

Dean Michael Berris via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 26 15:36:18 PST 2018

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

  rCRT Compiler Runtime


More information about the llvm-commits mailing list