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

[David Blaikie via llvm-commits]

dblaikie added inline comments.


================
Comment at: tools/llvm-xray/xray-extract.cc:51-57
+static cl::opt<InstrumentationMapExtractor::InputFormats> InstrMapFormat(
+    "instr-map-format", cl::desc("format of instrumentation map"),
+    cl::values(clEnumValN(InstrumentationMapExtractor::InputFormats::ELF, "elf",
+                          "instrumentation map in an ELF header"),
+               clEnumValN(InstrumentationMapExtractor::InputFormats::YAML,
+                          "yaml", "instrumentation map in YAML")),
+    cl::sub(Extract), cl::init(InstrumentationMapExtractor::InputFormats::ELF));
----------------
Could we detect which kind of file we're reading with file magic instead of passing an argument to specify the input format?

Is it useful to specify the input format as YAML if the only task to be performed is to generate YAML? Perhaps the only input format should be ELF/object file.


================
Comment at: tools/llvm-xray/xray-extract.cc:62-67
+using ::llvm::yaml::MappingTraits;
+using ::llvm::yaml::ScalarEnumerationTraits;
+using ::llvm::yaml::IO;
+using ::llvm::yaml::Hex64;
+using ::llvm::yaml::Output;
+using ::llvm::yaml::Input;
----------------
LLVM isn't too fussy about using declarations/directives - you're welcome to "using llvm::yaml" if that seems suitable, I think? (maybe the yaml namespace has particularly vague names that are problematic?)

(also we don't tend to bother with the '::' global scope qualifier)


================
Comment at: tools/llvm-xray/xray-extract.cc:104
+  StringRef Msg;
+  std::error_code EC;
+
----------------
If you can get away without needing to be convertible to error_code, that's probably a good thing (it's just there for backwards compatibility in APIs where we haven't migrated the whole stack to llvm::Error yet)

Also you might be able to get away without needing a custom Error type entirely - maybe just use StringError?


================
Comment at: tools/llvm-xray/xray-extract.cc:130-132
+    return make_error<BinaryInstrELFError>(
+        "File format not supported (only does ELF).",
+        std::make_error_code(std::errc::not_supported));
----------------
Test case for this codepath? (goes for all the error paths, ideally)


================
Comment at: tools/llvm-xray/xray-extract.cc:154
+  auto C = Contents.bytes_begin();
+  if (std::distance(C, Contents.bytes_end()) % sizeof(XRaySledEntry) != 0)
+    return make_error<BinaryInstrELFError>(
----------------
If you're already using a random access iterator, I'd probably suggest writing this using op- instead of std::distance. std::distance is handy when you need to write generic code which might be applied to non-random access iterators, but otherwise seems a bit obfuscatory.


================
Comment at: tools/llvm-xray/xray-extract.cc:192-203
+class YAMLInstrError : public llvm::ErrorInfo<YAMLInstrError> {
+  StringRef Msg;
+  std::error_code EC;
+
+public:
+  static char ID;
+  YAMLInstrError(StringRef Msg, std::error_code EC) : Msg(Msg), EC(EC) {}
----------------
Same feedback, might be fine to just use StringError


================
Comment at: tools/llvm-xray/xray-extract.cc:263-265
+    handleAllErrors(std::move(E), [&](const YAMLInstrError &YE) {
+      EC = YE.convertToErrorCode();
+    });
----------------
It'd probably be good to keep in the llvm::Error space rather than dropping to std::error_code - you'll probably be losing extra diagnostic information by collapsing into an error code like this. (same a few lines up with BinaryInstrELFError)


================
Comment at: tools/llvm-xray/xray-extract.cc:279
+         Sled.Kind == '\0' ? FunctionKinds::ENTRY : FunctionKinds::EXIT,
+         Sled.AlwaysInstrument ? true : false});
+  }
----------------
What's the purpose of this conditional operator ("AlwaysIntrument ? true : false") - one would assume AlwaysInstrument is already a boolean and this conditional operator is redundant.

Ah, because it matches the binary format on disk, it's a char.

Could we just get away from memcpying the struct off disk - only being 4 fields, llvm::DataExtractor might suffice to quickly pull 4 fields of known sizes out of a stream? & then the struct's members can be the right types, etc.

(I wonder, but assume it's not appropriate, if we could just use the YAML struct for everything? But I guess we want various APIs consuming these data structures/records that shouldn't be aware of any YAML binding, etc)


================
Comment at: tools/llvm-xray/xray-extract.cc:285-286
+
+namespace llvm {
+namespace xray {
+
----------------
a static variable inside a namespace - is the namespace there to have the same effect as "using"? Would using "using" be more obvious/straightforward? Oh, they're already 'using' at the top. So do these namespace decls provide anything?


================
Comment at: tools/llvm-xray/xray-extract.cc:288
+
+static CommandRegistration Unused(&Extract, [] {
+  std::error_code EC;
----------------
Might even be worth having the command registration functor return an llvm::Error, then the error printing could be handled up in the caller (if everyone's just going to print an error and return 1, at least - that could happen up there).


https://reviews.llvm.org/D21987