[PATCH] D43495: [XRay][compiler-rt] Add APIs for processing logs in memory

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

In https://reviews.llvm.org/D43495#1019473, @kpw wrote:

> Mostly looks good, but I think you should consider making the buffer iterator interface part of XRayLogImpl or some other mechanism to associate it with a logging mode.
>
> Iterating the buffers is going to be tightly coupled to the impl that already manages the buffers.


That makes sense, but I'm trying really hard to not change the ABI for the XRay APIs here -- changing the XRayLogImpl struct will be an ABI break, and we don't have an ABI versioning check implemented yet.

In the meantime, the suggestion is to make the installation of the log processing handler(s) be done in the initialisation of the log implementations. This would allow log implementations to tightly control what the functionality for iterating and processing the data would be.



================
Comment at: compiler-rt/include/xray/xray_log_interface.h:309-320
+/// Implementations MUST then provide:
+///
+/// 1) A function that will return an XRayBuffer. Functions that return an
+///    "empty" XRayBuffer signifies that there are no more buffers to be
+///    processed. This function should be registered through the
+///    `__xray_log_set_buffer_iterator(...)` function.
+///
----------------
kpw wrote:
> Should __xray_log_set_buffer_iterator be part of XRayLogImpl? A suitable default would simply return the empty XRayBuffer.
> 
> Since exposing the buffers is tightly coupled to the requirements of the handler, it is odd that it must be registered separately and can't be pinned to a mode.
It should be, but changing it would be an ABI break. We're not quite ready to do that yet at this point.


================
Comment at: compiler-rt/lib/xray/xray_log_interface.cc:166
+  auto Mode = CurrentMode ? CurrentMode->Mode : nullptr;
+  while (Buffer.Data != nullptr && Buffer.Size != 0) {
+    (*Processor)(Mode, Buffer);
----------------
kpw wrote:
> Isn't it enough to check Buffer.Data != nullptr?
> 
> If for some reason, an implementation maintains valid pointers to zero size buffers, that implementation is crummy, but there's no good reason to skip the rest of the buffers in the iterator.
Good catch, thanks, Keith!


https://reviews.llvm.org/D43495