This is an archive of the discontinued LLVM Phabricator instance.

[XRay][compiler-rt+docs] Introduce __xray_log_init_mode(...).
ClosedPublic

Authored by dberris on Apr 26 2018, 10:21 PM.

Details

Summary

This addresses http://llvm.org/PR36790.

The change Deprecates a number of functions and types in
include/xray/xray_log_interface.h to recommend using string-based
configuration of XRay through the __xray_log_init_mode(...) function. In
particular, this deprecates the following:

  • __xray_set_log_impl(...) -- users should instead use the

__xray_log_register_mode(...) and __xray_log_select_mode(...) APIs.

  • __xray_log_init(...) -- users should instead use the

__xray_log_init_mode(...) function, which also requires using the
__xray_log_register_mode(...) and __xray_log_select_mode(...)
functionality.

  • __xray::FDRLoggingOptions -- in following patches, we'll be

migrating the FDR logging implementations (and tests) to use the
string-based configuration. In later stages we'll remove the
__xray::FDRLoggingOptions type, and ask users to migrate to using the
string-based configuration mechanism instead.

  • __xray::BasicLoggingOptions -- same as __xray::FDRLoggingOptions,

we'll be removing this type later and instead rely exclusively on the
string-based configuration API.

We also update the documentation to reflect the new advice and remove
some of the deprecated notes.

Diff Detail

Repository
rL LLVM

Event Timeline

dberris created this revision.Apr 26 2018, 10:21 PM
kpw added a comment.May 3 2018, 4:10 PM

Everything looks good except I'd like to see us flexible for mode configuration that may contain nulls.

compiler-rt/include/xray/xray_log_interface.h
35–36 ↗(On Diff #144501)

Is each mode expected to document its own flag options? Is there a shared option set among all modes?

43 ↗(On Diff #144501)

If you're calling the sleds instrumentation points above, be consistent.

instrumentation sleds -> instrumentation points.

283–285 ↗(On Diff #144501)

I would prefer an alternative (or perhaps having both) that provides the ArgsSize as length of char buffer. It could be nice to be able to encode protos or binary data without worrying about null bytes, since interpreting the configuration payload is entirely up to the mode.

compiler-rt/test/xray/TestCases/Posix/logging-modes.cc
60 ↗(On Diff #144501)

Why volatile? I only see one thread of execution. If sync is needed, why volatile instead of atomic.

llvm/docs/XRay.rst
200 ↗(On Diff #144501)

Kind of regrettable camelCase on this function, but it is correct. :(

dberris updated this revision to Diff 145146.May 3 2018, 9:23 PM
dberris marked 5 inline comments as done.
  • fixup: address comments by kpw
dberris added inline comments.May 3 2018, 9:23 PM
compiler-rt/include/xray/xray_log_interface.h
35–36 ↗(On Diff #144501)

Yes, each mode is expected to document their own flags.

There is a shared set of "xray-specific" global options. Following patches define these global settings and separate the mode-specific settings.

43 ↗(On Diff #144501)

Also changed globally in the file.

283–285 ↗(On Diff #144501)

Makes sense -- added __xray_log_init_mode_bin(...) to support the pointer+length API for character buffers.

compiler-rt/test/xray/TestCases/Posix/logging-modes.cc
60 ↗(On Diff #144501)

Yeah, this was an artifact of some debugging/exploration. Unnecessary change.

llvm/docs/XRay.rst
200 ↗(On Diff #144501)

Yeah... someday, we will have the time to clean this all up. :)

kpw accepted this revision.May 3 2018, 10:28 PM

LGTM.

This revision is now accepted and ready to land.May 3 2018, 10:28 PM
This revision was automatically updated to reflect the committed changes.