Skip to content

[TSan] Do not report benign race on _swiftEmptyArrayStorage #27808

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

Merged
merged 2 commits into from
Oct 22, 2019

Conversation

yln
Copy link
Contributor

@yln yln commented Oct 21, 2019

A previous commit [1] identified and fixed a benign race on
_swiftEmptyArrayStorage. Unfortunately, we have some code duplication
and the fix was not applied to all the necessary places. Essentially,
we need to ensure that the "empty array storage" object that backs many
"array like types" is never written to.

I tried to improve the test to capture this, however, it is very
finicky. Currently, it does not go red on my system even when I remove
the check to avoid the benign race in Release mode.

Relevant classes: Array, ArraySlice, ContiguousArray, ArrayBuffer,
ContiguousArrayBuffer, SliceBuffer.

[1] b9b4c78

rdar://55161564

A previous commit [1] identified and fixed a benign race on
`_swiftEmptyArrayStorage`.  Unfortunately, we have some code duplication
and the fix was not applied to all the necessary places.  Essentially,
we need to ensure that the "empty array storage" object that backs many
"array like types" is never written to.

I tried to improve the test to capture this, however, it is very
finicky.  Currently, it does not go red on my system even when I remove
the check to avoid the benign race in Release mode.

Relevant classes: Array, ArraySlice, ContiguousArray, ArrayBuffer,
ContiguousArrayBuffer, SliceBuffer.

[1] b9b4c78

rdar://55161564
@yln
Copy link
Contributor Author

yln commented Oct 21, 2019

Also: it might be worth to reduce/remove this code duplication, if possible.

Remove the argument label to reduce the verbosity in the test.
@yln
Copy link
Contributor Author

yln commented Oct 21, 2019

@swift-ci please test

@swiftlang swiftlang deleted a comment from swift-ci Oct 21, 2019
@swiftlang swiftlang deleted a comment from swift-ci Oct 21, 2019
@jrose-apple jrose-apple requested review from lorentey and Catfish-Man and removed request for jrose-apple October 21, 2019 19:59
Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

This looks good, thank you Julian!

@yln
Copy link
Contributor Author

yln commented Oct 21, 2019

@swift-ci Please test Linux platform

@yln yln merged commit e819acd into master Oct 22, 2019
@yln yln deleted the tsan-empty-array-storage branch October 22, 2019 01:39
@mikeash
Copy link
Contributor

mikeash commented Oct 22, 2019

I'm a bit late, but this looks good, thanks!

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

Successfully merging this pull request may close these issues.

3 participants