From 8c3868e43b4f65e602dee19e40371c313b934e7d Mon Sep 17 00:00:00 2001 From: Benoit Jacob Date: Tue, 12 Apr 2022 15:59:40 +0000 Subject: [PATCH 1/4] Make `disconnect` volatile as it's written by a signal handler. --- capture/src/capture.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/capture/src/capture.cpp b/capture/src/capture.cpp index ee126bdd..94afe036 100644 --- a/capture/src/capture.cpp +++ b/capture/src/capture.cpp @@ -25,7 +25,7 @@ #endif -bool disconnect = false; +volatile bool disconnect = false; void SigInt( int ) { From 55ae38a13867680d567872e9de1c9fe26bd9d802 Mon Sep 17 00:00:00 2001 From: Benoit Jacob Date: Tue, 12 Apr 2022 17:33:47 +0000 Subject: [PATCH 2/4] Make disconnect atomic because it's written by a signal handler --- capture/src/capture.cpp | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/capture/src/capture.cpp b/capture/src/capture.cpp index 94afe036..9f92dad7 100644 --- a/capture/src/capture.cpp +++ b/capture/src/capture.cpp @@ -4,6 +4,7 @@ # include #endif +#include #include #include #include @@ -25,11 +26,19 @@ #endif -volatile bool disconnect = false; +// This atomic is written by a signal handler (SigInt). Traditionally that would +// have had to be `volatile sig_atomic_t`, and annoyingly, `bool` was +// technically not allowed there, even though in practice it would work. +// The good thing with C++11 atomics is that we can use atomic instead +// here and be on the actually supported path. +std::atomic disconnect; void SigInt( int ) { - disconnect = true; + // Relaxed order is closest to a traditional `volatile` write. + // We don't need stronger ordering since this signal handler doesn't do + // anything else that would need to be ordered relatively to this. + disconnect.store(true, std::memory_order_relaxed); } [[noreturn]] void Usage() @@ -137,10 +146,15 @@ int main( int argc, char** argv ) const auto t0 = std::chrono::high_resolution_clock::now(); while( worker.IsConnected() ) { - if( disconnect ) + // Relaxed order is sufficient here because `disconnect` is only ever + // set by this thread or by the SigInt handler, and that handler does + // nothing else than storing `disconnect`. + if( disconnect.load( std::memory_order_relaxed ) ) { worker.Disconnect(); - disconnect = false; + // Relaxed order is sufficient because only this thread ever reads + // this value. + disconnect.store(false, std::memory_order_relaxed ); break; } @@ -172,7 +186,9 @@ int main( int argc, char** argv ) const auto dur = std::chrono::high_resolution_clock::now() - t0; if( std::chrono::duration_cast(dur).count() >= seconds ) { - disconnect = true; + // Relaxed order is sufficient because only this thread ever reads + // this value. + disconnect.store(true, std::memory_order_relaxed ); } } } From 331f39e6a51324703ebe19247fe9d76b27ea43e8 Mon Sep 17 00:00:00 2001 From: Benoit Jacob Date: Tue, 12 Apr 2022 17:45:33 +0000 Subject: [PATCH 3/4] rename to s_disconnect and make file-scope static --- capture/src/capture.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/capture/src/capture.cpp b/capture/src/capture.cpp index 9f92dad7..a0c83973 100644 --- a/capture/src/capture.cpp +++ b/capture/src/capture.cpp @@ -31,14 +31,14 @@ // technically not allowed there, even though in practice it would work. // The good thing with C++11 atomics is that we can use atomic instead // here and be on the actually supported path. -std::atomic disconnect; +static std::atomic s_disconnect; void SigInt( int ) { // Relaxed order is closest to a traditional `volatile` write. // We don't need stronger ordering since this signal handler doesn't do // anything else that would need to be ordered relatively to this. - disconnect.store(true, std::memory_order_relaxed); + s_disconnect.store(true, std::memory_order_relaxed); } [[noreturn]] void Usage() @@ -146,15 +146,15 @@ int main( int argc, char** argv ) const auto t0 = std::chrono::high_resolution_clock::now(); while( worker.IsConnected() ) { - // Relaxed order is sufficient here because `disconnect` is only ever + // Relaxed order is sufficient here because `s_disconnect` is only ever // set by this thread or by the SigInt handler, and that handler does - // nothing else than storing `disconnect`. - if( disconnect.load( std::memory_order_relaxed ) ) + // nothing else than storing `s_disconnect`. + if( s_disconnect.load( std::memory_order_relaxed ) ) { worker.Disconnect(); // Relaxed order is sufficient because only this thread ever reads // this value. - disconnect.store(false, std::memory_order_relaxed ); + s_disconnect.store(false, std::memory_order_relaxed ); break; } @@ -188,7 +188,7 @@ int main( int argc, char** argv ) { // Relaxed order is sufficient because only this thread ever reads // this value. - disconnect.store(true, std::memory_order_relaxed ); + s_disconnect.store(true, std::memory_order_relaxed ); } } } From ce1f6d0526e146bef6b29e21ade60b54c27aa57d Mon Sep 17 00:00:00 2001 From: Benoit Jacob Date: Tue, 12 Apr 2022 17:47:14 +0000 Subject: [PATCH 4/4] explicitly initialize as false - hope the compiler optimizes that --- capture/src/capture.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/capture/src/capture.cpp b/capture/src/capture.cpp index a0c83973..04ae243c 100644 --- a/capture/src/capture.cpp +++ b/capture/src/capture.cpp @@ -31,7 +31,7 @@ // technically not allowed there, even though in practice it would work. // The good thing with C++11 atomics is that we can use atomic instead // here and be on the actually supported path. -static std::atomic s_disconnect; +static std::atomic s_disconnect { false }; void SigInt( int ) {