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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: include/xray/xray_log_interface.h:15
 //===----------------------------------------------------------------------===//
+//
+// XRay allows users to implement their own logging handlers and install them to
----------------
kpw wrote:
> Is this comment block intended to show up in Doxygen? I think you'll want an additional slash for these notes.
Wow, good catch -- yes, otherwise this would be all for nought! :)


================
Comment at: include/xray/xray_log_interface.h:62
+//   }
+//
 #ifndef XRAY_XRAY_LOG_INTERFACE_H
----------------
kpw wrote:
> 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.
Note, that `__xray_log_remove_impl()` doesn't need to be called at all -- some implementations can be re-initialized just by users directly calling `__xray_log_init(...)` again. `__xray_log_remove_impl()` is a "nuclear" option that uninstalls the implementation.


================
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);
----------------
kpw wrote:
> 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.
Good point. I've extended this to say when it's safe to call and when it's implementation defined.


================
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.
----------------
kpw wrote:
> Same comment as for set_impl. 
> 
> Also, is this a no-op or an error if no implementation is installed?
I'm not sure whether mentioning whether it's a no-op is appropriate at this level -- it may be an implementation detail. The side-effect though would be that if there was something installed, then there's no longer something 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);
----------------
kpw wrote:
> 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)
I thought about whether we should return something for setting the log implementation (or removing it). Because XRay initialisation happens pre-main (see xray_init) and is linked for all xray-instrumented binaries, we let users discover the error when patching.

Semantically, changing the log implementation is not something that can "fail" -- you can set the logging implementation but not be able to patch sleds because there is no instrumentation map. Those two are completely different axes.

Do you have a suggestion on how/where that documentation should show up?


https://reviews.llvm.org/D32579