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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: compiler-rt/lib/xray/xray_buffer_queue.h:75
+
+    T &operator*() const { return Buffers[Offset].Buff; }
+
----------------
kpw wrote:
> 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.
I don't -- clang-format does it for 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;
----------------
kpw wrote:
> 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.
This is a non-salient property of the type. This means, technically, the iterator which share the same base and offset compare equal to each other. Whether or not the bounds are the same don't determine whether two iterators are equal. The  DCHECK is there precisely to only catch the case where the implementation has diverged from the intent, and only matters in cases where we want to catch that bug.


================
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 =
----------------
kpw wrote:
> Why did you invert this condition?
We change which flag(s) we're looking at in the fdr_impl header, which means we're consolidating the flags into the global `fdrFlags()` structure. This means we only change the FDR-specific implementation if there was no data in `XRAY_FDR_OPTIONS`, and revert to the data in the deprecated flags in `XRAY_OPTIONS`.


================
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
----------------
kpw wrote:
> 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?
Yes.


https://reviews.llvm.org/D46574