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

[Keith via Phabricator via llvm-commits]

kpw accepted this revision.
kpw added inline comments.
This revision is now accepted and ready to land.


================
Comment at: include/xray/xray_log_interface.h:62
+//   }
+//
 #ifndef XRAY_XRAY_LOG_INTERFACE_H
----------------
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.


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


https://reviews.llvm.org/D32579