Patch gtest to terminate as soon as it detects that a ScopedTrace has migrated to another worker thread
#1258
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
msimberg
commented
Jan 14, 2025
msimberg
commented
Jan 14, 2025
Closed
|
cscs-ci run |
msimberg
force-pushed
the
gtest-terminate-on-thread-migrate
branch
from
78fdf9c to
a15bb32
Compare
|
cscs-ci run |
msimberg
force-pushed
the
gtest-terminate-on-thread-migrate
branch
from
a15bb32 to
83a4712
Compare
|
cscs-ci run |
msimberg
commented
Jan 17, 2025
aurianer
approved these changes
Jan 17, 2025
msimberg
force-pushed
the
gtest-terminate-on-thread-migrate
branch
from
83a4712 to
07cf5f1
Compare
|
cscs-ci run |
msimberg
force-pushed
the
gtest-terminate-on-thread-migrate
branch
from
07cf5f1 to
64e970c
Compare
|
cscs-ci run |
|
The patch seems to be correctly applied now, but can't be tested because CI needs to be moved to alps. |
rasolca
approved these changes
Jan 27, 2025
…read different than where it was destroyed gtest ScopedTrace/SCOPED_TRACE uses thread local stacks to keep track of traces. When combined with pika tasks that may yield and be stolen onto other worker threads, destroying a ScopedTrace on a different thread can segfault, cause hangs, or simply corrupt data. Instead of allowing that to happen every now and then we instead patch gtest to check if this happens and terminate as soon as it's detected. Use of ScopedTrace should be avoided in situations where tasks can yield. This patch does not fix the issue, but helps recognizing when e.g. a segfault is the cause of "only" improper use of ScopedTrace or if it is a result of other algorithmic or implementation issues.
msimberg
force-pushed
the
gtest-terminate-on-thread-migrate
branch
from
64e970c to
a8ad787
Compare
|
cscs-ci run |
Yep, I still want to confirm that it does the right thing in CI. Now that #1262 is merged we should get some results soon enough... |
msimberg
force-pushed
the
gtest-terminate-on-thread-migrate
branch
from
a8ad787 to
1ca6fc5
Compare
|
Seems to be working as intended: https://gitlab.com/cscs-ci/ci-testing/webhook-ci/mirrors/4700071344751697/7514005670787789/-/jobs/8962660045#L2249. I've removed the temporary commit. |
|
cscs-ci run |
github-actions bot
pushed a commit
that referenced
this pull request
Jan 29, 2025
…ce` has migrated to another worker thread (#1258)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See first commit message for more details.
I've added a temporary commit which explicitly adds more yields while holding a
ScopedTraceto check that it actually works in CI.I've opted for a local patch which I think is easily updated if we update the gtest tag. An alternative strategy could be to apply the patch through the spack package and I don't have a strong preference either way, other than me not (yet) knowing how to apply the patch with spack to something that is pulled only at the CMake configure stage. One benefit of patching gtest like this is that it will always apply, even if one uses a spack build-env which doesn't run CMake through spack.