wasm2c: parametrize memory bounds checks on a per-memory basis #2507
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
This was referenced Nov 11, 2024
keithw
force-pushed
the
w2c-per-memory-strategy
branch
3 times, most recently
from
167eae8 to
985d89d
Compare
keithw
force-pushed
the
w2c-harmonize-types
branch
from
2ece28b to
176ec50
Compare
keithw
force-pushed
the
w2c-per-memory-strategy
branch
from
985d89d to
5604e96
Compare
keithw
force-pushed
the
w2c-harmonize-types
branch
from
176ec50 to
7bea8ee
Compare
keithw
force-pushed
the
w2c-per-memory-strategy
branch
from
5604e96 to
4e60e9c
Compare
keithw
force-pushed
the
w2c-harmonize-types
branch
2 times, most recently
from
d15bfb9 to
beb8809
Compare
shravanrn
approved these changes
Nov 11, 2024
|
|
||
| #define DEFINE_STORE(name, t1, t2) \ | ||
| static inline void name(wasm_rt_memory_t* mem, u64 addr, t2 value) { \ | ||
| MEMCHECK(mem, addr, t1); \ | ||
| static inline void name##_unchecked(wasm_rt_memory_t* mem, u64 addr, \ |
There was a problem hiding this comment.
Not a strong opinion, but would it be simpler to have something like?
#define DEFINE_STORE(name, t1, t2, MEMCHK, suffix) \
static inline void name##_##suffix(wasm_rt_memory_t* mem, u64 addr, t2 value) { \
MEMCHK(mem, addr, t1); \
t1 wrapped = (t1)value; \
wasm_rt_memcpy(MEM_ADDR_MEMOP(mem, addr, sizeof(t1)), &wrapped, sizeof(t1)); \
}
DEFINE_STORE(i32_store, u32, u32, MEMCHECK_DEFAULT32, _default32);
DEFINE_STORE(i32_store, u32, u32, MEMCHECK_GENERAL, _general);
There was a problem hiding this comment.
I guess I was trying to avoid the churn of duplicating every occurrence of DEFINE_LOAD/DEFINE_STORE/DEFINE_SIMD_LOAD_FUNC/DEFINE_SIMD_LOAD_LANE/DEFINE_SHARED_LOAD/etc. ... I think on balance the status quo is probably the less-bad but I don't feel that strongly either.
| memory->data = addr; | ||
| #else | ||
| memory->data = calloc(byte_length, 1); | ||
| if (WASM_RT_USE_MMAP && !is64) { |
There was a problem hiding this comment.
Nit: Might be cleaner to have
// Check if we should reallocate using mmap
#if WASM_RT_USE_MMAP
if(!is64) {
... // original code
return;
}
#endif
// Fallback to malloc
memory->data = calloc(byte_length, 1);
There was a problem hiding this comment.
Yeah... I agree this one is clearer but it seemed nice to have consistency with grow_memory_impl where the early-return is harder to do. :-/
keithw
force-pushed
the
w2c-harmonize-types
branch
2 times, most recently
from
487cb8a to
326e3cb
Compare
keithw
force-pushed
the
w2c-per-memory-strategy
branch
from
4e60e9c to
8abbc62
Compare
|
@sbc100 Did you want to review this one? @shravanrn would prefer to land #2506 and #2507 together, so I was planning to wait until everybody is comfortable with this one before merging either. |
keithw
added a commit
that referenced
this pull request
Dec 4, 2024
(Sequenced behind #2506) This PR allows "software-bounds-checked" memories and "guard-page-checked" memories to coexist in the same module. It creates two versions of every memory operation: an unrestricted version (that works with any memory) and a `_default32` version (for memories with default page size and i32 indexing). The unrestricted version calls `MEMCHECK_GENERAL`, which does a 64-bit software `RANGE_CHECK` to check that the operation reads/writes within the bounds of the memory. The `_default32` version calls `MEMCHECK_DEFAULT32`, which is the same as the old `MEMCHECK`: if the runtime declares `WASM_RT_MEMCHECK_GUARD_PAGES`, it will do nothing. Otherwise it will do a 32-bit software `RANGE_CHECK` (which seems to be one less instruction than the 64-bit `RANGE_CHECK`). This is a stepping stone to supporting custom-page-sizes (which will need to be software bounds-checked) (#2508).
keithw
added a commit
that referenced
this pull request
Dec 6, 2024
(Sequenced behind #2506) This PR allows "software-bounds-checked" memories and "guard-page-checked" memories to coexist in the same module. It creates two versions of every memory operation: an unrestricted version (that works with any memory) and a `_default32` version (for memories with default page size and i32 indexing). The unrestricted version calls `MEMCHECK_GENERAL`, which does a 64-bit software `RANGE_CHECK` to check that the operation reads/writes within the bounds of the memory. The `_default32` version calls `MEMCHECK_DEFAULT32`, which is the same as the old `MEMCHECK`: if the runtime declares `WASM_RT_MEMCHECK_GUARD_PAGES`, it will do nothing. Otherwise it will do a 32-bit software `RANGE_CHECK` (which seems to be one less instruction than the 64-bit `RANGE_CHECK`). This is a stepping stone to supporting custom-page-sizes (which will need to be software bounds-checked) (#2508).
keithw
added a commit
that referenced
this pull request
Dec 18, 2024
…hecks per-memory (#2507) The PR updates the bulk memory operations (memory.fill, memory.copy, table.fill, etc.) to support 64-bit addresses and counts. Previously these functions only took u32's, even with memory64 enabled. (#2506) This PR also allows "software-bounds-checked" memories and "guard-page-checked" memories to coexist in the same module. It creates two versions of every memory operation: an unrestricted version (that works with any memory) and a _default32 version (for memories with default page size and i32 indexing). (#2507) #2506 and #2507 have been squashed together to avoid a performance regression. This is a stepping stone to supporting custom-page-sizes (which will need to be software-bounds-checked) (#2508).
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.
(Sequenced behind #2506)
This PR allows "software-bounds-checked" memories and "guard-page-checked" memories to coexist in the same module.
It creates two versions of every memory operation: an unrestricted version (that works with any memory) and a
_default32version (for memories with default page size and i32 indexing).The unrestricted version calls
MEMCHECK_GENERAL, which does a 64-bit softwareRANGE_CHECKto check that the operation reads/writes within the bounds of the memory.The
_default32version callsMEMCHECK_DEFAULT32, which is the same as the oldMEMCHECK: if the runtime declaresWASM_RT_MEMCHECK_GUARD_PAGES, it will do nothing. Otherwise it will do a 32-bit softwareRANGE_CHECK(which seems to be one less instruction than the 64-bitRANGE_CHECK).This is a stepping stone to supporting custom-page-sizes (which will need to be software bounds-checked) (#2508).