support aligned_alloc for unixes.

[devnexen]

Fixes https://github.com/rust-lang/miri/issues/3577

[RalfJung]

Thanks for the PR!

This is for https://github.com/rust-lang/miri/issues/3577, right? When a PR works on an issue, please mention the issue in the PR description. Generally, PR descriptions should hardly ever be empty. Please spend a bit of time giving context to the change -- which issue it resolves, which new feature it implements and how, things like that. It makes review a lot easier.

[RalfJung]

We should only support it on platforms that have it in libc.

Do it like the other similar functions: cfg(not(any(...))) in the test, and a check against the target OS in the implementation. We shouldn't pretend support on OSes that don't have support.

In some cases maybe the OS has support but the libc crate does not -- in that case, that should be filed as an issue against the libc crate.

[RalfJung]

On size 0 this should probably behave like https://github.com/rust-lang/miri/pull/3580. That's already what you implemented now, but there should be tests ensuring we detect the memory leak when a size-zero allocation was not freed, and size-0-double-free.

[RalfJung]

I realized I missed an important part of the spec.

Fundamental alignments are always supported.

So... I think we have to support small alignments as well.

I think the point of the posix_memalign example is to mention that the alignment must be a power of two?

[RalfJung]

@rustbot author (please do rustbot ready when this is ready for the next round of review)

[RalfJung]

Looking at the libc crate, it doesn't seem like it has this function on any target we support. :joy:

Okay, so... how do we find out which targets support this function? Linux definitely does, at least I have a manpage for it.

[RalfJung]

I did a bit of googling and found evidence that it exists for all platforms we support (see the links in https://github.com/rust-lang/libc/issues/3689). For Iluumos the man page says

The value of alignment is constrained, it must be a power of two and it must be greater than or equal to the size of a word on the platform.

That is clearly a violation of the C standard, and hence a platform bug. I don't think we should emulate that.

So, yeah seems fine to just implement it on all targets then (with it being a C11 standard function -- that's more than 10 years old at this point), and declare it in the test like you did.

[devnexen]

@rustbot ready

[RalfJung]

@rustbot author Please do not mark a PR as ready when there are outstanding comments from the previous review that have not been taken care of yet. Please go over all my comments from the previous review and either make sure they are handled, or argue why you think they shouldn't be handled.

Examples of comments not handled:

• https://github.com/rust-lang/miri/pull/3585#discussion_r1593475321
• https://github.com/rust-lang/miri/pull/3585#discussion_r1593480234

There may be more.

[devnexen]

alright .. going to reactivate wasi then, we shall see.

[RalfJung]

Oh d'oh, wasi is not a unix, I keep forgetting that. Sorry!

We shouldn't duplicate the function. Please either move the function to alloc.rs so it can be shared - or it's also okay to remove wasi again and leave it to future work.

[devnexen]

@rustbot ready

[RalfJung]

I assume the "WIP" in the PR description can be removed now.

[RalfJung]

Thanks for bearing with me through all these rounds of review. :) @bors r+

[bors]

:pushpin: Commit 78ed7ac2188ac41f9c9c0dd5bbd0a04095ae319f has been approved by RalfJung

It is now in the queue for this repository.

[bors]

:hourglass: Testing commit 78ed7ac2188ac41f9c9c0dd5bbd0a04095ae319f with merge b850b4dfba93bbf9a6e17f3c21692ba665dc196e...

[bors]

:sunny: Test successful - checks-actions Approved by: RalfJung Pushing b850b4dfba93bbf9a6e17f3c21692ba665dc196e to master...