[PATCH] D54201: [XRay] Improve FDR trace handling and error messaging

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: llvm/include/llvm/XRay/FDRRecords.h:129
+
+  static bool classof(const Record *R) {
+    return R->getRecordType() == RecordKind::RK_Metadata_BufferExtents;
----------------
mboerger wrote:
> This is a weird interface. You basically want to associate a type enum with a class type. But here you do it in a kind of inverse way. Since you don't really use it, I don't know what better to do though.
We use it in LLVM's own RTTI implementation (see https://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html).


================
Comment at: llvm/lib/XRay/FDRRecordProducer.cpp:84
+  while (!R) {
+    auto PreReadOffset = OffsetPtr;
+    uint8_t FirstByte = E.getU8(&OffsetPtr);
----------------
mboerger wrote:
> one would expect the external 'OffsetPtr' to stay unaffected and instead of 'PreReadOffset' having a 'CurrentOffset' or so variable. Please clarify or change.
The API for the DataExtractor documents the semantics of these get*(...) functions, which says that the offset pointer is updated only if there were no errors encountered while reading the data. This is why we need to check against whether the offset we got before reading the data is the same, and if so deal with the error condition.


================
Comment at: llvm/lib/XRay/FDRRecords.cpp:34
+  switch (K) {
+  case RecordKind::RK_Metadata:
+    return "Metadata";
----------------
mboerger wrote:
> this screams for using a macro :-)
Haha -- or a language feature. :(


================
Comment at: llvm/lib/XRay/FDRTraceWriter.cpp:46
 Error writeMetadata(support::endian::Writer &OS, Values &&... Ds) {
-  uint8_t FirstByte = (Kind << 1) | uint8_t{0x01};
+  uint8_t FirstByte = (Kind << 1) | uint8_t{0x01u};
   auto T = std::make_tuple(std::forward<Values>(std::move(Ds))...);
----------------
mboerger wrote:
> so this should probably also get a helper function
This is only done here, where we're synthesising the record being serialised. There's no other place AFAICT where we're doing this particular operation in the LLVM side of the project.

In compiler-rt, we do this serialization but using bitfields.


https://reviews.llvm.org/D54201