[PATCH] D50441: [XRay] FDR Trace Dump Tool (FDR Trace Loading Refactor)

[Keith via Phabricator via llvm-commits]

kpw added a comment.

Suggestion for splitting this up.

Patch 1: FDRRecords and RecordVisitor and the Record subclasses.
Patch 2: Visitor implementations
Patch 3: fdr-dump command

I only read code from the hypothetical Patch 1 for these comments.



================
Comment at: llvm/include/llvm/XRay/BlockPrinter.h:61
+#endif // LLVM_INCLUDE_LLVM_XRAY_BLOCKPRINTER_H_
\ No newline at end of file

----------------
newline?


================
Comment at: llvm/include/llvm/XRay/BlockVerifier.h:25
+class BlockVerifier : public RecordVisitor {
+  enum class RecordKind : unsigned {
+    Unknown,
----------------
If you forward declare this and move the TransitionTable to the cc file, you could use the .inc macro trick to automatically get two string from the list.


================
Comment at: llvm/include/llvm/XRay/BlockVerifier.h:44-66
+  std::map<RecordKind, std::vector<RecordKind>> TransitionTable{
+      {RecordKind::Unknown, {RecordKind::BufferExtents, RecordKind::NewBuffer}},
+      {RecordKind::BufferExtents, {RecordKind::NewBuffer}},
+      {RecordKind::NewBuffer, {RecordKind::WallClockTime}},
+      {RecordKind::WallClockTime, {RecordKind::PIDEntry, RecordKind::NewCPUId}},
+      {RecordKind::PIDEntry, {RecordKind::NewCPUId}},
+      {RecordKind::NewCPUId,
----------------
It's hard to tell whether this is right. I didn't notice any problems, but it is dense.

Is there a better way? Especially with regards to new record types?


================
Comment at: llvm/include/llvm/XRay/FDRRecords.h:29-33
+  enum class RecordKind : unsigned {
+    UNKNOWN,
+    FUNCTION_RECORD,
+    METADATA_RECORD,
+  };
----------------
Could be confusing for name lookup that this has the same name (modulo qualifiers) as BlockVerifier::RecordKind


================
Comment at: llvm/include/llvm/XRay/FDRRecords.h:50
+  // appropriate function in the visitor to invoke, given its own type.
+  virtual Error apply(RecordVisitor &V) = 0;
+
----------------
It kind of seems like this member function should be const qualified and the visit method should take a const reference to Record.


================
Comment at: llvm/include/llvm/XRay/FDRRecords.h:228
+class FunctionRecord : public Record {
+  RecordTypes Kind;
+  int32_t FuncId;
----------------
FnType? Since there are already a few RecordKind types to possibly confuse.


================
Comment at: llvm/include/llvm/XRay/FDRRecords.h:243-245
+  RecordTypes record_type() const { return Kind; }
+
+  int32_t functionId() const { return FuncId; }
----------------
Mixed casing convention. You have like_this() and likeThat().


================
Comment at: llvm/include/llvm/XRay/FDRRecords.h:257
+  // Support all specific kinds of records:
+  virtual Error visit(BufferExtents &) = 0;
+  virtual Error visit(WallclockRecord &) = 0;
----------------
I think the Record references can and should be const.


================
Comment at: llvm/include/llvm/XRay/FDRRecords.h:283
+  template <class R, class... T> LogBuilder &add(T &&... A) {
+    Records.emplace_back(new R(std::forward<T>(A)...));
+    return *this;
----------------
push_back(&&) of wrap_unique(new R(std::forward<T>(A)...)) would be my preference.

Emplacement coupled with the unique pointer within the vector template parameter is equivalent but less obvious to me.


================
Comment at: llvm/include/llvm/XRay/FDRRecords.h:298-299
+public:
+  /// Loads FDR records raw from a @DataExtractor, starting from @OffsetPtr.
+  Error load(DataExtractor &E, uint32_t &OffsetPtr);
+
----------------
Does this replace or append to the Records? Can it even be called multiple times?


================
Comment at: llvm/include/llvm/XRay/RecordPrinter.h:45
+#endif // LLVM_INCLUDE_LLVM_XRAY_RECORDPRINTER_H
\ No newline at end of file

----------------
No newline.


================
Comment at: llvm/lib/XRay/Common.h:1
+//===- Common.h - XRay Trace Loading Common Utilities ---------------------===//
+//
----------------
This is almost never a good name and this file is doomed to become a miscellaneous catch all.


================
Comment at: llvm/lib/XRay/FDRRecords.cpp:1
+//===- FDRRecords.cpp -  XRay Flight Data Recorder Mode Records -----------===//
+//
----------------
I'm going to only skim this and ask you whether you made any changes other than just copy and paste to the logic that lived elsewhere other than the state transition map and assigning to fields instead of structs directly.


================
Comment at: llvm/lib/XRay/FDRRecords.cpp:276-277
+    if (FirstByte & 0x01) {
+      // FIXME: Use a registry, and dispatch based on record types more
+      // automatically.
+      switch (FirstByte >> 1) {
----------------
Meh. This seems harmless to me. You could declare an enum with the constants if you like as an easy fix.


https://reviews.llvm.org/D50441