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

Implement aligned_alloc #3577

Closed
RalfJung opened this issue May 6, 2024 · 10 comments · Fixed by #3585
Closed

Implement aligned_alloc #3577

RalfJung opened this issue May 6, 2024 · 10 comments · Fixed by #3585
Labels
A-shims Area: This affects the external function shims C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement E-good-first-issue A good way to start contributing, mentoring is available

Comments

@RalfJung
Copy link
Member

RalfJung commented May 6, 2024

C11 has a new allocation function: aligned_alloc. We should implement it in Miri. Docs can be found here.

@RalfJung RalfJung added C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement A-shims Area: This affects the external function shims E-good-first-issue A good way to start contributing, mentoring is available labels May 6, 2024
@RalfJung
Copy link
Member Author

RalfJung commented May 6, 2024

Ah, but the function spec is completely terrible of course...

alignment - specifies the alignment. Must be a valid alignment supported by the implementation.

There is no guarantee at all for what is a "valid alignment supported by the implementation". So this can basically always return null, great.

Given the comments in the docs, it seems like a reasonable choice of "valid alignment supported by the implementation" is to require the alignment to be at least the size of a pointer. I have no clue why an implementation would reject smaller alignment, but it is what posix_memalign does so it seems to be common... somehow...

All "fundamental" alignments have to be supported -- which I hope doesn't mean we have to support non-power-of-two alignments smaller than max_align_t. I think for now the best course of action is to support all powers of two and return NULL otherwise. A size of 0 should be handled like malloc.

@workingjubilee
Copy link
Contributor

indeed, void* minimum in practice, it seems: https://www.gnu.org/software/gnulib/manual/html_node/aligned_005falloc.html

@workingjubilee
Copy link
Contributor

though asking for too-large alignments is also bad: rust-lang/rust@53bd38b

@RalfJung
Copy link
Member Author

RalfJung commented May 6, 2024

indeed, void* minimum in practice, it seems: https://www.gnu.org/software/gnulib/manual/html_node/aligned_005falloc.html

That's for that one implementation. But who knows about others...

though asking for too-large alignments is also bad: rust-lang/rust@53bd38b

That's "obviously" a platform bug though, I don't think Miri should model that.
(Returning NULL for very big alignments would be allowed by the spec, but returning an unaligned value is clearly violating the spec.)

@RalfJung
Copy link
Member Author

RalfJung commented May 9, 2024

indeed, void* minimum in practice, it seems: https://www.gnu.org/software/gnulib/manual/html_node/aligned_005falloc.html

Looking at the spec again, it says fundamental alignments must be supported. So I think a void* minimum is actually non-compliant. macOS (and Illumos) implementations of this function are buggy.

@RalfJung
Copy link
Member Author

RalfJung commented May 9, 2024

😂 turns out libc doesn't have this function yet... rust-lang/libc#3689. That certainly makes testing harder.

@workingjubilee
Copy link
Contributor

Looking at the spec again, it says fundamental alignments must be supported. So I think a void* minimum is actually non-compliant. macOS (and Illumos) implementations of this function are buggy.

And some platforms just don't implement it at all, of course, for legacy reasons.

@RalfJung
Copy link
Member Author

RalfJung commented May 9, 2024

According to my research, at least all the Unixes Miri currently has any support for do all have it. That's good enough for me to say we just provide it on all Unixes.

@workingjubilee
Copy link
Contributor

Oh yes, the notable exception is Windows.

@RalfJung
Copy link
Member Author

RalfJung commented May 10, 2024 via email

@bors bors closed this as completed in b850b4d May 19, 2024
RalfJung pushed a commit to RalfJung/rust that referenced this issue May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-shims Area: This affects the external function shims C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement E-good-first-issue A good way to start contributing, mentoring is available
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants