Allow 65536 pages #2258
Allow 65536 pages #2258
Conversation
k-nasa
requested review from
MarkMcCaskey,
nlewycky and
syrusakbary
as code owners
|
I would like to add a test to detect this issue. But I don't understand structure of tests in this repository so I don't know where to add it. Can you give me a signpost? |
|
Thank you for the patch! A |
lib/vm/src/memory.rs
Outdated
| @@ -259,6 +259,7 @@ impl LinearMemory { | |||
| let mapped_pages = memory.minimum; | |||
| let mapped_bytes = mapped_pages.bytes(); | |||
|
|
|||
| println!("{}", minimum_bytes); | |||
There was a problem hiding this comment.
🙈 I fixed it. 755639c
|
Thanks @nlewycky! I was able to add test. |
lib/types/src/units.rs
Outdated
| impl From<u32> for Bytes { | ||
| fn from(other: u32) -> Self { | ||
| impl From<u64> for Bytes { | ||
| fn from(other: u64) -> Self { |
There was a problem hiding this comment.
Please keep both implementations, otherwise it's a regression :-).
| @@ -340,7 +340,7 @@ pub struct VMMemoryDefinition { | |||
| pub base: *mut u8, | |||
|
|
|||
| /// The current logical size of this linear memory in bytes. | |||
| pub current_length: u32, | |||
| pub current_length: u64, | |||
There was a problem hiding this comment.
I wonder if we should not create a type alias to represent the memory size, something like pub type MemorySize = u64. Thoughts @nlewycky, @MarkMcCaskey, @syrusakbary?
There was a problem hiding this comment.
I don't think so in this case, we aren't going to be changing this type or confusing it with other values much.
There was a problem hiding this comment.
This:
https://sourcegraph.com/github.com/wasmerio/wasmer/-/blob/lib/compiler-llvm/src/translator/intrinsics.rs#L620:13
also need to be updated from i32 to i64 or else we'll compute the wrong value, and that may require some further changes (llvm is a strongly-typed IR and so any place it's used with another i32 will fail the verifier).
Similarly singlepass loads a 32-bit value into a register here:
https://sourcegraph.com/github.com/wasmerio/wasmer/-/blob/lib/compiler-singlepass/src/codegen_x64.rs#L1280
I'm not sure what the equivalent is in cranelift, possibly the type of the global_value in make_heap in src/func_environ.rs (not src/translator/func_environ.rs)?
| @@ -340,7 +340,7 @@ pub struct VMMemoryDefinition { | |||
| pub base: *mut u8, | |||
|
|
|||
| /// The current logical size of this linear memory in bytes. | |||
| pub current_length: u32, | |||
| pub current_length: u64, | |||
There was a problem hiding this comment.
I don't think so in this case, we aren't going to be changing this type or confusing it with other values much.
syrusakbary
added
the
🕵️ needs investigation
|
Superseded by #2677 |
Description
fix: #2187
I fixed this issue.
Review