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

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

pelikan added a comment.

Apologies for yet another round trip of bad news :-(



================
Comment at: lib/xray/xray_fdr_logging.cc:203
+    NewBuffer.RecordKind = MetadataRecord::RecordKinds::NewBuffer;
+    *reinterpret_cast<pid_t *>(&NewBuffer.Data) = getpid();
+  }
----------------
warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]

I don't usually care about strict aliasing but I hear that real compiler people do ;-)  I think what this wants you to do is a temporary + a memcpy(x, y, 4);.


================
Comment at: lib/xray/xray_fdr_logging.cc:208
+  {
+    static_assert(sizeof(time_t) == 8, "time_t needs to be 8 bytes");
+    auto &WalltimeMarker = *reinterpret_cast<MetadataRecord *>(&Records[1]);
----------------
Due to the unique way Linux handles its ABI compatibility, this won't work on older 32-bit architectures (and yes, they will all break in 2038).  Currently it does error the ARM cross-build.  (OpenBSD fixed this a couple of years ago but XRay isn't supported there yet due to W^X .text enforcement.)


================
Comment at: lib/xray/xray_fdr_logging.cc:214
+    clock_gettime(CLOCK_MONOTONIC, &TS);
+    std::memcpy(WalltimeMarker.Data, &TS, sizeof(TS));
+  }
----------------
GCC says this will always overflow.  Can you please add a static_assert() that sizeof TS <= sizeof MetadataRecord::Data?  Although, that alone won't exactly help AArch64 to build...


================
Comment at: lib/xray/xray_fdr_logging.cc:229-230
+  // Total = 12 bytes.
+  *reinterpret_cast<uint16_t *>(&NewCPUId.Data) = CPU;
+  *reinterpret_cast<uint64_t *>(&NewCPUId.Data[sizeof(uint16_t)]) = TSC;
+  std::memcpy(RecordPtr, &NewCPUId, sizeof(MetadataRecord));
----------------
warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]

On both of these.  Here the memcpy is obvious because there's a chance it'll get replaced with a builtin and optimised correctly.


================
Comment at: lib/xray/xray_fdr_logging.cc:252
+  // Total = 8 bytes.
+  *reinterpret_cast<uint64_t *>(&TSCWrap.Data) = TSC;
+  std::memcpy(RecordPtr, &TSCWrap, sizeof(MetadataRecord));
----------------
warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]


================
Comment at: lib/xray/xray_fdr_logging.cc:483
+    auto Delta = LastTSC - TSC;
+    if (Delta > (1L << 32) - 1)
+      writeTSCWrapMetadata(TSC);
----------------
1L<<32 does upset the 32-bit build.  Also, because LastTSC is unsigned, the auto is unsigned and this wants to be ULL.


https://reviews.llvm.org/D27038