WebAssembly / memory64

Memory with 64-bit indexes
Other
194 stars 21 forks source link

64-bit shared memories specified with 32-bit limits #21

Closed alexcrichton closed 1 year ago

alexcrichton commented 2 years ago

I was reading over the spec again today and noticed that the binary encoding for limits for memories is specified as:

limits ::= 0x00 n:u32        ⇒ i32, {min n, max ϵ}, 0
        |  0x01 n:u32 m:u32  ⇒ i32, {min n, max m}, 0
        |  0x02 n:u32        ⇒ i32, {min n, max ϵ}, 1  ;; from threads proposal
        |  0x03 n:u32 m:u32  ⇒ i32, {min n, max m}, 1  ;; from threads proposal
        |  0x04 n:u64        ⇒ i64, {min n, max ϵ}, 0
        |  0x05 n:u64 m:u64  ⇒ i64, {min n, max m}, 0
        |  0x06 n:u32        ⇒ i64, {min n, max ϵ}, 1  ;; from threads proposal
        |  0x07 n:u32 m:u32  ⇒ i64, {min n, max m}, 1  ;; from threads proposal

It seems like the interpretation seems to be for the leading flags byte:

This doesn't quite align with the table, though, specifically the last two cases:

        |  0x06 n:u32        ⇒ i64, {min n, max ϵ}, 1  ;; from threads proposal
        |  0x07 n:u32 m:u32  ⇒ i64, {min n, max m}, 1  ;; from threads proposal

these are currently specified as a 32-bit specification of the min/max sizes instead of what I was naively expecting as a 64-bit specification.

Is this intentional? (or perhaps a typo?) If this is intentional, could a few words be added as to why shared memories don't get 64-bit limits just yet (I'm not sure myself, mostly just curious!)

aardappel commented 2 years ago

Pretty sure that's a typo, limits are 64-bit independently of the other 2 flags, and that's how it is implemented so far in the tools.

Care to make a PR?

@rossberg

alexcrichton commented 2 years ago

Ok makes sense and that's what I figured, just wanted to confirm! I'll send a PR to update the overview tomorrow

rossberg commented 2 years ago

Yes, that looks like a C&P error.

davnavr commented 1 year ago

I was fuzzing a WASM parser I made, and found that the current overview still has the typo. Maybe someone can merge the changes into main?

sbc100 commented 1 year ago

That is strange. I have no clue why that PR was closed rather than merged..