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

[David Blaikie via llvm-commits]

dblaikie added inline comments.


> simple-instrmap.yaml:1
> +# This is a simple instrumentation map with bogus addresses and offsets, but
> +# follow the recommended format.

What's this file used for? I couldn't see its name mentioned anywhere else and it has no RUN lines.

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

Do we need both yaml and json? If this is only for testing purposes, seems one would suffice? (is this for other purposes?)

> llvm-xray.cc:37-38
> +    if (*SC) {
> +      auto C = dispatch(SC);
> +      if (C) {
> +        return C();

Roll the variable declaration into the condition

> llvm-xray.cc:38-40
> +      if (C) {
> +        return C();
> +      }

Drop the {} on single-line blocks

(possibly even on the "if (*SC)" block too - though that's more questionable, some people take teh LLVM convention as "no braces on single line blocks" others as "no braces on single statement blocks, even if the statement is spread over multiple lines")

> xray-extract.cc:114
> +// Returns {"message", false} on errors; {"", true} otherwise.
> +std::pair<StringRef, std::error_code> LoadBinaryInstrELF(
> +    StringRef Filename, std::deque<XRaySledEntry> &Sleds,

pair<StringRef, error_code> looks a bit like it should be llvm::Error, no?

> xray-extract.cc:118-119
> +    InstrumentationMapExtractor::FunctionAddressReverseMap &FunctionIds) {
> +  if (Filename.empty())
> +    llvm_unreachable("Provided Filename is empty.");
> +

Branch-to-unreachable should be an assert instead, but chances are this should be a handled error case, right? Or is filename non-empty a precondition to this function?

> xray-extract.cc:302-303
> +
> +static auto Unused = [] {
> +  registerCommand(&Extract, [] {
> +    std::error_code EC;

Might be worth making the registerCommand into a "CommandRegistration" (since they'll all want to do it in a global init anyway, so it's not like you'll have other callers/uses of registerCommand except in idioms that all look like this global ctor):

  static CommandRegistration Unused(&Extract, [] { ... });

> xray-extract.h:46
> +  /// instrumentation map is in.
> +  explicit InstrumentationMapExtractor(std::string Filename,
> +                                       InputFormats Format,

No need for explicit on multi-arg ctors - we haven't bothered to try to do that across the LLVM codebase.

> xray-extract.h:61-63
> +int extract(StringRef Input,
> +            InstrumentationMapExtractor::InputFormats InputFormat,
> +            StringRef Output, ExtractOutputFormats OutputFormat);

Documentation comment, perhaps?

It's not clear, looking at the signature, what this might do (how could the output be provided in a StringRef? What would the ownership semantics be? Should this be called "convert" rather than "extract" if it maps from one format to another? Should this have an intermediate representation so that different input and output formats can be generically composed?)

https://reviews.llvm.org/D21987