Skip to content

Merge compiler-builtins as a Josh subtree #141229

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

Open
wants to merge 2,504 commits into
base: master
Choose a base branch
from

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented May 18, 2025

Use the Josh 1 utility to add compiler-builtins as a subtree, which
will allow us to stop using crates.io for updates. This is intended to
help resolve some problems when unstable features change and require
code changes in compiler-builtins, which sometimes gets trapped in a
bootstrap cycle.

This was done using josh-filter built from the r24.10.04 tag:

git fetch https://github.com/rust-lang/compiler-builtins.git 233434412fe7eced8f1ddbfeddabef1d55e493bd
josh-filter ":prefix=library/compiler-builtins" FETCH_HEAD
git merge --allow-unrelated FILTERED_HEAD

The HEAD in the compiler-builtins repository is 233434412f ("fix an if
statement that can be collapsed").

tgross35 and others added 30 commits January 23, 2025 22:30
Add `fminf16`, `fmaxf16`, `fminf128`, and `fmaxf128`
This can replace `fmod` and `fmodf`. As part of this change I was able
to replace some of the `while` loops with `leading_zeros`.
With the new routines, some of our tests are running close to their
timeouts. Increase the timeout for test jobs, and set a short timeout
for all other jobs that did not have one.
Certain functions (`fmodf128`) are significantly slower than others,
to the point that running the default number of tests adds tens of
minutes to PR CI and extensive test time increases to ~1day. It does not
make sense to do this by default; so, introduce `EXTREMELY_SLOW_TESTS`
to test configuration that allows setting specific tests that need to
have a reduced iteration count.
This function is significantly slower than all others so includes an
override in `EXTREMELY_SLOW_TESTS`. Without it, PR CI takes ~1hour and
the extensive tests in CI take ~1day.
A few new functions were added but this list did not get updated. Do so
here.
Since Rug 1.27.0, `az` is reexported. This means we no longer need to
track it as a separate dependency.
Rug 1.27.0 exposes `frexp`. Make use of it for our tests.
Rug 1.27.0 exposes `remquo`; make use of it for our tests. Removing our
workaround also allows removing the direct dependency on `gmp-mpfr-sys`
Upgrade all dependencies to the latest version
The Cargo feature `checked` was added in 410b0633a6b9 ("Overhaul tests")
and later removed in e4ac1399062c ("swap stable to be unstable, checked
is now debug_assertions"). However, there are a few remaining uses of
`feature = "checked"` that did not get removed. Clean these up here.
Currently the default release profile enables LTO and single CGU builds,
which is very slow to build. Most tests are better run with
optimizations enabled since it allows testing a much larger number of
inputs, so it is inconvenient that building can sometimes take
significantly longer than the tests.

Remedy this by doing the following:

* Move the existing `release` profile to `release-opt`.
* With the above, the default `release` profile is untouched (16 CGUs
  and thin local LTO).
* `release-checked` inherits `release`, so no LTO or single CGU.

This means that the simple `cargo test --release` becomes much faster
for local development. We are able to enable the other profiles as
needed in CI.

Tests should ideally still be run with `--profile release-checked` to
ensure there are no debug assetions or unexpected wrapping math hit.

`no-panic` still needs a single CGU, so must be run with `--profile
release-opt`. Since it is not possible to detect CGU or profilel
configuration from within build scripts, the `ENSURE_NO_PANIC`
environment variable must now always be set.
There seems to be a case of unsoundness with the `i586` version of
`atan2`. For the following test:

    assert_eq!(atan2(2.0, -1.0), atan(2.0 / -1.0) + PI);atan2(2.0, -1.0)

The output is optimization-dependent. The new `release-checked` profile
produces the following failure:

    thread 'math::atan2::sanity_check' panicked at src/math/atan2.rs:123:5:
    assertion `left == right` failed
      left: 2.0344439357957027
     right: 2.0344439357957027

Similarly, `sin::test_near_pi` fails with the following:

    thread 'math::sin::test_near_pi' panicked at src/math/sin.rs:91:5:
    assertion `left == right` failed
      left: 6.273720864039203e-7
     right: 6.273720864039205e-7

Mark the tests ignored on `i586` for now.
`#![feature(start)]` was removed in [1], but we make use of it in the
intrinsics example. Replace use of this feature with `#[no_mangle]`
applied to `#[main]`.

We don't actually run this example so it is not a problem if this is not
entirely accurate. Currently the example does not run to completion,
instead invoking `rust_begin_unwind`.

[1]: rust-lang#134299
…ates

Rework the available Cargo profiles
Rather than keeping a script that downloads the tarball, we can just add
musl as a submodule and let git handle the synchronizatoin. Do so here.
0.17.10 introduced a change that removes `Sync` from `ProgressStyle`,
which makes it more difficult to share in a callback. Pin the dependency
for now until we see if `indicatif` will change this back or if we need
to find a workaround.
llvm/llvm-project#116706 added Windows
support to cpu_model. Compiling for UEFI also goes through that
code path, because we treat it as a windows target. However,
including windows.h is not actually going to work (and the used
API would not be available in an UEFI environment).

Disable building of cpu_model on UEFI to fix this.
This reverts commit 1dacdabdb6186f97144c50f8952575576deb3730.
This isn't very useful for constants since the trait constants are
available, but does enable roundtripping via hex float syntax.
Add support for printing hex float syntax
Simplify the SPDX string to the user-facing version to make it easier for
users and tooling to understand. Contributions must still be `MIT OR Apache-2.0`. 

[ add commit body with context - Trevor ]
`EXP_MAX` sounds like it would be the maximum value representable by
that float type's exponent, rather than the maximum unsigned value of
its bits. Clarify this by renaming to `EXP_SAT`, the "saturated"
exponent representation.
@tgross35 tgross35 added the A-compiler-builtins Area: compiler-builtins (https://github.com/rust-lang/compiler-builtins) label May 18, 2025
@tgross35 tgross35 force-pushed the builtins-josh-subtree branch 2 times, most recently from 349c588 to 2c2c6f1 Compare May 20, 2025 13:53
@Kobzol
Copy link
Contributor

Kobzol commented May 22, 2025

Note that https://github.com/rust-lang/compiler-builtins will have to switch to merge commits (instead of rebases), otherwise you're not going to have a good time there with josh 😆 Are you 100% sure that the selected directory path will be the final one? I don't think it's very easy to change the directory once we start the Josh migration.

@Kobzol
Copy link
Contributor

Kobzol commented May 22, 2025

I confirm that I was able to reproduce the commits from this PR with this:

$ git fetch https://github.com/rust-lang/compiler-builtins.git 233434412fe7eced8f1ddbfeddabef1d55e493bd
$ josh-filter ":prefix=library/compiler-builtins" FETCH_HEAD
$ git merge --allow-unrelated FILTERED_HEAD

It produced abbf8fe just before the final merge commit (which will be different between this PR and my local setup, ofc). My master commit was 3bd1def7ffab238b7f7f6d4738fc491a87ee6635.

I then used josh-proxy to undo the transformation (https://hackmd.io/7pOuxnkdQDaL1Y1FQr65xg?both#How-to-migrate-from-git-submodule-to-josh-proxy) and got back to 233434412fe7eced8f1ddbfeddabef1d55e493bd:

$ josh-proxy --local $HOME/.cache/josh --remote https://github.com --port 42042 --no-background
$ git fetch http://localhost:42042/kobzol/rust.git@3bd1def7ffab238b7f7f6d4738fc491a87ee6635:/library/compiler-builtins.git

@tgross35
Copy link
Contributor Author

Note that https://github.com/rust-lang/compiler-builtins will have to switch to merge commits (instead of rebases), otherwise you're not going to have a good time there with josh 😆

This is only for rustc->compiler-builtins josh merges correct? I'm assuming normal PRs to compiler-builtins must still be able to apply directly.

Are you 100% sure that the selected directory path will be the final one? I don't think it's very easy to change the directory once we start the Josh migration.

Other sysroot crates like stdarch and backtrace are also in the same directory, so library/compiler-builtins seems like a reasonable home. I guess @Amanieu do you have any preference here?

@Kobzol
Copy link
Contributor

Kobzol commented May 22, 2025

This is only for rustc->compiler-builtins josh merges correct? I'm assuming normal PRs to compiler-builtins must still be able to apply directly.

Hmm, I guess so, but if you don't disable rebases in the repository settings, sooner or later someone will rebase a sync PR and that will screw up everything. I would strongly advise to use only merge commits and only enable merge commits in the repository settings.

@tgross35
Copy link
Contributor Author

Enabled the merge strategy 👍. I don't really want it as the default though; the PRs are usually small and merges wind up with pretty complicated history. Keeping things linear seems more robust especially when the history is going to get synced two places. I'll add a warning to the PR message, though.

GH could really use a --ff-only merge option. You can actually get this behavior locally with git merge --ff-only some-pr-branch; git push upstream master (which only succeeds if there is an open PR with approvals and passing CI), but there isn't any way to do this with the UI.

@Kobzol
Copy link
Contributor

Kobzol commented May 22, 2025

GH could really use a --ff-only merge option. You can actually get this behavior locally with git merge --ff-only some-pr-branch; git push upstream master (which only succeeds if there is an open PR with approvals and passing CI), but there isn't any way to do this with the UI.

You can configure this:
image

to require PRs to be based on the latest master. But if you merge with the rebase strategy, GH will create new commits anyway, which defeats the point w.r.t. josh.

@tgross35 tgross35 changed the title Add compiler-builtins as a subtree Merge compiler-builtins as a Josh subtree May 28, 2025
tgross35 added 3 commits May 28, 2025 15:15
compiler-builtins has a symlink to the `libm` source directory so the
two crates can share files but still act as two separate crates. This
causes problems with some sysroot-related tooling, however, since
directory symlinks seem to not be supported.

The reason this was a symlink in the first place is that there isn't an
easy for Cargo to publish two crates that share source (building works
fine but publishing rejects `include`d files from parent directories, as
well as nested package roots). However, after the switch to a subtree,
we no longer need to publish compiler-builtins; this means that we can
eliminate the link and just use `#[path]`.

If we need to publish compiler-builtins again for any reason, it would
be easy to revert this in a preprocess step.
@tgross35 tgross35 force-pushed the builtins-josh-subtree branch from 2c2c6f1 to e8a36fb Compare May 28, 2025 15:15
@tgross35 tgross35 marked this pull request as ready for review May 28, 2025 15:17
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 28, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 28, 2025

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@rustbot
Copy link
Collaborator

rustbot commented May 28, 2025

⚠️ Warning ⚠️

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 28, 2025
@tgross35
Copy link
Contributor Author

Okay, I verified that syncs seem to work as well as they can without having this merged yet. I have some tooling ready at rust-lang/compiler-builtins#921 that I'll verify once this does merge.

The relevant merge commit is fcb3000. The changes applied on top of the merge are at fcb3000...e8a36fb.

@Kobzol ready for your r+ if everything looks right :)

@tgross35 tgross35 removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 28, 2025
@tgross35
Copy link
Contributor Author

There are a couple of commits in compiler-builtins since the merge here, but I figure that will be a good chance to test syncing.

@Kobzol
Copy link
Contributor

Kobzol commented Jun 2, 2025

Sorry this took so long! I tried to do back-to-back pulls (Kobzol/compiler-builtins#3) and pushes (Kobzol#56) on my forks and everything seems to be working fine.

I would like to use the existing Rust-based tooling, just to make sure that we consistently use the same pull/push logic, so I'm less sure about rust-lang/compiler-builtins#921, especially it since doesn't work for me locally due to Bash script inconsistencies 😅

But that's orthogonal from the actual rustc merge. To the best of my knowledge, the Josh sync was performed correctly. So:

@bors r+ rollup=never

@bors
Copy link
Collaborator

bors commented Jun 2, 2025

📌 Commit e8a36fb has been approved by Kobzol

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiler-builtins Area: compiler-builtins (https://github.com/rust-lang/compiler-builtins) A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-tidy Area: The tidy tool has-merge-commits PR has merge commits, merge with caution. rla-silenced Silences rust-log-analyzer postings to the PR it's added on. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.