http://d.puremagic.com/issues/show_bug.cgi?id=10108



--- Comment #10 from Igor Stepanov <wazar.leollone@yahoo.com> 2013-05-17 16:25:34 PDT ---
(In reply to comment #9)
> I see. Once again, simplified:
> 
> import core.thread;
> 
> struct Foo {
>     int[] arr;
> }
> 
> Foo[] arr = [Foo([1,2,3])]; // Should have failed? (1)
> 
> void main( ) {
>     int* p = arr[0].arr.ptr;
>     auto thr = new Thread({assert(arr[0].arr.ptr == p);}); // Should have
> failed. (2)
>     thr.start();
>     thr.join();
> }
> 
> In this case, for the assert to fail, we'd have to deep-dup the array (COW might make that unnecessary, but that's beside the point).
> 
> This is in a way related to the issue of array literals being mutable, in that it is an example of the compiler erroneously assuming some state may be shared when in fact it shouldn't.
> 
> I contend that (1) above should simply not compile. It should be required to be placed in a module constructor instead.
Yep
int[] x = [1,2,3]; should not be compiled, but
shared int[] x = [1,2,3]; //OK
const int[] x = [1,2,3]; //OK, because const is global scope == immutable
immutable int[] x = [1,2,3]; //OK
__gshared int[] x = [1,2,3]; //Same



>A case can be made that the compiler
> should automagically place it in a module constructor for you, but I am not of that opinion.
I agree, this is not D way, I think.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=10108



--- Comment #11 from Martin Nowak <code@dawg.eu> 2013-05-17 17:10:43 PDT ---
> Asserts are generally included to show what currently PASSES,
not what FAILS.

OK, I always write unittests that should pass but I'll be more explicit.

> In other words implicit "thread local" modifier is not transitive.

It's not intended to be transitive, it is a storage class, not a type
qualifier.
Variables with thread local storage may reference any other data (__gshared,
shared, stack, heap) and vice versa.

> int[] x = [1,2,3]; // should not be compiled

It would be trivial to fix. As the initializer for static data must be a
compile time constant we'd just need to store this constant in TLS instead of
the data segment.
The problem is that ELF has no TLS relocations for data, i.e. we'd need a
dynamic initalizer that sets arr.ptr to the TLS data.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=10108



--- Comment #12 from Martin Nowak <code@dawg.eu> 2013-05-17 17:13:57 PDT ---
The simple fix is to only allow value types to have TLS initalizers and require static this() for everything else.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=10108



--- Comment #13 from Igor Stepanov <wazar.leollone@yahoo.com> 2013-05-18 00:46:39 PDT ---
> 
> > int[] x = [1,2,3]; // should not be compiled
> 
> It would be trivial to fix. As the initializer for static data must be a
> compile time constant we'd just need to store this constant in TLS instead of
> the data segment.
> The problem is that ELF has no TLS relocations for data, i.e. we'd need a
> dynamic initalizer that sets arr.ptr to the TLS data.
I dont know anything about relocation magic. But, as I understand you, we cannot to use it;

>The simple fix is to only allow value types to have TLS initalizers and require static this() for everything else.
Yes. This is all we need I think.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

This is expected because the global is __gshared and there's therefore no type protection from doing this. If you want safe sharing, make the global shared.

On May 17, 2013, at 9:38 AM, d-bugmail@puremagic.com wrote:

> http://d.puremagic.com/issues/show_bug.cgi?id=10108
> 
> 
> 
> --- Comment #2 from Martin Nowak <code@dawg.eu> 2013-05-17 09:38:22 PDT ---
> When a thread local variable is a reference type to modifiable data, we must make sure that it is initialized uniquely.
> 
> This is what the current implementation does which results in hidden sharing.
> 
> __gshared int[] gArr = [1,2,3];
> int[] arr = gArr;
> 
> -- 
> Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------

On Sat, 18 May 2013 16:59:53 +0200, Sean Kelly <sean@invisibleduck.org> wrote:

> This is expected because the global is __gshared and there's therefore no type protection from doing this. If you want safe sharing, make the global shared.
[snip]
>> __gshared int[] gArr = [1,2,3];
>> int[] arr = gArr;

Uhm, you are aware this is the Bugzilla newsgroup, right? Please reply in
Bugzilla.

Also, that's exactly what we're saying. This code has problems:

import core.thread;

int[] arr = [1,2,3];

void main() {
    new Thread({arr[0] = 3;});
    assert(arr[0] == 1);
}

And they are caused by the data inside arr being shared. This is unsafe,
unexpected (due to safety of other globals) and, we feel, goes against
the D ethos.

-- 
Simen

http://d.puremagic.com/issues/show_bug.cgi?id=10108


Sean Kelly <sean@invisibleduck.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sean@invisibleduck.org


--- Comment #14 from Sean Kelly <sean@invisibleduck.org> 2013-05-21 11:47:20 PDT ---
So I thought I understood this:

    import core.thread;
    int[] arr = [1,2,3].dup;

    void main() {
        auto t = new Thread({arr[0] = 3;});
        t.start();
        t.join();
        assert(arr[0] == 1);
    }

It looks like we have a thread-local reference "arr" to a __gshared array of int, so I would expect the assert to fail.  Except:

    import core.thread;
    int[] arr = [1,2,3].dup;

    void main() {
        auto t = new Thread({arr[0] = 3;});
        t.start();
        t.join();
        assert(arr[0] == 1);
    }

The .dup should fix the problem, as now each thread gets its own copy of the array, right?  But the assert still fails.  I suppose I should check the ASM, but the codegen seems kind of broken here.  Is a __gshared label being inferred for arr because it's statically slicing __gshared data?

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=10108



--- Comment #15 from Simen Kjaeraas <simen.kjaras@gmail.com> 2013-05-21 12:41:56 PDT ---
(In reply to comment #14)
>     import core.thread;
>     int[] arr = [1,2,3].dup;
> 
>     void main() {
>         auto t = new Thread({arr[0] = 3;});
>         t.start();
>         t.join();
>         assert(arr[0] == 1);
>     }
> 
> The .dup should fix the problem, as now each thread gets its own copy of the array, right?

It's still being done at compile time, so no. It basically creates a copy at compile time, then stores that in the data segment instead of the original.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------