[PATCH] D46574: [XRay][compiler-rt] Support in-memory processing of FDR mode logs

[Keith via Phabricator via llvm-commits]

kpw accepted this revision.
kpw added inline comments.
This revision is now accepted and ready to land.


================
Comment at: compiler-rt/lib/xray/xray_buffer_queue.h:75
+
+    T &operator*() const { return Buffers[Offset].Buff; }
+
----------------
I have to ask. Why do you prefer this style where reference and pointer qualifiers are detached from the type they qualify? I understand the logic of grouping them with a variable name instead of a type, but choosing to group them to a function name baffles me.


================
Comment at: compiler-rt/lib/xray/xray_buffer_queue.h:97
+    friend bool operator==(const Iterator &L, const Iterator<V> &R) {
+      DCHECK_EQ(L.Max, R.Max);
+      return L.Buffers == R.Buffers && L.Offset == R.Offset;
----------------
Why DCHECK here instead of including this in the equality condition? I guess different views into the same Buffers are prohibited for now but this seems more fragile than it has to be.


================
Comment at: compiler-rt/lib/xray/xray_fdr_logging.cc:74-76
+// This is the iterator implementation, which knows how to handle FDR-mode
+// specific buffers.
+XRayBuffer fdrIterator(const XRayBuffer B) {
----------------
Can you call this something like "nextBuffer" or "fdrNextBuffer" and write the comment so that it explains how it's supposed to be called? E.g. first call with a zero initialized XRayBuffer, then subsequent calls hand back the current XRayBuffer to advance forward.

Actually, since this maintains static iterators, you could keep static state about whether the header was returned and get rid of the parameter entirely if you choose. Although, you may need a way to reset the iterators, which is currently encoded in checks of the parameter.

Edit: It looks like this shouldn't be renamed since it's the function pointer installed by the __xray_log_set_buffer_iterator(). I still think explaining the usages and/or the assumptions about the state transitions is a good idea.


================
Comment at: compiler-rt/lib/xray/xray_fdr_logging.cc:439
     // FIXME: Remove this when we fully remove the deprecated flags.
-    if (internal_strlen(EnvOpts) != 0) {
-      flags()->xray_fdr_log_func_duration_threshold_us =
-          FDRFlags.func_duration_threshold_us;
-      flags()->xray_fdr_log_grace_period_ms = FDRFlags.grace_period_ms;
+    if (internal_strlen(EnvOpts) == 0) {
+      FDRFlags.func_duration_threshold_us =
----------------
Why did you invert this condition?


================
Comment at: compiler-rt/test/xray/TestCases/Posix/fdr-mode-inmemory.cc:1
+// RUN: %clangxx_xray -g -std=c++11 %s -o %t -fxray-modes=xray-fdr
+// RUN: rm fdr-inmemory-test-* || true
----------------
In case any DCHECK failures are encountered during test execution, that should be covered by buildbots that build LLVM in debug mode and run tests right?


================
Comment at: compiler-rt/test/xray/TestCases/Posix/fdr-mode-inmemory.cc:46
+  assert(flush_status == XRayLogFlushStatus::XRAY_LOG_FLUSHED);
+  assert(buffers == 11); // 11 because 10 buffers + 1 header buffer
+  return 0;
----------------
Clever FileCheck to just rely on asserts instead of CHECK statements.


https://reviews.llvm.org/D46574