This is an archive of the discontinued LLVM Phabricator instance.

[XRay] FDR Record Producer/Consumer Implementation
ClosedPublic

Authored by dberris on Aug 27 2018, 2:06 AM.

Details

Summary

This patch defines two new base types called RecordProducer and
RecordConsumer which have default implementations for convenience
(particularly for testing).

A RecordProducer implementation has one member function called
produce() which serves as a factory constructor for Record
instances. This code exercises the RecordInitializer code path in the
implementation for FileBasedRecordProducer.

A RecordConsumer has a single member function called consume(...)
which, as the name implies, consumes instances of
std::unique_ptr<Record>. We have two implementations, one of which is
used in the test to generate a vector of std::unique_ptr<Record>
similar to how the LogBuilder implementation works.

We introduce a test in FDRProducerConsumerTest which ensures that
records we write through the FDRTraceWriter can be loaded by the
FileBasedRecordProducer. The record(s) loaded this way are written
again through the FDRTraceWriter into a separate string, which we then
compare. This ensures that the read-in bytes to create the Record
instances in memory can be replicated when written out through the
FDRTraceWriter.

This change depends on D51210 and is part of the refactoring of D50441
into smaller, more focused changes.

Diff Detail

Repository
rL LLVM

Event Timeline

dberris created this revision.Aug 27 2018, 2:06 AM
eizan accepted this revision.Aug 30 2018, 10:50 PM
eizan added inline comments.
llvm/lib/XRay/FDRRecordProducer.cpp
31 ↗(On Diff #163464)

nit: Instead of doing the bitwise ops directly here and in the switch statement argument + elsewhere, why not declare something like bool is_metadata(uint8_t b) and enum MetadataType metadata_type(uint8t_t d). That way you don't need somebody to refer to your comment at the top of this function, and if you use enum values you don't need to annotate each 'case N:' line.

This revision is now accepted and ready to land.Aug 30 2018, 10:50 PM
dberris updated this revision to Diff 163469.Aug 31 2018, 12:56 AM
dberris marked an inline comment as done.

Address comment by @eizan.

dberris added inline comments.Aug 31 2018, 12:56 AM
llvm/lib/XRay/FDRRecordProducer.cpp
31 ↗(On Diff #163464)

Good point -- I've refactored this a bit to make it easier to see the mapping between the loaded type and the type of the record to create.

This revision was automatically updated to reflect the committed changes.
RKSimon added inline comments.
llvm/trunk/lib/XRay/FDRRecordProducer.cpp
50

@dberris Why are you mapping an integer to an enum with the same value?!? Replace the uint8_t T arg with an MetadataRecordKinds enum arg, or just get rid of the enum (AFAICT its only used in this function) and use the uint8_t directly.

dberris added inline comments.Aug 31 2018, 2:43 AM
llvm/trunk/lib/XRay/FDRRecordProducer.cpp
50

So, there are two reasons:

  • The data we're getting is a byte, and we need to decide whether the byte maps to the number we're expecting. The caller of the function (below) only has access to the byte.
  • I could just static cast the byte into a MetadataRecordKinds value, but that's undefined behaviour (not all the values of T will be valid values for the enum).

If I get rid of the enum, I have to keep the enum names and values as comments (previous version of this patch).

Ultimately the enum is a convenience. This mapping though is a little suspect.

This is where being able to just share the enum definition between compiler-rt and llvm would make things a lot easier. :(

dberris marked an inline comment as done.Aug 31 2018, 4:43 AM
dberris added inline comments.
llvm/trunk/lib/XRay/FDRRecordProducer.cpp
50

Actually, I think you're right -- there's a better way to do this. Fixed in r341205.