Use rustix instead of direct calls to libc. #76

sunfishcode · 2022-01-26T22:02:15Z

Use the rustix syscall wrapper crate to factor out error handling and
unsafe system calls. This reduces the amount of unsafe code here, and is
a step towards factoring it out entirely once AsFd is stabilized and
can replace AsRawFd for these kinds of uses.

This does require incrementing the minimum required Rust version to 1.48.
Please feel free to decline this PR if you don't wish to take on these new
requirements.

notgull · 2022-07-16T16:53:00Z

I support this in theory; async-io is only unsafe due to these lines of code. However, it looks like rustix is broken on Android. I'd support using nix, or even socket2 to make this cross platform.

sunfishcode · 2022-07-16T17:22:38Z

Ah, this PR was using rustix 0.35.6-beta.2. The Android build error was fixed in the 0.35.6 release. I've now updated the PR.

notgull · 2022-08-17T20:37:03Z

Are there any blockers to this being merged? I see how the extra dependency might give pause, but it's probably worth purging the remaining unsafe code in this crate, at least on Unix.

Use the [rustix] syscall wrapper crate to factor out error handling and unsafe system calls. This reduces the amount of unsafe code here, and is a step towards factoring it out entirely once [`AsFd`] is stabilized and can replace `AsRawFd` for these kinds of uses. This does require incrementing the minimum required Rust version to 1.48. Please feel free to decline this PR if you don't wish to take on these new requirements. [rustix]: https://crates.io/crates/rustix/ [`AsFd`]: https://doc.rust-lang.org/stable/std/os/unix/io/trait.AsFd.html

notgull

Truth be told, this looks good to me.

With rustix in the dependency tree, it may be a good idea to also reimplement some other code in terms of rustix. For instance, the package has support for safe epoll, which means we may be able to reimplement our epoll backend in polling in terms of it to reduce the amount of unsafe code we use.

There are three main points of contention, from what I see:

Is there a reason to use this instead of nix? While rustix is more popular, nix may also appear in the dependency tree.
rustix adds the bitflags dependency to smol's dependency tree, which compiles fairly quickly but is still another brick in the wall.
Is the unsafe code we eliminate worth it in the end, since it's just replicated in rustix anyways?

@smol-rs/admins This may be worth a more thorough discussion to determine what direction we'd like to move in.

notgull · 2022-11-29T00:37:32Z

examples/linux-timerfd.rs

