Add helgrind dubious locks suppresses

joelpinto · duartenfonseca · commit d58421766206 · 2025-02-27T14:58:16.000Z
Summary
Add helgrind dubious POSIX API misuse suppresses and update condition variable ordering.
Details
When using condition variables and notifying threads, the signaling thread can
unlock the associated mutex before or after the signaling (or might not even acquire
the mutex if no predicate state needs to be updated).
Despite the fact that signaling with the associated mutex acquired might be considered
a pessimization by some references, valgrind documentation states that helgrind will
consider an error when a thread signals through pthread_cond_signal or
pthread_cond_broadcast and the mutex associated with the signal is not locked
at the time the it is sent as it is considered that it can cause subtle race conditions and
unpredictable behavior.
This commit updates the pessimization removal added in the network tests and
suppresses all related POSIX API misuses from boost library.
An alternative could be disabling the report-signal-unlocked helgrind flag.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -145,7 +145,7 @@ if (NOT MSVC)
         endif ()
 
         if (VALGRIND_TYPE STREQUAL "helgrind")
-            set(TEST_ENTRYPOINT ${TEST_ENTRYPOINT} --suppressions=${VALGRIND_SUPPRESS_FILE} --gen-suppressions=all --log-file=${VALGRIND_LOGS_DIR}/test_name.out)
+            set(TEST_ENTRYPOINT ${TEST_ENTRYPOINT} --show-error-list=yes --suppressions=${VALGRIND_SUPPRESS_FILE} --gen-suppressions=all --log-file=${VALGRIND_LOGS_DIR}/test_name.out)
         endif ()
 
         if (VALGRIND_TYPE STREQUAL "massif")
diff --git a/test/network_tests/offer_tests/offer_test_multiple_offerings.cpp b/test/network_tests/offer_tests/offer_test_multiple_offerings.cpp
@@ -35,13 +35,15 @@ class common {
     std::condition_variable condition_availability_;
     std::atomic_bool availability_;
     std::atomic_bool msg_sent_;
+    std::mutex mutex_;
 };
 
 void common::on_availability(service_t _service_id, instance_t _instance_id, bool _is_available) {
     if (_service_id == service_id_ && _instance_id == instance_id_) {
         if (_is_available) {
             // NOTE: Using the most strict memory ordering operation.
             // Refer to https://en.cppreference.com/w/cpp/atomic/memory_order for possible options
+            std::lock_guard<std::mutex> lock(mutex_);
             availability_.store(true);
             condition_availability_.notify_one();
         } else {
@@ -160,7 +162,6 @@ class client : common {
 private:
     std::condition_variable condition_message_received_;
     std::condition_variable condition_message_sent_;
-    std::mutex mutex_;
 
     std::atomic_bool message_received_;
 
@@ -228,9 +229,6 @@ class server : common {
     void on_availability(service_t _service_id, instance_t _instance_id, bool _is_available) {
         common::on_availability(_service_id, _instance_id, _is_available);
     }
-
-private:
-    std::mutex mutex_;
 };
 
 class vsomeip_daemon {
diff --git a/test/network_tests/subscribe_notify_tests/subscribe_notify_test_service.cpp b/test/network_tests/subscribe_notify_tests/subscribe_notify_test_service.cpp
@@ -129,8 +129,8 @@ class subscribe_notify_test_service : public vsomeip_utilities::base_logger {
         if (_state == vsomeip::state_type_e::ST_REGISTERED) {
             std::lock_guard<std::mutex> its_lock(mutex_);
             wait_until_registered_ = false;
+            condition_.notify_one();
         }
-        condition_.notify_one();
     }
 
     void on_availability(vsomeip::service_t _service,
@@ -157,8 +157,8 @@ class subscribe_notify_test_service : public vsomeip_utilities::base_logger {
                                 return v.second;})) {
                 std::lock_guard<std::mutex> its_lock(mutex_);
                 wait_until_other_services_available_ = false;
+                condition_.notify_one();
             }
-            condition_.notify_one();
         }
     }
 
@@ -206,9 +206,10 @@ class subscribe_notify_test_service : public vsomeip_utilities::base_logger {
             // notify the notify thread to start sending out notifications
             std::lock_guard<std::mutex> its_lock(notify_mutex_);
             wait_for_notify_ = false;
+            notify_condition_.notify_one();
             notified = true;
         }
-        notify_condition_.notify_one();
+
         return true;
     }
 
@@ -245,8 +246,8 @@ class subscribe_notify_test_service : public vsomeip_utilities::base_logger {
             if(all_notifications_received()) {
                 std::lock_guard<std::mutex> its_lock(stop_mutex_);
                 wait_for_stop_ = false;
+                stop_condition_.notify_one();
             }
-            stop_condition_.notify_one();
         }
     }
 
