Skip to content

Conversation

kalmard0
Copy link
Contributor

This was found by compiling and running a toy project using clang's ThreadSanitizer (--tsan) feature.

I haven't investigated it in detail but from the looks of it, renderthread can in theory access m_singleThreaded before its correct value (false) is set. After this change, ThreadSanitizer no longer complains.

This was found by compiling and running a toy project using clang's ThreadSanitizer (`--tsan`) feature.

I haven't investigated it in detail but from the looks of it, `renderthread` can in theory access `m_singleThreaded` before its correct value (`false`) is set. After this change, ThreadSanitizer no longer complains.
@kalmard0 kalmard0 requested a review from bkaradzic as a code owner September 20, 2025 08:18
@bkaradzic
Copy link
Owner

bkaradzic commented Sep 20, 2025

I haven't investigated it in detail but from the looks of it, renderthread can in theory access m_singleThreaded before its correct value (false) is set. After this change,

If you look default value and code above you'll see it doesn't matter.

ThreadSanitizer no longer complains.

Using ThreadSanitizers and similar analyzers is actually pointless if you don't try to understand what's going on. Randomly moving code until they stop warnings is not good idea.

@bkaradzic bkaradzic closed this Sep 20, 2025
@kalmard0
Copy link
Contributor Author

If you look default value and code above you'll see it doesn't matter.

It doesn't matter until someone decides to change the default value, I guess. But I respect your choice, as it's your project.

@mcourteaux
Copy link
Contributor

I disagree. The sanitizer correctly detects a race condition. The fact that the value is overwritten from false to false, doesn't mean the code is inherently doing a suspicious thing. If you stand by the argument that the default value already was false, hence it's not a problem, then you should remove that assignment altogether.

This PR is a strict improvement over the current situation, and there is no harm in merging it. The alternative is removing the assignment.

Sanitizers are useful tools to discover bugs, and deliberately leaving a race condition in, just increases the noise when you work with them. Training people to ignore sanitizer warnings is not a good idea, if the fix is this simple. To me, it's similar bad practice to ignoring compiler warnings: most of the time, they are a bug.

@bkaradzic
Copy link
Owner

bkaradzic commented Sep 20, 2025

and deliberately leaving a race condition in, just increases the noise when you work with them.

Yes, it's there to eliminate these shallow analysis, where someone runs sanitizer without understanding what's going on.

I'm not going to react on uninformed sanitizer results, or AI suggestions...

See previous discussions: #1186, #1189, #1307.

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.

3 participants