[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:49-50
+  std::ostringstream F;
+  auto ResOrErr = Symbolizer.symbolizeCode(BinaryInstrMap, It->second);
+  if (ResOrErr) {
+    auto &DI = *ResOrErr;
----------------
dberris wrote:
> 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.
I'll say, on reflection, that I kind of prefer keeping the diagnostic handling closer - rather than "if (success) { next bit of code } else { error handling for the if condition }"

but I'll leave that up to you - can get a feel for it and always change it later if it starts to get unwieldy 


================
Comment at: tools/llvm-xray/xray-converter.cc:152
+    break;
+  };
+
----------------
Spurious semicolon?


================
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));
----------------
dberris wrote:
> 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.
Dead/untested code is a to be avoided - so if the YAMLLogLoader is unused/untested, it probably shouldn't be committed yet.

It's difficult to keep track of test coverage if code is committed without coverage - then when the code becomes live it's not obvious that it was previously untested, etc.


================
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)
----------------
dberris wrote:
> 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).
Can be nice to be able to read streaming input (general good for uniformity - lots of tools accept '-' as the filename to read from stdin), etc. Not a strong requirement by any means.


================
Comment at: tools/llvm-xray/xray-log-reader.cc:113
+    uint32_t OffsetPtr = 0;
+    Records.push_back({});
+    auto &Record = Records.back();
----------------
Could use 'emplace_back()' if you reckon that's more readable (I don't think it's less readable than {} at least).


================
Comment at: tools/llvm-xray/xray-record.h:38
+  // The type of the event. Usually either ENTER = 0 or EXIT = 1.
+  uint8_t Type;
+
----------------
dblaikie wrote:
> Is there an enum that should be used for this member?
The comment "Usually either ENTER = 0 or  EXIT = 1" is no longer needed, as it's implied by the type. (perhaps could be reworded, etc - "Identifies this as an enter or exit record" or somesuch if that's particularly useful - though the enum is only a few lines away)


https://reviews.llvm.org/D24376