Take a look at this (I think?) supposed-to-be-thread-safe code according to the rules of D:

import std.stdio;
import core.thread;
import core.atomic;

int* foo() pure
{
    auto ret = new int;
    *ret = 3;  // MODIFY
    return ret;
}

shared immutable(int)* g;

void bar()
{
    immutable(int)* d = null;
    while (d is null)
    	d = g.atomicLoad;
    assert(*d == 3); // READ
    assert(*d == 3); // READ AGAIN
}

void main()
{
    auto t = new Thread(&bar).start();
    immutable(int)* a = foo();
    g.atomicStore(a);
    t.join;
}

What stops the CPU executing this such that MODIFY happens between READ and READ AGAIN ?

To aid in the thought experiment, imagine if we replace `*ret = 3` with `*ret = longComputation()`? It might help your reasoning about re-ordering if you consider `foo` inlined.

Is implicit conversion to the shared part of immutable actually safe, or does it secretly hide data races?

Another way of putting this:
Is `ret` secretly shared due to implicit conversion to immutable?

On 11/11/18 2:21 PM, John Colvin wrote:
> Take a look at this (I think?) supposed-to-be-thread-safe code according to the rules of D:
> 
> import std.stdio;
> import core.thread;
> import core.atomic;
> 
> int* foo() pure
> {
>      auto ret = new int;
>      *ret = 3;  // MODIFY
>      return ret;
> }
> 
> shared immutable(int)* g;
> 
> void bar()
> {
>      immutable(int)* d = null;
>      while (d is null)
>          d = g.atomicLoad;
>      assert(*d == 3); // READ
>      assert(*d == 3); // READ AGAIN
> }
> 
> void main()
> {
>      auto t = new Thread(&bar).start();
>      immutable(int)* a = foo();
>      g.atomicStore(a);
>      t.join;
> }
> 
> What stops the CPU executing this such that MODIFY happens between READ and READ AGAIN ?
> 
> To aid in the thought experiment, imagine if we replace `*ret = 3` with `*ret = longComputation()`? It might help your reasoning about re-ordering if you consider `foo` inlined.

Hm... I wouldn't expect the compiler would allow reordering across a return boundary. Even if the inlining occurs. Implicit conversion of pure return values depends on that.

I don't know all the rules of reordering, but this would definitely cause races if it was allowed to reorder the storage of data in *d, and the storage of d into g.

> 
> Is implicit conversion to the shared part of immutable actually safe, or does it secretly hide data races?
> 
> Another way of putting this:
> Is `ret` secretly shared due to implicit conversion to immutable?

If it is, we need to make it so it's not.

-Steve

On 11/11/18 3:47 PM, Steven Schveighoffer wrote:
> On 11/11/18 2:21 PM, John Colvin wrote:
>> Take a look at this (I think?) supposed-to-be-thread-safe code according to the rules of D:
>>
>> import std.stdio;
>> import core.thread;
>> import core.atomic;
>>
>> int* foo() pure
>> {
>>      auto ret = new int;
>>      *ret = 3;  // MODIFY
>>      return ret;
>> }
>>
>> shared immutable(int)* g;
>>
>> void bar()
>> {
>>      immutable(int)* d = null;
>>      while (d is null)
>>          d = g.atomicLoad;
>>      assert(*d == 3); // READ
>>      assert(*d == 3); // READ AGAIN
>> }
>>
>> void main()
>> {
>>      auto t = new Thread(&bar).start();
>>      immutable(int)* a = foo();
>>      g.atomicStore(a);
>>      t.join;
>> }
>>
>> What stops the CPU executing this such that MODIFY happens between READ and READ AGAIN ?

Wait, wouldn't the atomicStore create a barrier, such that all the code before it must execute?

-Steve

On Sunday, 11 November 2018 at 19:21:10 UTC, John Colvin wrote:
> Take a look at this (I think?) supposed-to-be-thread-safe code according to the rules of D:
>
> import std.stdio;
> import core.thread;
> import core.atomic;
>
> int* foo() pure
> {
>     auto ret = new int;
>     *ret = 3;  // MODIFY
>     return ret;
> }
>
> shared immutable(int)* g;
>
> void bar()
> {
>     immutable(int)* d = null;
>     while (d is null)
>     	d = g.atomicLoad;
>     assert(*d == 3); // READ
>     assert(*d == 3); // READ AGAIN
> }
>
> void main()
> {
>     auto t = new Thread(&bar).start();
>     immutable(int)* a = foo();
>     g.atomicStore(a);
>     t.join;
> }
>
> What stops the CPU executing this such that MODIFY happens between READ and READ AGAIN ?
>
> To aid in the thought experiment, imagine if we replace `*ret = 3` with `*ret = longComputation()`? It might help your reasoning about re-ordering if you consider `foo` inlined.
>
> Is implicit conversion to the shared part of immutable actually safe, or does it secretly hide data races?
>
> Another way of putting this:
> Is `ret` secretly shared due to implicit conversion to immutable?


Maybe I'm missing something but:

void bar()
{
    immutable(int)* d = null;
    while (d is null)
    	d = g.atomicLoad;
    assert(*d == 3); // READ
    assert(*d == 3); // READ - these two reads will always be the same
}

They will always be the same as what you are changing is the pointer. But here you atomicLoad() the pointer. So you will always have the same pointer. The value will only change if you do another atomicLoad or change the value that the pointer points to (which you don't do anywhere in your example).

int* foo() /* pure */
{
    static auto ret = new int; // Note: Static here
    *ret = 3;  // MODIFY
    return ret;
}

You might have a problem if "ret" here is static. But then I don't think the function would be pure anymore. Or rather at the very least it shouldn't be pure.

On Sun, 11 Nov 2018 15:48:31 -0500, Steven Schveighoffer wrote:
> Wait, wouldn't the atomicStore create a barrier, such that all the code before it must execute?

It creates a smaller barrier that's supposed to be good enough (`LOCK XCHG` instead of `MFENCE`). And it *is* good enough when dealing with value types. I'm not good with multithreading, so I'm not entirely certain whether this is an actual problem in druntime.

`LOCK XCHG` will serialize access for the global variable and any cache line containing it. It will also limit reordering. I haven't found anything remotely definitive on whether it provides a strong enough barrier for reordering and memory operations to suffice here, but I'd put a small bet in favor of it.

On Sunday, 11 November 2018 at 20:47:16 UTC, Steven Schveighoffer wrote:
> On 11/11/18 2:21 PM, John Colvin wrote:
>> Take a look at this (I think?) supposed-to-be-thread-safe code according to the rules of D:
>> 
>> import std.stdio;
>> import core.thread;
>> import core.atomic;
>> 
>> int* foo() pure
>> {
>>      auto ret = new int;
>>      *ret = 3;  // MODIFY
>>      return ret;
>> }
>> 
>> shared immutable(int)* g;
>> 
>> void bar()
>> {
>>      immutable(int)* d = null;
>>      while (d is null)
>>          d = g.atomicLoad;
>>      assert(*d == 3); // READ
>>      assert(*d == 3); // READ AGAIN
>> }
>> 
>> void main()
>> {
>>      auto t = new Thread(&bar).start();
>>      immutable(int)* a = foo();
>>      g.atomicStore(a);
>>      t.join;
>> }
>> 
>> What stops the CPU executing this such that MODIFY happens between READ and READ AGAIN ?
>> 
>> To aid in the thought experiment, imagine if we replace `*ret = 3` with `*ret = longComputation()`? It might help your reasoning about re-ordering if you consider `foo` inlined.
>
> Hm... I wouldn't expect the compiler would allow reordering across a return boundary. Even if the inlining occurs. Implicit conversion of pure return values depends on that.
>
> I don't know all the rules of reordering, but this would definitely cause races if it was allowed to reorder the storage of data in *d, and the storage of d into g.

The compiler can definitely re-order over return boundaries in general, but perhaps there is some special logic to prevent mistakes in these cases?

> Wait, wouldn't the atomicStore create a barrier, such that all the code before it must execute?
> 
> -Steve

Not a full memory barrier on most (all relevant?) platforms.

On Sunday, 11 November 2018 at 23:29:13 UTC, Rubn wrote:
> On Sunday, 11 November 2018 at 19:21:10 UTC, John Colvin wrote:
>> Take a look at this (I think?) supposed-to-be-thread-safe code according to the rules of D:
>>
>> import std.stdio;
>> import core.thread;
>> import core.atomic;
>>
>> int* foo() pure
>> {
>>     auto ret = new int;
>>     *ret = 3;  // MODIFY
>>     return ret;
>> }
>>
>> shared immutable(int)* g;
>>
>> void bar()
>> {
>>     immutable(int)* d = null;
>>     while (d is null)
>>     	d = g.atomicLoad;
>>     assert(*d == 3); // READ
>>     assert(*d == 3); // READ AGAIN
>> }
>>
>> void main()
>> {
>>     auto t = new Thread(&bar).start();
>>     immutable(int)* a = foo();
>>     g.atomicStore(a);
>>     t.join;
>> }
>>
>> What stops the CPU executing this such that MODIFY happens between READ and READ AGAIN ?
>>
>> To aid in the thought experiment, imagine if we replace `*ret = 3` with `*ret = longComputation()`? It might help your reasoning about re-ordering if you consider `foo` inlined.
>>
>> Is implicit conversion to the shared part of immutable actually safe, or does it secretly hide data races?
>>
>> Another way of putting this:
>> Is `ret` secretly shared due to implicit conversion to immutable?
>
>
> Maybe I'm missing something but:
>
> void bar()
> {
>     immutable(int)* d = null;
>     while (d is null)
>     	d = g.atomicLoad;
>     assert(*d == 3); // READ
>     assert(*d == 3); // READ - these two reads will always be the same
> }
>
> They will always be the same as what you are changing is the pointer. But here you atomicLoad() the pointer. So you will always have the same pointer. The value will only change if you do another atomicLoad or change the value that the pointer points to (which you don't do anywhere in your example).

The MODIFY line changes the value where the pointer looks. I'm asking whether it's possible that either READ happens before MODIFY (which would be v. bad). Using two reads instead of one was just to turn the (potential) ordering bug in to a (potential) breach of immutability.

> int* foo() /* pure */
> {
>     static auto ret = new int; // Note: Static here
>     *ret = 3;  // MODIFY
>     return ret;
> }
>
> You might have a problem if "ret" here is static. But then I don't think the function would be pure anymore. Or rather at the very least it shouldn't be pure.

Yes, that isn't pure, so the implicit conversion to immutable wouldn't work. It also doesn't compile at all due to using non-constant ctfe pointer as an initialiser.

On Monday, 12 November 2018 at 09:19:26 UTC, John Colvin wrote:
> The MODIFY line changes the value where the pointer looks. I'm asking whether it's possible that either READ happens before MODIFY (which would be v. bad). Using two reads instead of one was just to turn the (potential) ordering bug in to a (potential) breach of immutability.

For that you will need READ to happen before atomicLoad (in which case it will just dereference null pointer?) or MODIFY to happen after atomicStore?

On Monday, 12 November 2018 at 11:54:36 UTC, Kagamin wrote:
> On Monday, 12 November 2018 at 09:19:26 UTC, John Colvin wrote:
>> The MODIFY line changes the value where the pointer looks. I'm asking whether it's possible that either READ happens before MODIFY (which would be v. bad). Using two reads instead of one was just to turn the (potential) ordering bug in to a (potential) breach of immutability.
>
> For that you will need READ to happen before atomicLoad (in which case it will just dereference null pointer?) or MODIFY to happen after atomicStore?

The first one is definitely not allowed, I'm not worried about that. But is it actually forbidden for the MODIFY to happen after atomicStore?

On Monday, 12 November 2018 at 13:17:09 UTC, John Colvin wrote:
> The first one is definitely not allowed, I'm not worried about that. But is it actually forbidden for the MODIFY to happen after atomicStore?

Well, the topic is a little abstract, https://en.cppreference.com/w/c/atomic/memory_order - but this description looks pretty decent, way better than whatever I saw before.