[PATCH] D21987: [XRay] Implement `llvm-xray extract`, start of the llvm-xray tool

[Dean Michael Berris via llvm-commits]

dberris marked an inline comment as done.
dberris added inline comments.


================
Comment at: test/tools/llvm-xray/X86/extract-instrmap.ll:5-6
+; RUN: llc -filetype=obj -o %t -mtriple=x86_64-unknown-linux-gnu < %s
+; RUN: llvm-xray extract -format=json %t | FileCheck %s --check-prefix=CHECK-JSON
+; RUN: llvm-xray extract -format=yaml %t | FileCheck %s --check-prefix=CHECK-YAML
+
----------------
dblaikie wrote:
> dberris wrote:
> > dblaikie wrote:
> > > Do we need both yaml and json? If this is only for testing purposes, seems one would suffice? (is this for other purposes?)
> > JSON is convenient but not strictly required. Removed.
> Convenient how?
> I suppose what i'm trying to understand is: why /both/? (which is to say, what does each offer that the other doesn't)
JSON was convenient for making it easily parse-able and view-able by anything that will show it on a browser. :)


================
Comment at: test/tools/llvm-xray/X86/extract-instrmap.ll:12
+; CHECK:      ---
+; CHECK-NEXT: - { id: 1, address: 0x{{[0-9A-F]+}}, function: 0x{{[0-9A-F]+}}, kind: function-enter,
+; CHECK-NEXT:     always-instrument: true }
----------------
dblaikie wrote:
> Is it worth checking the specific addresses? If this test is simple enough to provide fairly stable values here - to make sure the algorithms, etc, are working correctly & not just printing garbage values?
Yep, added a few more functions to be a bit more complete.


================
Comment at: tools/llvm-xray/xray-extract.cc:262-266
+    return joinErrors(make_error<StringError>(
+                          Twine("Cannot extract instrumentation map from '") +
+                              ExtractInput + "'",
+                          std::make_error_code(std::errc::protocol_error)),
+                      std::move(Err));
----------------
dblaikie wrote:
> dberris wrote:
> > dblaikie wrote:
> > > Would it make sense to sink this code into InstrumentationMapExtractor's ctor? (it has all the information - it knows it's trying to extract an instrumentation map, and the name of the input - so it seems it should be responsible for creating this message, maybe?)
> > Actually, nope -- because users of this class could choose to ignore the errors (i.e. treat it as if there was no available instrumentation map). It's just that for this sub-command, it wouldn't work if extraction actually failed. :)
> I'm not sure I understand this - could you rephrase?
> 
> What I mean is: The joining of this "Cannot extract instrumentation map from" seems like it could go inside the InstrumentationMapExtractor ctor (since it has all the context to make that message and the message seems appropriate at that level). Then at this level (in the CommandRegistration) we just propagate the error up ("if (Err) return Err; or whatever)
Ah, right -- I've moved the concatenation of the Twine into the error generation parts.


================
Comment at: tools/llvm-xray/xray-extract.h:50
+
+  const FunctionAddressMap &getFunctionAddresses() { return FunctionAddresses; }
+
----------------
dblaikie wrote:
> Unused/dead code
> Unused/dead code

Yes, but this is actually used in a later (stacked) change...


https://reviews.llvm.org/D21987