Skip to content

Should atomic loads and failing atomic RMWs be writes for the purpose of the aliasing model and data races? #355

Open
@RalfJung

Description

@RalfJung
Member

Specifically, should this code be UB?

use std::sync::atomic::{AtomicI32, Ordering};

fn main() {
    let x = &AtomicI32::new(0);
    let y = x as *const AtomicI32 as *const i32;
    let y = unsafe { &*y };
    x.compare_exchange(1, 2, Ordering::Relaxed, Ordering::Relaxed).unwrap_err();
    let _val = y;
}

Right now Miri accepts this, since the failed RMW is just considered a read by the aliasing model. But maybe it should be considered a write? We almost certainly want to disallow it on read-only memory since that pagefaults on x86, so making it a write also for other concerns seems more consistent.

OTOH, that could mean that a failing RMW that races with a non-atomic read might be considered a data race, and I am not sure if that is the right semantics.

Thanks to @chorman0773 for bringing up the question.

Activity

changed the title [-]Should atomic RMWs be write for the purpose of the aliasing model and data races?[/-] [+]Should failing atomic RMWs be writes for the purpose of the aliasing model and data races?[/+] on Aug 5, 2022
RalfJung

RalfJung commented on Aug 5, 2022

@RalfJung
MemberAuthor

OTOH, that could mean that a failing RMW that races with a non-atomic read might be considered a data race, and I am not sure if that is the right semantics.

Oh and for the weak memory model I think this would be plain wrong; failed RMWs do not participate in the mo order to my knowledge.

chorman0773

chorman0773 commented on Aug 5, 2022

@chorman0773
Contributor

We almost certainly want to disallow it on read-only memory since that pagefaults on x86, so making it a write also for other concerns seems more consistent.

This was my consideration as well - "Read-only memory" doesn't mean much of anything (that I'm aware of), but allocations that you can only get SRO to, so doing something that is invalid on SRO (or equivalent) is what I saw as necessary.

Oh and for the weak memory model I think this would be plain wrong; failed RMWs do not participate in the mo order to my knowledge.

At least llvm seems to think that lowering an identity rmw to a read is acceptable (as long as it preserves the ordering semantics). Other than the ordering, IDK if I can think of too many ways its observable without a preexisting data race.

RalfJung

RalfJung commented on Aug 5, 2022

@RalfJung
MemberAuthor

At least llvm seems to think that lowering an identity rmw to a read is acceptable

That's just a bug: llvm/llvm-project#56450

RalfJung

RalfJung commented on Aug 5, 2022

@RalfJung
MemberAuthor

"Read-only memory" doesn't mean much of anything (that I'm aware of)

At least in Miri each allocation has a flag indicating whether it is mutable or not. I don't know yet if we will need this for the spec as well.

chorman0773

chorman0773 commented on Aug 5, 2022

@chorman0773
RalfJung

RalfJung commented on Aug 5, 2022

@RalfJung
Author
chorman0773

chorman0773 commented on Aug 5, 2022

@chorman0773
chorman0773

chorman0773 commented on Aug 5, 2022

@chorman0773
RalfJung

RalfJung commented on Aug 5, 2022

@RalfJung
Author
RalfJung

RalfJung commented on Aug 5, 2022

@RalfJung
MemberAuthor

It was pointed out that even atomic loads can fail on read-only memory, because they might be implemented via compare_exchange loops. This indicates to me we should have a special case checking, for all atomic accesses, that they occur on writeable memory, but otherwise consider read-only atomic operations (loads and failing RMWs) to be read-only for the purpose of the aliasing model.

chorman0773

chorman0773 commented on Aug 5, 2022

@chorman0773
Contributor

Right yeah, because of AtomicU64 on i386. I don't think there's a way w/o relying on SSE existing to do just an atomic load of 8 bytes on 32-bit x86, but cmpxchg8b exists.

comex

comex commented on Aug 6, 2022

@comex

I’ve written C++ code before that performs atomic loads on read-only memory: it’s shared memory which a different process has read-write access to but the current process only has read access to. I know shared memory is a whole topic of its own, with the question of whether ‘atomic volatile’ is needed, but still, I’d argue that atomic loads should only require read access on targets where it’s truly necessary. Similarly to how targets vary in which widths of atomics they support, they can vary in whether they support truly read-only access to atomics.

I can also think of hypothetical use cases for atomic access to read-only memory that don’t involve communication over shared memory. Something like: some objects of a given type are mutable, while others are immutable and come from the program’s read-only data segment. There are a large number of immutable objects, so it’s valuable to keep them in a read-only segment to ensure the kernel can share the memory between multiple instances of the program, saving on RAM. When reading a field from an object of that type, you must use an atomic load in case it’s a mutable object which is being concurrently mutated, but it might also be an immutable object. (On OSes with copy-on-write plus overcommit, you could just put the immutable objects in a read-write segment and rely on the assumption that the pages will never actually be faulted for write - assuming you’re on a target where atomic loads aren’t writes. But other OSes are more pessimistic and either account for memory as if all the copy-on-write pages will be faulted for write (Windows) or don’t support copy-on-write at all (some embedded kernels).)

RalfJung

RalfJung commented on Aug 6, 2022

@RalfJung
MemberAuthor

I’ve written C++ code before that performs atomic loads on read-only memory: it’s shared memory which a different process has read-write access to but the current process only has read access to.

Ah, interesting. This would then require us to spell out explicitly for which targets we guarantee read-only atomic loads, I think? Having target-dependent UB is not great but I am not sure if it an be avoided here.

14 remaining items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @comex@RalfJung@thomcc@chorman0773@taiki-e

        Issue actions

          Should atomic loads and failing atomic RMWs be writes for the purpose of the aliasing model and data races? · Issue #355 · rust-lang/unsafe-code-guidelines