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

Keith via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 26 10:35:06 PST 2018


kpw added a comment.

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.



================
Comment at: compiler-rt/include/xray/xray_log_interface.h:241
 
+/// Returns an identifier for the currently installed XRay mode installed
+/// through the __xray_log_select_mode(...) function call. Returns nullptr if
----------------
I would change "currently installed" to "currently selected". Multiple nodes may be registered (installed) but only one selected.


================
Comment at: compiler-rt/include/xray/xray_log_interface.h:276
 
+/// An XRayBUffer represents a section of memory which can be treated by log
+/// processing functions as bytes stored in the logging implementation's
----------------
Nit: XRayBUffer -> XRayBuffer


================
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.
+///
----------------
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.


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


================
Comment at: compiler-rt/test/xray/TestCases/Linux/logging-modes.cc:78
+         XRayLogFlushStatus::XRAY_LOG_FLUSHED);
+  assert(buffer_counter != 0);
   assert(__xray_log_flushLog() == XRayLogFlushStatus::XRAY_LOG_FLUSHED);
----------------
Why not be more specific and assert it's equal to 1 exactly?


https://reviews.llvm.org/D43495





More information about the llvm-commits mailing list