[PATCH] D32844: [XRay] [compiler-rt] FDR logging arg1 handler

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: lib/xray/xray_fdr_logging.cc:293
   __xray_set_handler(fdrLoggingHandleArg0);
+  __xray_set_handler_arg1(fdrLoggingHandleArg1);
   __xray_set_customevent_handler(fdrLoggingHandleCustomEvent);
----------------
Actually, you want to install the arg1 logging handler first, so that all the functions that have arg1 logging enabled don't sometimes use the arg0 then switch to the arg1 handler.


================
Comment at: lib/xray/xray_fdr_logging_impl.h:670
     __sanitizer::atomic_sint32_t &LoggingStatus,
-    const std::shared_ptr<BufferQueue> &BQ) XRAY_NEVER_INSTRUMENT {
+    const std::shared_ptr<BufferQueue> BQ) XRAY_NEVER_INSTRUMENT {
   // Prevent signal handler recursion, so in case we're already in a log writing
----------------
Why make a copy of the shared_ptr here? I don't think we actually need a copy, do we?


================
Comment at: lib/xray/xray_fdr_logging_impl.h:747
+    const std::shared_ptr<BufferQueue> BQ) XRAY_NEVER_INSTRUMENT {
+  thread_local bool Running = false;
+  RecursionGuard Guard{Running};
----------------
You want to use the global instead of a function-local here. See what we do in the logArg0 case.


================
Comment at: lib/xray/xray_fdr_logging_impl.h:776
+  TLD.CurrentCPU = CPU;
+
+  writeFunctionRecord(FnId, TSCDelta, XRayEntryType::LOG_ARGS_ENTRY,
----------------
No validation here on the type of the entry? Nothing like what we do in the logArg0 handler to see that the EntryType isn't one of the unexpected values?


================
Comment at: test/xray/TestCases/Linux/fdr-mode.cc:92
+// TRACE-DAG: - { type: 0, func-id: [[FIDARG:[0-9]+]], function: 'fArg(int)', args: [ 1 ], cpu: {{.*}}, thread: [[THREAD2]], kind: function-enter-arg, tsc: {{[0-9]+}} }
+// TRACE-NEXT: - { type: 0, func-id: [[FIDARG]], function: 'fArg(int)', cpu: {{.*}}, thread: [[THREAD2]], kind: function-exit, tsc: {{[0-9]+}} }
+
----------------
I think you want 'TRACE-DAG' still, but also need to look for the actual arguments too. Aren't we writing the list of arguments yet?


https://reviews.llvm.org/D32844