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

           Summary: Thread local slice to array literal references the
                    same data
           Product: D
           Version: unspecified
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: nobody@puremagic.com
        ReportedBy: code@dawg.eu


--- Comment #0 from Martin Nowak <code@dawg.eu> 2013-05-17 09:12:58 PDT ---
cat > bug.d << CODE
import core.thread;

int[] arr = [1,2,3];
__gshared int* thrPtr;

void main()
{
    auto thr = new Thread({thrPtr = arr.ptr;});
    thr.start();
    thr.join();
    assert(arr.ptr !is thrPtr);
}
CODE

----

Only the `arr` variable is store in TLS, but `arr.ptr` references the literal which is in the shared data segment.

-- 
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


Alex Rønne Petersen <alex@lycus.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |alex@lycus.org


--- Comment #1 from Alex Rønne Petersen <alex@lycus.org> 2013-05-17 18:17:00 CEST ---
I'm not sure I understand why this is a bug. Can you elaborate?

As far as I know, being able to access TLS data from one thread in another thread through pointers passed via globals is a feature (it's even one I rely on).

But maybe I'm misunderstanding this.

-- 
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 #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: -------

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


Steven Schveighoffer <schveiguy@yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |schveiguy@yahoo.com


--- Comment #3 from Steven Schveighoffer <schveiguy@yahoo.com> 2013-05-17 10:47:13 PDT ---
Be careful with your code snippets, unless you are more explicit about the error reporting.  Asserts are generally included to show what currently PASSES, not what FAILS.  I was about to mark this as invalid, because the code SHOULD pass, but you are correct in that it fails.

Your code should say:

assert(arr.ptr !is thrPtr); // FAILS!!!

or

assert(arr.ptr is thrPtr);

-- 
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 #4 from Alex Rønne Petersen <alex@lycus.org> 2013-05-17 20:12:27 CEST ---
(In reply to comment #2)
> 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;

Ah, I understand. Carry on. :)

-- 
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


Walter Bright <bugzilla@digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|                            |bugzilla@digitalmars.com
         Resolution|                            |INVALID


--- Comment #5 from Walter Bright <bugzilla@digitalmars.com> 2013-05-17 11:25:36 PDT ---
The __gshared storage class is an end run around the type system. That's why it has the __ prefix. So, yes, you can use it to get multithreaded "local" access to global data without a compiler error.

This is as designed - it's a bug in the code example.

-- 
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


Steven Schveighoffer <schveiguy@yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |


--- Comment #6 from Steven Schveighoffer <schveiguy@yahoo.com> 2013-05-17 11:29:07 PDT ---
(In reply to comment #5)

> This is as designed - it's a bug in the code example.

No, it's not.  It's a bug in the code generation.  The __gshared variable just demonstrates the bug.

What is happening is that each thread-local instance of arr is getting a pointer to the SAME data.

The assert should have been written differently.  as written, it fails, but this is not noted.

-- 
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


Simen Kjaeraas <simen.kjaras@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |simen.kjaras@gmail.com


--- Comment #7 from Simen Kjaeraas <simen.kjaras@gmail.com> 2013-05-17 11:53:53 PDT ---
Simplified example without __gshared:

import core.thread;

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

void main( ) {
    int* p = arr.ptr;
    auto thr = new Thread({assert(arr.ptr == p);}); // Should have failed.
    thr.start();
    thr.join();
}

-- 
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


Igor Stepanov <wazar.leollone@yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |wazar.leollone@yahoo.com


--- Comment #8 from Igor Stepanov <wazar.leollone@yahoo.com> 2013-05-17 13:57:50 PDT ---
(In reply to comment #7)
> Simplified example without __gshared:
> 
> import core.thread;
> 
> int[] arr = [1,2,3];
> 
> void main( ) {
>     int* p = arr.ptr;
>     auto thr = new Thread({assert(arr.ptr == p);}); // Should have failed.
>     thr.start();
>     thr.join();
> }

In other words implicit "thread local" modifier is not transitive.
Yes, all threads have a local copy of "arr symbol" and &arr differs in
different threads.
But all of this "local" symbols points to single array object. The problem lies
deeper than it seems.

struct Foo
{
  int[] arr;
}

Foo[] getFooArr()
{
   Foo[] ret;
   foreach(i; 1 .. 10)
   {
      Foo cur;
      foreach(j; 1 .. 5)
      {
         cur.arr ~= j;
      }
      ret ~= cur;
   }
   return ret;
}

Foo[] arr = getFooArr();

Now foo points to the arr, each members of it points to array literal.
And if we want to create Foo[] is true TLS object, we must to set, that
1. arr must point to tls object (arr.ptr must be
thread_tls_start+arr_tls_offset)
2. for each i: arr[i].arr must point to tls object.
In other words
If compiler see ptr dereference expression (with * or []) it must know, is this
ptr is TLS. If it is TLS compiler must add to it value thread_tls_start,
otherwise - use it value as is.
This functional can be provided, if we declare transitive threadlocal storage
class (like shared) and implement special reference behaviour. (e.g.
dereference, casting and other.) for example:

theradlocal int[][] tls = [[1,2],[3,4],[5,6]];

int* getNthMthElemPointer(theradlocal int[][] a, int n, int m)
{
   return &ptr[n][m]; //implicit cast to non-tls pointer. returned value points
to elem it function caller thread
}

void main()
{
  int* p1 = getNthMthElemPointer(tls, 1, 1);
  theradlocal int* p2 = &tls[1][1];


  void threadFunc(int num)()
  {
    writeln(num, " shared ptr: ", p1);
    writeln(num, " thread local ptr: ", cast(int*)p2);
  }
  auto thr1 = new Thread(&threadFunc!1);
  thr1.start();
  auto thr2 = new Thread(&threadFunc!2);
  thr2.start();

  thr1.join();
  thr2.join();
}

therads will print same "shared ptr" value but different "thread local ptr"



However this future is disharmonious with language design I think. Other way: disallow all of tls static initialized values.

shared int[] a = [1,2,3]; //OK
_gshared int[] b = [1,2,3]; //OK
const int[] c = [1,2,3]; //OK
int[] d = [1,2,3]; //Disallowed
int[] e;//OK

static this()
{
  e = [1,2,3]; //If e value will be allocated in heap this code doesn't break
type system.
}
The same applies to classes, pointers and associative arrays in future.

-- 
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 #9 from Simen Kjaeraas <simen.kjaras@gmail.com> 2013-05-17 15:26:26 PDT ---
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. 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.

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