@@ -396,8 +397,8 @@ class subscribe_notify_test_service : public vsomeip_utilities::base_logger {
         {
             std::lock_guard<std::mutex> its_lock(mutex_);
             wait_until_notified_from_other_services_ = false;
+            condition_.notify_one();
         }
-        condition_.notify_one();
 
         stop_offer();
 
diff --git a/test/network_tests/suspend_resume_tests/suspend_resume_test_service.cpp b/test/network_tests/suspend_resume_tests/suspend_resume_test_service.cpp
@@ -57,7 +57,10 @@ class suspend_resume_test_service : public vsomeip_utilities::base_logger {
     void stop() {
 
         is_running_ = false;
-        sr_cv_.notify_one();
+        {
+            std::lock_guard<std::mutex> its_lock(sr_mutex_);
+            sr_cv_.notify_one();
+        }
 
         app_->stop();
 
@@ -143,10 +146,16 @@ class suspend_resume_test_service : public vsomeip_utilities::base_logger {
 
                 switch (its_control_byte) {
                 case TEST_SUSPEND:
-                    sr_cv_.notify_one();
+                    {
+                        std::lock_guard<std::mutex> its_lock(sr_mutex_);
+                        sr_cv_.notify_one();
+                    }
                     break;
                 case TEST_STOP:
-                    cv_.notify_one();
+                    {
+                        std::lock_guard<std::mutex> its_lock(mutex_);
+                        cv_.notify_one();
+                    }
                     break;
                 default:;
                 }
diff --git a/zuul/network-tests/valgrind/helgrind.supp b/zuul/network-tests/valgrind/helgrind.supp
@@ -22,3 +22,91 @@
    ...
    obj:/usr/lib/libdlt.so.*
 }
+{
+   # Dubious: lock is not held by any thread when closing socket.
+   deregister_application
+   Helgrind:Misc
+   obj:/usr/libexec/valgrind/vgpreload_helgrind-amd64-linux.so
+   fun:_ZN5boost4asio6detail13epoll_reactor21deregister_descriptorEiRPNS2_16descriptor_stateEb
+   ...
+   obj:/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.30
+   obj:/usr/libexec/valgrind/vgpreload_helgrind-amd64-linux.so
+   fun:start_thread
+   fun:clone
+}
+{
+   # Dubious: lock is not held by any thread on strand handler dispatch.
+   strand_dispatch
+   Helgrind:Misc
+   obj:/usr/libexec/valgrind/vgpreload_helgrind-amd64-linux.so
+   fun:_ZN5boost4asio6detail14strand_service11do_dispatchERPNS2_11strand_implEPNS1_19scheduler_operationE
+   ...
+   obj:/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.30
+   obj:/usr/libexec/valgrind/vgpreload_helgrind-amd64-linux.so
+   fun:start_thread
+   fun:clone
+}
+{
+   # Dubious: lock is not held by any thread when a job is posted to a strand.
+   initiate_strand
+   Helgrind:Misc
+   obj:/usr/libexec/valgrind/vgpreload_helgrind-amd64-linux.so
+   ...
+   fun:initiate<boost::asio::io_context::strand::initiate_post, std::_Bind<*>, boost::asio::io_context::strand*>
+   ...
+   obj:/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.30
+   obj:/usr/libexec/valgrind/vgpreload_helgrind-amd64-linux.so
+   fun:start_thread
+   fun:clone
+}
+{
+   # Dubious: lock is not held by any thread when boost io scheduler tries to execute a thread.
+   unlock_and_signal_one
+   Helgrind:Misc
+   obj:/usr/libexec/valgrind/vgpreload_helgrind-amd64-linux.so
+   fun:unlock_and_signal_one<boost::asio::detail::conditionally_enabled_mutex::scoped_lock>
+   ...
+   fun:_ZN5boost4asio6detail9scheduler*
+   ...
+}
+{
+   # Dubious: lock is not held by any thread when boost io scheduler tries to execute a thread.
+   maybe_unlock_and_signal_one
+   Helgrind:Misc
+   obj:/usr/libexec/valgrind/vgpreload_helgrind-amd64-linux.so
+   fun:maybe_unlock_and_signal_one<boost::asio::detail::conditionally_enabled_mutex::scoped_lock>
+   ...
+   fun:_ZN5boost4asio6detail9scheduler*
+   ...
+}
+{
+   # Dubious: lock is not held by any thread when canceling steady timer
+   cancel_steady_timer
+   Helgrind:Misc
+   obj:/usr/libexec/valgrind/vgpreload_helgrind-amd64-linux.so
+   fun:cancel_timer<boost::asio::detail::chrono_time_traits<std::chrono::_V2::steady_clock, boost::asio::wait_traits<std::chrono::_V2::steady_clock> > >
+   fun:cancel
+   ...
+}
+{
+   # Dubious: lock is not held by any thread when canceling steady timer
+   cancel_steady_timer_diff_func_sign
+   Helgrind:Misc
+   obj:/usr/libexec/valgrind/vgpreload_helgrind-amd64-linux.so
+   fun:_ZN5boost4asio6detail13epoll_reactor12cancel_timerINS1_18chrono_time_traitsINSt6chrono3_V212steady_clockENS0_11wait_traitsIS7_EEEEEEmRNS1_11timer_queueIT_EERNSD_14per_timer_data*
+   ...
+   obj:/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.30
+   obj:/usr/libexec/valgrind/vgpreload_helgrind-amd64-linux.so
+   fun:start_thread
+   fun:clone
+}
+{
+   # Dubious: lock is not held by any thread upon beginning of socket asynchronous operation.
+   async_operation_start
+   Helgrind:Misc
+   obj:/usr/libexec/valgrind/vgpreload_helgrind-amd64-linux.so
+   fun:start_op
+   fun:start_op
+   ...
+   obj:/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.30
+}

Original file line number	Diff line number	Diff line change
`@@ -129,8 +129,8 @@ class subscribe_notify_test_service : public vsomeip_utilities::base_logger {`
`129`	`129`	`if (_state == vsomeip::state_type_e::ST_REGISTERED) {`
`130`	`130`	`std::lock_guard<std::mutex> its_lock(mutex_);`
`131`	`131`	`wait_until_registered_ = false;`
	`132`	`+ condition_.notify_one();`
`132`	`133`	`}`
`133`		`- condition_.notify_one();`
`134`	`134`	`}`
`135`	`135`
`136`	`136`	`void on_availability(vsomeip::service_t _service,`
`@@ -157,8 +157,8 @@ class subscribe_notify_test_service : public vsomeip_utilities::base_logger {`
`157`	`157`	`return v.second;})) {`
`158`	`158`	`std::lock_guard<std::mutex> its_lock(mutex_);`
`159`	`159`	`wait_until_other_services_available_ = false;`
	`160`	`+ condition_.notify_one();`
`160`	`161`	`}`
`161`		`- condition_.notify_one();`
`162`	`162`	`}`
`163`	`163`	`}`
`164`	`164`
`@@ -206,9 +206,10 @@ class subscribe_notify_test_service : public vsomeip_utilities::base_logger {`
`206`	`206`	`// notify the notify thread to start sending out notifications`
`207`	`207`	`std::lock_guard<std::mutex> its_lock(notify_mutex_);`
`208`	`208`	`wait_for_notify_ = false;`
	`209`	`+ notify_condition_.notify_one();`
`209`	`210`	`notified = true;`
`210`	`211`	`}`
`211`		`- notify_condition_.notify_one();`
	`212`	`+`
`212`	`213`	`return true;`
`213`	`214`	`}`
`214`	`215`
`@@ -245,8 +246,8 @@ class subscribe_notify_test_service : public vsomeip_utilities::base_logger {`
`245`	`246`	`if(all_notifications_received()) {`
`246`	`247`	`std::lock_guard<std::mutex> its_lock(stop_mutex_);`
`247`	`248`	`wait_for_stop_ = false;`
	`249`	`+ stop_condition_.notify_one();`
`248`	`250`	`}`
`249`		`- stop_condition_.notify_one();`
`250`	`251`	`}`
`251`	`252`	`}`
`252`	`253`
`@@ -396,8 +397,8 @@ class subscribe_notify_test_service : public vsomeip_utilities::base_logger {`
`396`	`397`	`{`
`397`	`398`	`std::lock_guard<std::mutex> its_lock(mutex_);`
`398`	`399`	`wait_until_notified_from_other_services_ = false;`
	`400`	`+ condition_.notify_one();`
`399`	`401`	`}`
`400`		`- condition_.notify_one();`
`401`	`402`
`402`	`403`	`stop_offer();`
`403`	`404`