Wasmtime GitHub notifications bot (Jan 07 2021 at 21:07): Wasmtime GitHub notifications bot (Jan 07 2021 at 21:07):yurydelendik opened PR #2556 from bug1684861 to main:
Related isseue https://bugzilla.mozilla.org/show_bug.cgi?id=1684861
There are two parts:
- cranelift/wasm/src/code_translator.rs the logic required for SM to work properly
- rest of it is testing of the broken logic
I don't think it is necessary to check alignment in the
fold_atomic_mem_addrthough it is the only way at this moment to verify/test fix in the wasmtime.Does makes sense for
wasmtime_memory_atomic_to operate with 64-bit address instead of 32-bit one? But that's different question we have to address.TODO:
- [ ] check for i32 adderss overflow?
Wasmtime GitHub notifications bot (Jan 07 2021 at 21:08):yurydelendik requested julian-seward1 for a review on PR #2556.
Wasmtime GitHub notifications bot (Jan 07 2021 at 21:09):yurydelendik edited PR #2556 from bug1684861 to main:
Related isseue https://bugzilla.mozilla.org/show_bug.cgi?id=1684861
There are two parts:
- cranelift/wasm/src/code_translator.rs the logic required for SM to work properly
- rest of it is testing of the broken logic
I don't think it is necessary to check alignment in the
fold_atomic_mem_addrthough it is the only way at this moment to verify/test fix in the wasmtime.Does it make sense for
wasmtime_memory_atomic_to operate with 64-bit address instead of 32-bit one? But that's different question we have to address.TODO:
- [ ] check for i32 address overflow?
Wasmtime GitHub notifications bot (Jan 07 2021 at 22:13):yurydelendik edited PR #2556 from bug1684861 to main:
Related isseue https://bugzilla.mozilla.org/show_bug.cgi?id=1684861
There are two parts:
- cranelift/wasm/src/code_translator.rs the logic required for SM to work properly
- rest of it is testing of the broken logic
I don't think it is necessary to check alignment in the
fold_atomic_mem_addrthough it is the only way at this moment to verify/test fix in the wasmtime.Does it make sense for
wasmtime_memory_atomic_to operate with 64-bit address instead of 32-bit one? But that's different question we have to address.TODO:
- [x] check for i32 address overflow?
Wasmtime GitHub notifications bot (Jan 07 2021 at 22:15):yurydelendik updated PR #2556 from bug1684861 to main:
Related isseue https://bugzilla.mozilla.org/show_bug.cgi?id=1684861
There are two parts:
- cranelift/wasm/src/code_translator.rs the logic required for SM to work properly
- rest of it is testing of the broken logic
I don't think it is necessary to check alignment in the
fold_atomic_mem_addrthough it is the only way at this moment to verify/test fix in the wasmtime.Does it make sense for
wasmtime_memory_atomic_to operate with 64-bit address instead of 32-bit one? But that's different question we have to address.TODO:
- [x] check for i32 address overflow?
Wasmtime GitHub notifications bot (Jan 07 2021 at 22:22):alexcrichton submitted PR Review.
Wasmtime GitHub notifications bot (Jan 07 2021 at 22:22):alexcrichton submitted PR Review.
Wasmtime GitHub notifications bot (Jan 07 2021 at 22:22):alexcrichton created PR Review Comment:
I figured that we already had a function somewhere in this file to calculate an address, could that perhaps be used instead of duplicating the logic here? That handles things with the guard page and whatnot too to deduplicate checks.
Wasmtime GitHub notifications bot (Jan 07 2021 at 22:22):alexcrichton created PR Review Comment:
Due to the relative cost of these functions, could the distinction between local/imported memories be removed to halve the number of intrinsics added?
Wasmtime GitHub notifications bot (Jan 07 2021 at 22:22):alexcrichton created PR Review Comment:
Similar to above I figured we'd already have alignment checks somewhere else for other atomic ops
Wasmtime GitHub notifications bot (Jan 07 2021 at 22:52):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Jan 07 2021 at 22:52):bjorn3 created PR Review Comment:
In Cranelift memory instructions directlt get an offset as argument, so there is probably no need for such a function.
Wasmtime GitHub notifications bot (Jan 07 2021 at 22:54):yurydelendik submitted PR Review.
Wasmtime GitHub notifications bot (Jan 07 2021 at 22:54):yurydelendik created PR Review Comment:
I could not find specifically one that combines offsets, I saw only those that calculate a pointer based on memory base.
Wasmtime GitHub notifications bot (Jan 07 2021 at 22:56):yurydelendik submitted PR Review.
Wasmtime GitHub notifications bot (Jan 07 2021 at 22:56):yurydelendik created PR Review Comment:
yep, I copied a part of the logic from
finalise_atomic_mem_addrthough it calculates a host memory pointer.
Wasmtime GitHub notifications bot (Jan 07 2021 at 23:00):yurydelendik submitted PR Review.
Wasmtime GitHub notifications bot (Jan 07 2021 at 23:00):yurydelendik created PR Review Comment:
I agree, though I adapted a pattern used by
imported_memory_fill/imemory_fillorimported_memory32_grow/memory32_grow. Are there plans to do the same with above?
Wasmtime GitHub notifications bot (Jan 08 2021 at 11:36):julian-seward1 submitted PR Review.
Wasmtime GitHub notifications bot (Jan 08 2021 at 11:36):julian-seward1 created PR Review Comment:
@bjorn3 But for atomic wait/notify, the actual handling code is generated by calling
environ.translate_atomic_waitetc. And that takes only the final linear memory offset as an argument. There's no way to pass it an additional constant offset as a second arg. Hence we have no option here but to "manually" fold inmemarg.offset(and adding a second bounds check, sigh.)
Wasmtime GitHub notifications bot (Jan 08 2021 at 11:40):julian-seward1 submitted PR Review.
Wasmtime GitHub notifications bot (Jan 08 2021 at 11:40):julian-seward1 created PR Review Comment:
@yurydelendik Do any of these tests test the case where the test created in
fold_atomic_mem_addrfails? I mean, where the checklinear_mem_addr + memarg.offset >= 0x_1_0000_0000u64? If not, can you add one? If yes, can you add a one-liner comment above each test, to say what case it tests?
Wasmtime GitHub notifications bot (Jan 08 2021 at 11:40):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Jan 08 2021 at 11:40):bjorn3 created PR Review Comment:
Indeed. I was reacting to
I figured that we already had a function somewhere in this file to calculate an address
Wasmtime GitHub notifications bot (Jan 08 2021 at 13:13):yurydelendik submitted PR Review.
Wasmtime GitHub notifications bot (Jan 08 2021 at 13:13):yurydelendik created PR Review Comment:
yes, line of 18 of the wast tests that
Wasmtime GitHub notifications bot (Jan 08 2021 at 15:36):alexcrichton submitted PR Review.
Wasmtime GitHub notifications bot (Jan 08 2021 at 15:36):alexcrichton created PR Review Comment:
Eventually I'd like to get to those and do the same yeah, we did it awhile ago for a few reference types intrinsics but haven't gotten around to fixing things generally.
Wasmtime GitHub notifications bot (Jan 08 2021 at 15:38):alexcrichton submitted PR Review.
Wasmtime GitHub notifications bot (Jan 08 2021 at 15:38):alexcrichton created PR Review Comment:
Could raw pointers get passed to the native functions which handle the implementation? The bounds-checking code is so tricky it seems like it'd be best to avoid duplicating it if we can.
Wasmtime GitHub notifications bot (Jan 08 2021 at 17:01):julian-seward1 submitted PR Review.
Wasmtime GitHub notifications bot (Jan 08 2021 at 17:48):yurydelendik submitted PR Review.
Wasmtime GitHub notifications bot (Jan 08 2021 at 17:48):yurydelendik created PR Review Comment:
Yep, raised the related question at https://bugzilla.mozilla.org/show_bug.cgi?id=1684861#c7 (consumer side)
Wasmtime GitHub notifications bot (Jan 08 2021 at 17:54):yurydelendik submitted PR Review.
Wasmtime GitHub notifications bot (Jan 08 2021 at 17:54):yurydelendik created PR Review Comment:
Opened https://github.com/bytecodealliance/wasmtime/issues/2561 to address it in the future.
Wasmtime GitHub notifications bot (Jan 08 2021 at 17:55):yurydelendik merged PR #2556.
Last updated: Dec 23 2024 at 12:05 UTC