[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:
> > 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).
> I agree -- on the ELF section (and the stuff embedded in the binaries), I think we're a bit constrained by what we can effectively write on the compiler-side (and read on the tooling side). On the trace files though I'm a bit more open to working with a unified encoding format, that was a bit more clever than it currently is.
> I agree -- on the ELF section (and the stuff embedded in the binaries), I think we're a bit constrained by what we can effectively write on the compiler-side (and read on the tooling side). On the trace files though I'm a bit more open to working with a unified encoding format, that was a bit more clever than it currently is.

Sorry I think I'm still confused/we're talking past each other. All I mean is that, rather than this tool splatting into a carefully laid out struct, it would use something like DataExtractor to extract the carefully laid out bytes into a more generic struct.

The file formats and writer would remain the same - just the reader would be more robust and more a usable type for the rest of the APIs.

(I suppose put another way: Using the YAML struct with its YAML types isn't really ideal for general tool code that's trying to process these records. Equally, using a carefully laid out struct with surprising types (char instead of bool/enum, explicit padding, etc) has the same problem. The main interchange structure should probably be the generic semantic thing, not YAML or binary file related - and once that happens, we could skip the packed struct entirely and just read in the few records with DataExtractor or a similar API)




https://reviews.llvm.org/D21987