Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

ignore setLoggerLevel: when compiling w/thread-sanitizer on #163

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

johnkdoe
Copy link

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 .

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 .
@coveralls
Copy link

Coverage Status

Coverage remained the same at 76.243% when pulling d6c03e1 on johnkdoe:develop-with-thread-sanitizer into 065fb0f on twitter:develop.

@NSProgrammer
Copy link
Collaborator

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.

@kgoodier
Copy link
Contributor

kgoodier commented Dec 27, 2016

I did some research and volatile is not necessary with the std::atomic variables, of which __sharedLoggerLevel is one. We should probably skip this change, and just remove the volatile keyword. I'm happy to do that, but I don't know how to repro this sanitizer error you're seeing; @johnkdoe can you try that change and see if stops?

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.

@johnkdoe
Copy link
Author

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 …

@johnkdoe
Copy link
Author

deleted last 2 comments with stack-traces of thread-sanitizer … and also got the latest proper SHA1 for the develop branch …

@kgoodier
Copy link
Contributor

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.

@CLAassistant
Copy link

CLAassistant commented Jul 18, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants