Tracking issue for #[cfg(target_has_atomic = ...)]

[alexcrichton]

Tracking rust-lang/rfcs#1543

[sfackler]

This is still blocked on some reasonable way to talk about platform-specificity in a more granular way than what we currently support.

[Manishearth]

Is there any reason why AtomicU32 would be hard to stabilize? We need a way to store multi-word bitflags in an atomic thingy. AtomicUsize won't work here since it's too large on 64 bit.

[sfackler]

What do you mean by "too large"?

[Manishearth]

Let's say I have a 64-bit bucket of flags. I need two atomic u32s to represent it. I can't use two usizes because that's too large on 64 bit.

Alternatively, let's say I have a 32-bit bucket of flags. This is on a struct where size matters. I need a u32, not something larger on 64-bit.

I guess I could cfg() it and abstract over it. That solves the first problem but not the second.

[nagisa]

@Manishearth 16-bit-usize targets would give you 16 bit atomic width. 100% savings over 32bit systems! (and potential loss of data)

There isn’t really much problem stabilising any of these IMO, but you’ll still have to CFG on target_has_atomic="32" in your code.

[Manishearth]

16-bit-usize targets would give you 16 bit atomic width.

ugh, these exist, right.

What do I have to do to get these stabilized? I'm okay with the cfgs.

I'm also okay with just u8 being stabilized, as long as I can pack it (not sure if this is possible with atomics)

[Manishearth]

What do I have to do to get these stabilized?

Any answer on this? cc @alexcrichton

The rfc looks like it's been completely implemented. IMO stabilizing it with target_has_atomic is fine, and we can add further granularity if we need in new rfcs.

[alexcrichton]

@Manishearth state hasn't changed from before. The libs team is essentially waiting for a conclusion on the scenarios story before stabilizing.

[Manishearth]

That discussion has stalled. Last I checked it seemed to mostly have come to consensus on the general idea? Who are we waiting on for a conclusion here? What would the ETA on this be? How can I help?

It also seems like that's mostly orthogonal to this. We can stabilize this using cfgs, and add scenarios later. They seem to be pretty compatible.

[Manishearth]

(After discussing in IRC, it seems like there have been a lot of recent discussions about this, and the libs team is currently moving on pushing this forward.)

[archshift]

Has there been any progress on this since?

[bholley]

Just FYI: Per [1], Gecko's Quantum CSS project is no longer blocked on this issue.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1336646

[nagisa]

We can stabilise AtomicU8 and AtomicI8 the same way we have stabilised AtomicBool – by using Atomic{I/U}Size operations everywhere.

[alexcrichton]

@RalfJung correct, that's what convinced me personally that we can't do this on crates.io, which means if we want it at all we need it in the standard library (via LLVM intrinsics). I think we want it, so I'm convinced to put it into libstd :)

[Amanieu]

@RalfJung This lowering is done either within LLVM, or through a function in compiler_builtins.

The latter is currently only used on armv5te-unknown-linux-gnu at the moment, and uses this code. It could be argued that this is UB since intrinsics::atomic_load_unordered could be used to read out-of-bounds data, however this is guaranteed not to fault because it doesn't cross a page boundary.

[RalfJung]

@alexcrichton makes sense!

@Amanieu

It could be argued that this is UB

And the argument would be correct :)

is guaranteed not to fault because it doesn't cross a page boundary.

And as in the last N cases we have had this argument (and as I am sure you are aware, but not everybody else might be), that doesn't change anything about this being UB when we are talking about code written in Rust, MIR or LLVM IR. ;) (I am beginning to feel sorry for being so annoying about this, but LLVM is way too smart and getting smarter every day, so I am actively worried that such arguments will blow in our face some day.)

Is this a pattern supported/intended by LLVM? Is there advise from the LLVM devs for how to do this?

Is there any chance of LLVM ever inlining those compiler-builtins functions? Actually even having them in the same translation unit could be enough to cause problems, because LLVM could infer attributes on the functions to propagate information about what they do out to use sites.

One safer alternative would be to use inline assembly to implement such operations, that would most likely exclude any way for LLVM to notice that there are out-of-bounds accesses. But I am not sure if that's an option here.

[Amanieu]

The code is more-or-less based on the GCC implementation, which gets away with a normal atomic load.

Would changing the load to a volatile atomic load help in this case?

[alexcrichton]

To add to what @RalfJung is saying, @Amanieu just because it works at the hardware layer doesn't mean it's UB in LLVM's IR. For example this function:

define i8 @bar() {
start:                                  
  %a = alloca i8                        
  store i8 0, i8* %a                    
  %b = call i8 @foo(i8* %a)             
  ret i8 %b                             
}                                       
                                        
define internal i8 @foo(i8*) {          
start:                                  
  %b = getelementptr i8, i8* %0, i32 1  
  %a = load i8, i8* %b                  
  ret i8 %a                             
}                                       

is sort of a simplisitic view but it's guaranteed to never fault because the out-of-bounds load will just load some byte of the return address on the call stacsk or something weird like that. When optimized, however, it yields:

define i8 @bar() local_unnamed_addr #0 {
start:
  ret i8 undef
}

(a showing that this is undefined behavior)

LLVM can't automatically deduce that all instances of this pattern is undefined behavior, in isolation foo optimized just fine. That's why compiler-builtins happens to work, we're forcing LLVM to have less knowledge about the inputs so it just-so-happens that it can't deduce that undefined behavior is happening.

All that's just to say that @RalfJung I think is totally correct here, a crates.io based implementation of smaller-sized atomics with larger-sized atomics I think is just a segfault waiting to happen. LLVM may not even detect it's UB today, but it's definitely UB at the LLVM IR layer (and probably the Rust layer) to read out of bounds on objects. Why exactly it's UB or what exactly happens is always up for grabs which is why it works most of the time, but this is fundamentally why we need LLVM's backend to do the lowering because the IR passes need to see that we're just modifying/loading one byte, not the bytes around it

[gnzlbg]

The operations that @Amanieu wants to perform cannot be performed by a programming language generating LLVM-IR directly. Inline assembly appears to be the only way to perform these right now, so we could still expose them I think (@RalfJung ? I don't know whether compiler-builtins would work too).

In the meantime, I think it would be better to open an issue in the LLVM bugzilla about this, explaining why these operations are useful, why the LLVM-IR generated for them has undefined behavior, and how that requires us to use inline assembly (or modify compiler-builtins) instead. We should ask: what should we do? Should we use inline assembly / our own compiler built-ins ? Will LLVM expose intrinsics to allow these safely? etc.

It might be worth mentioning that this is not the only situation in which we need to perform reads out-of-bounds (see rust-lang/unsafe-code-guidelines#2).

[RalfJung]

Would changing the load to a volatile atomic load help in this case?

No. Volatile reads in practice have some positive effects on racy reads (but LLVM may change those rules any time as we are relying on de-facto behavior here). It doesn't change anything about the requirement that accesses must be in-bounds.

The proper way to fox this is (as @gnzlbg mentioned) to add an attribute to LLVM that can be set on reads/writes and that indicates that the access may be partially out-of-bounds. Then we need a matching intrinsic in Rust, and methods such as read_out_of_bounds and write_out_of_bounds on pointers. Considering we need this for concurrency, we'd also need to think about how to expose atomic out-of-bounds accesses in Rust. Anything else (anything just arguing based on page boundaries but not informing LLVM) will remain a hack. Given that this seems to be a useful pattern, I absolutely think we should lobby for LLVM to add such an attribute!

just because it works at the hardware layer doesn't mean it's UB in LLVM's IR. For example this function

Thanks for the example, I'll link to this when such discussions come up again in the future. :)

That's why compiler-builtins happens to work, we're forcing LLVM to have less knowledge about the inputs so it just-so-happens that it can't deduce that undefined behavior is happening.

That sounds way less confident than I had hoped...

When does compiler-builtins get linked with the real program? Is there a chance that LTO might inline compiler-builtins functions (which then would mean LLVM could deduce the UB)?

[nikic]

When does compiler-builtins get linked with the real program? Is there a chance that LTO might inline compiler-builtins functions (which then would mean LLVM could deduce the UB)?

As rtlib calls are only inserted at the SelectionDAG layer, while LTO still operates on LLVM IR, I don't believe there is any possibility of these getting inlined.

[alexcrichton]

@nikic is correct, we explicitly don't LTO compiler-builtins as well (it's a very special crate). In that sense there's no worry for inlining compiler-builtins intrinsics.

[RalfJung]

Okay. I can live with that. We should keep it in mind though for the future, if/when compiler-builtins treatment ever changes.

So, yeah, I agree we should go forward with such "emulated" small-int atomics implemented via LLVM lowering or compiler-builtints.

[alexcrichton]

For those following this thread, the stabilization proposal is now in FCP

[macpp]

Can i ask a question (just curious) ? It seems that constants like ATOMIC_I64_INIT are marked as stable since 1.34 and deprecated since 1.34 at the same time. Why stabilize something that is deprecated? It may be just my opinion, but i think that getting new stable feature that is deprecated from the beginning is rather strange...

[Amanieu]

Nice catch! I think we should just remove those constants.

[scottmcm]

That's convincing to me, @macpp -- opened #58089 to track it.

[jonas-schievink]

This is listed as the tracking issue for cfg_target_has_atomic, which is still unstable. Should this be reopened?

[sfackler]

Yep - reopened.