Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

wasm proposal memory64 #2964

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

wasm proposal memory64 #2964

wants to merge 1 commit into from

Conversation

dannypsnl
Copy link
Member

@dannypsnl dannypsnl commented Oct 24, 2023 •
edited

resolve #2779

@juntao Flows.network Integration
Copy link
Member

juntao commented Oct 24, 2023 •
edited

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.

Commit 3665603f934b4ac12298887afdcdefd7599f8682

The pull request titled "wasm proposal: memory64" contains changes pertaining to the introduction of 64-bit memory support for WebAssembly. Here are the key changes identified in this pull request:

  1. Loader: The primary changes in the loader entail encoding limits and the introduction of a missing index type setup for memory type. The 64-bit memory support assumes the default to be a 32-bit model, hence the related values have been updated to uint64_t.

  2. Validator: A helper function getItValueType was introduced for type checking. Additional type checks have been added for store/load instructions, atomic instructions, and v128 instructions. The validator now recognizes 64-bit page counts and offsets.

  3. Execution: Code sharing for 32/64-bit modes has been improved, with relevant changes made in memory instructions related to load/store and atomic operations.

  4. Additional changes have been made to the Stack Manager, Driver, and Spec Tests to support 64-bit memory.

  5. Fixups for unresolved issues, such as the bus error (resulting from two combined problems in the executor and memory checks), issues with f32.load (due to incorrect uint64_t usage), and Windows platform building issues.

Potential problems:

  1. Backward Compatibility: This change modifies many areas of the codebase and may pose backward compatibility issues with existing 32-bit code. Thorough testing is required to ensure backward compatibility.

  2. Testing: It's hard to be confident in this change without seeing the accompanying tests. The description mentions fixes to tests, but the coverage and whether the tests also include 64-bit scenarios is unclear.

  3. Performance Impact: The changes could have an impact on performance, especially when working with a 32-bit model, as values have been updated to uint64_t. Detailed performance testing and conclusions should be done.

  4. Windows Building Compatibility: There have been changes made to improve Windows compatibility, but the summary doesn't provide specifics about which builds this impacts, nor is there any commentary as to whether these changes would impact builds on other platforms.

  5. Bugs & Errors: The proposed changes introduce fixes for a bus error and overflow problem, but this shows that changes can introduce serious runtime errors. Rigorous testing is important to address this.

@github-actions github-actions bot added c-CLI An issue related to WasmEdge CLI tools c-CAPI An issue related to the WasmEdge C API c-Test An issue/PR to enhance the test suite labels Oct 24, 2023
dannypsnl
dannypsnl commented Oct 30, 2023
View reviewed changes
test/loader/typeTest.cpp
Comment on lines -359 to +361
0x02U, // Unknown limit type
0x08U, // Unknown limit type
Copy link
Member Author

@dannypsnl dannypsnl Oct 30, 2023 •
edited

Choose a reason for hiding this comment

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

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:u64        ⇒ i64, {min n, max ϵ}, 1  ;; from threads proposal
0x07 n:u64 m:u64  ⇒ i64, {min n, max m}, 1  ;; from threads proposal

@dannypsnl dannypsnl force-pushed the wasm-proposal-memory64 branch 4 times, most recently from 4b5d70b to 30eb26e Compare October 31, 2023 07:34
@dannypsnl

This comment was marked as resolved.

Sign in to view
@dannypsnl dannypsnl force-pushed the wasm-proposal-memory64 branch 3 times, most recently from 482068e to c28d9ea Compare November 9, 2023 01:19
@dannypsnl
Copy link
Member Author

dannypsnl commented Nov 9, 2023 •
edited

Update status

platform status
windows 5 tests failed
macos 2 tests failed (only AOT failed)
ubuntu 5 tests failed
fedora 3 tests failed

The status of linux is interesting, since I don't I touch any platform specific program.

@github-actions github-actions bot added the c-AOT label Nov 9, 2023
@dannypsnl dannypsnl force-pushed the wasm-proposal-memory64 branch 2 times, most recently from 08bc089 to a7c4a28 Compare November 14, 2023 03:50
@q82419 q82419 force-pushed the proposal/func-ref branch 3 times, most recently from 54be59d to 7f2f73e Compare December 2, 2023 17:32
@q82419 q82419 force-pushed the proposal/func-ref branch 2 times, most recently from e3a795c to da5de5a Compare December 7, 2023 17:59
Base automatically changed from proposal/func-ref to master December 13, 2023 01:54
@dannypsnl
wasm proposal: memory64
3665603 
* loader
  * limits encoding changes
  * `MemType` missing index type setup
  * assuming default is 32 mode
  * changes related values to uint64_t
* validator
  * introduce helper `getItValueType` to do index type checking (index
    type is a new concept that introduced in the proposal, stands for
    indexing type for memory modes, memory64 use I64 and default is I32)
  * type check
    * store/load instructions
    * atomic instructions
    * v128 instructions
  * page count is possible up to 64-bit now
  * offset is 64bit now
* execution
  * share more code for 32/64-bit mode
  * update related memory instructions
    * instruction load/store and load/store many
    * instruction atomic
* driver: add option
* stack manager: move `popIndexType` that pop value based on index type
  (refers to above description)
* spec test
    * use `wasm-dev` branch to enable memory64 test suites
    * fix `wasmedgeExecutorCoreTests` by fixing the `grow` instruction
    * fix memory limit test (32-bit mode testing)

Later fixup

* fix bus error, overflow problem

  The root cause is combined from two trouble

  1. In executor check we only check the offset didn't overflow by using
     maximal of uint64_t to substract it
  2. In memory check we assume
     `const uint64_t AccessLen = Offset + Length`
     will not overflow!

  but the first check didn't ensure the second won't happen, thus, the
  error occurs.

* fix `f32.load`

  If `ValVariant` stores `uint32_t` then must use `uint32_t`, but I use
  `uint64_t` and cause the problem. `valToIndex` now will judge index type
  of memory64 mode, then use the correct type to expand `ValVariant`.

* fix windows build

Signed-off-by: Lîm Tsú-thuàn <dannypsnl@secondstate.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@q82419 q82419 Awaiting requested review from q82419 q82419 will be requested when the pull request is marked ready for review q82419 is a code owner

@ibmibmibm ibmibmibm Awaiting requested review from ibmibmibm ibmibmibm will be requested when the pull request is marked ready for review ibmibmibm is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
c-AOT c-CAPI An issue related to the WasmEdge C API c-CLI An issue related to WasmEdge CLI tools c-Test An issue/PR to enhance the test suite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: Implement wasm proposal: Memory64
2 participants
@dannypsnl @juntao