[PATCH] D24376: [XRay] Implement `llvm-xray convert` -- trace file conversion

[David Blaikie via llvm-commits]

dblaikie added inline comments.


================
Comment at: tools/llvm-xray/func-id-helper.cc:28-29
+
+  auto ResOrErr = Symbolizer.symbolizeCode(BinaryInstrMap, It->second);
+  if (ResOrErr) {
+    auto &DI = *ResOrErr;
----------------
Fold ResOrErr into the if condition to reduce/match its scope to its use?


================
Comment at: tools/llvm-xray/func-id-helper.cc:49-50
+  std::ostringstream F;
+  auto ResOrErr = Symbolizer.symbolizeCode(BinaryInstrMap, It->second);
+  if (ResOrErr) {
+    auto &DI = *ResOrErr;
----------------
Fold variable into if condition.

(alternatively - and this applies to the other case of the same - consider a short-exit:

  if (!ResOrErr) {
    handleAllErrors(...)
    return F.str();
  }

I know this means duplicating the "Return F.str()" code - but allows the main non-error code to be unindented, can make it easier to follow (by keeping the error handling close to the error, too).)




================
Comment at: tools/llvm-xray/func-id-helper.cc:53-57
+    if (LastSlash != std::string::npos) {
+      F << DI.FileName.substr(LastSlash + 1);
+    } else {
+      F << DI.FileName;
+    }
----------------
Drop {} on single line blocks, probably


================
Comment at: tools/llvm-xray/func-id-helper.h:39
+
+  FuncIdConversionHelper(const FuncIdConversionHelper &) = default;
+
----------------
This should be implicit? Or did you want it to disable the other special members? Any particular reason? (might be worth a comment if that's the case)


================
Comment at: tools/llvm-xray/xray-converter.cc:121-124
+  if (ConvertInputFormat == ConvertOutputFormat)
+    return make_error<StringError>(
+        "-input-format and -output-format are the same, bailing out.",
+        std::make_error_code(std::errc::invalid_argument));
----------------
If we need to support these formats for both input and output - I don't so much mind whether we support the null conversion/roundtrip. The point in the previous review was that we didn't need to handle binary output nor yaml input - but it sounds like you need all the different input and output modes here? (at the very least the native format will be a valid input (for conversion to non-native formats) and output (for upgrading) format, for example.

So, up to you - if supporting roundtripping is convenient/nice generalization, that's fine


================
Comment at: tools/llvm-xray/xray-converter.cc:126-129
+  if (ConvertOutputFormat == ConvertFormats::BINARY)
+    return make_error<StringError>(
+        "Convertion to binary unsupported.",
+        std::make_error_code(std::errc::function_not_supported));
----------------
Hmm - now I'm a bit confused given after my last comment.

If we don't support roundtripping, and we don't support binary output - then the only thing we support is binary input and YAML output, so why the switch a few lines below that chooses the input format between binary and yaml? Isn't the yaml reading case in that switch unreachable and thus the YAMLogLoader unused/untested?

Indeed, no test seems to exercise -i/-input-format?


================
Comment at: tools/llvm-xray/xray-extract.cc:193-194
+  int Fd;
+  auto EC = sys::fs::openFileForRead(Filename, Fd);
+  if (EC)
+    return make_error<StringError>(
----------------
Roll variable into if condition


================
Comment at: tools/llvm-xray/xray-extract.cc:203-204
+
+  sys::fs::mapped_file_region MappedFile(
+      Fd, sys::fs::mapped_file_region::mapmode::readonly, FileSize, 0, EC);
+  if (EC)
----------------
Guessing it's mapped rather than streamed because that's the API for DataExtractor (dealing with a buffer)? Or does it have other needs to be mapped?

(might be nice if it didn't have to be - but I suppose there's no strong reason/benefit to it... )


================
Comment at: tools/llvm-xray/xray-extract.cc:210
+  std::vector<YAMLXRaySledEntry> YAMLSleds;
+  Input In(StringRef(MappedFile.data(), FileSize));
+  In >> YAMLSleds;
----------------
Might make sense to pass MappedFile.size() rather than fileSize, even though they're the same (& even maybe just having MappedFile have a way to access a StringRef of the range)


================
Comment at: tools/llvm-xray/xray-log-reader.cc:87
+
+  {
+    DataExtractor HeaderExtractor(Data, true, 8);
----------------
I probably wouldn't bother with explicit scope here (I usually only use them if I need the dtor behavior) - but I appreciate the benefit to scoping the variables and don't mind it if you prefer it this way.


================
Comment at: tools/llvm-xray/xray-log-reader.cc:112
+  //   (4)   -      : padding
+  {
+    for (auto S = Data.drop_front(32); !S.empty(); S = S.drop_front(32)) {
----------------
unneeded extra scope here?


================
Comment at: tools/llvm-xray/xray-record.h:23
+
+struct alignas(32) XRayFileHeader {
+  uint16_t Version = 0;
----------------
Guessing this doesn't need an alignment attribute anymore?


================
Comment at: tools/llvm-xray/xray-record.h:38
+  // The type of the event. Usually either ENTER = 0 or EXIT = 1.
+  uint8_t Type;
+
----------------
Is there an enum that should be used for this member?


https://reviews.llvm.org/D24376