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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris marked 3 inline comments as done.
dberris added a comment.

Thanks @kpw -- landing now, we can iterate on documenting more if things become unclear/confusing.



================
Comment at: include/xray/xray_log_interface.h:62
+//   }
+//
 #ifndef XRAY_XRAY_LOG_INTERFACE_H
----------------
kpw wrote:
> dberris wrote:
> > 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.
> I missed the state transition from finalized to initializing via xray_log_init. Having that transition serves the same purpose as the proposed xray_log_reset_impl without adding another method.
> 
> We might want to point out that this may block on the completion of a flush in the case that the log flush status is XRAY_LOG_FLUSHING.
Yes, we don't need `__xray_log_reset_impl()` anymore. I thought about that in an early iteration of this API, but decided it's too hard to keep the invariants for that which calling `__xray_log_init()` already achieves.

`__xray_log_remove_impl()` does something very different though. It literally removes the implementation that's currently 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:
> dberris wrote:
> > 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?
> I see. Since xray_init is in .preinit_array, I have no problems with swallowing the error codes.
> 
> On a related note, do we have to revisit the  patch_premain flag? AFAICT, it doesn't allow users to choose their implementation, and more importantly, it doesn't call __xray_log_init. We would have to do something clever to define an interface to have this flag choose the logging implementation. Maybe we could reserve a section header where we can check for user defined function pointers that set up their implementation and if we don't find anything, install and initialize the default.
We let users choose the implementation for `patch_premain` by letting users define either `xray_naive_log=` or `xray_fdr_log=`. This API allows users to install an implementation that can't be installed through the flags. It's also for controlling an implementation explicitly (init, finalize, flush, repeat).

We might want to consider an explicit registration/initialisation mechanism, but users can already do this at init-time, by doing a global initializer that runs before main() but after premain.


https://reviews.llvm.org/D32579