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

[Dean Michael Berris via llvm-commits]

dberris added a comment.

Thanks @dblaikie, PTAL.



================
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;
----------------
dblaikie wrote:
> 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).)
> 
> 
I like just limiting the scope of the variable, so I stuck with that. Also, now using sys::path::filename instead of manually futzing with the string.


================
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));
----------------
dblaikie wrote:
> 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?
Right, I think the better message here is "not yet supported". When we need the functionality then it seems better to change this then.


================
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)
----------------
dblaikie wrote:
> 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... )
Yeah, the API for the DataExtractor deals with a buffer. We could be reading things in chunks at a time explicitly, but that seems unnecessary (unless mmapping is undesirable for other reasons).


================
Comment at: tools/llvm-xray/xray-log-reader.cc:87
+
+  {
+    DataExtractor HeaderExtractor(Data, true, 8);
----------------
dblaikie wrote:
> 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.
No strong preference, this was a remnant of attempting to use the same variable name for the extractor -- but there's really no good reason for the explicit scope. :)


https://reviews.llvm.org/D24376