Initial Wasm memory allocation #400
Conversation
There was a problem hiding this comment.
From the looks of things, this set of test cases probably belongs in the api_tests suite that @elmato put together (and its coordinating test_api contract). Is there a compelling reason to have this memory API tested outside of that rig?
I realize it would be a non-trivial change from what is presented so, I have not bothered to comment in the contract code until we resolve this top-level issue.
| @@ -0,0 +1,2 @@ | |||
| file(GLOB SOURCE_FILES "*.cpp") | |||
| add_wast_target(memory_test "${SOURCE_FILES}" "${CMAKE_SOURCE_DIR}/contracts" ${CMAKE_CURRENT_SOURCE_DIR}) | |||
There was a problem hiding this comment.
This directive will build the memory test contract code intp contracts/ however you later attempt to include the .wast.hpp file relative to the slow tests source.
we have precedence for storing "test" contracts in the contracts tree that are used in slow tests. the memory_test contract should be placed there (along side api_test). Then you can include the .wast.hpp using brackets and it should work just fine.
tests/CMakeLists.txt
Outdated
| @@ -11,6 +11,7 @@ add_executable( chain_test ${UNIT_TESTS} ${COMMON_SOURCES} ) | |||
| target_link_libraries( chain_test eos_native_contract eos_chain chainbase eos_utilities eos_egenesis_none wallet_plugin fc ${PLATFORM_SPECIFIC_LIBS} ) | |||
|
|
|||
| if(WASM_TOOLCHAIN) | |||
| add_subdirectory(slow_tests/memory_test) | |||
There was a problem hiding this comment.
This should not be necessary once the memory test contract is moved to the contracts tree.
There was a problem hiding this comment.
See my previous response.
tests/slow_tests/slow_tests.cpp
Outdated
| @@ -47,6 +47,7 @@ | |||
| #include <currency/currency.wast.hpp> | |||
| #include <exchange/exchange.wast.hpp> | |||
| #include <infinite/infinite.wast.hpp> | |||
| #include "memory_test/memory_test.wast.hpp" | |||
There was a problem hiding this comment.
once memory_test is moved to the contracts/ tree, this should become #include <memory_test/memory_test.wast.hpp>
There was a problem hiding this comment.
See my previous response.
CMakeModules/wasm.cmake
Outdated
| @@ -121,7 +121,7 @@ macro(add_wast_target target SOURCE_FILES INCLUDE_FOLDERS DESTINATION_FOLDER) | |||
|
|
|||
| add_custom_command(OUTPUT ${DESTINATION_FOLDER}/${target}.wast | |||
| DEPENDS ${target}.s | |||
| COMMAND ${BINARYEN_BIN}/s2wasm -o ${DESTINATION_FOLDER}/${target}.wast -s 1024 ${target}.s | |||
| COMMAND ${BINARYEN_BIN}/s2wasm -o ${DESTINATION_FOLDER}/${target}.wast -s 8192 ${target}.s | |||
There was a problem hiding this comment.
We should consider making this something each contract can override. Setting 1 true stack size for all contracts seems overly limiting.
There was a problem hiding this comment.
Yeah I agree, but I'd like @bytemaster 's input, since I am not sure how we want to handle this. Any wasm *.cpp file that included memory.hpp would need to have it set, or else we need to set the memory space allocated based on the user stack. So I'll keep this as an open issue for now (while I fix everything else), unless you have more input on this.
brianjohnson5972
force-pushed
the
wasm_memory_129
branch
from
dad5d98
to
daa5b3b
Compare
brianjohnson5972
force-pushed
the
wasm_memory_129
branch
from
daa5b3b
to
54fdfea
Compare
|
@wanderingbort I'm fine with moving memory_test in with contracts, if you still want it there, just wanted to indicate why I did it initially and that I think eventually it would be good to have a place for usable or emulatable contracts and a place for test contracts that would cause confusion or would would be examples of what not to do (since they are test) . But with the short time I will defer to you and move it, just let me know. |
|
I don't mean to block this, I agree @bytemaster should weigh in. given that this falls into the category of "unit tests for contract API", I think it should be with the other unit tests. If those tests should be under the test/ tree rather than contracts, then I am fine with it. We can do that move in another issue. I'm going to change my review to an approval for now and start a new issue to unify the memory tests and existing API tests and determine the correct directory for them. |
fix issue EOSIO#400 extract signed_int from datastream
#129
Added malloc, realloc, and free to eoslib. Currently does not free memory, just allocates/reallocates what is available. Changed user stack to 8K.