[musl] use posix_spawn if a directory change was requested #131851

sunshowers · 2024-10-17T20:19:45Z

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 #99740.

try-job: dist-various-1
try-job: dist-various-2

rustbot · 2024-10-17T20:19:54Z

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-10-17T20:19:56Z

These commits modify the library/Cargo.lock file. Unintentional changes to library/Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

the8472 · 2024-10-17T20:42:34Z

I couldn't find any tests for whether the posix_spawn path is used

Well, posix_spawn is an implementation detail, so there's no API that would let you distinguish that.

But you can do the opposite: Have a test that covers command.current_dir() (this test already exists) and hope that it uses posix_spawn (which can be manually verified via strace) and then write a second test that combines current_dir and pre_exec to make sure the fork path gets exercised too.

rust/library/std/src/sys/pal/unix/linux/pidfd/tests.rs

Lines 47 to 49 in 86bd459

 // exercise the fork/exec path since the earlier attempts may have used pidfd_spawnp() 

 let mut child = 

 unsafe { Command::new("false").pre_exec(|| Ok(())) }.create_pidfd(true).spawn().unwrap();

workingjubilee · 2024-10-17T21:17:59Z

Hm. I guess one can write internal unit-tests where the APIs are exposed? But doing that with multiprocess situations sounds... treacherous.

sunshowers · 2024-10-18T01:59:38Z

Yeah, a test attempting to exercise both paths sounds good. I'll work on that once #131823 lands.

Hm. I guess one can write internal unit-tests where the APIs are exposed? But doing that with multiprocess situations sounds... treacherous.

What I would personally do is abstract the posix_spawn/fork decision into a function which returns an enum, and test that. I don't know how feasible that is, and I'd rather not block this relatively straightforward PR on that refactor :)

workingjubilee · 2024-10-18T02:27:29Z

Yeah, I don't wanna scope creep ya.

sunshowers · 2024-10-18T18:23:39Z

All right, rebased and added a test to exercise both paths.

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.

library/std/src/process/tests.rs

workingjubilee · 2024-10-19T05:54:00Z

library/std/src/sys/pal/unix/process/process_unix.rs

+#[cfg(not(all(target_os = "linux", target_env = "musl")))]
+fn get_posix_spawn_addchdir() -> Option<PosixSpawnAddChdirFn> {


You've moved this to the top of scope in a sensible-seeming fashion. Unfortunately, this is only called within posix_spawn, which not every platform calls to begin with. Indeed, many cfg it away! So this will not be called at all on platforms that don't match this cfg:

#[cfg(any( target_os = "freebsd", all(target_os = "linux", target_env = "gnu"), all(target_os = "linux", target_env = "musl"), target_os = "nto", target_vendor = "apple", ))]

But it will still be defined for those. For those which aren't in the above set, but do use process_unix.rs, then they will see an unused function, and then libstd will not build.

Ah, right...

I guess I could probably move the function item to be function-local.

workingjubilee · 2024-10-19T05:55:12Z

@bors try

bors · 2024-10-19T05:56:23Z

⌛ Trying commit a9016c8 with merge b641764...

[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

rust-log-analyzer · 2024-10-19T06:48:11Z

The job dist-various-2 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

file:.git/config remote.origin.url=https://github.com/rust-lang-ci/rust
file:.git/config remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
file:.git/config gc.auto=0
file:.git/config http.https://github.com/.extraheader=AUTHORIZATION: basic ***
file:.git/config branch.try.remote=origin
file:.git/config branch.try.merge=refs/heads/try
file:.git/config remote.upstream.fetch=+refs/heads/*:refs/remotes/upstream/*
file:.git/config submodule.library/backtrace.active=true
file:.git/config submodule.library/backtrace.url=https://github.com/rust-lang/backtrace-rs.git
file:.git/config submodule.library/stdarch.active=true

bors · 2024-10-19T06:48:41Z

💔 Test failed - checks-actions

workingjubilee · 2024-10-19T07:04:42Z

yep, goes kaput on the solaris build.

you may want to cross-compile for the illumos stdlib while testing this.

rustbot assigned workingjubilee Oct 17, 2024

rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 17, 2024

sunshowers mentioned this pull request Oct 17, 2024

Bump libc to 0.2.161 #131823

Merged

This comment has been minimized.

Sign in to view

sunshowers force-pushed the musl-posix branch from 99677ad to 695bd58 Compare October 18, 2024 18:23

This comment has been minimized.

Sign in to view

sunshowers force-pushed the musl-posix branch from 695bd58 to a9016c8 Compare October 18, 2024 18:52

klensy reviewed Oct 18, 2024

View reviewed changes

library/std/src/process/tests.rs Show resolved Hide resolved

workingjubilee reviewed Oct 19, 2024

View reviewed changes

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[musl] use posix_spawn if a directory change was requested #131851

[musl] use posix_spawn if a directory change was requested #131851

sunshowers commented Oct 17, 2024 •

edited by workingjubilee

Loading

rustbot commented Oct 17, 2024

rustbot commented Oct 17, 2024

This comment has been minimized.

the8472 commented Oct 17, 2024 •

edited

Loading

workingjubilee commented Oct 17, 2024

sunshowers commented Oct 18, 2024

workingjubilee commented Oct 18, 2024

sunshowers commented Oct 18, 2024

This comment has been minimized.

workingjubilee Oct 19, 2024 •

edited

Loading

sunshowers Oct 19, 2024

workingjubilee commented Oct 19, 2024

bors commented Oct 19, 2024

rust-log-analyzer commented Oct 19, 2024

bors commented Oct 19, 2024

workingjubilee commented Oct 19, 2024 •

edited

Loading

		#[cfg(not(all(target_os = "linux", target_env = "musl")))]
		fn get_posix_spawn_addchdir() -> Option<PosixSpawnAddChdirFn> {

[musl] use posix_spawn if a directory change was requested #131851

Are you sure you want to change the base?

[musl] use posix_spawn if a directory change was requested #131851

Conversation

sunshowers commented Oct 17, 2024 • edited by workingjubilee Loading

rustbot commented Oct 17, 2024

rustbot commented Oct 17, 2024

This comment has been minimized.

the8472 commented Oct 17, 2024 • edited Loading

workingjubilee commented Oct 17, 2024

sunshowers commented Oct 18, 2024

workingjubilee commented Oct 18, 2024

sunshowers commented Oct 18, 2024

This comment has been minimized.

workingjubilee Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

sunshowers Oct 19, 2024

Choose a reason for hiding this comment

workingjubilee commented Oct 19, 2024

bors commented Oct 19, 2024

rust-log-analyzer commented Oct 19, 2024

bors commented Oct 19, 2024

workingjubilee commented Oct 19, 2024 • edited Loading

sunshowers commented Oct 17, 2024 •

edited by workingjubilee

Loading

the8472 commented Oct 17, 2024 •

edited

Loading

workingjubilee Oct 19, 2024 •

edited

Loading

workingjubilee commented Oct 19, 2024 •

edited

Loading