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

Upgrade musl supported version to 1.2.3 #3068

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

Conversation

wesleywiser
Copy link
Member

@wesleywiser wesleywiser commented Jan 12, 2023

Rebase of #2088.

This PR changes the supported version of musl libc to 1.2.3. The bulk of the changes in this PR are a result of time_t changes to support 64-bit time on all platforms (the time64 change). This is an API breaking change because of an additional private padding field which is added on some platforms.

This change has been run through crater several times. The last run reported 959 regressions, of which upgrading to a newer major or minor version of a dependency would resolve the regression for >700 crates.

A breakdown of which targets contain this breaking change and what Target Tier they are is available in the compiler team MCP to upgrade the *-linux-musl targets to musl 1.2.

Thanks to @kaniini, @richfelker, @danielframpton and @Amanieu for their work on this!

Fixes #1848
Closes #2088

danielframpton and others added 12 commits January 12, 2023 17:13
musl 1.1 maintenance has for all practical purposes ended.  Accordingly, there is
no point in supporting both 1.1 and 1.2 in the libc crate, so follow the
time_t type transition to 64-bit.
- add padding members for musl 1.2
- ensure the padding members have an appropriate type (always c_int)
Only some 32-bit targets use the time64 family of functions and that set
will not change going forward (new 32-bit targets added will use Y2038
compliant syscalls and types by default).

This patch introduces a cfg flag that controls when the time64 abi
related changes are enabled and sets that flag from libc and libc-tests'
build.rs files.
@rustbot
Copy link
Collaborator

rustbot commented Jan 12, 2023

r? @JohnTitor

(rustbot has picked a reviewer for you, use r? to override)

@Amanieu
Copy link
Member

Amanieu commented Jan 12, 2023

This is technically a breaking change due to the change to time_t, and quite a significant change. However, it's something that will have to be done anyways as we follow upstream musl.

@rfcbot fcp merge

@JohnTitor
Copy link
Member

FCP has to be run on repos that enable @rfcbot.

This also affects the library users, I guess we have to release this as 0.3.

@rfcbot
Copy link

rfcbot commented Jan 13, 2023

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@cuviper
Copy link
Member

cuviper commented Jan 13, 2023

@rfcbot reviewed

This also affects the library users, I guess we have to release this as 0.3.

Strictly, yes, but I think we can exercise leeway here -- it only affects a subset of targets, and those targets made the same breaking change at their root.

@tmandry
Copy link
Member

tmandry commented Jan 13, 2023

