Wasmtime GitHub notifications bot (Oct 17 2022 at 14:57): Wasmtime GitHub notifications bot (Oct 17 2022 at 14:57):alexcrichton commented on issue #5063:
These intrisnics were added quite some time ago and AFAIK haven't ever really been designed for "ok this is how they should actually work", instead they've just sort of "accidentally" been kept working as the bare-bones ability to run the
threadstest suite. In that sense instead of continuing to patch them it might be best to take a step back and see whether these intrinsics have the right interface.For example the intrinsics currently do
checked_addto "double check" that the address is in-bounds, but given how low level libcalls are there should be a strict and well-documented contract between the code generator and these intrinsics. I believe I added the comments and usage ofchecked_adddue to the fact that I didn't take the time to understand the code generator. If these intrinsics are to be implemented for real, however, thechecked_addshould either go away because the generated code handles this or it should remain and have its error handled since it's part of the implementation.That's an example of what's going on here but I suspect there may be other things. Overall if you're interested to see these intrinsics implemented fully I think it might be best to flesh that all out at once rather than incrementally, especially given the trickiness of implementing them correctly and importance of implementing them correctly.
For the technical contents of this PR itself, personally I find it odd that a real address would be passed along here simply to get re-converted into a relative offsets. I personaly think that either the intrinsics shouldn't need the relative offset or they should take the relative offset, rather than being in a weird "go between" as they are now.
Wasmtime GitHub notifications bot (Oct 17 2022 at 15:15):haraldh commented on issue #5063:
@alexcrichton well, maybe the absolute address makes sense, if you look at a naive futex implementation of notify and wait:
https://github.com/haraldh/wasmtime/commit/2341d1bb1ae2892992b977c4162d495233d1700b
Wasmtime GitHub notifications bot (Oct 17 2022 at 15:42):alexcrichton commented on issue #5063:
Yes in that implementation it does look like it makes sense. That's somewhat orthogonal to my point though of this libcall, in my opinion, should either take the relative offset and calculate the real address or it should take the real address. I dont think it makes sense for cranelift code to do the math for the real address only to have the libcall undo the math again.
Wasmtime GitHub notifications bot (Oct 18 2022 at 13:01):haraldh commented on issue #5063:
Corrected the check. Now checks start and end address of the atomic.
Wasmtime GitHub notifications bot (Oct 18 2022 at 13:10):haraldh edited a comment on issue #5063:
@alexcrichton well, maybe the absolute address makes sense, if you look at a naive futex implementation of notify and wait:
https://github.com/haraldh/wasmtime/commit/216f6943a8a50c5149a7d27e464ba223c6b57452
Wasmtime GitHub notifications bot (Nov 10 2022 at 00:23):abrown commented on issue #5063:
@haraldh, I think this has been superseded by https://github.com/bytecodealliance/wasmtime/pull/5239; should we close this PR?
Wasmtime GitHub notifications bot (Nov 11 2022 at 11:03):haraldh commented on issue #5063:
@abrown, yes, closing
Last updated: Oct 23 2024 at 20:03 UTC