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

[Dean Michael Berris via llvm-commits]

dberris added inline comments.


================
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:
> 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.
Good point. I've re-implemented the binary output, and the round-tripping.


================
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:
> 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.
Actually, now that I read this part of the code again, it's the YAML `Input` that requires a full document to be provided in its constructor.

http://llvm.org/docs/YamlIO.html#input


================
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:
> 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)
Reworded to be short and sweet.


https://reviews.llvm.org/D24376