Open alexey-katranov opened 3 years ago
@dvyukov , I prototyped simple implementation of standalone fences. The idea is that each thread has release fence clock and acquire fence clock. Additionally, SyncVar
is extended with clock for the fences. The algorithm is following:
The prototype seems to work correctly for the example above:
#include <atomic>
#include <thread>
std::atomic<int> numThreadsFinished{};
std::atomic<int> epoch{};
int racy_data{};
void wait() {
if (numThreadsFinished++ == 0) {
while (epoch.load(std::memory_order_relaxed) == 0) {} // Y
volatile auto racy_load = racy_data; // RACE is reported!
(void)racy_load;
std::atomic_thread_fence(std::memory_order_acquire); // F
return;
}
epoch++; // X
}
int main() {
int data{};
std::thread thr([&] {
data = 1;
racy_data = 1;
wait();
});
wait();
if (data != 1) { // RACE is NOT reported!
return -1;
}
thr.join();
return 0;
}
Did you think about such implementation in the past? Does it contain hidden issues that are difficult to resolve? Are you interested in further development? Surely, the prototype works only for this example and really suboptimal.
Hi Alex,
Please see: https://reviews.llvm.org/D47107#1125052 It should answer some of your questions.
Overall I think tsan needs to support standalone fences. There are correct programs that use fences and there are no good workarounds for these cases. However, if it increases runtime costs, it should be guarded by a separate flag.
Another aspect is that I have a change in-flight which effectively rewrites all of the runtime: https://github.com/dvyukov/llvm-project/pull/3/files It's not ready but we should decide soon on its fate. I am not sure what's the right way to order this change vs your change, I guess it may depend on the size of your change.
But either way a good first step would be a set of positive/negative test cases that will now just document the current behavior, but will also contains the expected behavior as comments
https://reviews.llvm.org/D47107#1125052 It should answer some of your questions.
It seems I reinvent the wheel. It was just one evening effort, so it is definitely better to finish your major change. Moreover, I am not in hurry and it is our of scope of my job. As for initial problem, we just rework the code not to use standalone fences.
It seems that the above review has not been updated for three years. What did block the patch and why not to finish it?
As I understand, ktsan implements the fences. It seems that it reimplements the logic of tsan, why it was not separated into a common module?
As for tests, I can try prepare some tests, do you have any BKMs, how to build and run tests (it would be good if I do not need to build clang).
BKMs
https://reviews.llvm.org/D47107#1125052 It should answer some of your questions.
It seems I reinvent the wheel. It was just one evening effort, so it is definitely better to finish your major change. Moreover, I am not in hurry and it is our of scope of my job. As for initial problem, we just rework the code not to use standalone fences.
A non-upstreammed wheel does not count :)
It seems that the above review has not been updated for three years. What did block the patch and why not to finish it?
I have no idea. That's how OSS works. You spend time writing an extensive reply and then you just never hear from the author again :)
As I understand, ktsan implements the fences. It seems that it reimplements the logic of tsan, why it was not separated into a common module?
Different languages, different code styles, different environments, different specifics.
As for tests, I can try prepare some tests, do you have any BKMs, how to build and run tests (it would be good if I do not need to build clang).
I don't think there is a way to avoid building llvm, but you need to do this only once.
We have these docs, but there also should be some llvm docs:
https://github.com/google/sanitizers/wiki/AddressSanitizerHowToBuild
For testing you run ninja check-tsan
in the build dir. There is also a way to run a single test:
https://reviews.llvm.org/D103054
The problem with tsan not supporting fences still exists, right? Shouldn't the issue be reopened, or is this tracked somewhere else?
Not sure why I closed it. I usually write explanations when closing issues. Maybe just hit Close accidentally...
Is there any form of annotation or other to avoid a false positive due to TSan not taking into account an acquire fence?
I see. Thanks!
Thread Sanitizer reports a race over
data
in the following code snippet. I do not know if TSan supports standalone fences but I see some__tsan_atomic_thread_fence
in the code. In accordance with the C++ standard the code seems valid: the release operation X is observed with any fenced load Y followed with acquire fence F should give us happens before between operations before X and after F. (strictly speaking there is the second path overnumThreadsFinished++
but it seems pretty simple) (surely if Y operation with acquire fence, everything is Ok).