cranelift-entity: remove unused `get_raw_mut` and `into_boxed_slice` by b84330808 · Pull Request #15159 · bytecodealliance/wasmtime

b84330808 · 2026-11-05T07:37:22Z

This removes two methods in cranelift-entity whose unsafe code has no callers left in the workspace:

PrimaryMap::get_raw_mut — only referenced by its own unit test and by an equally unused wrapper in wasmtime-environ (TryPrimaryMap::get_raw_mut); both are removed. This also addresses bjorn3's point on the earlier revision: the wrapping_add there had no in-bounds guarantee to lean on, so the sound thing is for the method not to exist.
PrimaryMap::into_boxed_slice — only used by the BoxedSlice tests; they now construct BoxedSlice directly through its pub(crate) constructor.

Motivation: reducing the unsafe surface where the code is dead anyway. There is no bug behind this — the methods were not called, they just carried unsafe contracts for no benefit. This kind of unsafe-reduction is also the class of change covered by Google's Patch Rewards program for OSS-Fuzz projects (Tier 2 hardening), which is part of why I went looking for it.

An earlier version of this PR also added #![forbid(unsafe_code)] to the crate; I removed that again after fitzgen's review, since the crate may legitimately want unsafe in the future.

Testing: cargo test -p cranelift-entity — 59 unit tests and 2 doc tests pass; cargo check -p wasmtime-environ passes.

Open question from the review thread: BoxedSlice itself now has no users and cannot be constructed from outside the crate. I'm happy to remove it in this PR too if the maintainers want that.

`PrimaryMap::into_boxed_slice` turned its `Box<[V]>` into a raw pointer only to rebuild the same `Box` in the unsafe `BoxedSlice::from_raw`. Build the `BoxedSlice` from the `Box` directly instead, and remove `from_raw`, which has no other users in this repository. `PrimaryMap::get_raw_mut` already checks that the index is in bounds, so `wrapping_add` gives the same pointer as `add` without `unsafe`. The crate now has no `unsafe` code, so add `#![forbid(unsafe_code)]`.

bjorn3 · 2026-11-05T10:09:13Z

-            unsafe { Some(self.elems.as_mut_ptr().add(k.index())) }
+            // The index is in bounds per the check above, so `wrapping_add`
+            // produces the same pointer that `add` would, without `unsafe`.
+            Some(self.elems.as_mut_ptr().wrapping_add(k.index()))


This would likely cause LLVM to do a worse job at optimizing as the result of wrapping_add is not known to be in-bounds. And if the add was UB, the user would end up with UB anyway when dereferencing this pointer.

You're right — and it turns out nothing calls it. I grepped the whole workspace: the only references were its own unit test and the TryPrimaryMap::get_raw_mut wrapper in wasmtime-environ, which also has no callers. I've removed both (and the test) in a follow-up commit, so the wrapping_add is gone entirely and forbid(unsafe_code) no longer depends on it. cargo test -p cranelift-entity and cargo check -p wasmtime-environ pass.

bjorn3 · 2026-11-05T10:12:37Z

    pub fn into_boxed_slice(self) -> BoxedSlice<K, V> {
-        unsafe { BoxedSlice::<K, V>::from_raw(Box::<[V]>::into_raw(self.elems.into_boxed_slice())) }
+        BoxedSlice::new(self.elems.into_boxed_slice())
    }


Is this method actually used anywhere?

It isn't. The only callers in the workspace were the BoxedSlice tests in this crate; every other into_boxed_slice hit is the Vec/TryVec method on unrelated types. I've removed PrimaryMap::into_boxed_slice, and the tests now build the slice directly through the pub(crate) BoxedSlice::new.

That leaves BoxedSlice itself with no users and no way to construct one from outside the crate. Do you want me to remove BoxedSlice entirely in this PR as well, or keep it?

