test: add test case for failing start function in wasm_engine_test #381
Conversation
|
Not sure this is a fault of our binding or upstream, but it definitely isn't the fault of the patch we have as I have tried it without the patch. |
axic
force-pushed
the
engine-test
branch
2 times, most recently
from
6107d0e
to
c29e3fa
Compare
Codecov Report
@@ Coverage Diff @@
## master #381 +/- ##
==========================================
+ Coverage 99.28% 99.35% +0.06%
==========================================
Files 49 49
Lines 13227 13242 +15
==========================================
+ Hits 13133 13156 +23
+ Misses 94 86 -8 |
test/utils/wasm3_engine.cpp
Outdated
| @@ -84,14 +84,13 @@ bool Wasm3Engine::instantiate(bytes_view wasm_binary) | |||
| // Transfers ownership to runtime. | |||
| if (m3_LoadModule(m_runtime, module) != m3Err_none) | |||
| { | |||
| m3_FreeModule(module); | |||
| // NOTE: apparently m3_FreeModule isn't needed in neither the failure nor the success case | |||
| return false; | |||
| } | |||
|
|
|||
| auto ret = m3_LinkRawFunction(module, "env", "adler32", "i(ii)", env_adler32); | |||
| if (ret != m3Err_none && ret != m3Err_functionLookupFailed) | |||
There was a problem hiding this comment.
| if (ret != m3Err_none && ret != m3Err_functionLookupFailed) | |
| return (ret == m3Err_none || ret == m3Err_functionLookupFailed); |
There was a problem hiding this comment.
I really dislike this setting of clang-tidy.
| @@ -84,14 +84,13 @@ bool Wasm3Engine::instantiate(bytes_view wasm_binary) | |||
| // Transfers ownership to runtime. | |||
| if (m3_LoadModule(m_runtime, module) != m3Err_none) | |||
| { | |||
| m3_FreeModule(module); | |||
| // NOTE: apparently m3_FreeModule isn't needed in neither the failure nor the success case | |||
There was a problem hiding this comment.
By going through the wasm3 code. Basically it seem once a module is parsed you have two choices: LoadModule or FreeModule, and if LoadModule was called then it is the liability of that to deal with it. Started opening in issue on their repo to clarify this, but then I guess forgot to submit.
There was a problem hiding this comment.
Asked the question: wasm3/wasm3#160
test/utils/wasm3_engine.cpp
Outdated
| @@ -84,14 +84,13 @@ bool Wasm3Engine::instantiate(bytes_view wasm_binary) | |||
| // Transfers ownership to runtime. | |||
| if (m3_LoadModule(m_runtime, module) != m3Err_none) | |||
| { | |||
| m3_FreeModule(module); | |||
| // NOTE: apparently m3_FreeModule isn't needed in neither the failure nor the success case | |||
| return false; | |||
| } | |||
|
|
|||
| auto ret = m3_LinkRawFunction(module, "env", "adler32", "i(ii)", env_adler32); | |||
| if (ret != m3Err_none && ret != m3Err_functionLookupFailed) | |||
| return false; | ||
| } | ||
|
|
||
| auto ret = m3_LinkRawFunction(module, "env", "adler32", "i(ii)", env_adler32); | ||
| if (ret != m3Err_none && ret != m3Err_functionLookupFailed) | ||
| { | ||
| m3_FreeRuntime(m_runtime); |
There was a problem hiding this comment.
This is a second bugfix, since we didn't set m_runtime to null, it may have attempted to free it twice (in the destructor).
No description provided.