Illumos part3 #3575

devnexen · 2024-05-05T21:55:48Z

RalfJung · 2024-05-06T05:49:41Z

src/shims/unix/solarish/foreign_items.rs

+
+                // memalign requires the alignment to be a power of 2
+                // and to be, at least, of word size.
+                // http://docs.oracle.com/cd/E88353_01/html/E37743/memalign-3c.html


This URL shows an error 404 for me.

I found it at https://docs.oracle.com/cd/E88353_01/html/E37843/memalign-3c.html.

It doesn't say what happens when the alignment is violated though -- is it UB or a safe error?

Ah no it does say something:

The memalign() function may also fail if:

EINVAL

The value of the alignment parameter is not a power of two multiple of sizeof(void *), or the size parameter is zero, or the combination of the two would lead to an integer overflow.

But how is the error returned...?
This is where it'd probably be good to experiment with an actual Solaris/Illumos system and see what happens on a bad alignment.^^

I just tried on my illumos instance, it returns null and emits EINVAL. Same with 0 even if alignment is correct.

What does "emit" mean"?

In this document and in man memalign on my system it says that memalign returns NULL and sets errno on error.

RalfJung · 2024-05-06T05:55:21Z

src/shims/unix/solarish/foreign_items.rs

+                // memalign requires the alignment to be a power of 2
+                // and to be, at least, of word size.
+                // http://docs.oracle.com/cd/E88353_01/html/E37743/memalign-3c.html
+                if !align.is_power_of_two() || align < this.pointer_size().bytes() || size == 0 {


Suggested change

if !align.is_power_of_two() || align < this.pointer_size().bytes() || size == 0 {

// The docs also guarantee a null pointer is returned when the size is 0.

if !align.is_power_of_two() || align < this.pointer_size().bytes() || size == 0 {

RalfJung · 2024-05-06T06:26:27Z

That CI failure is actually a legit bug in std, I think: rust-lang/rust#124787.

devnexen · 2024-05-06T06:39:30Z

That CI failure is actually a legit bug in std, I think: rust-lang/rust#124787.

alright .. same will wait the getrandom part being merged to pick up this PR.

RalfJung · 2024-05-06T07:24:45Z

src/shims/unix/solarish/foreign_items.rs

+                // and a null pointer is returned.
+                // https://docs.oracle.com/cd/E88353_01/html/E37843/memalign-3c.html
+                if !align.is_power_of_two() || align < this.pointer_size().bytes() || size == 0 {
+                    this.set_last_error(this.eval_libc("EINVAL"))?;


I don't think size 0 should lead to EINVAL.

it does though.

Ah, it is listed in the error conditions below: "or the size parameter is zero". But it's not listed above. Wow that's bad documentation.^^

Please point this out in the comments.

bors · 2024-05-06T09:52:42Z

☔ The latest upstream changes (presumably #3579) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2024-05-12T10:47:27Z

src/shims/unix/solarish/foreign_items.rs

@@ -18,6 +19,30 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
    ) -> InterpResult<'tcx, EmulateItemResult> {
        let this = self.eval_context_mut();
        match link_name.as_str() {
+            // Allocation
+            "memalign" => {


With rust-lang/rust#124798, memalign will not be called on Solarish any more.

You can still add it here if you want, but then you should also add a test that calls this function (directly via libc). Alternatively, remove it from this PR.

This comment is not resolved yet: you're adding a new shim that's entirely untested. That's not going to fly.

RalfJung · 2024-05-12T10:49:37Z

CI fails because rust-lang/rust#124766 isn't picked up automatically; I have to do a rustc-pull for that.

But let's wait for rust-lang/rust#124798 to land first, so we can do it all at once.

RalfJung · 2024-05-12T15:05:17Z

#3602 landed, so if you rebase again it should pick up your rustc PRs.

devnexen · 2024-05-12T17:00:01Z

@rustbot ready

RalfJung · 2024-05-12T17:50:49Z

ci/ci.sh

@@ -148,8 +148,8 @@ case $HOST_TARGET in
    BASIC="$VERY_BASIC hello hashmap alloc align" # ensures we have the shims for stdout and basic data structures
    TEST_TARGET=x86_64-unknown-freebsd run_tests_minimal $BASIC panic/panic concurrency/simple atomic threadname libc-mem libc-misc libc-random libc-time fs env num_cpus
    TEST_TARGET=i686-unknown-freebsd   run_tests_minimal $BASIC panic/panic concurrency/simple atomic threadname libc-mem libc-misc libc-random libc-time fs env num_cpus
-    TEST_TARGET=x86_64-unknown-illumos run_tests_minimal $VERY_BASIC hello panic/panic concurrency/simple pthread-sync libc-mem libc-misc libc-random
-    TEST_TARGET=x86_64-pc-solaris run_tests_minimal $VERY_BASIC hello panic/panic concurrency/simple pthread-sync libc-mem libc-misc libc-random
+    TEST_TARGET=x86_64-unknown-illumos run_tests_minimal $VERY_BASIC hello panic/panic concurrency/simple pthread-sync libc-mem libc-misc libc-random alloc hashmap


Suggested change

TEST_TARGET=x86_64-unknown-illumos run_tests_minimal $VERY_BASIC hello panic/panic concurrency/simple pthread-sync libc-mem libc-misc libc-random alloc hashmap

TEST_TARGET=x86_64-unknown-illumos run_tests_minimal $BASIC hello panic/panic concurrency/simple pthread-sync libc-mem libc-misc libc-random

same for solaris

RalfJung · 2024-05-12T17:51:21Z

tests/fail/environ-gets-deallocated.rs

This test isn't actually run by this PR, is it? same for the environ change. Did you mean to add env to the CI filters?

RalfJung · 2024-05-12T17:51:36Z

@rustbot author

src/shims/unix/foreign_items.rs

fixing part of `miri test alloc/hashmap`.

devnexen · 2024-05-12T20:10:13Z

@rustbot ready

RalfJung · 2024-05-12T20:32:28Z

Looks good, thanks!
@bors r+

bors · 2024-05-12T20:32:32Z

📌 Commit 20a0257 has been approved by RalfJung

It is now in the queue for this repository.

bors · 2024-05-12T20:33:39Z

⌛ Testing commit 20a0257 with merge 8e36860...

bors · 2024-05-12T20:56:33Z

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

RalfJung reviewed May 6, 2024

View reviewed changes

RalfJung mentioned this pull request May 6, 2024

Solarish: standard allocator uses memalign incorrectly rust-lang/rust#124787

Closed

devnexen force-pushed the illumos_part3 branch from f9de4fd to 25eaa96 Compare May 6, 2024 06:39

RalfJung reviewed May 6, 2024

View reviewed changes

RalfJung added the S-blocked Status: blocked on something happening somewhere else label May 6, 2024

devnexen force-pushed the illumos_part3 branch from 25eaa96 to 6d3db2b Compare May 12, 2024 08:39

RalfJung reviewed May 12, 2024

View reviewed changes

devnexen force-pushed the illumos_part3 branch 2 times, most recently from 7083a45 to 84d965c Compare May 12, 2024 16:32

devnexen marked this pull request as ready for review May 12, 2024 16:59

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-blocked Status: blocked on something happening somewhere else labels May 12, 2024

RalfJung reviewed May 12, 2024

View reviewed changes

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 12, 2024

devnexen force-pushed the illumos_part3 branch 2 times, most recently from f02f5a1 to d2c2082 Compare May 12, 2024 18:26

RalfJung reviewed May 12, 2024

View reviewed changes

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

devnexen force-pushed the illumos_part3 branch from d2c2082 to 20a0257 Compare May 12, 2024 18:59

further illumos/solaris support.

20a0257

fixing part of `miri test alloc/hashmap`.

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 12, 2024

bors merged commit 8e36860 into rust-lang:master May 12, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Illumos part3 #3575

Illumos part3 #3575

devnexen commented May 5, 2024 •

edited by RalfJung

RalfJung May 6, 2024

RalfJung May 6, 2024

RalfJung May 6, 2024

devnexen May 6, 2024 •

edited

RalfJung May 6, 2024

RalfJung May 6, 2024

RalfJung commented May 6, 2024

devnexen commented May 6, 2024

RalfJung May 6, 2024

devnexen May 6, 2024

RalfJung May 6, 2024

bors commented May 6, 2024

RalfJung May 12, 2024 •

edited

RalfJung May 12, 2024

RalfJung commented May 12, 2024

RalfJung commented May 12, 2024

devnexen commented May 12, 2024

RalfJung May 12, 2024

RalfJung May 12, 2024

RalfJung commented May 12, 2024

devnexen commented May 12, 2024

RalfJung commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

	if !align.is_power_of_two() \|\| align < this.pointer_size().bytes() \|\| size == 0 {
	// The docs also guarantee a null pointer is returned when the size is 0.
	if !align.is_power_of_two() \|\| align < this.pointer_size().bytes() \|\| size == 0 {

	TEST_TARGET=x86_64-unknown-illumos run_tests_minimal $VERY_BASIC hello panic/panic concurrency/simple pthread-sync libc-mem libc-misc libc-random alloc hashmap
	TEST_TARGET=x86_64-unknown-illumos run_tests_minimal $BASIC hello panic/panic concurrency/simple pthread-sync libc-mem libc-misc libc-random

Illumos part3 #3575

Illumos part3 #3575

Conversation

devnexen commented May 5, 2024 • edited by RalfJung

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devnexen May 6, 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 6, 2024

devnexen commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 6, 2024

RalfJung May 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented May 12, 2024

RalfJung commented May 12, 2024

devnexen commented May 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented May 12, 2024

devnexen commented May 12, 2024

RalfJung commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

devnexen commented May 5, 2024 •

edited by RalfJung

devnexen May 6, 2024 •

edited

RalfJung May 12, 2024 •

edited