Went ahead with this: BoxedSlice is removed entirely in 7ddf528. After into_boxed_slice was deleted, BoxedSlice::new had no callers left, and CI (which builds with -D warnings) failed on the dead code, cascading into the dependent check and test jobs, so keeping the type was not viable. Locally, cargo check -p cranelift-entity --all-targets with -D warnings is clean, the tests pass (52 + 2; the seven dropped tests were BoxedSlice's own), and the crate is rustfmt-clean. Happy to restore the type if you would rather keep it.

Per review: wrapping_add in get_raw_mut loses the in-bounds information LLVM needs, and if the add were UB the dereference would be too. Neither get_raw_mut (including the TryPrimaryMap wrapper in wasmtime-environ) nor PrimaryMap::into_boxed_slice has any caller in the workspace, so remove them instead. The BoxedSlice tests now build the slice directly through the pub(crate) constructor. With get_raw_mut gone, forbid(unsafe_code) no longer relies on the wrapping_add workaround.

fitzgen

Is there some specific motivation here? I feel like this PR is missing context. I don't think we necessarily want to forbid(unsafe_code), there are valid reasons we might want to use unsafe in this crate in the future.

b84330808 · 2026-11-05T14:48:36Z

Fair question, and sorry for the missing context — I should have written this in the description from the start.

The motivation is honestly pretty modest. I've been looking for places where unsafe can be removed outright rather than justified, and in this crate the only two remaining pieces of unsafe turned out to be methods with no callers: get_raw_mut and PrimaryMap::into_boxed_slice. There's no bug behind it — nothing crashed and nobody hit the get_raw_mut contract in practice; it was dead code carrying an unsafe API for no benefit. For background, this kind of unsafe-reduction is also what Google's Patch Rewards program covers for OSS-Fuzz projects (Tier 2 hardening), which is part of why I went looking — but I'm not attached to landing this for that reason, and if the change isn't useful to you it shouldn't stay open just for that.

On forbid(unsafe_code): you're right, and I don't want to lock the crate against future legitimate uses of unsafe. I've pushed a commit (47f59e9) that drops the #![forbid(unsafe_code)] attribute, so what's left is only the removal of the two unused methods, which bjorn3 already reviewed above. The BoxedSlice question I asked bjorn3 in the thread is still open — happy to fold that removal in here or leave the type in place, whichever you both prefer.

If even the trimmed-down version isn't something you want, please feel free to close this — no hard feelings.

Per fitzgen's review: the crate may legitimately want unsafe in the future, and forbidding it at the crate level is stronger than this change needs. The crate currently compiles without any unsafe either way; this keeps only the removal of the two unused methods.

fitzgen · 2026-11-05T15:10:56Z

@b84330808 can you clarify whether your interactions here are abiding by the project's AI policy?

b84330808 · 2026-11-05T15:26:00Z

Yes, happy to clarify. I'm directing this work myself and I review what goes in before anything is submitted. AI tools are part of my process, but there is no unattended agent acting here — the changes, the description and these replies are mine to answer for, and I can walk through the diff if that's useful (it's a small one: two methods with no callers in the workspace, plus their tests). The missing motivation up front was my oversight; the description has it now.

I also took your point on forbid(unsafe_code) and dropped that part — the PR is just the dead-code removal at this point. If you still judge it not worth the review time, I understand closing it.

cfallin

The DCE seems fine to me; this is very old code that probably was written as a generic data structure library before most of Cranelift+Wasmtime grew up around it and solidified our usage patterns.

Agreed with Nick that we don't want to forbid unsafe code; passively reducing it (if free) is always great though.

Just one nit below re: formatting. Thanks!

cfallin · 2026-11-05T16:32:14Z

            Err(slice::GetDisjointMutError::IndexOutOfBounds)
        );
    }
+


stray blank line inserted here?

Fixed in 860dab5 — thanks, that was left over from removing the test.

Also ran rustfmt over the crate and fixed a second stray blank line of the same kind at the end of the impl block (e52e0eb). The crate is rustfmt-clean now.

…removals

With PrimaryMap::into_boxed_slice gone, BoxedSlice::new has no callers left and CI (-D warnings) fails on the dead code, which also cascades into the dependent check/test jobs. As suggested in review, remove the type entirely.

cfallin · 2026-11-05T18:27:45Z

Thanks; it looks like you have another rustfmt error with another stray line.

Left over from removing TryPrimaryMap::get_raw_mut; the blank line before the impl block's closing brace is what the rustfmt CI check flags. The other files in this PR check out clean.

b84330808 · 2026-11-05T18:46:09Z

Fixed in b33ef57 — you were right, there was one more. It was in crates/environ/src/collections/primary_map.rs: a stray blank line before the closing brace of the TryPrimaryMap impl, left over from removing get_raw_mut. My earlier rustfmt pass only covered the cranelift-entity crate, which is how I missed this one. I've now run rustfmt --check over every file this PR touches; they are clean.

b84330808 requested a review from a team as a code owner November 5, 2026 07:37

b84330808 requested review from cfallin and removed request for a team November 5, 2026 07:37

github-actions Bot added the cranelift Issues related to the Cranelift code generator label Nov 5, 2026

bjorn3 reviewed Nov 5, 2026

View reviewed changes

b84330808 requested a review from a team as a code owner November 5, 2026 10:22

fitzgen reviewed Nov 5, 2026

View reviewed changes

b84330808 changed the title ~~cranelift-entity: remove the remaining unsafe code~~ cranelift-entity: remove unused get_raw_mut and into_boxed_slice Nov 5, 2026

cfallin approved these changes Nov 5, 2026

View reviewed changes

b84330808 added 3 commits November 5, 2026 16:45

cranelift-entity: remove stray blank line in primary.rs tests

860dab5

cranelift-entity: remove another stray blank line left by the method …

e52e0eb

…removals

cranelift-entity: remove the now-unused BoxedSlice type

7ddf528

With PrimaryMap::into_boxed_slice gone, BoxedSlice::new has no callers left and CI (-D warnings) fails on the dead code, which also cascades into the dependent check/test jobs. As suggested in review, remove the type entirely.

environ: remove a stray blank line in primary_map.rs

b33ef57

Left over from removing TryPrimaryMap::get_raw_mut; the blank line before the impl block's closing brace is what the rustfmt CI check flags. The other files in this PR check out clean.

cfallin enabled auto-merge November 5, 2026 18:52

cfallin added this pull request to the merge queue Nov 5, 2026

Merged via the queue into bytecodealliance:main with commit 55a5321 Nov 5, 2026
52 checks passed

Conversation

b84330808 commented Nov 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

b84330808 commented Nov 5, 2026

Uh oh!

fitzgen commented Nov 5, 2026

Uh oh!

b84330808 commented Nov 5, 2026

Uh oh!

cfallin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfallin commented Nov 5, 2026

Uh oh!

b84330808 commented Nov 5, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

b84330808 commented Nov 5, 2026 •

edited

Loading