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

WIP: add clang thread safety annotations #1419

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

seanm
Copy link
Contributor

@seanm seanm commented Jan 1, 2024

libusb/libusbi.h Outdated
@@ -44,6 +44,17 @@
#define static_assert(cond, msg) _Static_assert(cond, msg)
#endif

/* https://clang.llvm.org/docs/ThreadSafetyAnalysis.html */
#ifdef __clang
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is how it is suggested by ThreadSafetyAnalysis.html

Suggested change
#ifdef __clang
#if defined(__clang__) && (!defined(SWIG))

Copy link
Member

@Youw Youw left a comment

Choose a reason for hiding this comment

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

shouldn't we explicitly add -Wthread-safety for this to kick in?
should be great to have those checks on CI if we're to add them on code level


generally looks good, don't see any code changes, only extra annotations

@mcuee mcuee added macOS core Related to common codes labels Jan 1, 2024
@seanm
Copy link
Contributor Author

seanm commented Jan 1, 2024

shouldn't we explicitly add -Wthread-safety for this to kick in?

Yes. I haven't even tried that yet, because also required is that the threading functions be annotated too. pthread.h on my OS is not. It might be possible to annotate the wrappers in threads_posix.h though, which I will try...

generally looks good, don't see any code changes, only extra annotations

Correct. The annotations also require that the mutex is declared before variables it protects, which is why I moved some things around.

@seanm
Copy link
Contributor Author

seanm commented Jan 1, 2024

@mcuee not sure the macOS tag is appropriate, there's nothing macOS specific here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Related to common codes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants