Fix table and memory import and improve memory storage #11
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
syrusakbary
reviewed
Nov 16, 2018
| - `call_indirect.wast` | ||
|
|
||
| - `SKIP_SHARED_TABLE` [elem.wast] | ||
| Currently sharing tables between instances/modules does not work. Below are some of the reasons it is hard to achieve. |
There was a problem hiding this comment.
I love seeing this analysis as part of the PR and spectests README.
It will make much easier to dig into a solution in the future and get input/solutions from other people.
Great 👍
syrusakbary
reviewed
Nov 16, 2018
| @@ -9,15 +9,19 @@ pub struct UncheckedSlice<T> { | |||
|
|
|||
| impl<T> UncheckedSlice<T> { | |||
| #[inline] | |||
| unsafe fn get_unchecked(&self, index: usize) -> &T { | |||
| pub fn get_unchecked(&self, index: usize) -> &T { | |||
There was a problem hiding this comment.
Is there any reason on make the function "safe" rather than moving the safe responsibility on the caller?
There was a problem hiding this comment.
If I don't do that, the unsafe annotation propagates. That is, the function that calls it needs to be annotated as unsafe and the function that calls that and ....
syrusakbary
reviewed
Nov 16, 2018
| pub struct BoundedSlice<T> { | ||
| data: UncheckedSlice<T>, | ||
| len: usize, | ||
| pub data: UncheckedSlice<T>, |
There was a problem hiding this comment.
Does it really need to be public other than for debugging proposes?
There was a problem hiding this comment.
If so, it might be better to make it private and comment the public access on the print_instance_offsets.
Thoughts?
There was a problem hiding this comment.
It's just for debugging purpose only.
There was a problem hiding this comment.
Maybe we don't really need print_instance_offsets function.
syrusakbary
reviewed
Nov 16, 2018
| } | ||
|
|
||
| impl Instance { | ||
| pub const TABLES_OFFSET: usize = 0; // 0 on 64-bit | 0 on 32-bit | ||
| pub const MEMORIES_OFFSET: usize = size_of::<TablesSlice>(); // 8 on 64-bit | 4 on 32-bit | ||
| pub const GLOBALS_OFFSET: usize = Instance::MEMORIES_OFFSET + size_of::<MemoriesSlice>(); // 16 on 64-bit | 8 on 32-bit |
There was a problem hiding this comment.
Interesting, I was using a dynamic calculation in module.rs based on the pointer size in the latests commits of wasmer.
But I think I like yours better 💯 (and actually it's similar on how SpiderMonkey handles it).
syrusakbary
added a commit
that referenced
this pull request
Aug 1, 2019
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
nlewycky
pushed a commit
that referenced
this pull request
Aug 13, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
How should this be manually tested?
cargo test.Background context
N/A
Areas needing improvement
All of these cause certain tests to fail.