[PATCH] D32840: [XRay] convert FDR arg1 log entries

[Dean Michael Berris via Phabricator via llvm-commits]

dberris requested changes to this revision.
dberris added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/XRay/XRayRecord.h:77-78
+
+  /// The function call argument.
+  uint64_t CallArg;
 };
----------------
For this API, I think it would make sense for us to use a `std::vector<...>` or even an `llvm::SmallVector<uint64_t, 1>` if we ever support more argument logging.


================
Comment at: lib/XRay/Trace.cpp:224-227
+  if (ArgNum != 1)
+    return make_error<StringError>(
+        "CallArgument only supports the first argument.",
+        std::make_error_code(std::errc::executable_format_error));
----------------
If you supported multiple arguments today, you can make this error go away.


================
Comment at: lib/XRay/Trace.cpp:228
+        std::make_error_code(std::errc::executable_format_error));
+  auto &Enter = Records.back();
+  if (Enter.Type != RecordTypes::ENTER)
----------------
Have you considered parsing the following metadata record greedily here, instead of trying to refer to the last record in the vector?


================
Comment at: lib/XRay/Trace.cpp:274-279
+  case 5: // CallArgument
+    if (auto E =
+            processFDRCallArgumentRecord(State, RecordFirstByte,
+                                         RecordExtractor, Records))
+      return E;
+    break;
----------------
Have you considered special handling for the "function with call arguments" instead of trying to handle the metadata records independently? Since this is already a greedy parser, it seems much simpler (and self-contained) to add another state for "we are parsing arguments". This way we can enforce that if we encounter records that indicate we are entering a function with arguments, that we should only look for metadata records following it that contain the arguments.


================
Comment at: lib/XRay/Trace.cpp:408-409
   }
-  FDRState State{0,          0, 0, FDRState::Token::NEW_BUFFER_RECORD_OR_EOF,
+  FDRState State{0, 0, 0, FDRState::Token::NEW_BUFFER_RECORD_OR_EOF,
                  BufferSize, 0};
   // RecordSize will tell the loop how far to seek ahead based on the record
----------------
Did clang-format do this?


https://reviews.llvm.org/D32840