[WIP] solaris/illumos support.

[devnexen]

[bors]

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

[RalfJung]

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

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

[RalfJung]

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]

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

[RalfJung]

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

[RalfJung]

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]

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]

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

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

https://github.com/rust-lang/miri/blob/baf32fd5c019785c26836145a4b0eca54326217c/src/shims/unix/sync.rs#L61-L68

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.

[RalfJung]

Ah no, it's not something else writing... the problem is the layout needs to be compatible with the PTHREAD_MUTEX_INITIALIZER. For Solarish that's defined here. This is the type declaration:

    pub struct pthread_mutex_t {
        __pthread_mutex_flag1: u16,
        __pthread_mutex_flag2: u8,
        __pthread_mutex_ceiling: u8,
        __pthread_mutex_type: u16,
        __pthread_mutex_magic: u16,
        __pthread_mutex_lock: u64,
        __pthread_mutex_data: u64
    }

All of the fields Miri needs need to be initialized to 0. However, bytes 4-7 (where we store the Mutex ID) contains this __pthread_mutex_magic field that is not zero-initialized.

So we'll need to pick a different offset for the Mutex ID on solarish. The offset is currently duplicated in mutex_get_id and mutex_reset_id; the 4 there needs to become something else on solarish. It looks like 0 should work. In fact we should probably use 0 everywhere except for macOS.

[devnexen]

cool with your suggestion changes vec tests pass now.

[RalfJung]

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

https://github.com/rust-lang/miri/blob/af37aca6c05e13afa0acfbed53bda24dea8b7526/src/shims/unix/thread.rs#L45

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]

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

[RalfJung]

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]

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

https://github.com/rust-lang/miri/blob/10e8bb87fe416bc190426789032594aa1aa311f2/src/shims/unix/mem.rs#L47-L49

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]

Great, thanks! @bors r+

[bors]

:pushpin: Commit de5464233a0e430943a9e886817efefe81f72e90 has been approved by RalfJung

It is now in the queue for this repository.

[bors]

:hourglass: Testing commit de5464233a0e430943a9e886817efefe81f72e90 with merge 8528cc6dd0ee5ea6712eb46778e305096acc09ea...

[bors]

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