Skip to content

Add thread annotation macros#2

Merged
emersonknapp merged 3 commits intoros2:masterfrom
emersonknapp:thread-annotation-macros
Mar 15, 2019
Merged

Add thread annotation macros#2
emersonknapp merged 3 commits intoros2:masterfrom
emersonknapp:thread-annotation-macros

Conversation

@emersonknapp
Copy link
Copy Markdown
Contributor

@emersonknapp emersonknapp commented Feb 26, 2019 •
edited
Loading

For clang thread safety analysis https://clang.llvm.org/docs/ThreadSafetyAnalysis.html

Related to ros2/ros2#664

@emersonknapp emersonknapp force-pushed the thread-annotation-macros branch from 5d47939 to 42f6d28 Compare February 26, 2019 18:50
thomas-moulard
thomas-moulard approved these changes Feb 26, 2019
View reviewed changes
Copy link
Copy Markdown

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

This mainly LGTM, we should probably have a no-op test which is just including the header so that we can detect compilation errors in this package w/o having to pull that in a dependency and discover there that it actually does not work.

@emersonknapp emersonknapp force-pushed the thread-annotation-macros branch 2 times, most recently from cc02ae9 to 720c5be Compare February 26, 2019 18:55
@thomas-moulard
Copy link
Copy Markdown

thomas-moulard commented Feb 26, 2019

Awesome - SGTM!

I think it's good we don't force everyone to compile with -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS but we should also try not to forget to pass that in each package we convert as needed...

@emersonknapp emersonknapp mentioned this pull request Feb 26, 2019
Merged
@thomas-moulard
Copy link
Copy Markdown

thomas-moulard commented Feb 26, 2019

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

@emersonknapp emersonknapp force-pushed the thread-annotation-macros branch from 8819e9f to 87820aa Compare February 27, 2019 00:12
@thomas-moulard
Copy link
Copy Markdown

thomas-moulard commented Feb 27, 2019

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

@thomas-moulard thomas-moulard mentioned this pull request Feb 27, 2019
Closed
3 tasks
tfoote
tfoote reviewed Feb 27, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

A little bit more documentation would be great. And it looks like we're only at ~25% coverage of the macros in the test. It would be good to try to increase that at least a little bit as I think getting to at least the majority of them would be relatively straight forward.

@emersonknapp
Copy link
Copy Markdown
Contributor Author

emersonknapp commented Feb 27, 2019

I'll make a go at hitting all the macros in unit test. The first pass was mainly to make sure that including the header doesn't break the build

@emersonknapp emersonknapp self-assigned this Mar 7, 2019
@emersonknapp emersonknapp force-pushed the thread-annotation-macros branch from e37de87 to 888493c Compare March 8, 2019 03:43
@emersonknapp
Copy link
Copy Markdown
Contributor Author

emersonknapp commented Mar 8, 2019

Updated the documentation, and updated tests to cover as many of the macros as possible, and left detailed analysis/documentation of the ones that could not be tested.

@emersonknapp emersonknapp force-pushed the thread-annotation-macros branch from 31b92f8 to 7f507db Compare March 8, 2019 04:32
@emersonknapp emersonknapp mentioned this pull request Mar 8, 2019
Merged
wjwwood
wjwwood reviewed Mar 8, 2019
View reviewed changes
@emersonknapp
Copy link
Copy Markdown
Contributor Author

emersonknapp commented Mar 12, 2019

Any feedback on latest revision?

@emersonknapp emersonknapp mentioned this pull request Mar 13, 2019
Merged
tfoote
tfoote approved these changes Mar 13, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. The changes lgtm. Starting CI

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

@emersonknapp
Copy link
Copy Markdown
Contributor Author

emersonknapp commented Mar 13, 2019

Latest commit should fix CI issues - was able to reproduce locally by doing clang w/ libstdc++ - I had been only build gcc and clang+libcxx on the latest versions

@tfoote
Copy link
Copy Markdown
Contributor

tfoote commented Mar 14, 2019

Retriggered CI:

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

@emersonknapp
Copy link
Copy Markdown
Contributor Author

emersonknapp commented Mar 14, 2019

@tfoote with the build fix, are we ok to merge this?

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Mar 15, 2019

All commits have to be signed-off, the DCO check is currently failing.

@emersonknapp emersonknapp force-pushed the thread-annotation-macros branch from 093685a to 0840c25 Compare March 15, 2019 17:38
Emerson Knapp added 3 commits March 15, 2019 18:00
Add thread annotation macros, tests for same, and documentation
15e56c3 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Change macro namespace to RCPPUTILS_TSA
cadd706 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Allow build for clang w/ libstdc++, add test for NO_THREAD_SAFETY_ANA…
d3c1c8a 
…LYSIS, and fix argument count of ASSERT_CAPABILITY macros

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp emersonknapp force-pushed the thread-annotation-macros branch from 0840c25 to d3c1c8a Compare March 15, 2019 18:00
@emersonknapp emersonknapp merged commit 028a6cf into ros2:master Mar 15, 2019
@tfoote tfoote removed the in review label Mar 15, 2019
@emersonknapp emersonknapp deleted the thread-annotation-macros branch March 15, 2019 18:18
@emersonknapp emersonknapp mentioned this pull request Sep 5, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wjwwood wjwwood wjwwood left review comments

@thomas-moulard thomas-moulard thomas-moulard approved these changes

@tfoote tfoote tfoote approved these changes

Assignees

@emersonknapp emersonknapp

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@emersonknapp @thomas-moulard @tfoote @wjwwood