This is an archive of the discontinued LLVM Phabricator instance.

[XRAY] [x86_64] Adding a Flight Data filetype reader to the llvm-xray Trace implementation.
ClosedPublic

Authored by kpw on Feb 7 2017, 5:40 PM.

Details

Summary

The file type packs function trace data onto disk from potentially multiple
threads that are aggregated and flushed during the course of an instrumented
program's runtime.

It is named FDR mode or Flight Data recorder as an analogy to plane
blackboxes, which instrument a running system without access to IO.

The writer code is defined in compiler-rt in xray_fdr_logging.h/cc

Diff Detail

Repository
rL LLVM

Event Timeline

kpw created this revision.Feb 7 2017, 5:40 PM
dberris requested changes to this revision.Feb 9 2017, 12:34 AM
dberris added inline comments.
lib/XRay/Trace.cpp
57 ↗(On Diff #87572)

s/Naive/naive/

70–72 ↗(On Diff #87572)

Consider:

if (auto E = readBinaryFormatHeader(Data, FileHeader))
  return E;
168–170 ↗(On Diff #87572)

Same simplification mentioned elsewhere applies here.

171 ↗(On Diff #87572)

You can reduce the repetition here by doing one of the following:

  • Use auto to not repeat yourself:
auto State = FDRState{...};
  • Use aggregate init on State directly:
FDRState State{...};
180–231 ↗(On Diff #87572)

Consider breaking out individual record type handling into smaller functions, and/or encapsulating this metadata-record specific logic into its own function.

257–258 ↗(On Diff #87572)

I suspect you could be able to use the enum values as case labels here for the record types (using the conversion to int support for enum classes). It would be great if that works here. :)

263 ↗(On Diff #87572)

Consider including the record type in the error message. Using a Twine to build up the error message would be good to do here.

287–291 ↗(On Diff #87572)

In the LLVM Style, it's acceptable (and preferred sometimes) to lose the {}'s for single-statement if bodies.

376 ↗(On Diff #87572)

We probably want to document somewhere (or name) that Type == 0 means the naive (basic mode?) format, while Type == 1 means the FDR mode format.

This revision now requires changes to proceed.Feb 9 2017, 12:34 AM
kpw updated this revision to Diff 88032.Feb 10 2017, 12:10 PM
kpw edited edge metadata.
kpw marked 9 inline comments as done.
  • Applying comments from code review.
kpw added a comment.Feb 10 2017, 12:14 PM

Thanks for the comments Dean. I've updated the diff with some changes, the most substantial of which is breaking out different RecordKind state transitions into individual functions.

lib/XRay/Trace.cpp
57 ↗(On Diff #87572)

I don't like that these functions are named as nouns instead of verbs either. loadNaiveFormatLog is more to my taste.

257–258 ↗(On Diff #87572)

Alas, enum classes require static_cast to convert to int, in contrast to humble enums. There's little harm in having case statements with the static_cast though (e.g. case static_cast<uint8_t>(RecordType::ENTER): )

This should work for ENTER/EXIT. TAIL_EXIT is not yet defined in RECORD_TYPES, and I left defining it for a follow up CL since there is code for TAIL exit deduction that will have to be touched.

dberris accepted this revision.Feb 13 2017, 9:55 AM

LGTM -- thanks @kpw!

This revision is now accepted and ready to land.Feb 13 2017, 9:55 AM
kpw added a comment.Feb 16 2017, 12:45 AM

I don't have commit access (this is my first patch). What should I do to see this land upstream?

This revision was automatically updated to reflect the committed changes.