Skip to content

Fix alignment issues when allocating in AtomicWaitQueue #42355

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 1 commit into from
Apr 15, 2022

Conversation

grynspan
Copy link
Contributor

@grynspan grynspan commented Apr 14, 2022

This change ensures that AtomicWaitQueue allocates its inner queues in an aligned fashion even when the compiler does not support the C++17 "over-aligned new" feature, and avoids using new anyway since it might be overridden by something else in the process.

On some platforms, the compiler does not support the C++17 "over-aligned new" feature. Allocating an instance of type T with new T() will produce a naturally-aligned pointer, which appears to be insufficient for use with AtomicWaitQueue on said platforms. Windows and Android in particular have been identified as affected.

The solution is to explicitly call std::aligned_alloc() (or, if the type's size is somehow not a multiple of its alignment, std::malloc()) and to use placement new to initialize the new value. Destruction is accomplished by directly calling the type's destructor and then passing the pointer to std::free().

Since swift_slowAlloc() and swift_slowDealloc() handle alignment already and are the preferred allocation mechanisms in Swift, sub them in for std::aligned_alloc() and std::free() above.

This fix has the added benefit of avoiding the global operator new, which can be overridden by other code in the process. If the global operator new is overridden in such a way that it re-enters the Swift runtime, sadness will result.

@grynspan grynspan requested review from compnerd and rjmccall April 14, 2022 00:28
@grynspan grynspan force-pushed the jgrynspan/AtomicWaitQueue-aligned-allocation branch 4 times, most recently from 787eacd to c616d73 Compare April 14, 2022 03:46
@grynspan
Copy link
Contributor Author

@swift-ci please test

@grynspan
Copy link
Contributor Author

There are a couple of typos in this PR but they don't affect the correctness of the code, so I'm going to wait until the current CI builds complete before fixing.

@grynspan grynspan requested review from mikeash and al45tair April 14, 2022 19:16
@grynspan grynspan force-pushed the jgrynspan/AtomicWaitQueue-aligned-allocation branch from c616d73 to 580210e Compare April 14, 2022 19:17
@grynspan
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

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

LGTM

@mikeash
Copy link
Contributor

mikeash commented Apr 14, 2022

LGTM. I'd like to suggest different names for the new functions. The "slow" in the existing functions is kind of leftover so I'd suggest we not bring that forward, and I'd like to make the names more clear about doing the same thing as new/delete. Perhaps swift_cxxNew and swift_cxxDelete?

…on even when the compiler does not support the C++17 over-aligned new feature (and avoid using new anyway since it might be overridden by something else in the process.)
@grynspan grynspan force-pushed the jgrynspan/AtomicWaitQueue-aligned-allocation branch from 580210e to 3f24533 Compare April 14, 2022 21:03
@grynspan
Copy link
Contributor Author

@swift-ci please test

@grynspan
Copy link
Contributor Author

Mike and I discussed and settled on swift_cxx_newObject() and swift_cxx_deleteObject() for maximum unambiguity.

@grynspan
Copy link
Contributor Author

@swift-ci please test windows

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

This definitely is a step in the right direction! This should save us from the global operator new replacement shenanigans and also module specific allocators.

@grynspan
Copy link
Contributor Author

grynspan commented Apr 15, 2022

Holding for @compnerd to approve. Oh, he did. Thanks!

The Windows build failure appears unrelated to this PR. Will try a rebuild in case it's been resolved on main, but otherwise I don't think it holds up merging.

@grynspan
Copy link
Contributor Author

@swift-ci please test windows

@grynspan
Copy link
Contributor Author

The Windows issue is indeed present on main, so merging!

@grynspan grynspan merged commit 155cd2e into main Apr 15, 2022
@grynspan grynspan deleted the jgrynspan/AtomicWaitQueue-aligned-allocation branch April 15, 2022 03:33
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.

4 participants