Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

posix_spawn_file_actions_addchdir_np lookup doesn't work on linux musl #99740

Open
sunshowers opened this issue Jul 26, 2022 · 8 comments · May be fixed by #131851
Open

posix_spawn_file_actions_addchdir_np lookup doesn't work on linux musl #99740

sunshowers opened this issue Jul 26, 2022 · 8 comments · May be fixed by #131851
Labels
C-bug Category: This is a bug. O-musl Target: The musl libc

Comments

@sunshowers
Copy link
Contributor

sunshowers commented Jul 26, 2022

I tried this code:

use std::process::Command;

let mut cmd = Command::new("ls");
cmd.current_dir(".");
cmd.output().unwrap();

(compiled under x86_64-unknown-linux-musl)

I expected to see this happen:

The posix_spawn fast path is used.

Instead, this happened:

Rust fell back to the fork/exec slow path. (Inspected this using gdb and strace).

This is presumably because

weak! {
fn posix_spawn_file_actions_addchdir_np(
*mut libc::posix_spawn_file_actions_t,
*const libc::c_char
) -> libc::c_int
}
does a dynamic symbol lookup but musl causes binaries to be statically linked.

Meta

rustc --version --verbose:

rustc 1.64.0-nightly (93ffde6f0 2022-07-23)
binary: rustc
commit-hash: 93ffde6f04d3d24327a4e17a2a2bf4f63c246235
commit-date: 2022-07-23
host: x86_64-unknown-linux-gnu
release: 1.64.0-nightly
LLVM version: 14.0.6
Backtrace

<backtrace>

@cuviper
Copy link
Member

cuviper commented Jul 28, 2022

This is presumably because (use of weak!) does a dynamic symbol lookup

That used to call dlsym, but it has used #[linkage = "extern_weak"] since #90846, released in Rust 1.59.0.

I do see posix_spawn_file_actions_addchdir_np in x86_64-unknown-linux-musl/lib/self-contained/libc.a, and the corresponding weak symbol reference in its libstd-*.rlib, so I don't know why that isn't working.

@sunshowers
Copy link
Contributor Author

Huh, thanks for looking at this. That's really weird then.

@cuviper cuviper added the O-musl Target: The musl libc label Jul 28, 2022
@12101111
Copy link
Contributor

I try this rust code and c code on musl and glibc linux system.

#![feature(linkage)]
use std::ffi::c_void;
extern "C" {
    #[linkage = "extern_weak"]
    static posix_spawn_file_actions_addchdir_np: *const c_void;
}
fn main() {
    println!("{}", unsafe {
        posix_spawn_file_actions_addchdir_np.is_null()
    });
}

musl:

> export RUSTFLAGS="-Ctarget-feature=-crt-static"
> cargo run
   Compiling test-weak-linkage v0.1.0 (/tmp/test-weak-linkage)
    Finished dev [unoptimized + debuginfo] target(s) in 0.11s
     Running `/var/tmp/rust/debug/test-weak-linkage`
false
> export RUSTFLAGS="-Ctarget-feature=+crt-static"
> cargo run
   Compiling test-weak-linkage v0.1.0 (/tmp/test-weak-linkage)
    Finished dev [unoptimized + debuginfo] target(s) in 0.12s
     Running `/var/tmp/rust/debug/test-weak-linkage`
true

glibc:

> export RUSTFLAGS="-Ctarget-feature=+crt-static"
> cargo run
   Compiling test-weak-linkage v0.1.0 (/tmp/test-weak-linkage)
    Finished dev [unoptimized + debuginfo] target(s) in 0.85s
     Running `target/debug/test-weak-linkage`
true
> export RUSTFLAGS="-Ctarget-feature=-crt-static"
> cargo run
   Compiling test-weak-linkage v0.1.0 (/tmp/test-weak-linkage)
    Finished dev [unoptimized + debuginfo] target(s) in 0.48s
     Running `target/debug/test-weak-linkage`
false
#include <stdio.h>
__attribute__((weak)) extern void *posix_spawn_file_actions_addchdir_np;

int main() {
    printf("%p\n", &posix_spawn_file_actions_addchdir_np);
}

musl:

