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

Usage of atomic_thread_fence breaks TSAN #1275

Closed
romasandu-gaijin opened this issue Sep 21, 2024 · 3 comments
Closed

Usage of atomic_thread_fence breaks TSAN #1275

romasandu-gaijin opened this issue Sep 21, 2024 · 3 comments

Comments

@romasandu-gaijin
Copy link
Contributor

The current state of the art thread sanitizer machinery is not capable of handling thread fences decoupled from atomic operations. And as a multi-threaded system, Jolt surely needs to be testable with TSAN (without getting false-positives due to thread fences being skipped).

Jolt uses atomic_thread_fences in two places, JobSystem.h (Job::Release) and Reference.h (RefTarget::Release). In both places, replacing them with an acq_rel fetch_sub would be (pretty much) semantically equivalent and testable with TSAN, but result in worse codegen on ARM on the hot path (yes, I checked it).

Hence, dirty ifdefs for TSAN builds seem like the only way to go here. I can do a PR if this idea is approved.

@jrouwe
Copy link
Owner

jrouwe commented Sep 21, 2024

Hello,

I tried making TSAN work in the (distant) past and couldn't get it to work. If you think you can make it work then yes please make a PR (make sure that it is part of the github build action and runs the unit tests)!

@jrouwe
Copy link
Owner

jrouwe commented Sep 21, 2024

B.t.w. I'm hoping this doesn't require lots of changes / annotations?

@romasandu-gaijin
Copy link
Contributor Author

@jrouwe we are already successfully running autotests that use Jolt w/ TSAN, but after manually trying to run it on a fresh Ubuntu setup I have noticed that GCC complains about thread fences (and rightly so). Alright, I'll do a PR sometime around this week.

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

No branches or pull requests

2 participants