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

[Dean Michael Berris via llvm-commits]

dberris planned changes to this revision.
dberris added a comment.

Thanks @dblaikie -- I'll make some changes and ping when ready. :)



================
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));
----------------
dblaikie wrote:
> 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.
> Could we detect which kind of file we're reading with file magic instead of passing an argument to specify the input format?

Not with how we've defined it in compiler-rt, no. :(

> 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.

Good point. For extraction it doesn't make sense. But the class that does extraction can be used (and actually is used in later patches) to load an instrumentation map in YAML. I'll remove the flag and assume that extraction will only ever make sense on ELF/object files.


================
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;
----------------
dblaikie wrote:
> 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)
> 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?)

Right -- I was merely following the examples. I'll try with just `using namespace llvm::yaml` here.

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

Will fix this -- force of habit. :)


================
Comment at: tools/llvm-xray/xray-extract.cc:104
+  StringRef Msg;
+  std::error_code EC;
+
----------------
dblaikie wrote:
> 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?
> 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)

Unfortunately that seems to be a pure virtual function in ErrorInfo<...>. :(

I like `StringError` though, I'll change this to use StringError instead (less types, better).


================
Comment at: tools/llvm-xray/xray-extract.cc:279
+         Sled.Kind == '\0' ? FunctionKinds::ENTRY : FunctionKinds::EXIT,
+         Sled.AlwaysInstrument ? true : false});
+  }
----------------
dblaikie wrote:
> 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)
Yeah, we're constrained by the structure that's in disk. I thought about only ever using the YAML struct for everything, and it boils down to allowing it to be ported/used by other classes/functions that need to load the instrumentation map and don't need any of the YAML functionality.


================
Comment at: tools/llvm-xray/xray-extract.cc:285-286
+
+namespace llvm {
+namespace xray {
+
----------------
dblaikie wrote:
> 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?
This is vestigial -- it used to be that there was a function defined in the namespace, and it's changed to this global registration approach. I'll remove the namespaces. :)


================
Comment at: tools/llvm-xray/xray-extract.cc:288
+
+static CommandRegistration Unused(&Extract, [] {
+  std::error_code EC;
----------------
dblaikie wrote:
> 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).
Good idea, yeah I'll change this to make the function return an llvm::Error and consolidate the logging and exit in main.


https://reviews.llvm.org/D21987