[PATCH] D39175: [XRay][compiler-rt] Remove C++ STL from the buffer queue implementation

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: compiler-rt/lib/xray/xray_buffer_queue.cc:68
+    if (*I == Buf.Buffer) {
+      Found = true;
+      break;
----------------
pelikan wrote:
> I'm slightly worried about what happens if we mark this "true", then a signal happens, the handler will be instrumented and this thing will be reentered and succeeds past the mutex-guarded thing at the bottom.  We will resume afterwards and continue with incrementing First yet again, possibly wasting a buffer.
> 
> SpinMutex is clever enough to yield in case there is a different thread waiting (which isn't here) but not to be aware of signal processing.  I think this should be addressed in a different diff though, using a lock-less mechanism.  Shouldn't be too tricky for this simple case.
This 'Found' is in the process of just finding whether we own the buffer. It's also outside the critical section for a reason, since we never update OwnedBuffers anywhere outside the constructor.

There are potential issues with the signal-safety of this function, but we have another synchronisation mechanism (recursion guard) that must guard us against having an instrumented signal handler coming in while already in the process of handling the event.


================
Comment at: compiler-rt/lib/xray/xray_buffer_queue.h:39
+    Buffer Buffer;
+    bool Used = false;
+  };
----------------
pelikan wrote:
> I was having a bit of a problem realizing whether this means "is being used" or "has been used".  Maybe you can think of a small comment to improve this, maybe not.
I'm not sure why the distinction matters.


https://reviews.llvm.org/D39175