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

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



================
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;
----------------
kpw wrote:
> 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.
I agree. Can we live with unsigned 16 bits worth of types and payloads? That would make a lot of this stuff more conservative.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D43668