-
Notifications
You must be signed in to change notification settings - Fork 232
ignore setLoggerLevel: when compiling w/thread-sanitizer on #163
base: develop
Are you sure you want to change the base?
ignore setLoggerLevel: when compiling w/thread-sanitizer on #163
Conversation
during unit-testing with the thread-sanitizer on, setLoggerLevel: was flagged unnecessarily as a potential data-race. __sharedLoggerLevel is already marked volatile, and the location of the data race indicated was a debug logging message .
What unit tests? How can I repro? The move to stdatomic SHOULD have solved this and for thread sanitizer to still elicit warnings makes me want to double check that we are properly accessing the variable everywhere. |
I did some research and UPDATE: std::atomic is for C++, which is not what we are using here. We are using <stdatomic.h>, the C version, which uses the _Atomic keyword. It is much more unclear about the use of the volatile keyword, but seems to be independent of it. They can be used together or independently. I still say we don't need the 'volatile' keyword, but it's not hurting anything. |
for Twitter, was getting this just running the "Bluebird Enterprise" scheme, performing unit tests with thread sanitizer turned on. can try to re-test with __sharedLoggerLevel established as a non-volatile … |
deleted last 2 comments with stack-traces of thread-sanitizer … and also got the latest proper SHA1 for the develop branch … |
I guess the thread sanitizer is technically correct, __sharedLoggerLevel is, by design, a race between writes & reads. It doesn't matter though, as long as it is updated atomically. I'm surprised the thread sanitizer isn't more cognizant or accepting of usage patterns like this. I suppose the only thing we can do is disable it the check, as you've proposed in this request. |
Kirk Beitz seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
during unit-testing with the thread-sanitizer on, setLoggerLevel: was
flagged unnecessarily as a potential data-race.
__sharedLoggerLevel is already marked volatile, and the location of
the data race indicated was a debug logging message .