[PATCH] D46173: [XRay][compiler-rt+docs] Introduce __xray_log_init_mode(...).

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: compiler-rt/include/xray/xray_log_interface.h:35-36
+///   // To do this, we provide the string flags configuration for the mode.
+///   auto config_status = __xray_log_init_mode(
+///       "xray-fdr", "verbosity=1 some_flag=1 another_flag=2");
+///   if (config_status != XRayLogInitStatus::XRAY_LOG_INITIALIZED) {
----------------
kpw wrote:
> Is each mode expected to document its own flag options? Is there a shared option set among all modes?
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.


================
Comment at: compiler-rt/include/xray/xray_log_interface.h:43
+///   // now patch the instrumentation points. Note that we could have patched
+///   // the instrumentation sleds first, but there's no strict ordering to
+///   // these operations.
----------------
kpw wrote:
> If you're calling the sleds instrumentation points above, be consistent.
> 
> instrumentation sleds -> instrumentation points.
Also changed globally in the file.


================
Comment at: compiler-rt/include/xray/xray_log_interface.h:283-285
+/// - ArgsSize = 0
+/// - Args will be the pointer to the null-terminated character buffer
+/// representing the configuration.
----------------
kpw wrote:
> 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.
Makes sense -- added `__xray_log_init_mode_bin(...)` to support the pointer+length API for character buffers.


================
Comment at: compiler-rt/test/xray/TestCases/Posix/logging-modes.cc:60
 
-static auto buffer_counter = 0;
+static auto volatile buffer_counter = 0;
 
----------------
kpw wrote:
> Why volatile? I only see one thread of execution. If sync is needed, why volatile instead of atomic.
Yeah, this was an artifact of some debugging/exploration. Unnecessary change.


================
Comment at: llvm/docs/XRay.rst:200
+an implementation's data can be cleared out through the
+``__xray_log_flushLog()`` function. For implementations that support in-memory
+processing, these should register an iterator function to provide access to the
----------------
kpw wrote:
> Kind of regrettable camelCase on this function, but it is correct. :(
Yeah... someday, we will have the time to clean this all up. :)


https://reviews.llvm.org/D46173