Add support for multi-memory proposal in classic interpreter#3570
Add support for multi-memory proposal in classic interpreter#3570wenyongh merged 20 commits intobytecodealliance:dev/multi_memoryfrom
Conversation
do you mean this kind of incompatibilities? #1353 |
| addr = POP_MEM_OFFSET(); | ||
| CHECK_MEMORY_OVERFLOW(4); | ||
| PUSH_I32(LOAD_I32(maddr)); | ||
| CHECK_READ_WATCHPOINT(addr, offset); |
There was a problem hiding this comment.
maybe you need to adapt watchpoint.
i suspect you might need to adapt lldb and its protocol as well.
in the mean time i guess you can make it perform watchpoint processing only when memidx == 0.
There was a problem hiding this comment.
Indeed there are more work needs to be done to support multi-memory interp debugging. So currently in the config_common.cmake I enforce to disable the interp debugging feature when enabling multi-memory.
| return NULL; | ||
| #if WASM_ENABLE_MULTI_MEMORY != 0 | ||
| /* Current assumption is if there doesn't exist a non-import memory, | ||
| * the first non-import memory would be default memory */ |
There was a problem hiding this comment.
i failed to understand this comment.
There was a problem hiding this comment.
So I am assuming that if there are multiple wasm linear memories, there will be one "main(default)" memory. Some data members like heap_base, data_end, and host-managed heap are only available in default memory. Other memories don't have such data members, that's why I have an extra function that contains some duplicate code to handle different cases.
There was a problem hiding this comment.
"the default memory" makes me think the memory with index==0, regardless of import/non-import.
and i guess it's where we should put the app heap. (well, at least until we decide how to fix #1353)
that's why I have an extra function that contains some duplicate code to handle different cases.
i prefer to have bool try_to_insert_app_heap or something along the line over having two very similar functions like these.
There was a problem hiding this comment.
so how about using the term "main memory"
There was a problem hiding this comment.
why do you need such a concept?
the app heap should be in the default memory, shouldn't it?
There was a problem hiding this comment.
So it's enough to use the default memory to insert the app heap. What do you think?
it's more than "enough". it's actually how it should be, i guess.
the app heap in non-default memory is almost useless because it can't be addressed by wasm modules written in ordinal languages like C.
There was a problem hiding this comment.
@yamt so is there any other comment from you? I guess we can merge it to dev/multi_memory for more testing and merge to main sometime in the future?
There was a problem hiding this comment.
@yamt so is there any other comment from you?
no other comments.
(i have no time to read the latest version of this PR today.)
I guess we can merge it to dev/multi_memory for more testing and merge to main sometime in the future?
i guess it's fine.
Yes |
TianlongLiang
force-pushed
the
dev/multi_memory
branch
2 times, most recently
from
aad625a to
f0760c6
Compare
… TODO: 1. enlarge memory sig modification for fast interpreter and aot runtime 2. wasm runtime? 3. multi-memory spec test
… enable CIs for multi memory
TianlongLiang
force-pushed
the
dev/multi_memory
branch
from
f0760c6 to
9c35d86
Compare
3 participants
Implement multi-memory for classic-interpreter. Support core spec (and bulk memory) opcodes now, will support atomic opcodes, and add multi-memory export APIs in the future.
PS: Multi-memory spec test patched a lot for linking test to adapt for multi-module implementation