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

[Martin Pelikán via Phabricator via llvm-commits]

pelikan added inline comments.


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


================
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));
----------------
Big endian won't like writing over those two bytes very much.  Is this code amd64 only or for every platform?


================
Comment at: lib/xray/xray_fdr_logging.h:37
+  // A MetadataRecord must always have a type of 1.
+  RecordType Type : 1;
+
----------------
rSerge wrote:
> dberris wrote:
> > pelikan wrote:
> > > In file included from /home/pelikan/src/llvm/projects/compiler-rt/lib/xray/xray_fdr_logging.cc:17:0:
> > > /home/pelikan/src/llvm/projects/compiler-rt/lib/xray/xray_fdr_logging.h:37:21: warning: ‘__xray::MetadataRecord::Type’ is too small to hold all values of ‘enum class __xray::RecordType’
> > >    RecordType Type : 1;
> > >                      ^
> > > /home/pelikan/src/llvm/projects/compiler-rt/lib/xray/xray_fdr_logging.h:48:28: warning: ‘__xray::MetadataRecord::RecordKind’ is too small to hold all values of ‘enum class __xray::MetadataRecord::RecordKinds’
> > >    RecordKinds RecordKind : 7; // Use 7 bits to identify this record type.
> > >                             ^
> > > /home/pelikan/src/llvm/projects/compiler-rt/lib/xray/xray_fdr_logging.h:56:21: warning: ‘__xray::FunctionRecord::Type’ is too small to hold all values of ‘enum class __xray::RecordType’
> > >    RecordType Type : 1;
> > >                      ^
> > > /home/pelikan/src/llvm/projects/compiler-rt/lib/xray/xray_fdr_logging.h:62:28: warning: ‘__xray::FunctionRecord::RecordKind’ is too small to hold all values of ‘enum class __xray::FunctionRecord::RecordKinds’
> > >    RecordKinds RecordKind : 3;
> > > 
> > > Is this a gcc (6.3.0 used) bug?
> > Unsure whether this is a bug, but that's a good question. RecordType is an enum class whose base is an unsigned 8-bit integral type -- and only has two values. 1 bit ought to be enough, no?
> Does the warning disappear if the integer values for enum members are specified explicitly?
It does not disappear.  It appears to be a genuine bug.  I'll try with the trunk GCC and then file a minimal case as a bug report.


https://reviews.llvm.org/D27038