Let me make sure I understand:

  • When building on affected targets (where the 3rd column is "yes" in this table: Upgrade *-linux-musl targets to musl 1.2 compiler-team#572),
  • Updating the Rust compiler
  • Will require you to install/build against a newer version of musl
  • Which if you happen to use the libc crate, will also require an upgrade (it must use the same musl version as the toolchain)
  • Which itself may be API incompatible with your other dependencies that also use the libc crate

The last two are what is being referred to here: "of which upgrading to a newer major or minor version of a dependency would resolve the regression for >700 crates"

Is that right?

@JohnTitor
Copy link
Member

Strictly, yes, but I think we can exercise leeway here -- it only affects a subset of targets, and those targets made the same breaking change at their root.

Fair enough! But we have some more breakage candidates and I guess we could use this opportunity to ship them together. The problem is that we don't have any policy about supported toolchain/OS/etc. versions and it may make the 0.3 release painful. Any thoughts on this?

@tmandry It sounds right to me except for the last one.
@wesleywiser I'm not sure about "which upgrading to a newer major or minor version of a dependency would resolve the regression for >700 crates" and the report is now unavailable for me. Does it mean releasing this change and upgrading the libc dependency will resolve regressions?

@wesleywiser
Copy link
Member Author

Great questions! I think I can answer both @tmandry and @JohnTitor's by explaining a bit more how the libc crate and the musl targets interact.

The libc crate is a bit unique in the space of -sys crates in that what library it binds to is chosen by your target rather than using feature flags or even just picking a different library. That means there is a relationship between the targets we ship with Rust and what the libc crate should provide bindings to. This change is really the first step to upgrading the version of musl our -linux-musl targets use. In order to do that though, we have to have this support in the libc crate because the standard library itself uses this crate for various syscalls.

The -linux-musl targets themselves vary a bit as some, such as x86_64-unknown-linux-musl, statically link by default and ship with our own build of musl while other targets dynamically link by default and you can provide your own musl version. It's possible today to use your own version of musl (including musl 1.2) but for any target affected by the time64 changes, you run the risk of mismatched bindings.

The main change that has caused issues in the ecosystem is that timespec now contains padding on some platforms (most notably i686). You can see this in the musl definition for the type and the corresponding definition in these changes. As a result of adding the padding field, code that used to initialize like a libc::timespec like this timespec { tv_sec: 0, tv_nsec: 0 } no longer compiles because of the private padding field. This usually manifests itself in dependencies that sit between libc and "user code" such as time, nix, parking_lot, polling, filetime, etc no longer compiling.

I've been working with maintainers for the most widely used crates to fix these issues and ship new releases. Many have even shipped point releases for older versions that are still widely used but there is a long tail of downstream crates that are on ancient versions of some of these dependencies and haven't been touched in years. These are the crates I'm referring to when I say "upgrading to a newer major or minor version of a dependency {such as time, nix, parking_lot, etc} would resolve the regression".

My overall plan here is to:

  • Merge these changes into libc and cut a new release.
  • Update my PR to upgrade Rust targets to musl 1.2.3 to use the new libc release.
  • Start an FCP to merge that PR (the compiler team has already approved the MCP to do this).
  • Write a post for the Rust blog describing what is changing and the timeline when the target changes are expected to hit stable.
  • Merge the target changes PR.

@tmandry
Copy link
Member

tmandry commented Jan 13, 2023

What will happen if a user upgrades the libc crate between when we cut a new release and the changes to the Rust targets hit stable?

Also, what happens when there's an incompatibility between the libc crate and the actual version of musl (when the user supplies their own)? Is there at least a linker error, or is it silent UB?

@cuviper
Copy link
Member

cuviper commented Jan 13, 2023

Can we detect the difference in the build script's is_musl_time64_abi?

@wesleywiser
Copy link
Member Author

What will happen if a user upgrades the libc crate between when we cut a new release and the changes to the Rust targets hit stable?

Also, what happens when there's an incompatibility between the libc crate and the actual version of musl (when the user supplies their own)? Is there at least a linker error, or is it silent UB?

From looking at the builds I have from crater, it seems (nearly?) all of the affected functions are new in musl 1.2. As a result, I think most any scenario where a newer version of the libc crate is being used with older targets will result in linking errors (but perhaps only if the affected functions are used). In the reverse situation (a newer target with an older libc), the symbols will line up with correct types so it should compile, link and execute fine (with no UB).

So having written that out, I think my plan has the wrong order: we should ship the updated Rust targets first and then land this change to libc. However, I think this FCP is still useful for confirming that we are going to ship these changes, we'll just hold off merging them until the updates to the targets have reached stable.

Can we detect the difference in the build script's is_musl_time64_abi?

This should be possible by detecting the presence of the *_time64 functions.

@tmandry
Copy link
Member

tmandry commented Jan 20, 2023

Okay so just to confirm, we're choosing to make a breaking change to the libc crate on these targets rather than add a second variant of all the affected APIs with a different name. (Sorry if you said that and I missed it.) Users won't be affected until they upgrade the libc crate, at which point hopefully all the affected ecosystem crates can be upgraded to fix any errors.

Seems ok to me. Now I wish we could mark structs as #[non_exhaustive] to prevent future breakage, though that would be a breaking change :).

Fair enough! But we have some more breakage candidates and I guess we could use this opportunity to ship them together. The problem is that we don't have any policy about supported toolchain/OS/etc. versions and it may make the 0.3 release painful. Any thoughts on this?

You mean there's no policy on supported platforms for the libc crate in particular? Procedurally, who/what team would be responsible for deciding that?

@tmandry
Copy link
Member

tmandry commented Jan 20, 2023

@rfcbot reviewed

@JohnTitor
Copy link
Member

You mean there's no policy on supported platforms for the libc crate in particular?

Yes, e.g. #1412. Since we have a lot of supported env, I guess it's hard to make a policy. 🤔

Procedurally, who/what team would be responsible for deciding that?

In the past the libc team took care of such a topic but it's now unified with the crate-maintainers team. Since there is an overlap between the members of this team and the libs, and since the libc is somewhere between the community crate and the internal crate, I feel it makes some sense to ask here incidentally.

@Amanieu
Copy link
Member

Amanieu commented Mar 3, 2023

@rfcbot fcp cancel

@rfcbot
Copy link

rfcbot commented Mar 3, 2023

@Amanieu proposal cancelled.

bors added a commit that referenced this pull request Mar 4, 2023
Add support for OpenHarmony

This PR adds support for [OpenHarmony](https://gitee.com/openharmony/docs/) targets:
- `aarch64-linux-ohos`
- `arm-linux-ohos`

Compiler team MCP: rust-lang/compiler-team#568

OpenHarmony uses a fork of musl with minor modifications, so most of the code is shared with other musl targets. Additionally, although OpenHarmony uses musl 1.2, it is still ABI-compatible with musl 1.1 so the existing bindings should continue to work until #3068 is merged.

A PR to add the targets to rustc will follow after this is merged.
@bors
Copy link
Contributor

bors commented Mar 5, 2023

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

@wesleywiser
Copy link
Member Author

@Amanieu that seems like a reasonable approach to me. My impression from reading other issues that mentioned the "libcpocalyse" was that a 0.3 was fairly unlikely. It sounds to me like that may no longer be the case? Is there any kind of known timeline for when a 0.3 release might happen?

@richfelker
Copy link

I was under the impression this would be a soon-upcoming thing the change would be rolled into. If this is tabling it long-term, that's extremely disappointing. Y2038 is rapidly approaching and programs need to be able to deal with timestamps for things like validity periods now. If there is any breakage from application code making wrong assumptions, dealing with that now will be a lot better than dealing with it when it becomes progressively more urgent.

@Amanieu
Copy link
Member

Amanieu commented Mar 7, 2023

At the very least, libc 0.3 would only be possible once rust-lang/rust#102891 is merged, and that would become the MSRV for libc 0.3 (since it requires rust to ship musl 1.2).

The best way to start would be to create a tracking issue for libc 0.3 with a list of all the breaking changes we would like to make. Once we have decided what changes we want and implemented them in a branch then we can go ahead and just release it.

The original libcpocalypse was due to crates using * version dependencies, which are now banned from crates.io. So the impact should be much less severe this time. There will be no immediate breakage, instead crates will incrementally upgrade to libc 0.3 as their dependencies do so.

bors added a commit to rust-lang-ci/rust that referenced this pull request May 6, 2023
…ochenkov

Update the version of musl used on `*-linux-musl` targets to 1.2.3

Update the version of musl used on our Linux musl targets from 1.1.24 to 1.2.3 as proposed in rust-lang/compiler-team#572. musl 1.2.3 is the latest version of musl and supports the same range of Linux kernels as the 1.1 series. As such, it does not affect the minimum supported version of Linux for any of the musl targets.

One of the major musl 1.2 features is support for [time64](https://musl.libc.org/time64.html). This support is both source and ABI compatible with programs built against musl 1.1 and so updating the musl version for these targets should not cause Rust programs to fail to run or compile (a [crater run](rust-lang#107129 (comment)) has been completed which demonstrates this for the `i686-unknown-linux-musl` target).

Once this change reaches stable, the `libc` crate will then be able to [update their definitions to support 64-bit time](rust-lang/libc#3068), matching the default musl 1.2 APIs exactly.

Fixes rust-lang#91178
RalfJung pushed a commit to RalfJung/miri that referenced this pull request May 7, 2023
Update the version of musl used on `*-linux-musl` targets to 1.2.3

Update the version of musl used on our Linux musl targets from 1.1.24 to 1.2.3 as proposed in rust-lang/compiler-team#572. musl 1.2.3 is the latest version of musl and supports the same range of Linux kernels as the 1.1 series. As such, it does not affect the minimum supported version of Linux for any of the musl targets.

One of the major musl 1.2 features is support for [time64](https://musl.libc.org/time64.html). This support is both source and ABI compatible with programs built against musl 1.1 and so updating the musl version for these targets should not cause Rust programs to fail to run or compile (a [crater run](rust-lang/rust#107129 (comment)) has been completed which demonstrates this for the `i686-unknown-linux-musl` target).

Once this change reaches stable, the `libc` crate will then be able to [update their definitions to support 64-bit time](rust-lang/libc#3068), matching the default musl 1.2 APIs exactly.

Fixes #91178
@joshtriplett joshtriplett mentioned this pull request May 15, 2023
19 tasks
| "i686-unknown-linux-musl"
| "mips-unknown-linux-musl"
| "mipsel-unknown-linux-musl"
| "powerpc-unknown-linux-musl" => true,
Copy link

@kanavin kanavin Aug 28, 2023

Choose a reason for hiding this comment

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

This will not work for custom targets, e.g. yocto is defining them like 'i686-poky-linux-musl'. It's better to split TARGET into parts and match against them, e.g. first check for presence of 'musl, and then against a list of affected 32 bit architectures. Same comment for libc-test/build.rs.

Here's how a similar issue was fixed (by me :) in crossbeam: all custom vendors (e.g. poky, gentoo, etc.) were converted to '-unknown-' and then matching against the standard list would work:
crossbeam-rs/crossbeam#922

@JohnTitor
Copy link
Member

triage: rust-lang/rust#107129 has been merged and was released as 1.71. Can we set 0.3's MSRV as 1.71 and restart an FCP?

@JohnTitor
Copy link
Member

@rust-lang/libs Could we restart FCP or are there any other blockers?

@wesleywiser
Copy link
Member Author

If there's consensus to bump the minimum supported version of musl to 1.2.x, I'm happy to pick this back up and get it ready to merge.

@Amanieu
Copy link
Member

Amanieu commented Dec 21, 2023

@rfcbot cancel

@Amanieu
Copy link
Member

Amanieu commented Dec 21, 2023

The main branch of libc now tracks the future libc 0.3 release. An FCP is not needed here since libc 0.3 is a major version bump anyways, so it's fine to make breaking changes including increasing the minimum rustc version.

I can restart the FCP if there is a desire to make this change for the 0.2 release branch.

@JohnTitor
Copy link
Member

Yeah, I'm happy to review and r+ once the conflict is resolved!

@JohnTitor
Copy link
Member

JohnTitor commented Mar 9, 2024

Seems rustc now only supports musl 1.2+, I think we have to move this to fix the CI issue: #3613

@wesleywiser Could you make this ready for review/shipping?

RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 20, 2024
Update the version of musl used on `*-linux-musl` targets to 1.2.3

Update the version of musl used on our Linux musl targets from 1.1.24 to 1.2.3 as proposed in rust-lang/compiler-team#572. musl 1.2.3 is the latest version of musl and supports the same range of Linux kernels as the 1.1 series. As such, it does not affect the minimum supported version of Linux for any of the musl targets.

One of the major musl 1.2 features is support for [time64](https://musl.libc.org/time64.html). This support is both source and ABI compatible with programs built against musl 1.1 and so updating the musl version for these targets should not cause Rust programs to fail to run or compile (a [crater run](rust-lang/rust#107129 (comment)) has been completed which demonstrates this for the `i686-unknown-linux-musl` target).

Once this change reaches stable, the `libc` crate will then be able to [update their definitions to support 64-bit time](rust-lang/libc#3068), matching the default musl 1.2 APIs exactly.

Fixes #91178
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
Update the version of musl used on `*-linux-musl` targets to 1.2.3

Update the version of musl used on our Linux musl targets from 1.1.24 to 1.2.3 as proposed in rust-lang/compiler-team#572. musl 1.2.3 is the latest version of musl and supports the same range of Linux kernels as the 1.1 series. As such, it does not affect the minimum supported version of Linux for any of the musl targets.

One of the major musl 1.2 features is support for [time64](https://musl.libc.org/time64.html). This support is both source and ABI compatible with programs built against musl 1.1 and so updating the musl version for these targets should not cause Rust programs to fail to run or compile (a [crater run](rust-lang/rust#107129 (comment)) has been completed which demonstrates this for the `i686-unknown-linux-musl` target).

Once this change reaches stable, the `libc` crate will then be able to [update their definitions to support 64-bit time](rust-lang/libc#3068), matching the default musl 1.2 APIs exactly.

Fixes #91178
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.

musl: Change time_t definition on 32-bit targets according to "time64"