From 95caa57fcce4393b9d6006c02cb349db1702a00f Mon Sep 17 00:00:00 2001 From: Emerson Knapp Date: Mon, 5 Apr 2021 12:17:30 -0700 Subject: [PATCH 1/3] Enable thread safety analysis for rosbag2_cpp and add annotations in TimeControllerClock Signed-off-by: Emerson Knapp --- rosbag2_cpp/CMakeLists.txt | 2 +- .../clocks/time_controller_clock.cpp | 34 ++++++++++++------- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/rosbag2_cpp/CMakeLists.txt b/rosbag2_cpp/CMakeLists.txt index d1415071e7..2c82b5974c 100644 --- a/rosbag2_cpp/CMakeLists.txt +++ b/rosbag2_cpp/CMakeLists.txt @@ -14,7 +14,7 @@ if(NOT CMAKE_CXX_STANDARD) endif() if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang") - add_compile_options(-Wall -Wextra -Wpedantic -Werror) + add_compile_options(-Wall -Wextra -Wpedantic -Werror -Wthread-safety) endif() # Windows supplies macros for min and max by default. We should only use min and max from stl diff --git a/rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp b/rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp index dc00bf14d2..409b54412e 100644 --- a/rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp +++ b/rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp @@ -38,7 +38,9 @@ class TimeControllerClockImpl std::chrono::steady_clock::time_point steady; }; - TimeControllerClockImpl() = default; + explicit TimeControllerClockImpl(PlayerClock::NowFunction now_fn) + : now_fn(now_fn) + {} virtual ~TimeControllerClockImpl() = default; template @@ -48,33 +50,35 @@ class TimeControllerClockImpl } rcutils_time_point_value_t steady_to_ros(std::chrono::steady_clock::time_point steady_time) + RCPPUTILS_TSA_REQUIRES(state_mutex) { return reference.ros + static_cast( rate * duration_nanos(steady_time - reference.steady)); } std::chrono::steady_clock::time_point ros_to_steady(rcutils_time_point_value_t ros_time) + RCPPUTILS_TSA_REQUIRES(state_mutex) { const auto diff_nanos = static_cast( (ros_time - reference.ros) / rate); return reference.steady + std::chrono::nanoseconds(diff_nanos); } - std::mutex mutex; - std::condition_variable cv; - PlayerClock::NowFunction now_fn RCPPUTILS_TSA_GUARDED_BY(mutex); - double rate RCPPUTILS_TSA_GUARDED_BY(mutex) = 1.0; - TimeReference reference RCPPUTILS_TSA_GUARDED_BY(mutex); + const PlayerClock::NowFunction now_fn; + + std::mutex state_mutex; + std::condition_variable cv RCPPUTILS_TSA_GUARDED_BY(state_mutex); + double rate RCPPUTILS_TSA_GUARDED_BY(state_mutex) = 1.0; + TimeReference reference RCPPUTILS_TSA_GUARDED_BY(state_mutex); }; TimeControllerClock::TimeControllerClock( rcutils_time_point_value_t starting_time, double rate, NowFunction now_fn) -: impl_(std::make_unique()) +: impl_(std::make_unique(now_fn)) { - std::lock_guard lock(impl_->mutex); - impl_->now_fn = now_fn; + std::lock_guard lock(impl_->state_mutex); impl_->reference.ros = starting_time; impl_->reference.steady = impl_->now_fn(); impl_->rate = rate; @@ -85,23 +89,27 @@ TimeControllerClock::~TimeControllerClock() rcutils_time_point_value_t TimeControllerClock::now() const { - std::lock_guard lock(impl_->mutex); + std::lock_guard lock(impl_->state_mutex); return impl_->steady_to_ros(impl_->now_fn()); } bool TimeControllerClock::sleep_until(rcutils_time_point_value_t until) { { - std::unique_lock lock(impl_->mutex); + // NOTE: we could accomplish this by simply creating a unique_lock + // but Clang Thread Safety Analysis does not know the unique_lock construction. + // The lock_guard informs static analysis, and the unique_lock adopts its lock. + std::lock_guard lock(impl_->state_mutex); + std::unique_lock ulock(impl_->state_mutex, std::adopt_lock); const auto steady_until = impl_->ros_to_steady(until); - impl_->cv.wait_until(lock, steady_until); + impl_->cv.wait_until(ulock, steady_until); } return now() >= until; } double TimeControllerClock::get_rate() const { - std::lock_guard lock(impl_->mutex); + std::lock_guard lock(impl_->state_mutex); return impl_->rate; } From fd821ae2a82fce606488f18bd0719d4a8626763d Mon Sep 17 00:00:00 2001 From: Emerson Knapp Date: Mon, 5 Apr 2021 14:35:03 -0700 Subject: [PATCH 2/3] Only enable thread-safety on Clang, not on GCC Signed-off-by: Emerson Knapp --- rosbag2_cpp/CMakeLists.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/rosbag2_cpp/CMakeLists.txt b/rosbag2_cpp/CMakeLists.txt index 2c82b5974c..d155273be3 100644 --- a/rosbag2_cpp/CMakeLists.txt +++ b/rosbag2_cpp/CMakeLists.txt @@ -14,7 +14,10 @@ if(NOT CMAKE_CXX_STANDARD) endif() if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang") - add_compile_options(-Wall -Wextra -Wpedantic -Werror -Wthread-safety) + add_compile_options(-Wall -Wextra -Wpedantic -Werror) +endif() +if(CMAKE_CXX_COMPILER_ID MATCHES "Clang") + add_compile_options(-Wthread-safety) endif() # Windows supplies macros for min and max by default. We should only use min and max from stl From e2ab94461df7bc463acd5fd82bb8bed359b3c3b5 Mon Sep 17 00:00:00 2001 From: Emerson Knapp Date: Mon, 5 Apr 2021 16:47:38 -0700 Subject: [PATCH 3/3] Windows-friendly version of the unique_lock Signed-off-by: Emerson Knapp --- .../clocks/time_controller_clock.cpp | 25 ++++++++++++++----- 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp b/rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp index 409b54412e..a51eb95938 100644 --- a/rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp +++ b/rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp @@ -21,6 +21,23 @@ #include "rosbag2_cpp/clocks/time_controller_clock.hpp" #include "rosbag2_cpp/types.hpp" +namespace +{ +/** + * Trivial std::unique_lock wrapper providing constructor that allows Clang Thread Safety Analysis. + * The std::unique_lock does not have these annotations. + */ +class RCPPUTILS_TSA_SCOPED_CAPABILITY TSAUniqueLock : public std::unique_lock +{ +public: + explicit TSAUniqueLock(std::mutex & mu) RCPPUTILS_TSA_ACQUIRE(mu) + : std::unique_lock(mu) + {} + + ~TSAUniqueLock() RCPPUTILS_TSA_RELEASE() {} +}; +} // namespace + namespace rosbag2_cpp { @@ -96,13 +113,9 @@ rcutils_time_point_value_t TimeControllerClock::now() const bool TimeControllerClock::sleep_until(rcutils_time_point_value_t until) { { - // NOTE: we could accomplish this by simply creating a unique_lock - // but Clang Thread Safety Analysis does not know the unique_lock construction. - // The lock_guard informs static analysis, and the unique_lock adopts its lock. - std::lock_guard lock(impl_->state_mutex); - std::unique_lock ulock(impl_->state_mutex, std::adopt_lock); + TSAUniqueLock lock(impl_->state_mutex); const auto steady_until = impl_->ros_to_steady(until); - impl_->cv.wait_until(ulock, steady_until); + impl_->cv.wait_until(lock, steady_until); } return now() >= until; }