> clang test.c && ./a.out
0x7fe282cb2800
> clang test.c -static && ./a.out
0

glibc:

> gcc test.c && ./a.out
0x7f7875c05550
> gcc test.c -static && ./a.out
(nil)

A blog https://maskray.me/blog/2021-04-25-weak-symbol say:

An undefined weak symbol does not extract archive members

So use weak symbol and checking null is not working in this case. However the old dlsym way also don't work with musl when the elf is statically linked . Maybe a compile-time check by rustc is needed when target-feature=+crt-static

@cuviper
Copy link
Member

cuviper commented Aug 12, 2022

Hmm. Maybe instead we could add a defined weak function that returns ENOSYS, and then libc's global definition should override that if present.

@sunshowers
Copy link
Contributor Author

If rustc bundles musl, can it just rely on the fact that it ships with this method?

@cuviper
Copy link
Member

cuviper commented Aug 19, 2022

Well, in theory musl is supposed to allow external sysroots too, but it currently uses the self-contained mode for all crt-static builds. If this FIXME is addressed, then we can't just rely on what we ship.

// FIXME: Find a better heuristic for "native musl toolchain is available",
// based on host and linker path, for example.
// (https://github.com/rust-lang/rust/pull/71769#issuecomment-626330237).
LinkSelfContainedDefault::Musl => sess.crt_static(Some(crate_type)),

Otherwise, we could ask the question in terms of minimum supported version -- posix_spawn_file_actions_addchdir_np was added in musl 1.1.24. #82632 has been working on musl 1.2 that would include changes for 64-bit time_t, so that might effectively become a new minimum for us.

In general, it would still be nice to have a weak! that works with crt-static, but that doesn't have to block here.

@sunshowers
Copy link
Contributor Author

Still interested in this. My understanding is that once rust-lang/libc#3068 lands and is incorporated into rustc, we'll set the minimum musl requirement to 1.2 -- and that at that point we can unconditionally assume that posix_spawn_file_actions_addchdir_np exists. (I could be wrong though, so please correct me if so!)

I did some reading on weak symbols and I think I understand how to use them, but if the above understanding is correct I think we can probably just wait.

@sunshowers
Copy link
Contributor Author

Now that musl has been bumped to a 1.2 requirement, I have a patch locally that unconditionally enables the function on musl, and appears to work in my testing. Need to get this libc change in and released: rust-lang/libc#3949 (I presume that's preferable to defining an extern function within the rustc source code.)

sunshowers added a commit to sunshowers/rust that referenced this issue Oct 17, 2024
Currently, not all libcs have the `posix_spawn_file_actions_addchdir_np` symbol
available to them. So we attempt to do a weak symbol lookup for that function.
But that only works if libc is a dynamic library -- with statically linked musl
binaries the symbol lookup would never work, so we would never be able to use it
even if the musl in use supported the symbol.

Now that Rust has a minimum musl version of 1.2.3, all supported musl versions
now include this symbol, so we can unconditionally expect it to be there. This
symbol was added to libc in rust-lang/libc#3949 -- use
it here.

I couldn't find any tests for whether the posix_spawn path is used, but I've
verified with cargo-nextest that this change works. This is a substantial
improvement to nextest's performance with musl. On my workstation with a Ryzen
7950x, against https://github.com/clap-rs/clap at
61f5ee514f8f60ed8f04c6494bdf36c19e7a8126:

Before:

```
     Summary [   1.071s] 879 tests run: 879 passed, 0 skipped
```

After:

```
     Summary [   0.392s] 879 tests run: 879 passed, 0 skipped
```

Fixes rust-lang#99740.
sunshowers added a commit to sunshowers/rust that referenced this issue Oct 17, 2024
Currently, not all libcs have the `posix_spawn_file_actions_addchdir_np` symbol
available to them. So we attempt to do a weak symbol lookup for that function.
But that only works if libc is a dynamic library -- with statically linked musl
binaries the symbol lookup would never work, so we would never be able to use it
even if the musl in use supported the symbol.

Now that Rust has a minimum musl version of 1.2.3, all supported musl versions
now include this symbol, so we can unconditionally expect it to be there. This
symbol was added to libc in rust-lang/libc#3949 -- use
it here.

