Refactor dynamic library error checking on *nix #75811
Conversation
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
cc @jclulow |
The old code was checking `dlerror` more often than necessary, since the return value of `dlopen` indicates whether an error occurred.
This works around behavior observed on illumos in rust-lang#74469, in which foreign code (libc according to the OP) was racing with rustc to check `dlerror`.
ecstatic-morse
force-pushed
the
better-dlerror
branch
from
6fe2a39 to
e2326a1
Compare
src/librustc_metadata/dynamic_lib.rs
Outdated
| libc::dlopen(s.as_ptr(), libc::RTLD_LAZY) as *mut u8 | ||
| }) | ||
| } | ||
| // `dlerror` is process global, so we can only allow a single thread at a |
There was a problem hiding this comment.
Nit: dlerror It is MT-safe (thread local) on most modern POSIX implementations. Exceptions include freebsd and dragonflybsd as per investigation I did early this year, and apparently illumos as I’ve just learnt.
POSIX in particular allows for dlerror to be MT-safe (since a comparatively recent standard revision), but does not require it.
|
r? @nagisa I guess I’m the most appropriate reviewer for this, given that I maintain a similar crate. I broadly agree with @ollie27 in that we should just consider |
This simplifies the code somewhat. Also updates comments to reflect notes from reviw about thread-safety of `dlerror`.
This happens to be the default on Linux, but the default is unspecified in the POSIX standard. Also switches to `cast` to keep line lengths in check.
|
The most recent changes treat a I don't feel comfortable removing the locking entirely in this PR, since it's not a performance issue and can give more predictable error messages on lower tier platforms that don't have thread-safe |
|
@bors r+ Yeah LGTM. |
|
📌 Commit aae6c0f has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
The old code was checking
dlerrormore often than necessary, since (unlikedlsym) checking the return value ofdlopenis enough to indicate whether an error occurred. In the first commit, I've refactored the code to minimize the number of system calls needed. It should be strictly better than the old version.The second commit is an optional addendum which fixes the issue observed on illumos in #74469, a PR I reviewed that was ultimately closed due to inactivity. I'm not sure how hard we try to work around platform-specific bugs like this, and I believe that, due to the way that
dlerroris specified in the POSIX standard, libc implementations that want to run on conforming systems cannot calldlsymin multi-threaded programs.