Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd Illumos support #31078
Conversation
rust-highfive
assigned
brson
Jan 21, 2016
This comment has been minimized.
This comment has been minimized.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
alexcrichton
reviewed
Jan 21, 2016
| let mut base = super::sunos_base::opts(); | ||
| base.pre_link_args.push("-m64".to_string()); | ||
| base.pre_link_args.push("-lsocket".to_string()); | ||
| base.pre_link_args.push("-lposix4".to_string()); |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 21, 2016
Member
This sorts of "default libraries" are typically linked where they're needed rather than by the compiler unconditionally. Do you know what symbols these are pulling in? I suspect they're only needed by the standard library so could the linkage directives go there instead?
This comment has been minimized.
This comment has been minimized.
nbaksalyar
Jan 21, 2016
Author
Contributor
Yes, these are mostly for socket functions and e.g. fork.
Could you please point out to the correct place to move it in libstd?
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 21, 2016
Member
Sure thing, they'll probably reside in rtdeps.rs.
Out of curiosity, is sunos posix? Or would a "true port" of libstd contain an entirely different implementation of std::sys basically (e.g. how Windows is entirely separate from Unix)
This comment has been minimized.
This comment has been minimized.
nbaksalyar
Jan 21, 2016
Author
Contributor
Oh, completely missed it, thank you!
is sunos posix?
Yes, absolutely. I believe historically it's derived from the Unix SVR4 codebase, and mostly compatible with BSD-like OSes. And, as I understand it, at least Illumos (the open source Solaris fork) strives to be maximally compatible with other *nixes, and it includes e.g. GCC toolkit by default instead of some proprietary compilers. Actually, I'm not even sure that this port would work on the original Sun/Oracle Solaris - there are much more diversions from standards.
This comment has been minimized.
This comment has been minimized.
alexcrichton
reviewed
Jan 21, 2016
| pub const F_UNLCK: libc::c_short = 3; | ||
| pub const F_SETLK: libc::c_int = 6; | ||
| pub const F_SETLKW: libc::c_int = 7; | ||
| } |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 21, 2016
Member
If you're feeling ambitious you could also feel free to remove all these definitions in favor of using the ones in libc, although that's fine to leave for another PR
alexcrichton
reviewed
Jan 21, 2016
| } else { | ||
| return NAN // log(-Inf) = NaN | ||
| } | ||
| ) |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 21, 2016
Member
Instead of a macro, could this be done with something like:
fn foo<F: Fn(f64) -> f64>(val: f64, f: F) -> f64 {
if !cfg!(target_os = "sunos") {
f(val)
} else {
// contents of this macro
}
}
This comment has been minimized.
This comment has been minimized.
nbaksalyar
Jan 21, 2016
Author
Contributor
Sure! I just wasn't sure about duplicate code & runtime checks.
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 21, 2016
Member
Oh these'll all be optimized away, and it's best to have as little #[cfg] or macro code as possible generally
alexcrichton
reviewed
Jan 21, 2016
| @@ -50,14 +50,14 @@ impl FileDesc { | |||
| Ok(ret as usize) | |||
| } | |||
|
|
|||
| #[cfg(not(target_env = "newlib"))] | |||
| #[cfg(not(any(target_env = "newlib", target_os = "sunos")))] | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
nbaksalyar
Jan 21, 2016
Author
Contributor
It does, but FIOCLEX apparently doesn't work there (although it's available). So the solution is to set the cloexex flag by the means of fcntl.
I figured it out through some indirect references, e.g.: https://forum.nginx.org/read.php?29,232210,232210 (notice the Solaris workaround).
This comment has been minimized.
This comment has been minimized.
alexcrichton
reviewed
Jan 21, 2016
| @@ -132,6 +139,32 @@ impl FromInner<raw::mode_t> for FilePermissions { | |||
| impl Iterator for ReadDir { | |||
| type Item = io::Result<DirEntry>; | |||
|
|
|||
| #[cfg(target_os = "sunos")] | |||
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 21, 2016
Member
Does sunos not have readdir_r? In posix at least the readdir function isn't thread safe (I believe) and can be harmful to use, so it would be best to share implementations here if possible.
This comment has been minimized.
This comment has been minimized.
nbaksalyar
Jan 21, 2016
Author
Contributor
There is a readdir_r function, but it's non-standard (i.e. it doesn't comply to POSIX). And if you refer to the manpage for readdir(3C) on Illumos, it says the following:
It is safe to use readdir() in a threaded application, so long as only
one thread reads from the directory stream at any given time. The
readdir() function is generally preferred over the readdir_r() function.
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 21, 2016
Member
Hm ok, but in the interest of sharing an implementation would it be possible to use the readdir_r function if it's present?
This comment has been minimized.
This comment has been minimized.
nbaksalyar
Jan 21, 2016
Author
Contributor
Hm. Well, it looks like there is a standard-compliant readdir_r, actually, though it seems to be linked in some special way. I'll try to figure it out, thanks!
This comment has been minimized.
This comment has been minimized.
nbaksalyar
Jan 26, 2016
Author
Contributor
@alexcrichton, as it seems, the implementation can't be shared - not because of readdir_r (it does have a standard-compliant version), but because of a quite interesting dirent structure. This struct def on Solaris/Illumos contains a zero-length array:
typedef struct dirent {
ino_t d_ino; /* "inode number" of entry */
off_t d_off; /* offset of disk directory entry */
unsigned short d_reclen; /* length of this record */
char d_name[1]; /* name of file */
} dirent_t;
Notice the d_name field of length 1. I asked a question on this issue on the Rust users forum, but haven't found a definite answer. Experiments showed that Rust supports such structures, but not when they're nested in another structure - as is the case with std::fs::DirEntry and libc::dirent.
So, I guess, a separate implementation would be required, and as it's OS-specific anyway, I used the Solaris/Illumos-specific readdir function, as recommended by the man page.
What do you think?
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 26, 2016
Member
Interesting! In that case if we need a whole separate implementation anyway then I'm fine sticking with readdir as a separate implementation. Can you add some documentation indicating that although this function is avoided on Linux it's ok to call on sunos?
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 26, 2016
Member
Er hm, actually on second though it may be best to call readdir_r anyway because we already have to store the path locally. I guess whatever's easiest really
This comment has been minimized.
This comment has been minimized.
nbaksalyar
Jan 27, 2016
Author
Contributor
it may be best to call readdir_r anyway because we already have to store the path locally
Well, in this case I guess it's required to pre-allocate memory to store the path - the specific instructions from the man page are:
The caller must allocate storage pointed to by entry to be large enough
for a dirent structure with an array of char d_name member containing at
least NAME_MAX (that is, pathconf(directory, _PC_NAME_MAX)) plus one
elements. (_PC_NAME_MAX is defined in <unistd.h>.)
I.e. that'd also require a separate handling too, as it seems.
I'm not quite sure about what would work best here, TBH, so for now I've added some comments describing the current implementation. Please let me know if it requires polishing or a better implementation!
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 27, 2016
Member
Yeah it seems like that won't necessarily be any less onerous than this is already doing, so let's stick to what you've implemented here.
alexcrichton
reviewed
Jan 21, 2016
| } else { | ||
| let filename = CStr::from_ptr(path).to_bytes(); | ||
| if filename[0] == b'/' { | ||
| Ok(PathBuf::from(<OsStr as OsStrExt>::from_bytes(filename))) |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 21, 2016
Member
You may be able to lift these &[u8] -> &OsStr conversions outside of the if branch as it happens in both branches
alexcrichton
reviewed
Jan 21, 2016
| } else { | ||
| // Prepend current working directory to the path if | ||
| // it doesn't contain an absolute pathname. | ||
| return getcwd().map(|cwd| cwd.join(<OsStr as OsStrExt>::from_bytes(filename))) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Quite exciting, thanks @nbaksalyar! |
This comment has been minimized.
This comment has been minimized.
|
Thanks for the review, Alex! :) |
brson
reviewed
Jan 22, 2016
| @@ -1,5 +1,13 @@ | |||
| #!/bin/sh | |||
|
|
|||
| # /bin/sh on Solaris is not a POSIX compatible shell, but /usr/bin/ksh is. | |||
This comment has been minimized.
This comment has been minimized.
brson
reviewed
Jan 22, 2016
| export POSIX_SHELL | ||
| exec /usr/bin/bash $0 "$@" | ||
| fi | ||
| unset POSIX_SHELL # clear it so if we invoke other scripts, they run as ksh as well |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
lgtm r? @alexcrichton |
rust-highfive
assigned
alexcrichton
and unassigned
brson
Jan 22, 2016
alexcrichton
reviewed
Jan 26, 2016
| self // log2(Inf) = Inf | ||
| } else { | ||
| NAN // log2(-Inf) = NaN | ||
| } |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 26, 2016
Member
Ah sorry I was just indicating that the macro could probably be removed in favor of generic functions and/or otherwise non-meta-programming pieces. Could the duplication between the functions here be reduced?
This comment has been minimized.
This comment has been minimized.
nbaksalyar
Jan 26, 2016
Author
Contributor
Oh, sorry, got it - I think it should be possible to just pass a reference to a corresponding intrinsic function. Will give it a try in a moment.
EDIT: Actually, it turns out I totally missed your comment on this regard - sorry, will fix it. Thanks! :)
This comment has been minimized.
This comment has been minimized.
nbaksalyar
Jan 27, 2016
Author
Contributor
It hasn't worked with a generic signature fn foo<F: Fn(f64) -> f64>(val: f64, f: F) -> f64, because the intrinsic functions are unsafe & extern - they don't implement the Fn trait. However, it worked this way: fn foo(self, log_fn: unsafe extern "rust-intrinsic" fn(f64) -> f64) -> f64 - i.e., with a completely defined type in a function reference.
Would this work as a solution, or should I find a better way?
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 27, 2016
Member
Ah you may want to all it like:
foo(val, |f| unsafe { intrinsics::doit(f) })(or something like that). I think there are other bugs for taking intrinsics by-value unfortunately
alexcrichton
reviewed
Jan 26, 2016
|
|
||
| let ret = DirEntry { | ||
| entry: *entry_ptr, | ||
| name: Arc::new(::slice::from_raw_parts(name as *const u8, |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 26, 2016
Member
You can probably elide the Arc here in favor of just Box<[u8]> (no need to Clone a DirEntry)
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
Ok cool, I think this is getting pretty close to being good-to-go. I've got a few final questions about this:
|
This comment has been minimized.
This comment has been minimized.
potatosalad
commented
Jan 27, 2016
|
@alexcrichton I might be able to add a little explanation to your questions:
The So, it's essentially specifying that it can run on a "Solaris 11" compatible system. The same binary built for a current illumos system in many cases will run just fine on an Oracle Solaris 11 system without an issue.
I don't know about anywhere that it's an official standard, but based on Google search results alone (which I'm sure can always be trusted as a metric
The difference in terms of "solaris" and "sunos" might be more useful in the future, but are really no different than "darwin" and "macos" or "mingw32" and "windows". The benefit is that if there is ever a future target triple for something like However, beyond that I don't know if there is any explicit reason to use "sunos" over "solaris" for the |
This comment has been minimized.
This comment has been minimized.
@potatosalad has already answered them all, but I would add my 2c:
Other than Google results, that's the standard build target for GCC on Illumos (only with addition of version 2.11). E.g. compiler tools are called like
@potatosalad is correct, it's more like "darwin". Solaris and Illumos self-identify with Technically, I think But any would work here, so let me know if you want me to change it to some better name. Hopefully that explains it a bit. |
This comment has been minimized.
This comment has been minimized.
|
Thanks for the info! That's certainly quite enlightenting. Would it be possible to not pass down 2.11 to LLVM then? I don't think we do this for other platforms, and then LLVM would pick the "default level" I guess in theory. Or is solaris different where the OS version has major changes in ABI for things like thread locals each revision? Also after some discussion with @brson, we think that the |
This comment has been minimized.
This comment has been minimized.
Sure! I will make required changes and resolve the merge conflict then. |
nbaksalyar
force-pushed the
nbaksalyar:illumos
branch
from
f76db06
to
a69f3ee
Jan 28, 2016
nbaksalyar
referenced this pull request
Jan 29, 2016
Merged
Renames "sunos" to "solaris" (as a part of PR to add Illumos support to Rust) #161
This comment has been minimized.
This comment has been minimized.
|
|
nbaksalyar
added some commits
Jan 21, 2016
nbaksalyar
added some commits
Jan 28, 2016
nbaksalyar
force-pushed the
nbaksalyar:illumos
branch
from
55706f2
to
bb6e646
Jan 31, 2016
This comment has been minimized.
This comment has been minimized.
|
I've rebased the commits - should be ready to go! |
alexcrichton
reviewed
Jan 31, 2016
| @@ -25,6 +24,10 @@ use sys::platform::raw; | |||
| use sys::{cvt, cvt_r}; | |||
| use sys_common::{AsInner, FromInner}; | |||
| use vec::Vec; | |||
| #[cfg(target_os = "solaris")] | |||
| use core_collections::borrow::ToOwned; | |||
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 31, 2016
Member
I think you can remove the #[cfg] and the one below by just adding use prelude::v1::* to the top of this file (these are both in the prelude), and then you don't need the #[cfg] as well I believe.
This comment has been minimized.
This comment has been minimized.
|
Looks good to me, thanks @nbaksalyar! I had just one tiny nit but it's not very relevant, so I'm gonna send this to bors: |
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Jan 31, 2016
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
The issue should be fixed by now. Sorry it took so long - it turned out that I was building the bootstrap compiler from an incorrect base (the issue is described in #31142). So actually to run I've yet to make the new stage0 work correctly (it fails with SIGILL for now, for some reason). I guess the reason is that the codebase with the latest commits, on which I based my work, has diverged substantially. I'll try to find & fix the reason, but actually it might be better to wait for a snapshot update before rebuilding the bootstrapping compiler and adding it to the list of snapshots. Other than that, it should work correctly, as the only major change from my previous snapshot is renaming "sunos" to "solaris" (and that's why it wouldn't work). |
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Feb 3, 2016
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bors
merged commit fae883c
into
rust-lang:master
Feb 4, 2016
This was referenced Feb 4, 2016
This comment has been minimized.
This comment has been minimized.
|
Awesome! @alexcrichton, @potatosalad, and everyone - thanks for your help! :) |
This comment has been minimized.
This comment has been minimized.
dhuseby
commented
Feb 9, 2016
|
@nbaksalyar have you successfully bootstrapped rustc on illumos yet? |
This comment has been minimized.
This comment has been minimized.
|
@dhuseby |
nbaksalyar
deleted the
nbaksalyar:illumos
branch
Feb 21, 2016
This comment has been minimized.
This comment has been minimized.
dhuseby
commented
Mar 21, 2016
|
@nbaksalyar any update on the WIP branch? |
This comment has been minimized.
This comment has been minimized.
|
@dhuseby |
This comment has been minimized.
This comment has been minimized.
|
@nbaksalyar oh I should also mention that we've recently got cross-compiled nightlies from Linux to FreeBSD/NetBSD up and running, so we have nightly builds of rustc and the standard library for FreeBSD and NetBSD. If there's a cross compiler from Linux to Illumos then I'd be game to set one up as well for Illumos! |
This comment has been minimized.
This comment has been minimized.
cneira
commented
Jan 12, 2018
|
@alexcrichton |
This comment has been minimized.
This comment has been minimized.
jperkin
commented
Jan 12, 2018
|
FWIW we've been shipping rust/cargo packages in SmartOS for a while now, currently 1.22.1 but 1.23 should be ready soon. Our binaries should work on any illumos platform built after 20141030. |
faho
referenced this pull request
Jan 2, 2019
Closed
Error: Wrong color in test at index 3 in text (expected 0x104, actual 0x1): cd test/fish_highlight_test #5458
This comment has been minimized.
This comment has been minimized.
despair86
commented
Jan 21, 2019
•
not since 2.11, but a big chunk of userspace is still 32-bit (GNU CC always builds a 32-bit multilib-enabled compiler, for instance) @nbaksalyar: do you still have the target spec JSON files originally used to bootstrap this port? Looking into an |
nbaksalyar commentedJan 21, 2016
This pull request adds support for Illumos-based operating systems: SmartOS, OpenIndiana, and others. For now it's x86-64 only, as I'm not sure if 32-bit installations are widespread. This PR is based on #28589 by @potatosalad, and also closes #21000, #25845, and #25846.
Required changes in libc are already merged: rust-lang/libc#138
Here's a snapshot required to build a stage0 compiler:
https://s3-eu-west-1.amazonaws.com/nbaksalyar/rustc-sunos-snapshot.tar.gz
It passes all checks from
make check.There are some changes I'm not quite sure about, e.g. macro usage in
src/libstd/num/f64.rsandDirEntrystructure insrc/libstd/sys/unix/fs.rs, so any comments on how to rewrite it better would be greatly appreciated.Also, LLVM configure script might need to be patched to build it successfully, or a pre-built libLLVM should be used. Some details can be found here: https://llvm.org/bugs/show_bug.cgi?id=25409
Thanks!
r? @brson