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

Fix race condition in iterator debug machinery #2060

Merged
merged 17 commits into from
Jul 30, 2021

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Jul 14, 2021

Fixes #2058

@miscco miscco requested a review from a team as a code owner July 14, 2021 13:58
@CaseyCarter CaseyCarter added the bug Something isn't working label Jul 14, 2021
@CaseyCarter CaseyCarter added this to Initial Review in Code Reviews via automation Jul 14, 2021
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
tests/std/tests/GH_002058_debug_iterator_race/test.cpp Outdated Show resolved Hide resolved
Code Reviews automation moved this from Initial Review to Work In Progress Jul 14, 2021
Co-authored-by: Adam Bucior <35536269+AdamBucior@users.noreply.github.com>
Co-authored-by: Casey Carter <cacarter@microsoft.com>
stl/inc/xmemory Outdated
Comment on lines 1131 to 1145
_Orphan_me_v2();
#ifdef __cpp_lib_constexpr_dynamic_alloc
if (_STD is_constant_evaluated()) {
_Orphan_me_unlocked();
} else
#endif // __cpp_lib_constexpr_dynamic_alloc
{
_Orphan_me_locked();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this is a little bit of unrelated debug performance improvement, but I believe we should try to minimize the effect on debug performance

@miscco
Copy link
Contributor Author

miscco commented Jul 15, 2021

It seems we still have some race condition somewhere

@miscco
Copy link
Contributor Author

miscco commented Jul 15, 2021

Oh it turns out the bug is with _ITERATOR_DEBUG_LEVEL != 2 where we do not have any locks and I believe indeed destroying the underlying container is a bug.

So I would change the test to only clear the container

@CaseyCarter CaseyCarter moved this from Work In Progress to Final Review in Code Reviews Jul 15, 2021
Co-authored-by: Adam Bucior <35536269+AdamBucior@users.noreply.github.com>
stl/inc/xmemory Outdated Show resolved Hide resolved
tests/std/tests/GH_002058_debug_iterator_race/env.lst Outdated Show resolved Hide resolved
tests/std/tests/GH_002058_debug_iterator_race/test.cpp Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

@CaseyCarter @miscco @mnatsuhara FYI, I've pushed a series of commits, some significant.

@StephanTLavavej StephanTLavavej self-assigned this Jul 29, 2021
@StephanTLavavej
Copy link
Member

I'm going to add this to the next batch of changes to merge - please notify me if any further commits are pushed.

Copy link
Contributor

@mnatsuhara mnatsuhara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the wait on this review! This looks good to me though 🥳

@mnatsuhara mnatsuhara moved this from Final Review to Ready To Merge in Code Reviews Jul 29, 2021
@mnatsuhara mnatsuhara removed their assignment Jul 29, 2021
@StephanTLavavej StephanTLavavej merged commit 37b0ce0 into microsoft:main Jul 30, 2021
Code Reviews automation moved this from Ready To Merge to Done Jul 30, 2021
@StephanTLavavej
Copy link
Member

Thanks for fixing this high priority bug! 🪲 😸 🎉

@miscco miscco deleted the fix_debug_iterator_race branch August 3, 2021 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high priority Important!
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

_ITERATOR_DEBUG_LEVEL 2: crash when iterators are destroyed while container is modified
5 participants