search

ros / class_loader

ROS-independent library for dynamic class (i.e. plugin) introspection and loading from runtime libraries
http://www.ros.org/wiki/class_loader
34 stars 95 forks source link

Thread and Address Sanitizer CI #198

Open tylerjw opened 2 years ago

tylerjw commented 2 years ago

Signed-off-by: Tyler Weaver tylerjw@gmail.com

This adds a github actions CI job for tsan and asan.

This can be tested by building locally with the new cmake variables added. For example building and testing with colcon can be done like this:

tsan:

colcon build --cmake-args -DENABLE_SANITIZER_THREAD=ON -DCMAKE_BUILD_TYPE=Debug
colcon test --event-handlers console_direct+

asan:

colcon build --cmake-args -DENABLE_SANITIZER_ADDRESS=ON -DCMAKE_BUILD_TYPE=Debug
colcon test --event-handlers console_direct+

I'm creating this with the hope that we can fix the errors found by ASAN and TSAN here so that projects that depend on this library can use ASAN and TSAN themselves.

Locally I've applied this change over https://github.com/ros/class_loader/pull/186 and was able to observe it reduced the number of errors found by ASAN.

tylerjw commented 2 years ago

Note that we should probably figure out how to run these changes in CI from this PR before merging this (to test the CI config works as expected). Also, this probably shouldn't be merged until asan and tsan tests pass (they don't locally).

The reason I created this was to make it easier to test this project locally with asan and tsan to begin fixing memory errors and race conditions.

gbiggs commented 2 years ago

I think merging it while the asan and tsan tests don't pass is OK, but I agree with the need to have it running from CI.

tylerjw commented 2 years ago

I'll see if I can get it to run in my fork to test if it runs.

tylerjw commented 2 years ago

@gbiggs I created this PR within my own fork to test that these work as expected: https://github.com/tylerjw/class_loader/pull/1

tylerjw commented 2 years ago

I spent today trying to figure out these memory leaks and while I don't have a PR yet I'm getting closer using a combination of asan and valgrind. I hope to have a PR soon that cleans up all manual memory management.

tylerjw commented 2 years ago

I'm a little confused by the heavy usage of recursive mutex, does anyone know if there are specific use cases that explain why this repo uses recursive mutexes and if hopefully those use cases are reflected in the tests?

tylerjw commented 1 year ago

I pushed a rebase of this to see if the latest merged changes fix the bug found by asan.