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

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

pelikan added inline comments.


================
Comment at: include/llvm/XRay/XRayRecord.h:77-78
+
+  /// The function call argument.
+  uint64_t CallArg;
 };
----------------
dberris wrote:
> 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.
Makes sense.  Last discussion did clear up the intents of the API a lot.


================
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));
----------------
dberris wrote:
> If you supported multiple arguments today, you can make this error go away.
Changed everything as discussed.


================
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)
----------------
dberris wrote:
> Have you considered parsing the following metadata record greedily here, instead of trying to refer to the last record in the vector?
The rest of the code does 1 record at a time, it didn't look appealing to me to deviate.  It also isn't required for performance, and it's a very rare case.


================
Comment at: lib/XRay/Trace.cpp:274-279
+  case 5: // CallArgument
+    if (auto E =
+            processFDRCallArgumentRecord(State, RecordFirstByte,
+                                         RecordExtractor, Records))
+      return E;
+    break;
----------------
dberris wrote:
> 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.
Again, the rest of the code processes each record separately.  It would look uglier IMO if only one case differed.


================
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
----------------
dberris wrote:
> Did clang-format do this?
No, I had a previous version of the code there and later erased it.  clang-format wants it to be the old way :-(


https://reviews.llvm.org/D32840