Page MenuHomePhabricator

[XRay][compiler-rt] Remove C++ STL from the buffer queue implementation
ClosedPublic

Authored by dberris on Oct 22 2017, 8:50 PM.

Details

Summary

This change removes the dependency on C++ standard library
types/functions in the implementation of the buffer queue. This is an
incremental step in resolving llvm.org/PR32274.

Diff Detail

Repository
rL LLVM

Event Timeline

dberris created this revision.Oct 22 2017, 8:50 PM
pelikan accepted this revision.Oct 22 2017, 11:16 PM
pelikan added inline comments.
compiler-rt/lib/xray/xray_buffer_queue.cc
68 ↗(On Diff #119809)

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.

compiler-rt/lib/xray/xray_buffer_queue.h
39 ↗(On Diff #119809)

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.

This revision is now accepted and ready to land.Oct 22 2017, 11:16 PM
dberris updated this revision to Diff 119985.Oct 23 2017, 6:21 PM
dberris marked 2 inline comments as done.

Update to add comments.

dberris added inline comments.Oct 23 2017, 6:21 PM
compiler-rt/lib/xray/xray_buffer_queue.cc
68 ↗(On Diff #119809)

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.

compiler-rt/lib/xray/xray_buffer_queue.h
39 ↗(On Diff #119809)

I'm not sure why the distinction matters.

This revision was automatically updated to reflect the committed changes.