fix validate_atomic_addr()

[haraldh]

Apparently addr is a pointer to real memory.

Signed-off-by: Harald Hoyer [email protected]

[abrown]

checked_add?

[alexcrichton]

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 of checked_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, the checked_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]

@alexcrichton well, maybe the absolute address makes sense, if you look at a naive futex implementation of notify and wait:
haraldh@216f694

[alexcrichton]

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.

[bjorn3]

Does this also need to account for access sizes larger than 1? For example a 8 byte access where the address is the last byte of the memory should not be allowed.

[12101111]

Misaligned access will trap. This check is done in JIT code:

wasmtime/cranelift/wasm/src/code_translator.rs

Lines 1070 to 1071 in 32a7593

	let (_flags, addr) =
	prepare_atomic_addr(memarg, implied_ty.bytes(), builder, state, environ)?;

[bjorn3]

That would necessitate all users of this function to trap on misalignment. Currently all callers of this function seem to immediately be followed by an unimplemented trap. Once they are implemented it is easy to miss this requirement as misaligned atomic accesses in rust are not guaranteed to panic.

[haraldh]

Corrected the check. Now checks start and end address of the atomic.

[abrown]

@haraldh, I think this has been superseded by #5239; should we close this PR?

[haraldh]

@abrown, yes, closing