[PATCH] D24377: [XRay] Implement the `llvm-xray account` subcommand

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: tools/llvm-xray/xray-account.cc:34
+static cl::opt<bool>
+    AccountKeepGoing("keep_going", cl::desc("Keep going on errors encountered"),
+                     cl::sub(Account), cl::init(false));
----------------
dblaikie wrote:
> Is this necessary/useful?
> Is this tested?
> 
> Also, I think flags tend to be - not _ separated.
> Is this necessary/useful?

Yep -- because we cannot assume that trace files will have perfect information (TSC drift, and unmatched exit records, just some of the errors we encounter) we'd like to allow users to continue despite imperfections.

> Is this tested?

It wasn't but now should be. :)

> Also, I think flags tend to be - not _ separated.

Done


================
Comment at: tools/llvm-xray/xray-account.cc:39-42
+static cl::opt<bool> AccountDeduceSiblingCalls(
+    "deduce-sibling-calls",
+    cl::desc("Deduce sibling calls when unrolling function call stacks"),
+    cl::sub(Account), cl::init(false));
----------------
dblaikie wrote:
> Test coverage?
We have a test for this, which does tail call deduction.


================
Comment at: tools/llvm-xray/xray-account.cc:47-69
+static cl::opt<std::string>
+    AccountOutput("output", cl::value_desc("output file"), cl::init("-"),
+                  cl::desc("output file; use '-' for stdout"),
+                  cl::sub(Account));
+static cl::alias AccountOutput2("o", cl::aliasopt(AccountOutput),
+                                cl::desc("Alias for -output"),
+                                cl::sub(Account));
----------------
dblaikie wrote:
> Having to specify both input files, as well as input and output formats every time seems a bit tedious. Could we detect formats using file magic & perhaps have some way of unifying the two files (the map and the log) in common/some cases?
Changed to use the log file detection magic to at least remove the input format specification. I'll need to change the extract logic to also use file type detection magic (will do so in a different patch that makes the instrumentation map extraction a library in LLVM as well) which will greatly simplify this API.


https://reviews.llvm.org/D24377