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

[Dean Michael Berris via llvm-commits]

dberris added a comment.

In https://reviews.llvm.org/D24376#602808, @dblaikie wrote:

> (I was thinking more going the other direction: removing the dead code, rather than adding a use of it)
>
> What's the purpose of the binary/raw output format? (I think it was mentioned that the binary output format would be for testing - so we could write yaml, then generate raw files - then feed those into other tools for testing? But if the yaml input format is supported everywhere, what's the purpose of that? We'll want to have a couple of binary tests, but I imagine they would test checked in binary -> yaml output (if they test yaml -> binary -> yaml, then I'm not sure they achieve much because it's just roundtripping so we have as much code under test (binary -> yaml) as we do in the test harness (yaml -> binary), effectively)
>
> That's my mental model at least.


Other things going on concurrently (work on the FDR mode for the runtime library) is making me need to be able to turn versions of the binary log from one form to another. I've found that working with the binary versions is much more convenient from a tool perspective, and that this current simple format is more amenable to analysis than the condensed format I'm working on.

So effectively, version 2 of the log format is turning out to have both non-fixed-size records and more interleaving happening. This tool, that's able to take that complex format into something simpler/different doesn't make sense for generating just YAML files.

> (this commit seems to add several .bin test files - but I don't see where those are used?)

Some of the tests are looking at symbolization of the function id's, and associating functions with debug info, etc. for coverage of the various modes by which the YAML output could be generated.



================
Comment at: tools/llvm-xray/xray-converter.cc:175-176
+  handleAllErrors(std::move(Err), [&](const ErrorInfoBase &E) {
+    if (!ConvertInstrMap.empty())
+      E.log(errs());
+  });
----------------
dblaikie wrote:
> This seems like an odd construct - should Extractor not error-out in the case where ConvertInstrMap is empty?
Good point.

Technically, that is an error in the use of the extractor's constructor, hence it signalling an error. Think about it as if the construction of the extractor had thrown an exception -- and in this case, the appropriate action is to ignore that exception.

So this is saying, if the filename was empty, then the extractor couldn't be initialised and therefore we ignore the error. In the case that it wasn't empty, then we should emit the error, but still not cause the tool to fail in execution.

Changed both here and the extract sub-command where we check the filename explicitly to not be empty.


https://reviews.llvm.org/D24376