[PATCH] D32579: [XRay][compiler-rt] Document and update the XRay Logging API

[Keith via Phabricator via llvm-commits]

kpw added a comment.

Thanks for thinking this through Dean. Here's a few suggestions I'd like to hear your thoughts on.



================
Comment at: include/xray/xray_log_interface.h:15
 //===----------------------------------------------------------------------===//
+//
+// XRay allows users to implement their own logging handlers and install them to
----------------
Is this comment block intended to show up in Doxygen? I think you'll want an additional slash for these notes.


================
Comment at: include/xray/xray_log_interface.h:62
+//   }
+//
 #ifndef XRAY_XRAY_LOG_INTERFACE_H
----------------
I suggest a brief word of caution that before patching again, users should call __xray_log_remove_impl and reinitialize. I can see it being a common error that once logs are finalized and/or flushed users think it's OK to patch directly.

For some implementations, it would nice to be able to skip reinitialization. If buffers are already allocated it's a shame to have to remove_impl and reallocate them. Perhaps we should add a __xray_log_reset_impl that by default calls remove_impl and initializes with the same arguments. Log implementations could provide a function ptr to short circuit with appropriate state management.


================
Comment at: include/xray/xray_log_interface.h:138-140
+  /// events. In case the implementation wants to support arg1 (or other future 
+  /// extensions to XRay logging) those MUST be installed by the installed
+  /// 'log_init' handler.
----------------
Indicating where readers can find an arg1 installation could be helpful. (xray_interface.h or pointer to fdr logging log_init)


================
Comment at: include/xray/xray_log_interface.h:154-157
+///
+/// NOTE: This function does NOT attempt to finalize the currently installed
+/// implementation. Use with caution.
 void __xray_set_log_impl(XRayLogImpl Impl);
----------------
It's unclear whether the library prevents this from being called from any particular states.

Let's explicitly point out when this is guaranteed to be safe (no implementation installed and unitialized implementation?), when it is implementation defined, and when it is explicitly disallowed regardless of implementation.

I think it is defined by the implementations when this would be safe to call for an initialized log (or possibly a finalized log).

I can imagine some logging implementations that would want to chain together interceptions. E.g. once I see a request that seeds data for a particular id, install a tracer for subsequent operations that affect the data seeded for the id under inspection.


================
Comment at: include/xray/xray_log_interface.h:162-164
+///
+/// NOTE: This function does NOT attempt to finalize the currently installed
+/// implementation. Use with caution.
----------------
Same comment as for set_impl. 

Also, is this a no-op or an error if no implementation is installed?


================
Comment at: lib/xray/xray_log_interface.cc:30-38
+    __xray_remove_handler();
+    __xray_remove_handler_arg1();
     return;
   }
 
   __sanitizer::SpinMutexLock Guard(&XRayImplMutex);
   GlobalXRayImpl.reset(new XRayLogImpl);
----------------
Suspicious that the code ignores the return values from xray/xray_interface.h's xray_set_handler" and xray_remove_handler.

Looks like these have to do with checking that  xray_init was called.

Should xray_set_log_impl return something other than void? Should implementations call xray_init if necessary?

I'm in favor returning an error rather than hidden runtime cost.

(P.S. lots of underscores missing from function names above. Phabricator was doing markdown stuff with them)


https://reviews.llvm.org/D32579