Fix the definition of sigevent #2813

asomers · 2022-06-01T03:24:03Z

It was originally defined back before rust could represent C unions. So
instead of defining the union field correctly, it simply defined that
union's most useful field. Define it correctly now.

Include a backwards-compatibility mechanism: Rename sigevent's old
definition and hide it. Implement Deref and DerefMut from sigevent to
the old definition, so consumers will still be able to use the old field
name.

rust-highfive · 2022-06-01T03:24:06Z

r? @Amanieu

(rust-highfive has picked a reviewer for you, use r? to override)

asomers · 2022-06-01T03:26:03Z

@Amanieu what can we do about backwards-compat? It should only be a problem for crates that uses the fairly obscure sigev_notify_thread_id field. That includes Nix, which I can fix. Can we use crater in "grep mode" just to find affected crates?

JohnTitor · 2022-06-02T03:42:31Z

Can we use crater in "grep mode" just to find affected crates?

AFAIK no, the cheapest way is cargo check.

JohnTitor · 2022-06-02T03:45:33Z

@Amanieu Semi-related, is there any discussion/progress about libc crate versioning while I wasn't here? I'd like to resolve the root cause of such an issue someday...

Amanieu · 2022-06-02T15:24:23Z

No, there's not been any discussion on this. We could bring it up at the next libs team meeting, but I'm not really sure what needs to be done here.

asomers · 2022-06-02T15:56:44Z

Then here's what I suggest:

Run Crater in check mode on all of libc's direct dependencies.
If Nix is the only affected crate, then we can make the change, and publish a near synchronous patch release of Nix uses the newer libc crate.
If there are a lots of crates that would be affected, then we change the PR to create a sigevent2 structure and deprecate the original. At some future date we rename sigevent2 to sigevent and make sigevent2 a deprecated type alias.

Does that sound reasonable? Is craterbot already activated for this repo?

Amanieu · 2022-06-02T16:30:06Z

Crater is really designed to test builds of the Rust compiler & standard library, not external crates like libc. I'm sure it would be possible to adapt it, but I'm not familiar with crater and wouldn't know where to start.

As an alternative, it's possible to download the source code for all crates published on crates.io: https://twitter.com/m_ou_se/status/1433085053056262144

This would allow grepping for any uses of sigev_notify_thread_id to see if any crates other than nix would break from this change.

asomers · 2022-06-02T20:06:13Z

I used a command inspired by that Twitter post to download and search every crate version. What I found was:

Nix
A few Nix forks, all since abandoned
The autd-timer and autd3-timer crates, both abandoned and deleted from their upstream repo with the comments "start over" and "start over v2"
The async-timer crate, which used this symbol up until v0.2.7, but then stopped. And FWIW it never used that symbol correctly.
A bunch of false positives

When Tokio gets event notification from kevent, the pending future may belong to a different thread. If so, it signals the other thread and pends on kevent again. But the other thread may not have had time to call aio_return yet. In that case, the first thread's kevent will immediately return again. The correct solution is to set EV_ONESHOT on the aiocb. But Rust's libc doesn't currently expose the necessary field. Until it does, restrict Tokio to just a single thread. rust-lang/libc#2813

Depends on rust-lang/libc#2813 Blocks tokio-rs/tokio#4728

Also, disallow using SigevNotify::SigevThreadId on musl. I don't think it ever worked. Depends on rust-lang/libc#2813 Blocks tokio-rs/tokio#4728

asomers · 2022-06-11T00:53:16Z

@Amanieu do you approve of the plan to fix libc and deal with the breakage by immediately publishing patch releases of Nix?

Amanieu · 2022-06-11T00:56:45Z

I'm concerned about the amount of breakage: there's a lot of crates that aren't using the latest major version of Nix. Do you think we could implement a backward-compatibility hack using Deref/DerefMut so that .sigev_notify_thread_id continues to work?

asomers · 2022-06-11T01:23:28Z

We would definitely patch more than just the current major version. I think I should be able to do the top five or six. But what about this DerefMut proposal. How would that work?

Amanieu · 2022-06-11T01:27:40Z

If we implement DerefMut for sigevent such that it dereferences to an anonymous union with a sigev_notify_thread_id, existing users of sigev_notify_thread_id should still continue working. We also don't need to worry about accessing uninitialized data since reading out of a union is still unsafe.

asomers · 2022-06-11T01:38:29Z

That's a good idea, and it looks like it works! I'll amend the PR.

asomers · 2022-06-11T17:20:11Z

@Amanieu the remaining CI failure is due to a limitation in the style checker. It doesn't understand that the type declaration is part of an impl block. I'm not sure how to fix that. Do you have any ideas?

Amanieu · 2022-06-12T00:17:29Z

src/unix/bsd/freebsdlike/freebsd/mod.rs

+        pub sigev_notify: ::c_int,
+        pub sigev_signo: ::c_int,
+        pub sigev_value: ::sigval,
+        pub sigev_notify_thread_id: ::lwpid_t,


I would prefer if this was a union that just contains a single field. This forces the access to the sigev_notify_thread_id field to be in an unsafe block, which is necessary since this may be an uninitialized field in the union.

But the whole point of this structure is for backwards-compatibility with older libc versions, and they didn't require unsafe. Wouldn't it break backwards-compatibility then, if we put this field into a union?

It just needs to be backwards-compatible enough for existing users, which is basically just nix. Does nix access this field outside of an unsafe block?

Yes, existing Nix does not use unsafe.

Also, I don't think that it should be considered unsafe to access the union member when using the backwards-compatible definition. Since it's defined as a struct with a bunch of padding fields, the whole structure should always be initialized, and it shouldn't be a problem to read from the "union" member.

I think this is most likely fine, but I would like a second opinion from @JohnTitor.

Hmm, I'm not sure if I've got 100% of the context here so don't take my thought as a strong opinion, but I think it doesn't have to be perfectly correct while it's just for back-compat.

Amanieu · 2022-06-12T00:17:46Z

Not sure about how to deal with the style checker, @JohnTitor any ideas?

JohnTitor · 2022-06-12T06:27:39Z

Not sure about how to deal with the style checker, @JohnTitor any ideas?

Fixed in #2824

asomers · 2022-06-14T01:31:38Z

Rebased and squashed.

asomers · 2023-02-10T03:36:06Z

@dtolnay since #2845 has stalled, would you consider merging this PR, despite its style changes?

workingjubilee · 2023-03-01T23:24:54Z

This PR has seen a lot of action, and it starts with a note of "...will fail to build with these changes". I skimmed it and am not sure where it's at right now, so I'm marking it as a breakage candidate since we're likely to start evaluating things we want to pull in for a 0.3 release real soon now, and I don't want this to get missed in the review for that, even if it is not breaking anything. However if you could make the PR's opening message match what the current status of this PR is, that would be greatly appreciated.

asomers · 2023-03-01T23:40:06Z

This PR has seen a lot of action, and it starts with a note of "...will fail to build with these changes". I skimmed it and am not sure where it's at right now, so I'm marking it as a breakage candidate since we're likely to start evaluating things we want to pull in for a 0.3 release real soon now, and I don't want this to get missed in the review for that, even if it is not breaking anything. However if you could make the PR's opening message match what the current status of this PR is, that would be greatly appreciated.

Updated, @workingjubilee ! The current status of this PR is that, as far as I am concerned, it is complete. However, there is a trilemma: we cannot have both backwards-compatibility AND rust 1.13.0 support AND an unmodified style checker. The current state of the PR relaxes the style checker, but I would be ok with relaxes either or both of the other conditions, too.

asomers · 2023-07-17T23:03:33Z

@workingjubilee as explained in the current PR summary, this change should not cause any breakage. Can we move forward with it? And as far as the trilemma goes, which is your preference to drop?

100% Backwards-compatibility (though only one crate will be affected)
Rust 1.13.0 support
An unmodified style checker

workingjubilee · 2023-07-18T00:21:06Z

🤔

workingjubilee · 2023-07-18T00:22:48Z

Let me see if I can get a reduced version of #2813 through. Rust 1.13 is my first choice as to what to drop, at the moment.

asomers · 2023-08-06T23:56:27Z

@workingjubilee do you need help with said reduced version?

asomers · 2023-08-11T21:07:07Z

@workingjubilee I rebased on main. This version retains Rust 1.14.0 compatibility, but relaxes the style checker.

Also, disallow using SigevNotify::SigevThreadId on musl. I don't think it ever worked. Depends on rust-lang/libc#2813 Blocks tokio-rs/tokio#4728

asomers · 2023-08-19T17:28:09Z

BTW, I don't think I ever explained the motivation for this change. Without this change, it is impossible to use a multi-threaded Tokio runtime with POSIX AIO. Only a single-threaded runtime can be used. Switching to a multi-threaded runtime by using this libc branch improves my application's performance by 180% . So the PR isn't just for correctness's sake; it's got real value too.

Also, disallow using SigevNotify::SigevThreadId on musl. I don't think it ever worked. Depends on rust-lang/libc#2813 Blocks tokio-rs/tokio#4728

* Allow setting kevent_flags on struct sigevent Also, disallow using SigevNotify::SigevThreadId on musl. I don't think it ever worked. Depends on rust-lang/libc#2813 Blocks tokio-rs/tokio#4728 * Inline libc::sigevent Because the PR to libc is stalled for over one year, with no sign of progress.

bors · 2023-12-20T14:31:29Z

☔ The latest upstream changes (presumably #3487) made this pull request unmergeable. Please resolve the merge conflicts.

It was originally defined back before rust could represent C unions. So instead of defining the union field correctly, it simply defined that union's most useful field. Define it correctly now. Include a backwards-compatibility mechanism: Rename sigevent's old definition and hide it. Implement Deref and DerefMut from sigevent to the old definition, so consumers will still be able to use the old field name.

sigevent's Debug, PartialEq, and Hash trait impls might read union fields that could be potentially uninitialized by a standard initializer. Those trait impls shouldn't be present (see rust-lang#2816), but can't easily be removed. Until they get removed, the constructor must be `unsafe` to force the user to zero all fields. The same issue applies to the Deref<Target=sigevent_0_2_126> trait impl, which exists only for backwards compatibility.

Rust 1.14.0 and earlier don't correctly expand #[cfg()] attributes within the macro, sometimes necessitating multiple s! macros per file.

SteveLauC · 2024-08-11T07:59:40Z

If #3630 is a rebase of this PR, we can then close this PR? 🤔

rust-highfive assigned Amanieu Jun 1, 2022

rust-highfive added the S-waiting-on-review label Jun 1, 2022

asomers mentioned this pull request Jun 4, 2022

Only use one thread with Tokio bfffs/bfffs#202

Merged

asomers added a commit to asomers/nix that referenced this pull request Jun 4, 2022

Allow setting kevent_flags on struct sigevent

0ac0f47

Depends on rust-lang/libc#2813 Blocks tokio-rs/tokio#4728

asomers mentioned this pull request Jun 4, 2022

Allow setting kevent_flags on struct sigevent nix-rust/nix#1731

Merged

asomers added a commit to asomers/nix that referenced this pull request Jun 4, 2022

Allow setting kevent_flags on struct sigevent

255a6f6

Also, disallow using SigevNotify::SigevThreadId on musl. I don't think it ever worked. Depends on rust-lang/libc#2813 Blocks tokio-rs/tokio#4728

asomers added a commit to asomers/nix that referenced this pull request Jun 4, 2022

Allow setting kevent_flags on struct sigevent

e83418e

Also, disallow using SigevNotify::SigevThreadId on musl. I don't think it ever worked. Depends on rust-lang/libc#2813 Blocks tokio-rs/tokio#4728

Amanieu reviewed Jun 12, 2022

View reviewed changes

asomers force-pushed the sigevent branch from 32084f7 to a9ad9b6 Compare June 14, 2022 01:30

asomers marked this pull request as ready for review June 14, 2022 01:31

asomers force-pushed the sigevent branch from a9ad9b6 to ec3ef17 Compare June 14, 2022 02:12

asomers changed the title ~~WIP Fix the definition of sigevent~~ Fix the definition of sigevent Jun 14, 2022

workingjubilee added the breakage-candidate label Mar 1, 2023

asomers force-pushed the sigevent branch from 9d637b8 to d81b318 Compare August 11, 2023 21:05

asomers added a commit to asomers/nix that referenced this pull request Aug 11, 2023

Allow setting kevent_flags on struct sigevent

48cec20

Also, disallow using SigevNotify::SigevThreadId on musl. I don't think it ever worked. Depends on rust-lang/libc#2813 Blocks tokio-rs/tokio#4728

asomers added a commit to asomers/nix that referenced this pull request Aug 11, 2023

Allow setting kevent_flags on struct sigevent

262d5d9

Also, disallow using SigevNotify::SigevThreadId on musl. I don't think it ever worked. Depends on rust-lang/libc#2813 Blocks tokio-rs/tokio#4728

asomers added a commit to asomers/nix that referenced this pull request Aug 12, 2023

Allow setting kevent_flags on struct sigevent

14b3b44

Also, disallow using SigevNotify::SigevThreadId on musl. I don't think it ever worked. Depends on rust-lang/libc#2813 Blocks tokio-rs/tokio#4728

asomers force-pushed the sigevent branch from d81b318 to 5a092e5 Compare August 19, 2023 17:26

asomers added a commit to asomers/nix that referenced this pull request Aug 20, 2023

Allow setting kevent_flags on struct sigevent

78c85f3

Also, disallow using SigevNotify::SigevThreadId on musl. I don't think it ever worked. Depends on rust-lang/libc#2813 Blocks tokio-rs/tokio#4728

asomers added a commit to asomers/nix that referenced this pull request Aug 20, 2023

Allow setting kevent_flags on struct sigevent

c827089

Also, disallow using SigevNotify::SigevThreadId on musl. I don't think it ever worked. Depends on rust-lang/libc#2813 Blocks tokio-rs/tokio#4728

asomers added a commit to asomers/nix that referenced this pull request Aug 26, 2023

Allow setting kevent_flags on struct sigevent

656546b

Also, disallow using SigevNotify::SigevThreadId on musl. I don't think it ever worked. Depends on rust-lang/libc#2813 Blocks tokio-rs/tokio#4728

asomers added 6 commits December 20, 2023 08:03

Deprecate the sigevent_0_2_126 structures

c77fdd6

Fix the build with Rust 1.14.0

7064630

Disable the 1-s!-macro-per-file style rule

c72e45f

Rust 1.14.0 and earlier don't correctly expand #[cfg()] attributes within the macro, sometimes necessitating multiple s! macros per file.

style

62d6477

asomers force-pushed the sigevent branch from 5a092e5 to 62d6477 Compare December 20, 2023 15:04

asomers mentioned this pull request Mar 23, 2024

Fix the definition of sigevent on FreeBSD and Linux #3630

Open

SteveLauC mentioned this pull request Aug 11, 2024

Discussion on the possible implementation of SIGEV_THREAD nix-rust/nix#2466

Open

Fix the definition of sigevent #2813

Are you sure you want to change the base?

Fix the definition of sigevent #2813

Conversation

asomers commented Jun 1, 2022 • edited Loading

rust-highfive commented Jun 1, 2022

asomers commented Jun 1, 2022

JohnTitor commented Jun 2, 2022

JohnTitor commented Jun 2, 2022 • edited Loading

Amanieu commented Jun 2, 2022

asomers commented Jun 2, 2022

Amanieu commented Jun 2, 2022

asomers commented Jun 2, 2022

asomers commented Jun 11, 2022

Amanieu commented Jun 11, 2022

asomers commented Jun 11, 2022

Amanieu commented Jun 11, 2022

asomers commented Jun 11, 2022

asomers commented Jun 11, 2022

Amanieu Jun 12, 2022

Choose a reason for hiding this comment

asomers Jun 13, 2022

Choose a reason for hiding this comment

Amanieu Jun 13, 2022

Choose a reason for hiding this comment

asomers Jun 13, 2022

Choose a reason for hiding this comment

asomers Jun 14, 2022

Choose a reason for hiding this comment

Amanieu Jun 16, 2022

Choose a reason for hiding this comment

JohnTitor Jun 16, 2022

Choose a reason for hiding this comment

Amanieu commented Jun 12, 2022

JohnTitor commented Jun 12, 2022

asomers commented Jun 14, 2022

asomers commented Feb 10, 2023

workingjubilee commented Mar 1, 2023

asomers commented Mar 1, 2023

asomers commented Jul 17, 2023

workingjubilee commented Jul 18, 2023

workingjubilee commented Jul 18, 2023 • edited Loading

asomers commented Aug 6, 2023

asomers commented Aug 11, 2023

asomers commented Aug 19, 2023

bors commented Dec 20, 2023

SteveLauC commented Aug 11, 2024

asomers commented Jun 1, 2022 •

edited

Loading

JohnTitor commented Jun 2, 2022 •

edited

Loading

workingjubilee commented Jul 18, 2023 •

edited

Loading