Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions .github/workflows/cmake_ubuntu_sanitizers.yml
Original file line number Diff line number Diff line change
@@ -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
5 changes: 5 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
31 changes: 31 additions & 0 deletions cmake/sanitizers.cmake
Original file line number Diff line number Diff line change
@@ -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()
12 changes: 7 additions & 5 deletions include/behaviortree_cpp/blackboard.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string, std::shared_ptr<Entry>> storage_;
std::weak_ptr<Blackboard> parent_bb_;
Expand Down Expand Up @@ -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);
Expand All @@ -207,15 +207,15 @@ 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);
if(it == storage_.end())
{
// create a new entry
Any new_value(value);
lock.unlock();
storage_lock.unlock();
std::shared_ptr<Blackboard::Entry> entry;
// if a new generic port is created with a string, it's type should be AnyTypeAllowed
if constexpr(std::is_same_v<std::string, T>)
Expand All @@ -228,7 +228,7 @@ inline void Blackboard::set(const std::string& key, const T& value)
GetAnyFromStringFunctor<T>());
entry = createEntryImpl(key, new_port);
}
lock.lock();
storage_lock.lock();

entry->value = new_value;
entry->sequence_id++;
Expand All @@ -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;
Expand Down
33 changes: 20 additions & 13 deletions include/behaviortree_cpp/utils/timer_queue.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,7 @@ class Semaphore

void notify()
{
{
std::lock_guard<std::mutex> lock(m_mtx);
m_count++;
}
m_count.fetch_add(1);
m_cv.notify_one();
}

Expand All @@ -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;
}

Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -174,7 +181,7 @@ class TimerQueue

void run()
{
while(!m_finish)
while(!m_finish.load())
{
auto end = calcWaitTime();
if(end.first)
Expand Down Expand Up @@ -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
Expand Down
20 changes: 12 additions & 8 deletions src/blackboard.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,13 @@ Blackboard::getEntry(const std::string& key) const
return rootBlackboard()->getEntry(key.substr(1, key.size() - 1));
}

std::unique_lock<std::mutex> lock(mutex_);
auto it = storage_.find(key);
if(it != storage_.end())
{
return it->second;
std::unique_lock<std::mutex> 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())
Expand Down Expand Up @@ -130,7 +132,7 @@ std::vector<StringView> Blackboard::getKeys() const

void Blackboard::clear()
{
std::unique_lock<std::mutex> lock(mutex_);
std::unique_lock<std::mutex> storage_lock(storage_mutex_);
storage_.clear();
}

Expand All @@ -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<std::mutex> lk1(storage_mutex_, std::defer_lock);
std::unique_lock<std::mutex> lk2(dst.storage_mutex_, std::defer_lock);
std::lock(lk1, lk2);

// keys that are not updated must be removed.
std::unordered_set<std::string> keys_to_remove;
Expand Down Expand Up @@ -212,7 +216,7 @@ Blackboard::Ptr Blackboard::parent()
std::shared_ptr<Blackboard::Entry> Blackboard::createEntryImpl(const std::string& key,
const TypeInfo& info)
{
std::unique_lock<std::mutex> lock(mutex_);
std::unique_lock<std::mutex> 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

Expand Down
18 changes: 6 additions & 12 deletions src/tree_node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> execute_later(nullptr, [&](...) {
auto t2 = steady_clock::now();
if(monitor_tick)
{
monitor_tick(*this, new_status, duration_cast<microseconds>(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<microseconds>(t2 - t1));
}
}
}

Expand Down
16 changes: 14 additions & 2 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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=$<TARGET_FILE_DIR:behaviortree_cpp_test>;$ENV{PATH}"
)
else()
gtest_discover_tests(behaviortree_cpp_test)
endif()

endif()

target_include_directories(behaviortree_cpp_test PRIVATE include)
Expand Down
3 changes: 3 additions & 0 deletions tests/gtest_blackboard.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,8 @@ TEST(BlackboardTest, CheckTypeSafety)
ASSERT_TRUE(is);
}

#ifndef USE_SANITIZE_THREAD

TEST(BlackboardTest, AnyPtrLocked)
{
auto blackboard = Blackboard::create();
Expand Down Expand Up @@ -346,6 +348,7 @@ TEST(BlackboardTest, AnyPtrLocked)
ASSERT_NE(cycles, value);
}
}
#endif

TEST(BlackboardTest, SetStringView)
{
Expand Down
2 changes: 1 addition & 1 deletion tests/gtest_decorator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
Loading