Fixed Thread Sanitizer complaint by making variable atomic

Without this multiple threads are accessing IsParallel simultaneously without any synchroniztion/mutex.

Backport: release

Errors:

• commit 5b742b28 is not allowed because the following files are not formatted according to the 'clang-format' check: Common/Core/SMP/Common/vtkSMPToolsImpl.h. Post a comment ending in the line Do: reformat to rewrite the MR source branch automatically.

Warnings:

• please consider adding a changelog entry in a file ending with .md in Documentation/release/dev.

The warnings do not need to be fixed, but it is recommended to do so.

Please rewrite commits to fix the errors listed above (adding fixup commits will not resolve the errors) and force-push the branch again to update the merge request.

changed the description

Errors:

• commit 5b742b28 is not allowed because the following files are not formatted according to the 'clang-format' check: Common/Core/SMP/Common/vtkSMPToolsImpl.h. Post a comment ending in the line Do: reformat to rewrite the MR source branch automatically.

Warnings:

• please consider adding a changelog entry in a file ending with .md in Documentation/release/dev.

The warnings do not need to be fixed, but it is recommended to do so.

Please rewrite commits to fix the errors listed above (adding fixup commits will not resolve the errors) and force-push the branch again to update the merge request.

Errors:

• commit 5b742b28 is not allowed because the following files are not formatted according to the 'clang-format' check: Common/Core/SMP/Common/vtkSMPToolsImpl.h. Post a comment ending in the line Do: reformat to rewrite the MR source branch automatically.

Warnings:

• please consider adding a changelog entry in a file ending with .md in Documentation/release/dev.

The warnings do not need to be fixed, but it is recommended to do so.

Please rewrite commits to fix the errors listed above (adding fixup commits will not resolve the errors) and force-push the branch again to update the merge request.

@ben.boeckel @will.schroeder @mwestphal @charles.gueunet @estan

this->IsParallel.compare_exchange_strong(true, fromParallelCode); instead ?

No; the first argument is a T&. This is wrong anyways. A loop must be written around cmpxchg instructions to ensure it actually happens. Something like this:

bool trueFlag = true;
while (!this->IsParallel.compare_exchange_strong(trueFlag, fromParallelCode)) {}

However, if this->IsParallel is false, we're calling this at least twice. Once fails and sets trueFlag to false then it comes around again and finally sets it because trueFlag is the "expected" value. I think what is wanted is:

this->IsParallel.fetch_and(fromParallelCode);

this->IsParallel.fetch_and(fromParallelCode);

That was my first attempt in fact, but for some reason bool is not supported by fetch_add. In researching alternatives I found this: https://stackoverflow.com/questions/29390247/stdatomicbool-fetch-and-and-fetch-or-realization which after going through on paper I reasoned to be correct. I thought the loop was only needed for compare_exchange_weak and not the strong variant...

We could just change the bool to int and use fetch_and.

That's probably because fetch_add and fetch_or are defined on bitwise representations and two "true" values could & to "false" because they share no bits.

As for strong versus weak, strong just isn't allowed to give false negatives. The "expected wasn't what you thought it was" is still relevant. In addition, if it is currently false, replacing it with true seems to still be wrong, no?

I suppose for bool, if it isn't the expected value, you know what it must be, so maybe a loop isn't necessary. In any case, a comment describing what is going on would be helpful here. Atomic operations break my brain at least :) .

That's probably because fetch_add and fetch_or are defined on bitwise representations and two "true" values could & to "false" because they share no bits.

Yeah I suppose. But I thought storing anything but 0 and 1 in a bool was already undefined behaviour.

Atomic operations break my brain at least

You're not alone there!

I've tried to reason through it again, and come back to agreeing with myself before that it's correct as-is:

From this:

bool compare_exchange_strong (T& expected, T val);

Compares the contents of the contained value with expected:

• if true, it replaces the contained value with val (like store).
• if false, it replaces expected with the contained value.

Which is:

if (this == expected)
  this = val;
else
  expected = this;

which is:

if (IsParallel == trueFlag)
  IsParallel = fromParallelCode;
else
  trueFlag = IsParallel;

So if (IsParallel == false), it says false. And if (IsParallel == true), it becomes fromParallelCode.

That's the same as IsParallel &= fromParallelCode aka IsParallel = IsParallel && fromParallelCode.

And the else branch is just useless to us.

Or do I need more coffee?

Yes, this works because bool only has two values. Please add a comment to this effect to avoid future "huh, cmpxchg without a loop is weird" "fixes".

OK, I'll expand comments.

Also, thinking now that the weak version would be fine in fact, since we don't even use the return value.

With weak, you have to consider what happens when IsParallel is false. Which…is fine. Not immediately obvious, but typing this out helped me get through it :) .

changed this line in version 2 of the diff

OK, done. Let me know if you want further tweaks to the comments.

I also found similar code in other files, and changed it there too.

added 1 commit

• 024b1e6a - Fixed Thread Sanitizer complaint by making variable atomic

Compare with previous version

Errors:

• commit 024b1e6a is not allowed because the following files are not formatted according to the 'clang-format' check: Common/Core/SMP/Common/vtkSMPToolsImpl.h. Post a comment ending in the line Do: reformat to rewrite the MR source branch automatically.

Warnings:

• please consider adding a changelog entry in a file ending with .md in Documentation/release/dev.

The warnings do not need to be fixed, but it is recommended to do so.

Please rewrite commits to fix the errors listed above (adding fixup commits will not resolve the errors) and force-push the branch again to update the merge request.

Errors:

• commit 024b1e6a is not allowed because the following files are not formatted according to the 'clang-format' check: Common/Core/SMP/Common/vtkSMPToolsImpl.h. Post a comment ending in the line Do: reformat to rewrite the MR source branch automatically.

Warnings:

• please consider adding a changelog entry in a file ending with .md in Documentation/release/dev.

The warnings do not need to be fixed, but it is recommended to do so.

Please rewrite commits to fix the errors listed above (adding fixup commits will not resolve the errors) and force-push the branch again to update the merge request.

Do: test

Do: reformat

This topic has been reformatted and pushed; please fetch from the source repository and reset your local branch to continue with further development on the reformatted commits.

added 1 commit

• bca9e43e - Fixed Thread Sanitizer complaint by making variable atomic

Compare with previous version

Warnings:

• please consider adding a changelog entry in a file ending with .md in Documentation/release/dev.

The warnings do not need to be fixed, but it is recommended to do so.

Warnings:

• please consider adding a changelog entry in a file ending with .md in Documentation/release/dev.

The warnings do not need to be fixed, but it is recommended to do so.

@ben.boeckel do those "wheel" failures seen unrelated?

@dgobbi perhaps you could review too?

The change seems logical, but I don't know if I'm the best person to review.

One exceedingly minor point, now that IsParallel is atomic, it might be more efficient to place it second in this condition:

-   if (this->IsParallel && !this->NestedActivated)
+   if (!this->NestedActivated && this->IsParallel)

Done. Thanks for your review.

added 1 commit

• 19f14300 - Fixed Thread Sanitizer complaint by making variable atomic

Compare with previous version

Warnings:

• please consider adding a changelog entry in a file ending with .md in Documentation/release/dev.

The warnings do not need to be fixed, but it is recommended to do so.

Warnings:

• please consider adding a changelog entry in a file ending with .md in Documentation/release/dev.

The warnings do not need to be fixed, but it is recommended to do so.