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

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

pelikan marked an inline comment as done.
pelikan added inline comments.


================
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
----------------
dberris wrote:
> Why make a copy of the shared_ptr here? I don't think we actually need a copy, do we?
Good point.  I haven't gotten used to using shared_ptr properly yet.  (C programmer learning.)


================
Comment at: lib/xray/xray_fdr_logging_impl.h:586-595
+  } else {
+    // If the delta is greater than the range for a uint32_t, then we write out
+    // the TSC wrap metadata entry with the full TSC, and the TSC for the
+    // function record be 0.
+    auto Delta = TSC - TLD.LastTSC;
+    if (Delta > std::numeric_limits<int32_t>::max())
+      writeTSCWrapMetadata(TSC);
----------------
dberris wrote:
> You can turn this into an early return:
> 
> ```
> if (CPU != TLD.CurrentCPU) {
>   ...
>   return RecordTSCDelta;
> }
> 
> // rest of code that's in else branch.
> ```
Makes sense.


================
Comment at: lib/xray/xray_fdr_logging_impl.h:721-722
   writeFunctionRecord(FuncId, RecordTSCDelta, Entry, TLD.RecordPtr);
+  if (Entry == XRayEntryType::LOG_ARGS_ENTRY)
+    writeCallArgumentMetadata(Arg1);
 
----------------
dberris wrote:
> Why isn't this just in the cases above?
Because of the ordering.  The metadata need to be after the function record as that means it belongs to the function.


================
Comment at: test/xray/TestCases/Linux/fdr-mode.cc:100-101
+// UNWRITE: records:
+// UNWRITE-NEXT: - { type: 0, func-id: [[FIDARG:[0-9]+]], function: 'fArg(int)', args: [ 1 ], cpu: {{.*}}, thread: [[THREAD2:[0-9]+]], kind: function-enter-arg, tsc: {{[0-9]+}} }
+// UNWRITE-NEXT: - { type: 0, func-id: [[FIDARG]], function: 'fArg(int)', cpu: {{.*}}, thread: [[THREAD2]], kind: function-exit, tsc: {{[0-9]+}} }
 // UNWRITE-NOT: function-enter
----------------
dberris wrote:
> Probably use UNWRITE-DAG here instead, similar to how we use TRACE-DAG above.
As discussed, we can't do that here since the point of UNWRITE is testing the other calls will *not* fire.  The previous version of the test made sure of that which means putting UNWRITE-DAG here would actually destroy the tested behaviour.


https://reviews.llvm.org/D32844