Patch release 0.32.1. #3620
Merged
Patch release 0.32.1. #3620
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Uncovered by @bjorn3 (thanks!): 8- and 16-bit rotates were not working properly in recent versions of Cranelift with part of the lowering migrated to ISLE. This PR fixes a few issues: - 8- and 16-bit rotate-left needs to mask a constant amount, if any, because we use a 32-bit rotate instruction and so don't get the appropriate shift-amount masking for free from x86 semantics. - `operand_size_from_type` was incorrect: it only handled 32- and 64-bit types and silently returned `OperandSize::Size32` for everything else. Now uses the `OperandSize::from_ty(ty)` helper as the pre-ISLE code did. Our test coverage for narrow value types is not great; this PR adds some runtests for rotl/rotr but more would always be better!
…e-width for shifts.
Fixes bytecodealliance#3609. It turns out that `sha2` is a nontrivial dependency for Cranelift in many contexts, partly because it pulls in a number of other crates as well. One option is to remove the hash check under certain circumstances, as implemented in bytecodealliance#3616. However, this is undesirable for other reasons: having different dependency options in Wasmtime in particular for crates.io vs. local builds is not really possible, and so either we still have the higher build cost in Wasmtime, or we turn off the checks by default, which goes against the original intent of ensuring developer safety (no mysterious stale-source bugs). This PR uses `SipHash` instead, which is built into the standard library. `SipHash` is deprecated, but it's fixed and deterministic (across runs and across Rust versions), which is what we need, unlike the suggested replacement `std::collections::hash_map::DefaultHasher`. The result is only 64 bits, and is not cryptographically secure, but we never needed that; we just need a simple check to indicate when we forget a `rebuild-isle`.
cfallin
force-pushed
the
release-0.32.1
branch
from
e10171b
to
c1c4c59
Compare
github-actions
bot
added
cranelift
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "isle", "wasmtime:docs"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
alexcrichton
approved these changes
Dec 17, 2021
There was a problem hiding this comment.
👍
Looks good to me, some minor things:
- Could the branch be called
release-0.32.0orrelease-0.32.xor something like that? In theory we could do 0.32.2 in the future and we won't need a branch-per-release, just a branch-per-major-version. - I think in theory there's a button on the GitHub UI where after this merges we can trigger the patch release which should send another PR to the repo for us to merge with version bumps.
|
Cool, running the version-bump job now with |
|
OK, that job failed; I filed #3621 for the issue. It's probably a relatively simple fix, but not a one-liner, and might need some more debugging or run into other issues later in the process. I need to context-switch away to some other things before leaving for two weeks; so this patch release will have to wait (sorry @bjorn3) -- since the known consumer of Cranelift that runs into the 8/16-bit issues can pin to an earlier version for now, I don't think it's security-issue-level urgent. |
|
I understand. I will push a commit to cg_clif to revert back to the previous cranelift release. Edit: done in bjorn3/rustc_codegen_cranelift@f505157 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a patch release (nominally Cranelift, though also for Wasmtime due to process limitations at the moment) with cherry-picks of these three PRs: #3610, #3617, #3619. This fixes codegen bugs in Cranelift that prevented it from being used by cg_clif, and possible other consumers; any codegen bug is worthy of a patch release.
This is our first time using the patch release process; the PR merges the cherrypicked commits and a
RELEASES.mdupdate to arelease-0.32.1branch in the main repo, and then we can trigger the version-bump/release workflow from there.