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

[Serge Rogatch via llvm-commits]

rSerge added inline comments.


================
Comment at: lib/xray/xray_fdr_logging.cc:31
+
+// #include "sanitizer_common/sanitizer_common.h"
+
----------------
Dead code?


================
Comment at: lib/xray/xray_fdr_logging.cc:57
+  BQ = std::make_shared<BufferQueue>(BufferSize, BufferMax);
+  FDROptions.reset(new FDRLoggingOptions());
+  *FDROptions = *reinterpret_cast<FDRLoggingOptions *>(Options);
----------------
Wouldn't it be better to implement both via `.reset(new ...)`, or use `std::make_unique` here?


================
Comment at: lib/xray/xray_fdr_logging.cc:172
+  *reinterpret_cast<uint16_t *>(&NewCPUId.Data) = CPU;
+  *reinterpret_cast<uint64_t *>(&NewCPUId.Data[sizeof(uint16_t)]) = TSC;
+  std::memcpy(RecordPtr, &NewCPUId, sizeof(MetadataRecord));
----------------
Isn't the following simpler?
`*reinterpret_cast<uint64_t *>(NewCPUId.Data+sizeof(uint16_t)) = TSC;`


================
Comment at: lib/xray/xray_fdr_logging.h:33
+  // A MetadataRecord must always have a type of 1.
+  int Type : 1;
+
----------------
On some platform 1-bit signed integer will have values of 0 and -1, never 1. Also, see the comment for `RecordKind`.


================
Comment at: lib/xray/xray_fdr_logging.h:44
+  };
+  int RecordKind : 7; // Use 7 bits to identify this record type.
+  char Data[15];
----------------
If `int` is used here and for `Type` member, doesn't the structure take 19 bytes at least with some compilers? (because `int` bitfields take at least 4 bytes, I suppose). Can you change the type to `uint8_t` for these 2 members?


https://reviews.llvm.org/D27038