[PATCH] D27038: [XRay][compiler-rt] XRay Flight Data Recorder Mode

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

PTAL @pelikan and/or @rSerge



================
Comment at: lib/xray/xray_fdr_logging.cc:238
+  // The data for the New CPU will contain the following bytes:
+  //   - CPU ID (uint16_t, 4 bytes)
+  //   - Full TSC (uint64_t, 8 bytes)
----------------
pelikan wrote:
> uint16_t is not 4 bytes.  Maybe it is allowed by the spec (they are inequalities, I don't remember which way) but I'd punish every real-world implementation that made u16 out of 4 8-bit bytes just out of spite ;-)  If there's struct padding involved, call it u32.  If not, call it 2 bytes.
Whoops -- good catch!

No, I really meant to use uint16_t which gives us 2 bytes worth of CPU id's -- so in essence that's 2^16 potential CPU ids.


================
Comment at: lib/xray/xray_fdr_logging.cc:241-242
+  // Total = 12 bytes.
+  std::memcpy(&NewCPUId.Data, &CPU, sizeof(CPU));
+  std::memcpy(&NewCPUId.Data[sizeof(uint16_t)], &TSC, sizeof(TSC));
+  std::memcpy(RecordPtr, &NewCPUId, sizeof(MetadataRecord));
----------------
pelikan wrote:
> Big endian won't like writing over those two bytes very much.  Is this code amd64 only or for every platform?
Yep, fixed to use sizeof(CPU) instead.


https://reviews.llvm.org/D27038