support aligned_alloc for unixes. #3585

devnexen · 2024-05-07T22:13:06Z

RalfJung · 2024-05-08T06:43:12Z

Thanks for the PR!

This is for #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.

src/shims/unix/foreign_items.rs

tests/pass-dep/libc/libc-mem.rs

src/shims/unix/foreign_items.rs

tests/pass-dep/libc/libc-mem.rs

RalfJung · 2024-05-08T07:29:46Z

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 · 2024-05-09T07:27:15Z

On size 0 this should probably behave like #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 · 2024-05-09T07:34:38Z

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 · 2024-05-09T08:03:20Z

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

RalfJung · 2024-05-09T13:33:32Z

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

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

RalfJung · 2024-05-09T13:54:32Z

I did a bit of googling and found evidence that it exists for all platforms we support (see the links in rust-lang/libc#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.

src/shims/unix/foreign_items.rs

tests/pass-dep/libc/libc-mem.rs

src/shims/unix/foreign_items.rs

devnexen · 2024-05-15T20:52:49Z

@rustbot ready

src/shims/unix/foreign_items.rs

RalfJung · 2024-05-16T06:16:34Z

@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:

There may be more.

RalfJung · 2024-05-17T06:25:45Z

src/shims/unix/foreign_items.rs

+                // FreeBSD: https://man.freebsd.org/cgi/man.cgi?query=aligned_alloc&apropos=0&sektion=3&manpath=FreeBSD+9-current&format=html
+                match size.checked_rem(align) {
+                    Some(0) if align.is_power_of_two() => {
+                            let align = this.min_align(align, size);


You can just use align.max(this.malloc_align(size)), no need to write a new function for that

RalfJung · 2024-05-17T06:28:54Z

src/shims/unix/foreign_items.rs

+                // The size must be a multiple of the alignment.
+                // Alignment must be a power of 2, and "supported by the implementation".


Suggested change

// The size must be a multiple of the alignment.

// Alignment must be a power of 2, and "supported by the implementation".

// Alignment must be a power of 2, and "supported by the implementation".

// We decide that "supported by the implementation" means that the

// size must be a multiple of the alignment. (This restriction seems common

// enough that it is stated on <https://en.cppreference.com/w/c/memory/aligned_alloc>

// as a general rule, but the actual standard has no such rule.)

RalfJung · 2024-05-17T06:29:12Z

src/shims/unix/foreign_items.rs

+                // Alignment must be a power of 2, and "supported by the implementation".
+                // If any of these are violated, we have to return NULL.
+                // All fundamental alignments must be supported.
+                // We decide that our implementation supports all alignments.


Suggested change

// We decide that our implementation supports all alignments.

RalfJung · 2024-05-17T06:29:41Z

src/shims/unix/foreign_items.rs

+                // We decide that our implementation supports all alignments.
+                //
+                // macOS and Illumos are buggy in that they require the alignment
+                // to be at least the size of a pointer. We do not emulate those platform bugs.


Suggested change

// to be at least the size of a pointer. We do not emulate those platform bugs.

// to be at least the size of a pointer, so they do not support all fundamental

// alignments. We do not emulate those platform bugs.

RalfJung · 2024-05-17T06:29:57Z

src/shims/unix/foreign_items.rs

+                // to be at least the size of a pointer. We do not emulate those platform bugs.
+                //
+                // Linux also sets errno to EINVAL, but that's non-standard behavior that we do not
+                // emulate/


Suggested change

// emulate/

// emulate.

RalfJung · 2024-05-17T06:30:35Z

tests/fail-dep/libc/aligned_alloc.rs

Please name the test aligned_alloc_size_zero_leak or so to indicate what it tests.

RalfJung · 2024-05-17T06:30:54Z

tests/pass-dep/libc/libc-mem.rs

@@ -241,6 +241,42 @@ fn test_reallocarray() {
    }
 }

+#[cfg(not(any(target_os = "windows", target_os = "wasi")))]


Why not on wasi?

I ve seen unsupported operation for this platform.

Where have you seen that? libc even declares aligned_alloc for wasi so I'd think that it exists there.

RalfJung · 2024-05-17T06:32:57Z

tests/pass-dep/libc/libc-mem.rs

+    // alignment lesser than a word but still a successful allocation
+    unsafe {
+        let p = aligned_alloc(1, 4);
+        assert!(!p.is_null());
+        assert!(p.is_aligned_to(4));
+        libc::free(p);
+    }
+
+    // finally ..
+    unsafe {
+        let p = aligned_alloc(16, 16);
+        assert!(!p.is_null());
+        assert!(p.is_aligned_to(16));
+        libc::free(p);
+    }


Maybe put these in a for _ in 0..16 or so, because the actual alignment is random so we want to get some extra test coverage.

RalfJung · 2024-05-17T06:34:34Z

tests/fail-dep/libc/aligned_alloc.rs

+        fn aligned_alloc(alignment: libc::size_t, size: libc::size_t) -> *mut libc::c_void;
+    }
+
+    unsafe {


Suggested change

unsafe {

// Make sure even zero-sized allocations need to be freed.

unsafe {

RalfJung · 2024-05-17T16:06:50Z

tests/pass-dep/libc/libc-mem.rs

@@ -241,6 +241,42 @@ fn test_reallocarray() {
    }
 }

+#[cfg(not(any(target_os = "windows", target_os = "wasi")))]


Where have you seen that? libc even declares aligned_alloc for wasi so I'd think that it exists there.

devnexen · 2024-05-17T17:45:00Z

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

RalfJung · 2024-05-17T18:35:28Z

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 · 2024-05-17T19:58:44Z

@rustbot ready

RalfJung · 2024-05-18T11:08:17Z

tests/pass-dep/libc/libc-mem.rs

+    for i in 0..16 {
+        unsafe {
+            let p = aligned_alloc(i, i);


I don't understand the point of this -- most of these numbers aren't even valid alignments?

I asked you to run the exact same test 16 times in a loop as allocation involves non-determinism.

ah ok I did not get you.

RalfJung · 2024-05-18T11:08:29Z

tests/fail-dep/libc/aligned_alloc_size_zero_leak.rs

+    // Make sure event zero-sized allocations need to be freed.
+


Suggested change

// Make sure event zero-sized allocations need to be freed.

// Make sure even zero-sized allocations need to be freed.

RalfJung · 2024-05-19T09:43:13Z

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

RalfJung · 2024-05-19T09:49:18Z

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

bors · 2024-05-19T09:49:20Z

📌 Commit 78ed7ac has been approved by RalfJung

It is now in the queue for this repository.

bors · 2024-05-19T09:50:27Z

⌛ Testing commit 78ed7ac with merge b850b4d...

bors · 2024-05-19T10:13:34Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing b850b4d to master...

devnexen force-pushed the aligned_alloc branch 2 times, most recently from 0552e7a to 3e1a99b Compare May 8, 2024 05:13

RalfJung reviewed May 8, 2024

View reviewed changes

src/shims/unix/foreign_items.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 8, 2024

View reviewed changes

src/shims/unix/foreign_items.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 8, 2024

View reviewed changes

tests/pass-dep/libc/libc-mem.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 8, 2024

View reviewed changes

src/shims/unix/foreign_items.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 8, 2024

View reviewed changes

tests/pass-dep/libc/libc-mem.rs Show resolved Hide resolved

rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label May 9, 2024

RalfJung reviewed May 9, 2024

View reviewed changes

src/shims/unix/foreign_items.rs Show resolved Hide resolved

tests/pass-dep/libc/libc-mem.rs Show resolved Hide resolved

tests/pass-dep/libc/libc-mem.rs Outdated Show resolved Hide resolved

RalfJung changed the title ~~[WIP] support aligned_alloc for unixes support.~~ [WIP] support aligned_alloc for unixes. May 9, 2024

RalfJung reviewed May 11, 2024

View reviewed changes

src/shims/unix/foreign_items.rs Outdated Show resolved Hide resolved

devnexen force-pushed the aligned_alloc branch 2 times, most recently from a0914de to de0cb7a Compare May 15, 2024 19:13

devnexen marked this pull request as ready for review May 15, 2024 20:52

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels May 15, 2024

RalfJung reviewed May 16, 2024

View reviewed changes

src/shims/unix/foreign_items.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels May 16, 2024

devnexen force-pushed the aligned_alloc branch 2 times, most recently from ab9f83a to 7ea5ccb Compare May 16, 2024 19:41

devnexen force-pushed the aligned_alloc branch from 7ea5ccb to 636e8be Compare May 16, 2024 20:04

RalfJung reviewed May 17, 2024

View reviewed changes

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels May 17, 2024

RalfJung reviewed May 18, 2024

View reviewed changes

support aligned_alloc for unixes support.

d654b73

devnexen force-pushed the aligned_alloc branch from 9d0eb2a to d654b73 Compare May 18, 2024 11:45

RalfJung changed the title ~~[WIP] support aligned_alloc for unixes.~~ support aligned_alloc for unixes. May 19, 2024

a bit of refactoring and tweak the aligned-allocation tests

78ed7ac

bors merged commit b850b4d into rust-lang:master May 19, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support aligned_alloc for unixes. #3585

support aligned_alloc for unixes. #3585

devnexen commented May 7, 2024 •

edited by RalfJung

RalfJung commented May 8, 2024 •

edited

RalfJung commented May 8, 2024 via email •

edited

RalfJung commented May 9, 2024

RalfJung commented May 9, 2024

RalfJung commented May 9, 2024

RalfJung commented May 9, 2024

RalfJung commented May 9, 2024 •

edited

devnexen commented May 15, 2024

RalfJung commented May 16, 2024 •

edited

RalfJung May 17, 2024

RalfJung May 17, 2024

RalfJung May 17, 2024

RalfJung May 17, 2024

RalfJung May 17, 2024

RalfJung May 17, 2024 •

edited

RalfJung May 17, 2024

devnexen May 17, 2024

RalfJung May 17, 2024

RalfJung May 17, 2024

RalfJung May 17, 2024

RalfJung May 17, 2024

devnexen commented May 17, 2024 •

edited

RalfJung commented May 17, 2024 via email

devnexen commented May 17, 2024

RalfJung May 18, 2024 •

edited

devnexen May 18, 2024

RalfJung May 18, 2024

RalfJung commented May 19, 2024

RalfJung commented May 19, 2024

bors commented May 19, 2024

bors commented May 19, 2024

bors commented May 19, 2024

		// The size must be a multiple of the alignment.
		// Alignment must be a power of 2, and "supported by the implementation".

-                // The size must be a multiple of the alignment.
-                // Alignment must be a power of 2, and "supported by the implementation".
+                // Alignment must be a power of 2, and "supported by the implementation".
+                // We decide that "supported by the implementation" means that the
+                // size must be a multiple of the alignment. (This restriction seems common
+                // enough that it is stated on <https://en.cppreference.com/w/c/memory/aligned_alloc>
+                // as a general rule, but the actual standard has no such rule.)

	// to be at least the size of a pointer. We do not emulate those platform bugs.
	// to be at least the size of a pointer, so they do not support all fundamental
	// alignments. We do not emulate those platform bugs.

	unsafe {
	// Make sure even zero-sized allocations need to be freed.
	unsafe {

	// Make sure event zero-sized allocations need to be freed.
	// Make sure even zero-sized allocations need to be freed.

support aligned_alloc for unixes. #3585

support aligned_alloc for unixes. #3585

Conversation

devnexen commented May 7, 2024 • edited by RalfJung

RalfJung commented May 8, 2024 • edited

RalfJung commented May 8, 2024 via email • edited

RalfJung commented May 9, 2024

RalfJung commented May 9, 2024

RalfJung commented May 9, 2024

RalfJung commented May 9, 2024

RalfJung commented May 9, 2024 • edited

devnexen commented May 15, 2024

RalfJung commented May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devnexen commented May 17, 2024 • edited

RalfJung commented May 17, 2024 via email

devnexen commented May 17, 2024

RalfJung May 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented May 19, 2024

RalfJung commented May 19, 2024

bors commented May 19, 2024

bors commented May 19, 2024

bors commented May 19, 2024

devnexen commented May 7, 2024 •

edited by RalfJung

RalfJung commented May 8, 2024 •

edited

RalfJung commented May 8, 2024 via email •

edited

RalfJung commented May 9, 2024 •

edited

RalfJung commented May 16, 2024 •

edited

RalfJung May 17, 2024 •

edited

devnexen commented May 17, 2024 •

edited

RalfJung May 18, 2024 •

edited