[PATCH] D29697: [XRAY] [x86_64] Adding a Flight Data filetype reader to the llvm-xray Trace implementation.

[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: lib/XRay/Trace.cpp:57
+
+Error NaiveLogLoader(StringRef Data, XRayFileHeader &FileHeader,
+                     std::vector<XRayRecord> &Records) {
----------------
s/Naive/naive/


================
Comment at: lib/XRay/Trace.cpp:70-72
+  auto HeaderReadError = readBinaryFormatHeader(Data, FileHeader);
+  if (HeaderReadError)
+    return HeaderReadError;
----------------
Consider:

```
if (auto E = readBinaryFormatHeader(Data, FileHeader))
  return E;
```


================
Comment at: lib/XRay/Trace.cpp:168-170
+  auto HeaderReadError = readBinaryFormatHeader(Data, FileHeader);
+  if (HeaderReadError)
+    return HeaderReadError;
----------------
Same simplification mentioned elsewhere applies here.


================
Comment at: lib/XRay/Trace.cpp:171
+    return HeaderReadError;
+  FDRState State = FDRState{0, 0, 0, FDRState::Token::NEW_BUFFER_RECORD_OR_EOF};
+  // RecordSize will tell the loop how far to seek ahead based on the record
----------------
You can reduce the repetition here by doing one of the following:

- Use `auto` to not repeat yourself:

```
auto State = FDRState{...};
```

- Use aggregate init on State directly:

```
FDRState State{...};
```


================
Comment at: lib/XRay/Trace.cpp:180-231
+    if (isMetadataRecord) {
+      RecordSize = 16;
+      // The remaining 7 bits are the RecordKind enum.
+      uint8_t RecordKind = BitField >> 1;
+      switch (RecordKind) {
+      case 0: // NewBuffer
+        if (State.Expects != FDRState::Token::NEW_BUFFER_RECORD_OR_EOF) {
----------------
Consider breaking out individual record type handling into smaller functions, and/or encapsulating this metadata-record specific logic into its own function.


================
Comment at: lib/XRay/Trace.cpp:257-258
+          break;
+        case 1: // EXIT
+        case 2: // TAIL_EXIT
+          Record.Type = RecordTypes::EXIT;
----------------
I suspect you could be able to use the enum values as case labels here for the record types (using the conversion to int support for enum classes). It would be great if that works here. :)


================
Comment at: lib/XRay/Trace.cpp:263
+          return make_error<StringError>(
+              "Illegal function record type.",
+              std::make_error_code(std::errc::executable_format_error));
----------------
Consider including the record type in the error message. Using a `Twine` to build up the error message would be good to do here.


================
Comment at: lib/XRay/Trace.cpp:287-291
+  if (State.Expects != FDRState::Token::NEW_BUFFER_RECORD_OR_EOF) {
+    return make_error<StringError>(
+        "Encountered EOF without preceding End of Buffer record.",
+        std::make_error_code(std::errc::executable_format_error));
+  }
----------------
In the LLVM Style, it's acceptable (and preferred sometimes) to lose the `{}`'s for single-statement if bodies.


================
Comment at: lib/XRay/Trace.cpp:376
       return std::move(E);
+  } else if (Version == 1 && Type == 1) {
+    if (auto E = loadFDRLog(StringRef(MappedFile.data(), MappedFile.size()),
----------------
We probably want to document somewhere (or name) that `Type == 0` means the naive (basic mode?) format, while `Type == 1` means the FDR mode format.


https://reviews.llvm.org/D29697