diff --git a/.github/workflows/cmake_ubuntu_sanitizers.yml b/.github/workflows/cmake_ubuntu_sanitizers.yml new file mode 100644 index 000000000..c379171a1 --- /dev/null +++ b/.github/workflows/cmake_ubuntu_sanitizers.yml @@ -0,0 +1,69 @@ +name: cmake Ubuntu Sanitizers + +on: + push: + branches: + - master + pull_request: + types: [opened, synchronize, reopened] + +env: + # Customize the CMake build type here (Release, Debug, RelWithDebInfo, etc.) + BUILD_TYPE: Debug + +jobs: + build: + # The CMake configure and build commands are platform agnostic and should work equally + # well on Windows or Mac. You can convert this to a matrix build if you need + # cross-platform coverage. + # See: https://docs.github.com/en/free-pro-team@latest/actions/learn-github-actions/managing-complex-workflows#using-a-build-matrix + runs-on: ${{ matrix.os }} + strategy: + fail-fast: false + matrix: + os: [ubuntu-22.04] + sanitizer: [asan_ubsan, tsan] + + steps: + - uses: actions/checkout@v2 + + - name: Install Conan + id: conan + uses: turtlebrowser/get-conan@main + + - name: Create default profile + run: conan profile detect + + - name: Install conan dependencies + run: conan install conanfile.py -s build_type=${{env.BUILD_TYPE}} --build=missing + + - name: Normalize build type + shell: bash + # The build type is Capitalized, e.g. Release, but the preset is all lowercase, e.g. release. + # There is no built in way to do string manipulations on GHA as far as I know.` + run: echo "BUILD_TYPE_LOWERCASE=$(echo "${BUILD_TYPE}" | tr '[:upper:]' '[:lower:]')" >> $GITHUB_ENV + + - name: Configure CMake + shell: bash + run: | + if [[ "${{ matrix.sanitizer }}" == "asan_ubsan" ]]; then + cmake --preset conan-${{ env.BUILD_TYPE_LOWERCASE }} \ + -DBTCPP_ENABLE_ASAN:BOOL=ON -DBTCPP_ENABLE_UBSAN:BOOL=ON + else + cmake --preset conan-${{ env.BUILD_TYPE_LOWERCASE }} \ + -DBTCPP_ENABLE_TSAN:BOOL=ON + fi + + - name: Build + shell: bash + run: cmake --build --preset conan-${{ env.BUILD_TYPE_LOWERCASE }} + + - name: run test (Linux + Address and Undefined Behavior Sanitizers) + env: + GTEST_COLOR: "On" + ASAN_OPTIONS: "color=always" + UBSAN_OPTIONS: "halt_on_error=1:print_stacktrace=1:color=always" + TSAN_OPTIONS: "color=always" + # There is a known issue with TSAN on recent kernel versions. Without the vm.mmap_rnd_bits=28 + # workaround all binaries with TSan enabled crash with "FATAL: ThreadSanitizer: unexpected memory mapping" + run: sudo sysctl vm.mmap_rnd_bits=28 && ctest --test-dir build/${{env.BUILD_TYPE}} --output-on-failure diff --git a/CMakeLists.txt b/CMakeLists.txt index a0cb7b202..320877a91 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -13,6 +13,9 @@ option(BTCPP_EXAMPLES "Build tutorials and examples" ON) option(BUILD_TESTING "Build the unit tests" ON) option(BTCPP_GROOT_INTERFACE "Add Groot2 connection. Requires ZeroMQ" ON) option(BTCPP_SQLITE_LOGGING "Add SQLite logging." ON) +option(BTCPP_ENABLE_ASAN "Enable Address Sanitizer" OFF) +option(BTCPP_ENABLE_UBSAN "Enable Undefined Behavior Sanitizer" OFF) +option(BTCPP_ENABLE_TSAN "Enable Thread Sanitizer" OFF) option(USE_V3_COMPATIBLE_NAMES "Use some alias to compile more easily old 3.x code" OFF) option(ENABLE_FUZZING "Enable fuzzing builds" OFF) @@ -54,6 +57,8 @@ endif() set(CMAKE_CONFIG_PATH ${CMAKE_MODULE_PATH} "${CMAKE_CURRENT_LIST_DIR}/cmake") list(APPEND CMAKE_MODULE_PATH "${CMAKE_CONFIG_PATH}") +include(sanitizers) + set(BTCPP_LIBRARY ${PROJECT_NAME}) if(NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES) diff --git a/cmake/sanitizers.cmake b/cmake/sanitizers.cmake new file mode 100644 index 000000000..fa5dccc84 --- /dev/null +++ b/cmake/sanitizers.cmake @@ -0,0 +1,31 @@ +if(BTCPP_ENABLE_ASAN OR BTCPP_ENABLE_UBSAN OR BTCPP_ENABLE_TSAN) + if(NOT CMAKE_BUILD_TYPE MATCHES "Debug|RelWithDebInfo") + message(FATAL_ERROR "Sanitizers require debug symbols. Please set CMAKE_BUILD_TYPE to Debug or RelWithDebInfo.") + endif() + add_compile_options(-fno-omit-frame-pointer) +endif() + +# Address Sanitizer and Undefined Behavior Sanitizer can be run at the same time. +# Thread Sanitizer requires its own build. +if(BTCPP_ENABLE_TSAN AND (BTCPP_ENABLE_ASAN OR BTCPP_ENABLE_UBSAN)) + message(FATAL_ERROR "TSan is not compatible with ASan or UBSan. ASan and UBSan can run together, but TSan requires its own separate build.") +endif() + +if(BTCPP_ENABLE_ASAN) + message(STATUS "Address Sanitizer enabled") + add_compile_options(-fsanitize=address) + add_link_options(-fsanitize=address) + endif() + + if(BTCPP_ENABLE_UBSAN) + message(STATUS "Undefined Behavior Sanitizer enabled") + add_compile_options(-fsanitize=undefined) + add_link_options(-fsanitize=undefined) + endif() + + if(BTCPP_ENABLE_TSAN) + message(STATUS "Thread Sanitizer enabled") + add_compile_options(-fsanitize=thread) + add_link_options(-fsanitize=thread) + add_compile_definitions(USE_SANITIZE_THREAD) +endif() diff --git a/include/behaviortree_cpp/blackboard.h b/include/behaviortree_cpp/blackboard.h index 1c3aa96c6..412360bf8 100644 --- a/include/behaviortree_cpp/blackboard.h +++ b/include/behaviortree_cpp/blackboard.h @@ -141,7 +141,7 @@ class Blackboard const Blackboard* rootBlackboard() const; private: - mutable std::mutex mutex_; + mutable std::mutex storage_mutex_; mutable std::recursive_mutex entry_mutex_; std::unordered_map> storage_; std::weak_ptr parent_bb_; @@ -186,7 +186,7 @@ inline T Blackboard::get(const std::string& key) const inline void Blackboard::unset(const std::string& key) { - std::unique_lock lock(mutex_); + std::unique_lock storage_lock(storage_mutex_); // check local storage auto it = storage_.find(key); @@ -207,7 +207,7 @@ inline void Blackboard::set(const std::string& key, const T& value) rootBlackboard()->set(key.substr(1, key.size() - 1), value); return; } - std::unique_lock lock(mutex_); + std::unique_lock storage_lock(storage_mutex_); // check local storage auto it = storage_.find(key); @@ -215,7 +215,7 @@ inline void Blackboard::set(const std::string& key, const T& value) { // create a new entry Any new_value(value); - lock.unlock(); + storage_lock.unlock(); std::shared_ptr entry; // if a new generic port is created with a string, it's type should be AnyTypeAllowed if constexpr(std::is_same_v) @@ -228,7 +228,7 @@ inline void Blackboard::set(const std::string& key, const T& value) GetAnyFromStringFunctor()); entry = createEntryImpl(key, new_port); } - lock.lock(); + storage_lock.lock(); entry->value = new_value; entry->sequence_id++; @@ -239,6 +239,8 @@ inline void Blackboard::set(const std::string& key, const T& value) // this is not the first time we set this entry, we need to check // if the type is the same or not. Entry& entry = *it->second; + storage_lock.unlock(); + std::scoped_lock scoped_lock(entry.entry_mutex); Any& previous_any = entry.value; diff --git a/include/behaviortree_cpp/utils/timer_queue.h b/include/behaviortree_cpp/utils/timer_queue.h index 9132277ba..2a68ba872 100644 --- a/include/behaviortree_cpp/utils/timer_queue.h +++ b/include/behaviortree_cpp/utils/timer_queue.h @@ -23,10 +23,7 @@ class Semaphore void notify() { - { - std::lock_guard lock(m_mtx); - m_count++; - } + m_count.fetch_add(1); m_cv.notify_one(); } @@ -38,8 +35,15 @@ class Semaphore { return false; } - m_count--; + // Only decrement if there is a real count. If we woke because of manualUnlock, + // m_count may be zero and we must not decrement it. + if(m_count > 0) + { + m_count.fetch_sub(1); + } + // Clear the manual unlock flag m_unlock = false; + return true; } @@ -52,7 +56,7 @@ class Semaphore private: std::mutex m_mtx; std::condition_variable m_cv; - unsigned m_count = 0; + std::atomic_uint m_count = 0; std::atomic_bool m_unlock = false; }; } // namespace details @@ -74,15 +78,18 @@ class TimerQueue public: TimerQueue() { - m_th = std::thread([this] { run(); }); + m_finish.store(false); + m_thread = std::thread([this]() { run(); }); } ~TimerQueue() { - m_finish = true; + m_finish.store(true); cancelAll(); - m_checkWork.manualUnlock(); - m_th.join(); + if(m_thread.joinable()) + { + m_thread.join(); + } } //! Adds a new timer @@ -174,7 +181,7 @@ class TimerQueue void run() { - while(!m_finish) + while(!m_finish.load()) { auto end = calcWaitTime(); if(end.first) @@ -239,8 +246,8 @@ class TimerQueue } details::Semaphore m_checkWork; - std::thread m_th; - bool m_finish = false; + std::thread m_thread; + std::atomic_bool m_finish = false; uint64_t m_idcounter = 0; struct WorkItem diff --git a/src/blackboard.cpp b/src/blackboard.cpp index 3b1ba9844..462e7b41e 100644 --- a/src/blackboard.cpp +++ b/src/blackboard.cpp @@ -52,11 +52,13 @@ Blackboard::getEntry(const std::string& key) const return rootBlackboard()->getEntry(key.substr(1, key.size() - 1)); } - std::unique_lock lock(mutex_); - auto it = storage_.find(key); - if(it != storage_.end()) { - return it->second; + std::unique_lock storage_lock(storage_mutex_); + auto it = storage_.find(key); + if(it != storage_.end()) + { + return it->second; + } } // not found. Try autoremapping if(auto parent = parent_bb_.lock()) @@ -130,7 +132,7 @@ std::vector Blackboard::getKeys() const void Blackboard::clear() { - std::unique_lock lock(mutex_); + std::unique_lock storage_lock(storage_mutex_); storage_.clear(); } @@ -157,8 +159,10 @@ void Blackboard::createEntry(const std::string& key, const TypeInfo& info) void Blackboard::cloneInto(Blackboard& dst) const { - std::unique_lock lk1(mutex_); - std::unique_lock lk2(dst.mutex_); + // Lock both mutexes without risking lock-order inversion. + std::unique_lock lk1(storage_mutex_, std::defer_lock); + std::unique_lock lk2(dst.storage_mutex_, std::defer_lock); + std::lock(lk1, lk2); // keys that are not updated must be removed. std::unordered_set keys_to_remove; @@ -212,7 +216,7 @@ Blackboard::Ptr Blackboard::parent() std::shared_ptr Blackboard::createEntryImpl(const std::string& key, const TypeInfo& info) { - std::unique_lock lock(mutex_); + std::unique_lock storage_lock(storage_mutex_); // This function might be called recursively, when we do remapping, because we move // to the top scope to find already existing entries diff --git a/src/tree_node.cpp b/src/tree_node.cpp index 0d4dd66bd..946492318 100644 --- a/src/tree_node.cpp +++ b/src/tree_node.cpp @@ -106,19 +106,13 @@ NodeStatus TreeNode::executeTick() if(!substituted) { using namespace std::chrono; - - auto t1 = steady_clock::now(); - // trick to prevent the compile from reordering the order of execution. See #861 - // This makes sure that the code is executed at the end of this scope - std::shared_ptr execute_later(nullptr, [&](...) { - auto t2 = steady_clock::now(); - if(monitor_tick) - { - monitor_tick(*this, new_status, duration_cast(t2 - t1)); - } - }); - + const auto t1 = steady_clock::now(); new_status = tick(); + const auto t2 = steady_clock::now(); + if(monitor_tick) + { + monitor_tick(*this, new_status, duration_cast(t2 - t1)); + } } } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 9c62b335e..b268235df 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -43,16 +43,28 @@ if(ament_cmake_FOUND) else() + enable_testing() + find_package(GTest REQUIRED) + include(GoogleTest) - enable_testing() add_executable(behaviortree_cpp_test ${BT_TESTS}) - add_test(NAME btcpp_test COMMAND behaviortree_cpp_test) target_link_libraries(behaviortree_cpp_test GTest::gtest GTest::gtest_main) + # gtest_discover_tests queries the test executable for available tests and registers them on ctest individually + # On Windows it needs a little help to find the shared libraries + if(WIN32) + gtest_discover_tests(behaviortree_cpp_test + DISCOVERY_MODE PRE_TEST + DISCOVERY_ENVIRONMENT "PATH=$;$ENV{PATH}" + ) + else() + gtest_discover_tests(behaviortree_cpp_test) + endif() + endif() target_include_directories(behaviortree_cpp_test PRIVATE include) diff --git a/tests/gtest_blackboard.cpp b/tests/gtest_blackboard.cpp index 35be37dab..75c3db6d1 100644 --- a/tests/gtest_blackboard.cpp +++ b/tests/gtest_blackboard.cpp @@ -294,6 +294,8 @@ TEST(BlackboardTest, CheckTypeSafety) ASSERT_TRUE(is); } +#ifndef USE_SANITIZE_THREAD + TEST(BlackboardTest, AnyPtrLocked) { auto blackboard = Blackboard::create(); @@ -346,6 +348,7 @@ TEST(BlackboardTest, AnyPtrLocked) ASSERT_NE(cycles, value); } } +#endif TEST(BlackboardTest, SetStringView) { diff --git a/tests/gtest_decorator.cpp b/tests/gtest_decorator.cpp index 32abcff10..fde33ea9b 100644 --- a/tests/gtest_decorator.cpp +++ b/tests/gtest_decorator.cpp @@ -68,8 +68,8 @@ struct RetryTest : testing::Test struct TimeoutAndRetry : testing::Test { - BT::TimeoutNode timeout_root; BT::RetryNode retry; + BT::TimeoutNode timeout_root; BT::SyncActionTest action; TimeoutAndRetry() : timeout_root("deadline", 9), retry("retry", 1000), action("action")