Use u64 to track file offsets instead of usize

[WorldSEnder]

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.

[WorldSEnder]

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]

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]

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]

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]

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?

[alexcrichton]

After reading this in full again, personally I'd still subjectively disagree. I understand taht InMemData is used a fair bit, but it's a pretty nontrivial abstraction. I find it much easier to reason about possibly-slightly-more-verbose code using try_into conversions or such rather than seeing an InMemData abstraction and immediately having to go look up what it's doing and what it looks like under the hood.

At most I think it'd be ok to have a small set of helpers in wasmparser::* to do something with Range<u64> and get usize-like things out, or maybe indexing, but I'd want that to be constrained to maybe at most 2-3 functions if that were done. Otherwise I feel that InMemData is just a bit too foundational in the wrong place and it's not carrying its weight for being a foundational abstraction

[WorldSEnder]

Otherwise I feel that InMemData is just a bit too foundational in the wrong place and it's not carrying its weight for being a foundational abstraction

Not sure I follow the "foundational" aspect. To me, the wrapper struct is as much utility as the proposed "2-3 functions". It doesn't cross into parser code, or even other crate boundaries. It's certainly not meant to show up in any public API except as an optional utility to ease the conversion to u64 ranges. A consuming crate can choose to wrap the byte slice it passes to the parsing functions so that it itself can index into that slice with the ranges coming back the parser. The fact that a crate did that wrapping should be a transparent implementation detail, and the wrapper behaves as best it can as that byte slice, except for the additional indexing access.

But alas, I will push the indexing into the consuming crates and duplicate the small bits of code tomorrow.

[alexcrichton]

The foundational part I find odd is that every crate in this workspace is migrated to use it, while simultaneously nothing in wasmparser requires it. That puts the abstraction in this weird spot where it's supposed to be used but only if you're "in the know". I'm not actually sure if utility functions make sense over just using as casts myself, it's mostly just an idea to help reduce duplication if you'd like.

[WorldSEnder]

it's supposed to be used but only if you're "in the know".

I agree on the doc issue and that it will be a bit hard to discover. If you have a better spot where to put it (be it structs or helper methods) feel free to suggest.

The reason for using it in almost every crate in the workspace is simply that they all end up loading the full wasm into memory even if the parser rejects this model, some could reasonably be rewritten into the streaming mode without holding all of the wasm in memory;

• (1) wasm-metadata rewrites section by section as far as I can tell and shouldn't need to call parse_all. Currently it does and slices to emit some sections byte-by-byte from the input.
• (2) wit-component similar to wasm-metadata to copy whole sections (and to skip some bytes at the start).
• (3) demangle similar to wasm-metadata to copy whole sections from the input (and operate on the name custom sections).
• (4) strip similar to wasm-metadata to copy whole sections that don't get stripped
• (5) wasm-mutate reparses the code section to replace opcodes and also copies raw
• (6) dump to print bytes literally from the input
• (7) wasmprinter uses only the helper InMemData::range_len to skip the section in its input
• (8) wast similarly uses only InMemData::translate_offset in a test case
• (9) validate to check that the ranges are in bound. This also seems to needlessly be done in usize instead of u64 since the checks boil down to checking end < len. Is there even a point where the parser emits a potentially invalid range?

At least (1)-(4) serve the same purpose to copy over whole sections. (5) and (6) do some work on the bytes instead of just reencoding them raw. (7) uses the range to avoid parsing the unchecked range of component sections and skip some bytes in the input. (8) and (9) are mostly reuse for reuse sake.

---

Perhaps it would be easier to discover and use if the "address translation" into the parsed wasm bytes was returned from the parse function(s) instead of being passed in as a wrapped byte slice - as part of Chunk::Parsed? Imo, at least the first two uses deserve some support by the parser library to find the relevant bytes in the input.