Skip to content

Use u64 to track file offsets instead of usize#2573

Open
WorldSEnder wants to merge 10 commits into
bytecodealliance:mainfrom
WorldSEnder:u64-position
Open

Use u64 to track file offsets instead of usize#2573
WorldSEnder wants to merge 10 commits into
bytecodealliance:mainfrom
WorldSEnder:u64-position

Conversation

@WorldSEnder

@WorldSEnder WorldSEnder commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

This replaces usize in a lot of places. Besides being a breaking change, this should not impact functionality for most users. Tracking file offset with u64 is the more principled choice.

Fixes #838

Solves a TODO left long ago in the entry code of parse. Most of the places used usize to pass it along to other functions, sinking into Error code for nice error messages, and are thus not sensitive to the data type used. I found two cases that used usize::MAX as a dummy index in a path that called expect() on the eventual error.

There is a small abstraction in offsets.rs exposed as InMemData that allows conversion from Range<u64> to byte slices in case the whole data input fits in memory.

This replaces usize in a lot of places. Besides
being a technically breaking change, this should
not impact functionality.
Solves a TODO left long ago in the entry code
of parse.
@WorldSEnder

Copy link
Copy Markdown
Contributor Author

A lot of consumers seem to hold the entire wasm input in memory, hence these need some ergonomics to convert their offsets back to Range<usize>. Not sure what the best approach is.

@WorldSEnder WorldSEnder changed the title Use u64 internally to track file offsets instead of usize Use u64 to track file offsets instead of usize Jul 21, 2026
@WorldSEnder
WorldSEnder marked this pull request as ready for review July 21, 2026 10:12
@WorldSEnder
WorldSEnder requested a review from a team as a code owner July 21, 2026 10:12
@WorldSEnder
WorldSEnder requested review from alexcrichton and removed request for a team July 21, 2026 10:12
@WorldSEnder

Copy link
Copy Markdown
Contributor Author

I have not removed MemOffset outright, I think it's worthwhile the small indirection for BinaryReader but I have removed the additional cfg(debug_assertions) field. It's now a transparent usize.

@alexcrichton alexcrichton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Personally I still feel that InMemData and MemOffset aren't worth it. This is already a very large change due to how many places it has to touch, and in my mind it's a consequence of the change at all that as casts are going to be needed when in-memory data is indexed with the offsets coming out of wasmparser. While I've no doubt that certain abstractions could cut down on the number of resulting as casts I'm not sure if the benefit of such abstractions outweighs their costs (e.g. new primitives to learn).

@WorldSEnder

WorldSEnder commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Personally I still feel that InMemData and MemOffset aren't worth it. This is already a very large change due to how many places it has to touch, and in my mind it's a consequence of the change at all that as casts are going to be needed when in-memory data is indexed with the offsets coming out of wasmparser. While I've no doubt that certain abstractions could cut down on the number of resulting as casts I'm not sure if the benefit of such abstractions outweighs their costs (e.g. new primitives to learn).

I can see that for MemOffset, which is internal to the parser only, and serves partly as a namespace to attach the methods to. All the methods are readily inlined and the u64: Add<MemOffset> impl could just be written with as as cast (losing the debug assertion but oh well). I'm ready to do that at some point today.

InMemData on the other hand is repeatedly used in the consuming crates (and would be in my downstream consumer, too). I could doc-comment this a bit more to teach how to use it. The doc comment change in wasm-encoder should be a good example already. It wraps a slice to the data, it derefs to that slice so you can pass it instead of the data to BinaryReader::new or parse_all (e.g. here or here and here). It then lets you index into the data with the ranges and offset the parser emits.

One oddity for example is that since Range<u64> does not implement iterator, you can't call len() on it as before, which was used to calculate how many bytes. The alternative could be a custom range type that does have this method or writing it out everytime. As is, i think it leaves a clear marker at use site that yeah we are indeed talking about wasm that fits into memory.

In any case, conversion was much easier with InMemData than writing the same or similar thing out in eight different crates. Even if it's "just" as casts as I reader I would always wonder if we are not silently truncating away bits.

@WorldSEnder

Copy link
Copy Markdown
Contributor Author

In the course of adding docs to InMemData I have found that RelocationEntry currently reports the relocation_range as a usize. Should this be adjusted to either u32 (limited by the underlying binary representation?) or u64 (for consistency) instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

wasmparser: usize may not be the most appropriate type for offsets

2 participants