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

Dean Michael Berris via llvm-commits llvm-commits at lists.llvm.org
Mon Oct 17 22:43:05 PDT 2016


dberris added a comment.

Please have another look @dblaikie.



================
Comment at: tools/llvm-xray/xray-extract.cc:279
+         Sled.Kind == '\0' ? FunctionKinds::ENTRY : FunctionKinds::EXIT,
+         Sled.AlwaysInstrument ? true : false});
+  }
----------------
dblaikie wrote:
> 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)
> 
> 
Ah, yes, this makes sense. I've hidden the ELF64-specific sled layout and defined a higher level `SledEntry` type that could be exposed later.


https://reviews.llvm.org/D21987





More information about the llvm-commits mailing list