rust-lang / stdarch

Rust's standard library vendor-specific APIs and run-time feature detection
https://doc.rust-lang.org/stable/core/arch/
Apache License 2.0
599 stars 260 forks source link

Review and possibly deprecate all transactional memory intrinsics across architectures #1521

Closed RalfJung closed 5 months ago

RalfJung commented 5 months ago

Transactional memory intrinsics might affect the behavior of other instructions in ways that is incompatible with Rust being able to generate its own code properly: if a transactional memory "rollback" un-does some write to a stack variable Rust did, then things can start to go wrong in arbitrary ways.

We should probably deprecate all transactional memory intrinsics across architectures, similar to FP environment access. I don't know which architectures even have such intrinsics.

Amanieu commented 5 months ago

The rollback is a full state rollback to the point where the transaction started. This includes all memory accesses, register contents and the program counter. There is no observable difference compared to having the failed transaction never occurring in the first place.

RalfJung commented 5 months ago

What happens if memory accesses were observed by other threads/processes?

What happens if there was any kind of syscall?

bjorn3 commented 5 months ago

A syscall or context switch will immediately abort the transaction. The memory accesses can't be observed by other cpu cores afaik. I don't know if it will cause the reads from the other cpu cores to wait until the transaction has ended or if it will cause the transaction to abort though.

RalfJung commented 5 months ago

Hm, maybe there's no problem then? I opened this based on @jacobbramley's comment here, but maybe I misunderstood the scope of this.

bjorn3 commented 5 months ago

Something that makes intrinsics worse than inline asm is that unless you follow very specific rules about what the transaction does (eg at most N memory accesses, no instructions outside this specific set, at most M total instructions), it is almost certainly going to be aborted by the cpu.

jacobbramley commented 5 months ago

This includes all memory accesses, register contents and the program counter.

I had completely missed that detail, sorry.

unless you follow very specific rules about what the transaction does

Arm TME has some restrictions, but none look ominous. If Rust does something that causes a transaction to fail, then control flow is reset back to the start, and users of TME have to implement a fallback anyway (e.g. with a mutex). Not all failure reasons can be address by simply retrying.

This is all rather new to me, but the comments above suggest that this behaviour is likely to be similar to that in other architectures, too.

bjorn3 commented 5 months ago

Arm TME doesn't allow DSB barriers, which the compiler may emit. I am also very certain that the actual implementation inside arm cpu's puts a limit on the amount of buffered memory locations buffered by the transaction and exceeding this would result in an abort, likely constrained by the size and associativity of the L1 cache. On intel cpu's at least only 8 memory locations are guaranteed to be accessible on cpu's with 8 way associative L1 cache. I did expect up to 5 (store return value on stack, access return address and stack pointer to return from the block starting the transaction and push return address and stack pointer for the block ending the trabsaction) of these to be used up by the inline asm implementation in cg_clif, leaving only 3 for actual use, which in debug mode will more realistically be closer to 1 with the extra stack copies rustc inserts in the MIR it generates.

jacobbramley commented 5 months ago

I wouldn't expect the compiler to routinely emit a dsb in normal code, though even if it did:

These instructions cause a transaction failure if they are executed.

... which is not ideal, but not incorrect. You get a similar effect if you hit one of the implementation-defined limits on buffered memory, etc. We rely on the user implementing a suitable fallback, but they're supposed to do that anyway.

RalfJung commented 5 months ago

Okay so looks like there's not a problem here, at least not a safety / correctness issue. Closing the issue.