illumos support #3198

devnexen · 2023-12-01T23:42:21Z

No description provided.

bors · 2024-01-07T21:39:42Z

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

RalfJung · 2024-02-17T11:59:30Z

@devnexen what is the status of this PR? You opened it as a draft, so I assume you are not currently looking for feedback. If you don't plan to work on it in the near future, it'd be better to close the PR -- this way the open PRs reflect things that people are actively working on. We can always reopen when you have time to get back to this!

bors · 2024-03-31T09:57:36Z

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

RalfJung · 2024-05-04T08:36:04Z

marked this pull request as ready for review

Was this meant to ask for review? Github doesn't seem to send notifications on this event, so I missed this -- sorry. In the future, please just post a comment when a PR is ready. You can write @rustbot ready to automatically update the labels as well.

RalfJung

The code mostly looks good, but is lacking test coverage.

Also, it will need a rebase to resolve conflicts.

src/shims/unix/solarish/foreign_items.rs

ci/ci.sh

RalfJung · 2024-05-04T10:02:31Z

Also, after rebasing, please add solaris/illumos to the unofficial target list in the README and set yourself as maintainer.

ci/ci.sh

RalfJung · 2024-05-04T13:28:50Z

Looks like something changed in std, and pthread_condattr_init is now required to make it into main.

RalfJung · 2024-05-04T13:47:16Z

Looks like something changed in std, and pthread_condattr_init is now required to make it into main.

Or, rather, previously only no_std was tested so the pre-main code was not even executed.

I think the PR should be adjusted to add only what is needed for main. getentropy, getrandom, threadname... those are all nice things to have eventually, but we have to walk before we can run.

The goal should be for MIRI_TEST_TARGET=x86_64-unknown-illumos ./miri test vec to pass.

devnexen · 2024-05-04T15:31:16Z

Looks like something changed in std, and pthread_condattr_init is now required to make it into main.

Do I need to implement something in particular to fix the out-of-bounds issue ?

RalfJung · 2024-05-04T15:48:55Z

That looks like the pthread offsets don't work for Solarish.

This seems to be a mutex, so this is the relevant comment:

miri/src/shims/unix/sync.rs

Lines 61 to 68 in baf32fd

    
           // pthread_mutex_t is between 24 and 48 bytes, depending on the platform. 
        
           // Our chosen memory layout for the emulated mutex (does not have to match the platform layout!): 
        
           // bytes 0-3: reserved for signature on macOS 
        
           // (need to avoid this because it is set by static initializer macros) 
        
           // bytes 4-7: mutex id as u32 or 0 if id is not assigned yet. 
        
           // bytes 12-15 or 16-19 (depending on platform): mutex kind, as an i32 
        
           // (the kind has to be at its offset for compatibility with static initializer macros)

It looks like something else is writing to the field that Miri uses to store the MutexId, and then Miri doesn't deal well with the invalid MutexId.

src/shims/unix/solarish/foreign_items.rs

RalfJung · 2024-05-04T19:11:01Z

tests/pass-dep/shims/libc-misc.rs

If you are keeping this diff, please also add libc-misc to CI.

But probably it's better to drop this diff for now, and focus on the core that's needed to get started.

src/shims/unix/sync.rs

RalfJung · 2024-05-04T20:57:35Z

Almost there. :) There's something wrong with the signature of pthread_join... I assume pthread_t has a different size on this platform than on others. We usually assume it is usize, but on Solarish it is u32.

So here

miri/src/shims/unix/thread.rs

Line 45 in af37aca

let thread_id = this.read_target_usize(thread)?;

you need to use something like this instead

let thread_id = this.read_scalar(thread)?.to_int(this.libc_ty_layout("pthread_t").size)?;

RalfJung · 2024-05-04T21:00:35Z

Also, is it possible there's something wrong with your system clock? Your commits show up as coming 20-50minutes from the future...^^

RalfJung

Thanks! This looks good, apart from a few nits. Why is it still marked WIP? :)

RalfJung · 2024-05-05T07:23:52Z

ci/ci.sh

@@ -146,6 +146,7 @@ case $HOST_TARGET in
    MIRI_TEST_TARGET=x86_64-unknown-freebsd run_tests_minimal $BASIC panic/panic concurrency/simple atomic threadname libc-getentropy libc-getrandom libc-misc fs env num_cpus
    MIRI_TEST_TARGET=i686-unknown-freebsd   run_tests_minimal $BASIC panic/panic concurrency/simple atomic threadname libc-getentropy libc-getrandom libc-misc fs env num_cpus
    MIRI_TEST_TARGET=aarch64-linux-android  run_tests_minimal $VERY_BASIC hello panic/panic
