Adding OpenBSD support to rust #21754
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (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. 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 CONTRIBUTING.md for more information. |
|
This is relevant to my interests. Oh, and i also tested this on OpenBSD 5.6/amd64, and -current/soon-to-be-5.7/amd64. Built and worked fine with the provided bootstraps using the work-in-progress port in https://github.com/jasperla/openbsd-wip/tree/master/lang/rustc/. Looking forward to see this merged upstream, so that we can import the port in OpenBSD's ports-tree. |
| @@ -263,6 +266,16 @@ mod dl { | |||
| symbol: *const libc::c_char) -> *mut libc::c_void; | |||
| fn dlclose(handle: *mut libc::c_void) -> libc::c_int; | |||
| } | |||
|
|
|||
| #[cfg(target_os = "openbsd")] | |||
There was a problem hiding this comment.
These look pretty similar to the block above, what was the reason for duplication?
There was a problem hiding this comment.
The extern block below is missing the #[link_name="dl"] attribute on the other block. I'm not sure what the purpose of #[link_name] on an extern block is (I think it should be #[link(name="dl")] here?).
There was a problem hiding this comment.
Judging by the man page on OpenBSD you don't need to explicitly link with -ldl: http://www.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man3/dladdr.3?query=dlopen&sec=3
There was a problem hiding this comment.
Yeah I think that #[link_name] isn't ready any more and could probably be removed from the above block as well. A linkage of -ldl should probably happen in rtdeps.rs anyway
There was a problem hiding this comment.
the dl library don't exist under OpenBSD. A linkage to -ldl will fail. The dl* function are provide by libc.
BSD system generally don't use libdl for dl function:
DragonFly BSD (and FreeBSD) have the dlopen/dlfcn/... routines located in libc. However many applications and their configure scripts look for these routines in libdl. This adds a stub libdl with no subroutines, so that the configure scripts pass. The dl* routines are automatically available via libc.
Under OpenBSD, this stub don't exist.
There was a problem hiding this comment.
Since #[link_name="dl"] is actually a no-op on the first block, I think you could remove the second one with the #[cfg(target_os="openbsd")] and take off the #[link_name] attribute from the first block.
There was a problem hiding this comment.
#[link_name="dl] is no no-op but not for all target_os:
- for OpenBSD I am sure (tested)
- for FreeBSD and DragonFly, it should (according to the commit cited previously)
- for others (linux, android, macos, ios): I don't known
The problem is the prototype of functions is the same, but under Linux (and some others), it comes from libdl, whereas under some others (freebsd, dragonfly, openbsd) it comes from libc (but freebsd & dragonfly define a stub libdl).
If I remove the #[link_name="dl"] (and the second block with #[cfg(target_os="openbsd")]), the build should fail under Linux (and some others). Or did I miss something ?
There was a problem hiding this comment.
ok, I saw why I can remove #[link_name="dl"] and the second block: the linkage with libdl will be done (for Linux and others) via rtdeps.rs
There was a problem hiding this comment.
the code of dynamic_lib.rs include libdl for:
- linux (already defined in rtdeps.rs)
- android (already defined in rtdeps.rs)
- macos: not defined in rtdeps.rs
- ios: not defined in rtdeps.rs
- freebsd (libdl is a stub)
- dragonfly (libdl is a stub)
Could you check if libdl is need for use of ld... function under ios and macos ? If yes, it must be defined in rtdeps.rs.
There was a problem hiding this comment.
It's not needed for macos, and I suspect the story is probably the same for ios
- follow openbsd patch from rust-lang/rust#21754 (some adaptations from previous branch) - regen patches
- consolidate target_record_sp_limit and target_get_sp_limit functions for aarch64, powerpc, arm-ios and openbsd as there are all without segmented stacks (no need to duplicate functions). - rename __load_self function to rust_load_self - use a mutex inner load_self() as underline implementation is not thread-safe
This code is in a block (libc::consts::os) that openbsd don't include This one is for freebsd and dragonfly. There is another block for openbsd below. Remove the unneed declaration.
- the specific block for dl* function isn't need for openbsd if libdl isn't marked to be linked here. remove them. - remove the linkage for libdl: it should already specified in `rtdeps.rs`. the code of `dynamic_lib.rs` include libdl for: - linux (already defined in rtdeps.rs) - android (already defined in rtdeps.rs) - macos (not needed for macos) - ios (probably the same as macos) - freebsd (libdl is a stub) - dragonfly (libdl is a stub)
- incoporate changes introduced by rust-lang#21678
|
I will rebase the patches and incorporate the changes introduced by #21678. |
semarie
force-pushed
the
openbsd-rebased
branch
from
185e67c to
f6414b0
Compare
|
@alexcrichton I have rebase to master with the changes for #21678. But I found during testing a regression in stack-overflow detection (test run-pass/out-of-stack.rs failed). It not seems to be related to #21678, but to an update on my system. For now, I think it is ok : the out-of-stack detection is just crash with a message, instead of crash without message... |
|
Ok, sounds like we can at least get the base PR in. Thanks @semarie! |
|
⌛ Testing commit f6414b0 with merge 883e910... |
|
💔 Test failed - auto-mac-64-nopt-t |
|
@bors: retry |
Hi. Here a commit in order to add OpenBSD support to rust. - tests status: run-pass: test result: ok. 1879 passed; 0 failed; 24 ignored; 0 measured run-fail: test result: ok. 81 passed; 0 failed; 5 ignored; 0 measured compile-fail: test result: ok. 1634 passed; 0 failed; 22 ignored; 0 measured run-pass-fulldeps: test result: ok. 22 passed; 0 failed; 1 ignored; 0 measured compile-fail-fulldeps: test result: ok. 13 passed; 0 failed; 0 ignored; 0 measured - The current implementation of load_self function (src/libstd/sys/unix/os.rs) isn't optimal as under OpenBSD I haven't found a reliable method to get the filename of a running process. The current implementation is enought for bootstrapping purpose. - I have disable `run-pass/tcp-stress.rs` test under openbsd. When run manually, the test pass, but when run under `compiletest`, it timeout and echo continuoulsy `Too many open files`. - For building with jemalloc, a more recent version of jemalloc would be mandatory. See jemalloc/jemalloc#188 for more details.
Hi.
Here a commit in order to add OpenBSD support to rust.
run-pass: test result: ok. 1879 passed; 0 failed; 24 ignored; 0 measured
run-fail: test result: ok. 81 passed; 0 failed; 5 ignored; 0 measured
compile-fail: test result: ok. 1634 passed; 0 failed; 22 ignored; 0 measured
run-pass-fulldeps: test result: ok. 22 passed; 0 failed; 1 ignored; 0 measured
compile-fail-fulldeps: test result: ok. 13 passed; 0 failed; 0 ignored; 0 measured
run-pass/tcp-stress.rstest under openbsd. When run manually, the test pass, but when run undercompiletest, it timeout and echo continuoulsyToo many open files.