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

TSAN: warn about usage of atomic_thread_fence #1352

Open
marxin opened this issue Dec 4, 2020 · 6 comments
Open

TSAN: warn about usage of atomic_thread_fence #1352

marxin opened this issue Dec 4, 2020 · 6 comments

Comments

@marxin
Copy link

marxin commented Dec 4, 2020

As mentioned here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97868

It's quite a common case where people complain about TSAN that it doesn't support atomic_thread_fence.
Can we provide a warning (either compile-time or run-time) for it?
What do you think?

@dvyukov
Copy link
Contributor

dvyukov commented Dec 4, 2020

Good question.

I agree that the current failure mode is very suboptimal as it's hard to map the false reports back to tsan's mishandling on atomic_thread_fence.
What's the best way to warn users... hard to say. I think probably a compiler warning is a better option. This can break compilation with -Werror, but I think compilers add new warnings regularly. So assuming there is 1 location where build system adds -fsanitize=thread, now it can add -fsanitize=thread + -Wno-something.
We don't have good target for runtime warnings, printing to stdout/stderr can change runtime behavior of programs, and output of some subprocesses may be not visible. For changing behavior we could provide a runtime flag similar to -Wno-something, e.g. TSAN_OPTIONS=dont_warn_about_atomic_fence=1. But I think it's more cumbersome for users.

@marxin
Copy link
Author

marxin commented Dec 7, 2020

All right, I'm planning to introduce a new warning for the GCC compiler.

@mrichtarsky
Copy link

How do other projects deal with such missing support? Do they filter out all findings from a source file or library? I imagine it's hard to totally avoid using explicit barriers.

@dvyukov
Copy link
Contributor

dvyukov commented Dec 10, 2020

FWIW all of Chromium and most of our internal code is tested as is. This only affects acquire/release fences, and these are rarely needed (only some implementations of seqlock optimized for non-TSO arches come to mind). Some __tsan_acquire/release annotations can help to deal with this as well.

@mrichtarsky
Copy link

Unfortunately our code base has quite a lot of explicit fences. Some certainly can be removed, but others are needed for better performance.

Some __tsan_acquire/release annotations can help to deal with this as well.

Can you elaborate on this please? I could not find any references to such annotations.

@dvyukov
Copy link
Contributor

dvyukov commented Dec 15, 2020

Can you elaborate on this please? I could not find any references to such annotations.

Here they are:
https://github.com/llvm/llvm-project/blob/ad1161f9b5ff251b80788033e4db82b5a11b187b/compiler-rt/include/sanitizer/tsan_interface.h#L24-L25
Basically act as acquire/release atomic on the provided address.

robot-piglet pushed a commit to ydb-platform/ydb that referenced this issue Sep 19, 2022
For example, google/sanitizers#1352 - tsan is not working well with std::atomic_thread_fence

Minimal reproducible example (and one that bothers most in every fiber-aware service):

```
auto threadPool = NYT::New<NYT::NConcurrency::TThreadPool>(2, "thread");
    
TVector<NYT::TFuture<void>> futures;

for (size_t i = 0; i < 100000; ++i) {
    futures.emplace_back(BIND([]() {
        
    }).AsyncVia(threadPool->GetInvoker()).Run());
}

for (auto& future : futures) {
    future.Get().ThrowOnError();
}
```
arcadia-devtools pushed a commit to catboost/catboost that referenced this issue Sep 19, 2022
For example, google/sanitizers#1352 - tsan is not working well with std::atomic_thread_fence

Minimal reproducible example (and one that bothers most in every fiber-aware service):

```
auto threadPool = NYT::New(2, "thread");

TVector> futures;

for (size_t i = 0; i GetInvoker()).Run());
}

for (auto& future : futures) {
future.Get().ThrowOnError();
}
```

ref:3e00460e50688a7605e91115ffb25a041f2061bf
robot-piglet pushed a commit to catboost/catboost that referenced this issue Jan 17, 2023
For example, google/sanitizers#1352 - tsan is not working well with std::atomic_thread_fence

Minimal reproducible example (and one that bothers most in every fiber-aware service):

```
auto threadPool = NYT::New(2, "thread");

TVector> futures;

for (size_t i = 0; i GetInvoker()).Run());
}

for (auto& future : futures) {
future.Get().ThrowOnError();
}
```

ref:3e00460e50688a7605e91115ffb25a041f2061bf
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

3 participants