[PATCH] D21982: [compiler-rt][XRay] Initial per-thread inmemory logging implementation

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: include/xray/xray_records.h:41-42
@@ +40,4 @@
+  // reading the data in the file.
+  bool ConstantTSC = 0;
+  bool NonstopTSC = 0;
+
----------------
Should these be bitfields?

================
Comment at: lib/xray/xray_inmemory_log.cc:47
@@ +46,3 @@
+
+static void RetryingWrite(int Fd, char *Begin, char *End) {
+  if (Begin == End)
----------------
Are we following LLVM convention for XRay? If so, I'd make this function start with a lowercase letter.

================
Comment at: lib/xray/xray_inmemory_log.cc:52-53
@@ +51,4 @@
+  while (auto Written = write(Fd, Begin, TotalBytes)) {
+    if (Written <= 0) {
+      if (errno == EINTR)
+        continue; // Try again.
----------------
This should be `Written < 0`.  I don't believe `errno` will be set to anything good if `write` returns 0; I think you'd observe a previous `errno` value.

================
Comment at: lib/xray/xray_inmemory_log.cc:73
@@ +72,3 @@
+
+  ~ThreadExitFlusher() noexcept {
+    std::lock_guard<std::mutex> L(LogMutex);
----------------
Why `noexcept`?

================
Comment at: lib/xray/xray_inmemory_log.cc:90
@@ +89,3 @@
+
+bool ReadValueFromFile(const char *Filename, long *Value) {
+  int Fd = open(Filename, O_RDONLY);
----------------
Ditto regarding llvm convention.

================
Comment at: lib/xray/xray_inmemory_log.cc:90
@@ +89,3 @@
+
+bool ReadValueFromFile(const char *Filename, long *Value) {
+  int Fd = open(Filename, O_RDONLY);
----------------
majnemer wrote:
> Ditto regarding llvm convention.
Should the `long` be a `long long`?

================
Comment at: lib/xray/xray_inmemory_log.cc:91
@@ +90,3 @@
+bool ReadValueFromFile(const char *Filename, long *Value) {
+  int Fd = open(Filename, O_RDONLY);
+  if (Fd == -1)
----------------
Should we pass in O_CLOEXEC?

================
Comment at: lib/xray/xray_inmemory_log.cc:95-96
@@ +94,4 @@
+  char Line[256] = {};
+  read(Fd, Line, 256);
+  close(Fd);
+  char *End = nullptr;
----------------
How do you feel about these potentially failing?

================
Comment at: lib/xray/xray_inmemory_log.cc:98
@@ +97,3 @@
+  char *End = nullptr;
+  long Tmp = internal_simple_strtoll(Line, &End, 10);
+  bool Result = false;
----------------
Shouldn't this be `s64`?

================
Comment at: lib/xray/xray_inmemory_log.cc:156
@@ +155,3 @@
+    Header.CycleFrequency =
+        CPUFrequency == -1 ? uint64_t{0} : static_cast<uint64_t>(CPUFrequency);
+
----------------
Any reason not to just use 0?

================
Comment at: lib/xray/xray_inmemory_log.cc:175
@@ +174,3 @@
+  auto &R = reinterpret_cast<__xray::XRayRecord *>(InMemoryBuffer)[Offset];
+  uint32_t CPU;
+  R.RecordType = RecordTypes::NORMAL;
----------------
I'd just use `unsigned` here to match how `__rdtscp` is defined.


https://reviews.llvm.org/D21982