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
threads
test 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_add
to "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_add
due 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_add
should 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.
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
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.
haraldh commented on issue #5063:
Corrected the check. Now checks start and end address of the atomic.
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
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?
haraldh commented on issue #5063:
@abrown, yes, closing
Last updated: Dec 23 2024 at 13:07 UTC