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

[Keith via Phabricator via llvm-commits]

kpw added a comment.

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



================
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) {
----------------
Is each mode expected to document its own flag options? Is there a shared option set among all modes?


================
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.
----------------
If you're calling the sleds instrumentation points above, be consistent.

instrumentation sleds -> instrumentation points.


================
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.
----------------
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.


================
Comment at: compiler-rt/test/xray/TestCases/Posix/logging-modes.cc:60
 
-static auto buffer_counter = 0;
+static auto volatile buffer_counter = 0;
 
----------------
Why volatile? I only see one thread of execution. If sync is needed, why volatile instead of atomic.


================
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
----------------
Kind of regrettable camelCase on this function, but it is correct. :(


https://reviews.llvm.org/D46173