Skip to content

[Runtime] Use malloc_type_posix_memalign(). #70367

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 13, 2023
Merged

[Runtime] Use malloc_type_posix_memalign(). #70367

merged 2 commits into from
Dec 13, 2023

Conversation

al45tair
Copy link
Contributor

We can't use malloc_type_aligned_alloc() because aligned_alloc() requires that size be a multiple of alignment, which isn't something we expect here.

rdar://119137861

@al45tair
[Runtime] Use malloc_type_posix_memalign().
f392153 
We can't use `malloc_type_aligned_alloc()` because `aligned_alloc()` requires
that `size` be a multiple of `alignment`, which isn't something we expect here.

rdar://119137861
@al45tair al45tair requested a review from a team as a code owner December 11, 2023 12:43
@al45tair al45tair added 🍒 release cherry pick Flag: Release branch cherry picks swift 5.10 labels Dec 11, 2023
@al45tair
Copy link
Contributor Author

al45tair commented Dec 11, 2023 •
edited
Loading

Explanation: aligned_alloc() and by extension malloc_type_aligned_alloc() require that size be a multiple of alignment (at least in C11 they do; C17 appears to have dropped that requirement). This code was changed to avoid having to cache the default zone, prior to which it was using malloc_type_zone_memalign(), which doesn't have this behaviour. The simplest fix is to use malloc_type_posix_memalign() instead, which matches what happens in the un-typed allocation function (since that uses posix_memalign()).
Risk: Low. This fixes a bug.
Original PR: #70366
Reviewed by: @mikeash
Resolves: rdar://119137861
Tests: I have added some tests for the heap code, however because of the environment the tests currently run in, it wouldn't have caught the problem. At some point that will change (e.g. when CI systems are upgraded).

@al45tair
Copy link
Contributor Author

@swift-ci Please test

@al45tair
[Runtime][Test] Add some tests for the heap functions.
6374cc4 
We should have some tests for the heap functions.  Note that these
wouldn't have caught the problem that we fixed in the previous
commit, because the conditions under which they run presently mean
that the problematic code wouldn't have been active.  They will
*eventually* test that code, however.

rdar://119137861
@al45tair
Copy link
Contributor Author

@swift-ci Please test

@al45tair al45tair merged commit 7ee6144 into swiftlang:release/5.10 Dec 13, 2023
@al45tair al45tair mentioned this pull request Dec 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbkka tbkka tbkka approved these changes

Assignees
No one assigned
Labels
🍒 release cherry pick Flag: Release branch cherry picks swift 5.10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@al45tair @tbkka