+            .read_with(|t| {
+                // Safety: Assume `as_raw_fd()` returns a valid fd; when `AsFd`
+                // is stabilized, we can remove this unsafe and simplify.
+                let fd = unsafe { BorrowedFd::borrow_raw(t.as_raw_fd()) };


AsFd is stabilized now, so I think we can eliminate this unsafe.

Almost, but async-io still has an MSRV of 1.48, and I/O safety is stabilized in Rust 1.63. I've now added more to these comments to mention that.

We don't run the tests on the MSRV build, so it's alright.

taiki-e · 2022-11-29T07:56:21Z

@sunfishcode

Thanks for the PR! I'm basically agreed with merging this, but have a few questions.

Are there tests for outline asm implementations for x86/arm/riscv? I could not find pre-1.59 tests in the rustix's CI files.
(FWIW, if they are not tested, I think you can test those implementations with the latest rustc by inverting the asm cfg, like tokio does.)
Looking at the inline asm implementation of rustix, it seems that (in)lateout is used even where it seems unnecessary. At first glance, it looks fine, but you might want to make sure it is not affected by a bug like Incorrect handling of lateout pairs in inline asm rust-lang/rust#101346.
If we merge this, as notgull mentioned we also want to replace the direct dependencies on libc in other smol-rs crates (polling and async-process, IIRC) with rustix. I don't see a problem with async-process, but polling uses some platform-specific APIs such as bsd's kqueue, solarish's event ports, so probably can't switch to rustix right away except on linux.
Is there any plan to support those non-linux platform-specific APIs in rustix? Or is patch to add support for them acceptable?

@notgull

Is there a reason to use this instead of nix? While rustix is more popular, nix may also appear in the dependency tree.

Given nix's MSRV policy (current MSRV is 1.56), it is difficult to have nix as a dependency of smol-rs crates.

Is the unsafe code we eliminate worth it in the end, since it's just replicated in rustix anyways?

Personally, I think the optimization such as uses of raw system calls to be the important benefit of this PR, not only the removal of unsafe code. (If the removal of unsafe code were the only benefit of this PR, I might not have agreed with this PR.)

Cargo.toml

src/lib.rs

sunfishcode · 2022-11-29T19:36:02Z

* Are there tests for [_outline_](https://github.com/bytecodealliance/rustix/tree/main/src/backend/linux_raw/arch/outline) asm implementations for x86/arm/riscv? I could not find pre-1.59 tests in the `rustix`'s CI files.
  (FWIW, if they are not tested, I think you can test those implementations with the latest rustc by inverting the `asm` cfg, [like tokio does](https://github.com/tokio-rs/tokio/blob/28ec4a6161210a1ffb66cdcd7958b86b4b78b54e/tokio/build.rs#L97-L100).)

Ah, good catch. There are cargo check tests for 1.48, but not cargo test. And it looks like rustix has acquired some tests which are not compatible with Rust 1.48, so it's not trivial to just enable it. I'll look into fixing that.

Right now, I can confirm that with those tests commented out, the rest of the tests do still pass on 1.48 on all archtectures that have outline asm implementations.

* Looking at the inline asm implementation of `rustix`, it seems that `(in)lateout` is used even where it seems unnecessary. At first glance, it looks fine, but you might want to make sure it is not affected by a bug like [Incorrect handling of lateout pairs in inline asm rust-lang/rust#101346](https://github.com/rust-lang/rust/issues/101346).

Thanks for the heads-up here. Rustix doesn't have that specific pattern, with lateout with a virtual register, and has never seen a bug of this sort in any Rust version. But I'll also investigate this more.

* If we merge this, as notgull mentioned we also want to replace the direct dependencies on `libc` in other smol-rs crates ([`polling`](https://github.com/smol-rs/polling) and [`async-process`](https://github.com/smol-rs/async-process), IIRC) with `rustix`. I don't see a problem with `async-process`, but `polling` uses some platform-specific APIs such as bsd's kqueue, solarish's event ports, so probably can't switch to `rustix` right away except on linux.
  Is there any plan to support those non-linux platform-specific APIs in `rustix`? Or is patch to add support for them acceptable?

Rustix does have various non-Linux platform-specific APIs, for example fcopyfile. There's no specific plan; things get added as needed. Patches to add more are welcome. Filing issues is good too; polling APIs can be subtle, and I'd be happy to help figure things out.

This eliminates the need for a direct dependency on windows-sys.

taiki-e

LGTM

- Use [`rustix`] instead of [`libc`]/[`windows-sys`] for system calls (#76) - Add a `will_fire` method to `Timer` to test if it will ever fire (#106) - Reduce syscalls in `Async::new` (#107) - Improve the drop ergonomics of `Readable` and `Writable` (#109) - Change the "`wepoll`" in documentation to "`IOCP`" (#116) [`rustix`]: https://crates.io/crates/rustix/ [`libc`]: https://crates.io/crates/libc/ [`windows-sys`]: https://crates.io/crates/windows-sys/

sunfishcode force-pushed the main branch from 0e686fc to 4ca3356 Compare February 1, 2022 18:23

sunfishcode force-pushed the main branch from 4ca3356 to c1d9ae5 Compare March 17, 2022 17:00

sunfishcode force-pushed the main branch from c1d9ae5 to c4786f4 Compare June 9, 2022 19:48

sunfishcode force-pushed the main branch from 97c0680 to a64a491 Compare November 11, 2022 00:24

sunfishcode force-pushed the main branch from 6e119c0 to f18fd97 Compare November 29, 2022 00:12

notgull approved these changes Nov 29, 2022

View reviewed changes

taiki-e reviewed Nov 29, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

Disable the use-libc-auxv feature.

9c148f4

notgull reviewed Nov 29, 2022

View reviewed changes

src/lib.rs Show resolved Hide resolved

sunfishcode added 2 commits November 29, 2022 13:40

Use rustix to call FIONBIO on Windows.

d156bdc

This eliminates the need for a direct dependency on windows-sys.

Comment that we need Rust >= 1.63 to use io_safety.

e8f1da9

taiki-e approved these changes Nov 30, 2022

View reviewed changes

notgull merged commit 8e1fa43 into smol-rs:master Nov 30, 2022

notgull mentioned this pull request Dec 7, 2022

Add a raw file descriptor context bytecodealliance/rustix#471

Closed

taiki-e mentioned this pull request Dec 30, 2022

Replace direct dependency on libc with rustix smol-rs/async-process#31

Merged

notgull mentioned this pull request Mar 3, 2023

m: Use rustix instead of libc smol-rs/polling#91

Closed

notgull mentioned this pull request Mar 21, 2023

v1.13.0 #119

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use rustix instead of direct calls to libc. #76

Use rustix instead of direct calls to libc. #76

sunfishcode commented Jan 26, 2022

notgull commented Jul 16, 2022

sunfishcode commented Jul 16, 2022

notgull commented Aug 17, 2022

notgull left a comment

notgull Nov 29, 2022

sunfishcode Nov 29, 2022

notgull Nov 30, 2022

taiki-e commented Nov 29, 2022 •

edited

Loading

sunfishcode commented Nov 29, 2022

taiki-e left a comment

Use rustix instead of direct calls to libc. #76

Use rustix instead of direct calls to libc. #76

Conversation

sunfishcode commented Jan 26, 2022

notgull commented Jul 16, 2022

sunfishcode commented Jul 16, 2022

notgull commented Aug 17, 2022

notgull left a comment

Choose a reason for hiding this comment

notgull Nov 29, 2022

Choose a reason for hiding this comment

sunfishcode Nov 29, 2022

Choose a reason for hiding this comment

notgull Nov 30, 2022

Choose a reason for hiding this comment

taiki-e commented Nov 29, 2022 • edited Loading

sunfishcode commented Nov 29, 2022

taiki-e left a comment

Choose a reason for hiding this comment

taiki-e commented Nov 29, 2022 •

edited

Loading