[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:279
+         Sled.Kind == '\0' ? FunctionKinds::ENTRY : FunctionKinds::EXIT,
+         Sled.AlwaysInstrument ? true : false});
+  }
----------------
dberris wrote:
> 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.
Still, think it might be nice to move away from it being splatted back and forth to memory like that - and having the expected C++ types (like enums and bools).


================
Comment at: tools/llvm-xray/xray-extract.cc:237-239
+    errs() << "Cannot extract instrumentation map from '" << ExtractInput
+           << "'\n";
+    return Error(std::move(Err));
----------------
Previous code only printed this "Cannot extract" message, new code will print that as well as whatever text is in Err, right? Is that a desired change?

Is there some nice/easy way to append/prepend the "Cannot extract" text to the exsiting Error to pass up to main to print there? (splitting the diagnostic printing between the two places seems a bit awkward)


================
Comment at: tools/llvm-xray/xray-extract.cc:245-246
+  if (EC) {
+    errs() << "Cannot open file '" << ExtractOutput << "' for writing.\n";
+    return errorCodeToError(EC);
+  }
----------------
Ideally pass up the string and error code here, rather than printing then passing?


https://reviews.llvm.org/D21987