Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unique_lock implementation with clang thread safety annotations #180

Merged
merged 5 commits into from
Sep 6, 2023

Conversation

@emersonknapp
Copy link
Contributor Author

Probably needs a test

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having this in rcpputil makes sense, test would be ideal.

@clalancette
Copy link
Contributor

And just for verification, it looks like using this instead of std::unique_lock in rmw_fastrtps makes a difference: ros2/rmw_fastrtps#712 (comment) (there are about 140 fewer violations than on e.g. https://ci.ros2.org/view/nightly/job/nightly_linux_clang_libcxx/1629/)

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a possible bug inline.

As far as a test goes, I'm actually not entirely sure what we would test here. More-or-less it would be testing whether std::unique_lock works. Any thoughts from @emersonknapp or @fujitatomoya on what kind of test you think we should do?

include/rcpputils/unique_lock.hpp Outdated Show resolved Hide resolved
@fujitatomoya
Copy link
Collaborator

@clalancette that is actually good question. i was thinking that we can simulate https://ci.ros2.org/view/nightly/job/nightly_linux_clang_libcxx/1628/clang/new/source.cf52365c-c9dc-4c3e-8639-55ffa17e1b64/#115 but it actually is the test for RCPPUTILS_TSA_SCOPED_CAPABILITY.

we could test rcpputils::unique_lock works, but i dont think that is even necessary either.

@clalancette
Copy link
Contributor

@emersonknapp When you get a chance, can you rebase this to fix the DCO bot?

emersonknapp and others added 5 commits September 5, 2023 16:29
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Contributor Author

Added a simple compile-test that we can use multiple types of mutex.

@emersonknapp
Copy link
Contributor Author

Pulls: #180
Gist: https://gist.githubusercontent.com/emersonknapp/c4a008cb46d56f9548cffbcbaefac5a1/raw/22101fab9d965b83665b049428add3a1a64e682a/ros2.repos
BUILD args: --packages-above-and-dependencies rcpputils
TEST args: --packages-above rcpputils
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12608

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette clalancette merged commit a0fa6e3 into rolling Sep 6, 2023
3 checks passed
@clalancette clalancette deleted the emersonknapp/tsa-unique-lock branch September 6, 2023 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants