Constants can contain references that are not Sync

[jDomantas]

This compiles (playground):

#![feature(negative_impls)]

#[derive(Debug)]
struct Foo {
    value: u32,
}

impl !std::marker::Sync for Foo {}

fn inspect() {
    let foo: &'static Foo = &Foo { value: 1 };
    println!(
        "I am in thread {:?}, address: {:p}",
        std::thread::current().id(),
        foo as *const Foo,
    );
}

fn main() {
    let handle = std::thread::spawn(inspect);
    inspect();
    handle.join().unwrap();
}

And prints this (addresses vary, but always the same):

I am in thread ThreadId(0), address: 0x5590409f7adc
I am in thread ThreadId(1), address: 0x5590409f7adc

Which shows that two threads get the same 'static reference to non-Sync struct.

The problem is that promotion to static does not check if the type is Sync, while doing it manually does (this does not compile):

static PROMOTED: Foo = Foo { value: 1 };
let foo = &PROMOTED;

Reading the RFC, it seems that the relevant condition is about containing UnsafeCell, but does not account for other !Sync types, which are also not supposed to be shared across treads.

[hanna-kruppe]

cc @eddyb

[eddyb]

If you can make the constant, and it's immutable, I'm not sure what you can do with it that's unsound - only unsafe code relying on it not being possible for one reason or another.

Rvalue promotion is like const with a reference in it, not like static. Actually, static itself could be relaxed too when interior mutability is not involved.

I'm tempted to close as "working as intended", unless there is more code to look at, which gets broken in one way or another by this capability.

[eddyb]

cc @nikomatsakis

[hanna-kruppe]

It seems in pretty clear violation of the Sync contract. While unsoundness seems unlikely in practice (you'd need a set up where the non-Sync values are used as "tokens" for something thread-local or thread-unsafe), at least in theory unsafe code should really be allowed to rely on non-Sync data being truly not shared across threads.

Edit: Hmmm come to think of it, this is complicated by the fact that Sync is an "at any given time thing", since Mutex and the like can make things Sync. Still seems bad to muddy the waters here.

cc @RalfJung

[nikomatsakis]

For better or worse (worse, I guess), this also builds:

#![feature(optin_builtin_traits)]

#[derive(Debug)]
struct Foo {
    value: u32,
}

impl !std::marker::Sync for Foo {}

const F: &'static Foo = &Foo { value: 1 };

fn main() {
}

[RalfJung]

Yeah I think this is a problem.

I imagine some (rather silly) pseudo-code like this, based on @rkruppe's idea of using Foo as a "token":

struct Event {
  address: usize,
  kind: EventKind
}

enum EventKind { Acquire, Release }

// A global event registry thing
static mut EVENT_LIST : Mutex<Vec<Event>> = Mutex::new(Vec::new());

fn add_event(e: Event) {
  unsafe { EVENT_LIST.lock().push(e) }
}

fn get_events() -> Vec<Event> {
  unsafe { EVENT_LIST.lock().clone() }
}

// Now comes the bad code
struct Foo(u32);

impl !Sync for Foo {}

impl Foo {
  fn acquire_release(&self) {
    add_event(Event { address: self as *const _ as usize, EventKind::Acquire });
    add_event(Event { address: self as *const _ as usize, EventKind::Release });
  }
}

const F: &'static Foo = &Foo(0);

fn main() {
  std::thread::spawn(|| F.acquire_release());
  F.acquire_release();
}

Playground version

Given that Foo is non-Sync, which guarantees that all pointers to the same &Foo live in the same thread, I think unsafe code should be allowed to rely (for UB) on the fact that the event list will never, ever contain something like (a, Acquire) (a, Acquire) (a, Release) (a, Release) for any address a. However, the code above could actually have exactly that outcome, with a being the value of F.

EDIT: See here for further elaboration.

[eddyb]

Given that Foo is non-Sync, which guarantees that all pointers to the same &Foo live in the same thread

Do we guarantee this anywhere? My expectation is that the fields of such an abstraction must be private, and that means you can only create the problematic global references in the same module.
And you might want those references, for a small set of special values that are valid cross-thread.

As another example, there are non-Send types in the compiler with special const values that happen to usable everywhere, regardless of thread-local state (e.g. Span's DUMMY_SP)

[RalfJung]

Do we guarantee this anywhere? My expectation is that the fields of such an abstraction must be private, and that means you can only create the problematic global references in the same module.

I think my Foo above is a counterexample. The only field does not matter at all, it can be public. It could also be entirely removed, just making it a unit struct.

As another example, there are non-Send types in the compiler with special const values that happen to usable everywhere, regardless of thread-local state (e.g. Span's DUMMY_SP)

That's a different thing. Even types that are not in general Send can have elements that are Send -- just like Vec has an element that is Copy.

[RalfJung]

So the conclusion from the unsafe code guidelines meeting yesterday (as far as I understood) is that we want to disallow this. Rvalue promotion should be thought of as implicitly generating additional statics, but with loser guarantees about address uniqueness. All restrictions of static apply, including the type having to be Sync. The open question is if we can do this breaking change, it is not clear how much code relies on this. There will be a crater run.

[arielb1]

With my current understanding of the semantics, there's nothing wrong with several different zero-sized values having the same address.

#![feature(optin_builtin_traits)]

struct NotSync;
impl !Sync for NotSync {}

fn main() {
    let mut u = NotSync;
    let mut v = NotSync;
    println!("{:?}", &mut u as *mut NotSync);
    println!("{:?}", &mut v as *mut NotSync);
}

[hanna-kruppe]

I think that's true, but this issue applies to non-ZSTs as well.

[arielb1]

@rkruppe

Sure. Missed that.