libc
libc copied to clipboard
rust-lang
Fix the definition of sigevent
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.
r? @Amanieu
(rust-highfive has picked a reviewer for you, use r? to override)
@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?
Can we use crater in "grep mode" just to find affected crates?
AFAIK no, the cheapest way is cargo check.
@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...
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.
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
sigevent2structure and deprecate the original. At some future date we renamesigevent2tosigeventand makesigevent2a deprecated type alias.
Does that sound reasonable? Is craterbot already activated for this repo?
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.
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
@Amanieu do you approve of the plan to fix libc and deal with the breakage by immediately publishing patch releases of Nix?
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?
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?
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.
@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?
Not sure about how to deal with the style checker, @JohnTitor any ideas?
Fixed in #2824
:pushpin: Commit ec3ef178bedc9d501283647269bdd2cf6b37f71e has been approved by Amanieu
:hourglass: Testing commit ec3ef178bedc9d501283647269bdd2cf6b37f71e with merge f4be5776f9229cf39935634d0817a2f2df740379...
:pushpin: Commit b543967199223ad91bf7c37a6ddb274ef938ed64 has been approved by Amanieu
:hourglass: Testing commit b543967199223ad91bf7c37a6ddb274ef938ed64 with merge cab2ffed062c72446cf8eceed51cdf298a76eb64...
:pushpin: Commit d7420e6bf7be507bf0076025fbb9d9b432100992 has been approved by Amanieu
:hourglass: Testing commit d7420e6bf7be507bf0076025fbb9d9b432100992 with merge 6aead05faf27e0a86f64322ed0bccc82e395feed...