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

[David Blaikie via llvm-commits]

dblaikie added a comment.

Generally looking pretty good - a few little nits & more test coverage (error cases, for example) is probably a good idea.



================
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 }
----------------
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?


================
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
+
----------------
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)


================
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));
----------------
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)


================
Comment at: tools/llvm-xray/xray-extract.cc:85
+
+constexpr size_t ELF64SledEntrySize = 32;
+
----------------
I'd sink this down into the function/near the use (the general principle of keeping variables in the smallest scope needed)


================
Comment at: tools/llvm-xray/xray-extract.cc:163
+    auto AlwaysInstrument = Extractor.getU8(&OffsetPtr);
+    Entry.AlwaysInstrument = AlwaysInstrument == 0 ? false : true;
+
----------------
Drop the "? false : true" here, and use !=, perhaps:

"Entry.AlwaysInstrument = AlwaysInstrument != 0" (potentially even drop the "!= 0", but I can see how that helps readability.


================
Comment at: tools/llvm-xray/xray-extract.cc:181
+  }
+  Sleds.swap(OutputSleds);
+  return llvm::Error::success();
----------------
Move assignment rather than swap? (but equally I wouldn't mind the old code that left the OutputSleds in an unspecified state on failure)


================
Comment at: tools/llvm-xray/xray-extract.cc:233-237
+  case InputFormats::YAML: {
+    EC = LoadYAMLInstrFile(this->Filename, Sleds, FunctionAddresses,
+                           FunctionIds);
+    break;
+  }
----------------
This code (& LoadYAMLInstrFile) is dead/untested - perhaps should be moved into another/separate change.


================
Comment at: tools/llvm-xray/xray-extract.h:38
+private:
+  std::string Filename;
+  std::deque<SledEntry> Sleds;
----------------
Doesn't look like this needs to be a member? (it's only used during construction, if I'm reading it correctly)


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


================
Comment at: tools/llvm-xray/xray-registry.h:31-32
+//
+class CommandRegistration {
+public:
+  CommandRegistration(cl::SubCommand *SC, std::function<Error()> Command);
----------------
Could just make this a struct - since it's only member's public anyway.


================
Comment at: tools/llvm-xray/xray-registry.h:36
+
+// Requires that |SC| is not nullptr. If there was no command associated with
+// |SC| then returns an empty function.
----------------
It doesn't seem like it actually requires that SC is not null (& pedantically the terminology would be "not null" (or "not a null pointer" - 'nullptr' is just some specific null pointer literal)) - but, sure - seems OK to say that's not acceptable even if it'd be fine for the current implementation.

Is it worth just defining this function to only work if the SubCommand is registered? (assert when not found, instead of returning the empty std::function)


https://reviews.llvm.org/D21987