+    MIRI_TEST_TARGET=x86_64-unknown-illumos run_tests_minimal $VERY_BASIC hello panic/panic pthread-sync


Should we also run tests with x86_64-pc-solaris? You've added two OSes everywhere but we're only testing one of them.

RalfJung · 2024-05-05T07:24:15Z

README.md

@@ -227,6 +227,7 @@ degree documented below):
 - We have unofficial support (not maintained by the Miri team itself) for some further operating systems.
  - `freebsd`: **maintainer wanted**. Supports `std::env` and parts of `std::{thread, fs}`, but not `std::sync`.
  - `android`: **maintainer wanted**. Support very incomplete, but a basic "hello world" works.
+  - `solaris/illumos`: maintained by @devnexen. Support very incomplete, but a basic "hello world" works.


Suggested change

- `solaris/illumos`: maintained by @devnexen. Support very incomplete, but a basic "hello world" works.

- `solaris`/`illumos`: maintained by @devnexen. Support very incomplete, but a basic "hello world" works.

RalfJung · 2024-05-05T07:27:24Z

src/shims/unix/solarish/foreign_items.rs

+        let this = self.eval_context_mut();
+        match link_name.as_str() {
+            // errno
+            "__errno" => {


Is __errno (with two _ right)? libc and std seem to use triple-underscore ___errno.

If this is dead code (i.e., not needed for these tests), then please remove it entirely.

RalfJung · 2024-05-05T07:30:06Z

src/concurrency/thread.rs

+impl From<ThreadId> for i128 {
+    fn from(t: ThreadId) -> Self {
+        t.0.into()
+    }
+}


This is not used, is it?

RalfJung · 2024-05-05T08:25:38Z

Ah, Solaris in std has support for the stack guard, which seems to be disabled on illumos. That calls a bunch of very platform-specific functions (see this file).

This can be quite the rabbit hole of little functions that need to be added -- usually with stubs like this that don't actually do anything.

It's up to you whether you want to add that, or restrict the PR to illumos. (You should definitely test this locally though, iterating via CI will take way too long.)

RalfJung · 2024-05-05T09:18:28Z

You will probably have to extend this hack to also allow solaris, not just macos:

miri/src/shims/unix/mem.rs

Lines 47 to 49 in 10e8bb8

    
           if this.frame_in_std() && this.tcx.sess.target.os == "macos" && (flags & map_fixed) != 0 { 
        
               return Ok(Scalar::from_maybe_pointer(Pointer::from_addr_invalid(addr), this)); 
        
           }

It looks like stack_getbounds can just do nothing, but you're going to have to try this out. If not, this.machine.stack_addr and this.machine.stack_size are the fake values we are using elsewhere.

RalfJung · 2024-05-05T10:30:34Z

README.md

@@ -227,6 +227,7 @@ degree documented below):
 - We have unofficial support (not maintained by the Miri team itself) for some further operating systems.
  - `freebsd`: **maintainer wanted**. Supports `std::env` and parts of `std::{thread, fs}`, but not `std::sync`.
  - `android`: **maintainer wanted**. Support very incomplete, but a basic "hello world" works.
+  - `solaris` / `illumos`: maintained by @devnexen. Support very incomplete, but a basic "hello world" works.


Suggested change

- `solaris` / `illumos`: maintained by @devnexen. Support very incomplete, but a basic "hello world" works.

- `illumos`: maintained by @devnexen. Support very incomplete, but a basic "hello world" works.

RalfJung · 2024-05-05T11:07:17Z

Great, thanks!
@bors r+

bors · 2024-05-05T11:07:20Z

📌 Commit de54642 has been approved by RalfJung

It is now in the queue for this repository.

bors · 2024-05-05T11:08:26Z

⌛ Testing commit de54642 with merge 8528cc6...

bors · 2024-05-05T11:32:16Z

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

devnexen force-pushed the solarish_support branch 12 times, most recently from 8139798 to 92f989c Compare December 3, 2023 12:42

RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Feb 10, 2024

devnexen force-pushed the solarish_support branch from 92f989c to 4dac66a Compare February 17, 2024 14:19

devnexen marked this pull request as ready for review March 4, 2024 19:24