I couldn't find any tests for whether the posix_spawn path is used, but I've
verified with cargo-nextest that this change works. This is a substantial
improvement to nextest's performance with musl. On my workstation with a Ryzen
7950x, against https://github.com/clap-rs/clap at
61f5ee514f8f60ed8f04c6494bdf36c19e7a8126:

Before:

```
     Summary [   1.071s] 879 tests run: 879 passed, 0 skipped
```

After:

```
     Summary [   0.392s] 879 tests run: 879 passed, 0 skipped
```

Fixes rust-lang#99740.
sunshowers added a commit to sunshowers/rust that referenced this issue Oct 18, 2024
Currently, not all libcs have the `posix_spawn_file_actions_addchdir_np` symbol
available to them. So we attempt to do a weak symbol lookup for that function.
But that only works if libc is a dynamic library -- with statically linked musl
binaries the symbol lookup would never work, so we would never be able to use it
even if the musl in use supported the symbol.

Now that Rust has a minimum musl version of 1.2.3, all supported musl versions
now include this symbol, so we can unconditionally expect it to be there. This
symbol was added to libc in rust-lang/libc#3949 -- use
it here.

I couldn't find any tests for whether the posix_spawn path is used, but I've
verified with cargo-nextest that this change works. This is a substantial
improvement to nextest's performance with musl. On my workstation with a Ryzen
7950x, against https://github.com/clap-rs/clap at
61f5ee514f8f60ed8f04c6494bdf36c19e7a8126:

Before:

```
     Summary [   1.071s] 879 tests run: 879 passed, 0 skipped
```

After:

```
     Summary [   0.392s] 879 tests run: 879 passed, 0 skipped
```

Fixes rust-lang#99740.
sunshowers added a commit to sunshowers/rust that referenced this issue Oct 18, 2024
Currently, not all libcs have the `posix_spawn_file_actions_addchdir_np` symbol
available to them. So we attempt to do a weak symbol lookup for that function.
But that only works if libc is a dynamic library -- with statically linked musl
binaries the symbol lookup would never work, so we would never be able to use it
even if the musl in use supported the symbol.

Now that Rust has a minimum musl version of 1.2.3, all supported musl versions
now include this symbol, so we can unconditionally expect it to be there. This
symbol was added to libc in rust-lang/libc#3949 -- use
it here.

I couldn't find any tests for whether the posix_spawn path is used, but I've
verified with cargo-nextest that this change works. This is a substantial
improvement to nextest's performance with musl. On my workstation with a Ryzen
7950x, against https://github.com/clap-rs/clap at
61f5ee514f8f60ed8f04c6494bdf36c19e7a8126:

Before:

```
     Summary [   1.071s] 879 tests run: 879 passed, 0 skipped
```

After:

```
     Summary [   0.392s] 879 tests run: 879 passed, 0 skipped
```

Fixes rust-lang#99740.
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 19, 2024
[musl] use posix_spawn if a directory change was requested

Currently, not all libcs have the `posix_spawn_file_actions_addchdir_np` symbol available to them. So we attempt to do a weak symbol lookup for that function. But that only works if libc is a dynamic library -- with statically linked musl binaries the symbol lookup would never work, so we would never be able to use it even if the musl in use supported the symbol.

Now that Rust has a minimum musl version of 1.2.3, all supported musl versions now include this symbol, so we can unconditionally expect it to be there. This symbol was added to libc in rust-lang/libc#3949 -- use it here.

I couldn't find any tests for whether the posix_spawn path is used, but I've verified with cargo-nextest that this change works. This is a substantial improvement to nextest's performance with musl. On my workstation with a Ryzen 7950x, against https://github.com/clap-rs/clap at
61f5ee514f8f60ed8f04c6494bdf36c19e7a8126:

Before:

```
     Summary [   1.071s] 879 tests run: 879 passed, 0 skipped
```

After:

```
     Summary [   0.392s] 879 tests run: 879 passed, 0 skipped
```

Fixes rust-lang#99740.

try-job: dist-various-1
try-job: dist-various-2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-musl Target: The musl libc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants