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

Fix the definition of sigevent #2813

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

asomers
Copy link
Contributor

@asomers asomers commented Jun 1, 2022

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
Copy link

r? @Amanieu

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

@asomers
Copy link
Contributor Author

asomers commented Jun 1, 2022

@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
Copy link
Member

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

AFAIK no, the cheapest way is cargo check.

@JohnTitor
Copy link
Member

JohnTitor commented Jun 2, 2022

@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
Copy link
Member

Amanieu commented Jun 2, 2022

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
Copy link
Contributor Author

asomers commented Jun 2, 2022

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
Copy link
Member

Amanieu commented Jun 2, 2022

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
Copy link
Contributor Author

asomers commented Jun 2, 2022

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

asomers added a commit to bfffs/bfffs that referenced this pull request Jun 4, 2022
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
asomers added a commit to asomers/nix that referenced this pull request Jun 4, 2022
asomers added a commit to asomers/nix that referenced this pull request Jun 4, 2022
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
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
Copy link
Contributor Author

asomers commented Jun 11, 2022

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

@Amanieu
Copy link
Member

Amanieu commented Jun 11, 2022

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
Copy link
Contributor Author

asomers commented Jun 11, 2022

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
Copy link
Member

Amanieu commented Jun 11, 2022

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
Copy link
Contributor Author

asomers commented Jun 11, 2022

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

@asomers
Copy link
Contributor Author

asomers commented Jun 11, 2022

@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?

pub sigev_notify: ::c_int,
pub sigev_signo: ::c_int,
pub sigev_value: ::sigval,
pub sigev_notify_thread_id: ::lwpid_t,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, existing Nix does not use unsafe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Amanieu commented Jun 12, 2022

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

@JohnTitor
Copy link
Member

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

Fixed in #2824

@asomers
Copy link
Contributor Author

asomers commented Jun 14, 2022

Rebased and squashed.

@asomers asomers marked this pull request as ready for review June 14, 2022 01:31
@asomers asomers changed the title WIP Fix the definition of sigevent Fix the definition of sigevent Jun 14, 2022
@asomers
Copy link
Contributor Author

asomers commented Feb 10, 2023

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

@workingjubilee
Copy link
Member

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
Copy link
Contributor Author

asomers commented Mar 1, 2023

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
Copy link
Contributor Author

asomers commented Jul 17, 2023

@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
Copy link
Member

🤔

@workingjubilee
Copy link
Member

workingjubilee commented Jul 18, 2023

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
Copy link
Contributor Author

asomers commented Aug 6, 2023

@workingjubilee do you need help with said reduced version?

@asomers
Copy link
Contributor Author

asomers commented Aug 11, 2023

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

asomers added a commit to asomers/nix that referenced this pull request Aug 11, 2023
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
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
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
Copy link
Contributor Author

asomers commented Aug 19, 2023

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.

asomers added a commit to asomers/nix that referenced this pull request Aug 20, 2023
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
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
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
github-merge-queue bot pushed a commit to nix-rust/nix that referenced this pull request Aug 27, 2023
* 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
Copy link
Contributor

bors commented Dec 20, 2023

☔ 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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants