[PATCH] D32840: [XRay] convert FDR arg1 log entries
Dean Michael Berris via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 3 17:49:54 PDT 2017
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
More information about the llvm-commits
mailing list