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

Fix grow_memory issues #7

Merged
merged 10 commits into from
Nov 7, 2018
Merged

Fix grow_memory issues #7

merged 10 commits into from
Nov 7, 2018

Conversation

appcypher
Copy link
Contributor

@appcypher appcypher commented Nov 6, 2018 •
edited
Loading

What does this PR do?

  • Fix grow_memory function
  • Refactor compiled wasm functions and replace occurrences of vmctx with result_object.instance
  • Make heap dynamic and assign a heap bound
  • Create utility function to print instance struct field offsets

How should this be manually tested?

  • Run cargo test.

Background context

We had an issue where grow_memory function panics as we try to get a mutable memory from instance.

The issue boils down to having two mutable references to instance.
result_object owns the instance, while vmctx mutably borrows it.

The instance has a field called memories that is wrapped by an Arc, and if we try to get a mutable reference to it, Rust will complain.

After thinking of several ways to resolve this, I concluded/realized that result_object.instance can replace all occurences of vmctx.

In fact, from vmctx usage so far, only its instance field is needed
Instance struct can also contain the pointers to data needed by compiled Cranelift functions.

Along the way I also fixed other bugs I encountered.

Issues / areas that need improvement

  • memmap may already zero out new-allocated regions. Need to investigate that.
  • moving memories is inefficient. How about reserving space ahead of time with static heap?
  • remove VmCtx and generate_context function once all refs to them has been cleaned.

@appcypher appcypher changed the title [WIP] Fix/grow memory [WIP] Fix grow_memory issue Nov 6, 2018
@appcypher appcypher changed the title [WIP] Fix grow_memory issue Fix grow_memory issue Nov 7, 2018
@appcypher appcypher changed the title Fix grow_memory issue Fix grow_memory issues Nov 7, 2018
@syrusakbary
Copy link
Member

Great work!
As commented in Slack, I'm not super convinced on using the instance directly rather than vmctx because it might complicate a bit the pointer offsets and how to load them from the cranelift IR (once we go to multiple architectures), and I'm a bit suspicious on why no other impl (wasmtime, nebulet, SpiderMonkey) do this.

However, I think for that we can just keep vmctx code there for now and try to remove once we get more context and input that the instance approach is the final and chosen one.

@syrusakbary syrusakbary merged commit 993f88e into wasmerio:master Nov 7, 2018
syrusakbary added a commit that referenced this pull request Aug 1, 2019
@syrusakbary
Tryin gto make c_api_tests verbose mitigates the flaky error
676bccf 
Each time `make capi` is run, there is a flaky error:

```
Running target/release/deps/runtime_c_api_tests-3df0f74fcea1252d

running 1 test
test test_c_api ... FAILED

failures:

---- test_c_api stdout ----
Running command: `cmake` arg: Some(".")
output:
status: 0
stdout:
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/syrusakbary/Development/wasmer/lib/runtime-c-api/tests

stderr:

Running command: `make` arg: Some("-Wdev -Werror=dev")
output:
status: 0
stdout:
[  7%] Built target test-tables
[ 15%] Built target test-module-exports
[ 23%] Built target test-module-imports
[ 30%] Built target test-globals
[ 38%] Built target test-imports
[ 46%] Built target test-module
[ 53%] Built target test-module-serialize
[ 61%] Built target test-memory
[ 69%] Built target test-validate
[ 76%] Built target test-import-function
[ 84%] Built target test-instantiate
[ 92%] Built target test-exports
[100%] Built target test-exported-memory

stderr:

Running command: `make` arg: Some("test")
output:
status: 2
stdout:
Running tests...
Test project /Users/syrusakbary/Development/wasmer/lib/runtime-c-api/tests
      Start  1: test-exported-memory
 1/13 Test  #1: test-exported-memory .............Child aborted***Exception:   0.00 sec
      Start  2: test-exports
 2/13 Test  #2: test-exports .....................   Passed    0.01 sec
      Start  3: test-globals
 3/13 Test  #3: test-globals .....................   Passed    0.00 sec
      Start  4: test-import-function
 4/13 Test  #4: test-import-function .............   Passed    0.01 sec
      Start  5: test-imports
 5/13 Test  #5: test-imports .....................   Passed    0.01 sec
      Start  6: test-instantiate
 6/13 Test  #6: test-instantiate .................   Passed    0.01 sec
      Start  7: test-memory
 7/13 Test  #7: test-memory ......................   Passed    0.00 sec
      Start  8: test-module
 8/13 Test  #8: test-module ......................   Passed    0.01 sec
      Start  9: test-module-exports
 9/13 Test  #9: test-module-exports ..............   Passed    0.01 sec
      Start 10: test-module-imports
10/13 Test #10: test-module-imports ..............   Passed    0.01 sec
      Start 11: test-module-serialize
11/13 Test #11: test-module-serialize ............   Passed    0.01 sec
      Start 12: test-tables
12/13 Test #12: test-tables ......................   Passed    0.00 sec
      Start 13: test-validate
13/13 Test #13: test-validate ....................   Passed    0.00 sec

92% tests passed, 1 tests failed out of 13

Total Test time (real) =   0.08 sec

The following tests FAILED:
	  1 - test-exported-memory (Child aborted)

stderr:
Errors while running CTest
make[1]: *** [test] Error 8

thread 'test_c_api' panicked at 'Command failed with exit status: ExitStatus(ExitStatus(512))', lib/runtime-c-api/tests/runtime_c_api_tests.rs:43:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
```
MarkMcCaskey pushed a commit that referenced this pull request Apr 14, 2020
@rossberg
Implement C++ API and reimplement C on top (#7)
26eac1b 
* Add a C++ version of the API
* Refactor wasm-v8 to implement C++ API, which is safer
* Reimplement C API on top of C++ API
* Various fixes, additions, and clean-ups
* Some minor changes to C API:
  * Enforce `const` correctness in various places
  * Replace `init`, `deinit` global functions with `engine` object to be created
  * Store creation requires `engine` argument
  * Add `wasm_*_vec_new_empty` functions for creating *own* vectors of size 0
  * Changed `wasm_limits_t` from `size_t` to `uint32_t`
  * Changed `wasm_val_t` from `uint32_t`, `uint64_t` to `int32_t`, `int64_t`
  * Removed `wasm_numkind_t`, `wasm_refkind_t` and `wasm_numtype_t` `wasm_reftype_t` type aliases
  * Renamed `wasm_valkind_is_numkind`, `wasm_valkind_is_refkind` to `wasm_valkind_is_num`, `wasm_valkind_is_ref` and `wasm_valtype_is_numtype`, `wasm_valtype_is_reftype` to `wasm_valtype_is_num`, `wasm_valtype_is_ref`
  * Removed `wasm_ref_null` and `wasm_ref_is_null` (use NULL pointer)
  * Extended `wasm_func_new_with_env` function to take finalizer for env (which may be NULL)
  * Turned `wasm_extern_t` into an abstract type `wasm_external_t` and added conversion functions
  * Removed `wasm_instance_export` function
* Add comprehensive TODO list to README
nlewycky pushed a commit that referenced this pull request Aug 13, 2020
@syrusakbary
Merge pull request #7 from wasmerio/frame-info
b5d9ac1 
Frame info
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@appcypher @syrusakbary