[phobos] Typo (bug) in std.concurrency

[Sean Kelly]

Hm... and then the mutex and condition variable in MessageBox would be declared as shared and I suppose everything would work.  I was worried that making Mutex shared, for example, would add synchronized loads every time a handle was used, but it looks like I'm wrong.  I can't recall what rule was provided for where synchronization is used for shared concrete variables... can someone refresh my memory?  For example, the following code has no synchronized ops at all:

    import std.stdio;

    class Mutex
    {
        shared void lock() {
            if (x == 0)
                x = 1;
        }
        int x;
    }

    void main()
    {
        shared(Mutex) m = new shared(Mutex);
        m.lock();
    }

__Dmain:
		push	EBP
		mov	EBP,ESP
		sub	ESP,8
		call	  LB
LB:		pop	EAX
		sub	ESP,0Ch
		push	dword ptr 0EDh[EAX]
		call	  L174
		add	ESP,010h
		mov	ECX,[EAX]
		call	dword ptr 018h[ECX]
		xor	EAX,EAX
		leave
		ret
_D5ytest5Mutex4lockMOFZv:
		push	EBP
		mov	EBP,ESP
		sub	ESP,8
		mov	-4[EBP],EAX
		call	  LA5
		mov	EAX,-4[EBP]
		cmp	dword ptr 8[EAX],0
		jne	L21
		mov	ECX,-4[EBP]
		mov	dword ptr 8[ECX],1
L21:		leave
		ret
		nop

I was worried I'd see a synchronized load when m.lock() was called and a synchronized load and/or store when x is accessed.  Is the current behavior a bug?  On x86, stores aren't reordered with other stores and loads aren't (supposed to be) reordered with other loads, but loads can be reordered to occur before preceding stores.  Does that mean I'll only see a memory barrier when I'm interleaving shared loads and stores?

On Jul 2, 2010, at 11:50 AM, Andrei Alexandrescu wrote:

> I think we should bite the bullet and mark the signatures as shared.
> 
> Andrei
> 
> Sean Kelly wrote:
>> As I'd feared, this is getting more and more complicated.  I tried making the lock() and unlock() methods in Mutex shared and now I'm getting these errors:
>> xtest.d(98): Error: function core.sys.posix.pthread.pthread_mutex_lock (pthread_mutex_t*) is not callable using argument types (shared(pthread_mutex_t)*)
>> xtest.d(98): Error: cannot implicitly convert expression (&this.m_hndl) of type shared(pthread_mutex_t)* to pthread_mutex_t*
>> xtest.d(120): Error: function core.sys.posix.pthread.pthread_mutex_unlock (pthread_mutex_t*) is not callable using argument types (shared(pthread_mutex_t)*)
>> xtest.d(120): Error: cannot implicitly convert expression (&this.m_hndl) of type shared(pthread_mutex_t)* to pthread_mutex_t*
>> xtest.d(146): Error: function core.sys.posix.pthread.pthread_mutex_trylock (pthread_mutex_t*) is not callable using argument types (shared(pthread_mutex_t)*)
>> xtest.d(146): Error: cannot implicitly convert expression (&this.m_hndl) of type shared(pthread_mutex_t)* to pthread_mutex_t*
>> While technically correct, I find this behavior quite frustrating.  I suppose I could go through the Posix API and mark all the thread stuff as shared to make sure it will compile correctly, but that would break any existing code that is using this stuff in an unshared data type (ie. basically everyone), and since these are extern (C) routines I can't overload them on shared either.  The simplest alternative fix I've found so far is basically what I described below:
>> class Mutex
>> {
>>    shared void lock()
>>    {
>>        (cast(Mutex) this).doLock();
>>    }
>>    private void doLock()
>>    {
>>        // lock the mutex
>>    }
>> }
>> I'm coming to wish there were a way to tell the compiler "this is part of the TCB, shut up, I know what I'm doing."
>> I think for the next release I'm simply going to allow sending Tid instances regardless of whether they contain an unshared reference so I can have more time to figure out the best way to handle this.
>> On Jun 30, 2010, at 1:37 PM, Sean Kelly wrote:
>>> Okay, alternate evil solution:
>>> 
>>> class Condition
>>> {
>>>   shared void notify()
>>>   {
>>>       static void doNotify() {}
>>>       (cast(Condition) this).doNotify();
>>>   }
>>> }
>>> 
>>> Seems like it should work, though I'm hoping that the classes in core.sync are about the only ones where it's necessary (something similar will probably be necessary for Mutex to make lock() and unlock() explicitly callable, etc).  I'm half tempted to try labeling these functions as __gshared and see if it compiles.
>>> 
>>> On Jun 30, 2010, at 1:24 PM, Sean Kelly wrote:
>>> 
>>>> Well that was a disaster.  I have this:
>>>> 
>>>> class MessageBox
>>>> {
>>>>  this()
>>>>  {
>>>>      // this makes m_lock the monitor for the MessageBox instance.
>>>>      m_lock = new Mutex( this );
>>>>      m_cond = new Condition( m_lock );
>>>>  }
>>>> 
>>>>  final synchronized void put( Message msg )
>>>>  {
>>>>      // m_lock is locked now
>>>>      m_list.add( msg );
>>>>      m_cond.notify(); // error: notify () is not callable using argument types () shared
>>>>  }
>>>> }
>>>> 
>>>> struct Tid
>>>> {
>>>>  shared MessageBox mbox;
>>>> }
>>>> 
>>>> class Condition
>>>> {
>>>>  void notify()
>>>>  {
>>>>      // fancy stuff involving semaphores
>>>>  }
>>>> }
>>>> 
>>>> How do I fix this?  Condition.notify() is technically shared, but I don't want to pay the cost for memory barriers that are utterly pointless.  Should I change the call to notify() to:
>>>> 
>>>> (cast(Condition)m_cond).notify();
>>>> 
>>>> This should work, but it seems like an even worse subversion of the shared system than I'm already doing for MessageBox.  If it helps, MessageBox is a thread-local class instance with put() as its one shared public method, so other threads have a shared reference to MessageBox while the object's owner has a non-shared reference to it.  This is functionally correct, though it seems theoretically wrong to have both shared and unshared references to the same instance in the first place.
>> _______________________________________________
>> phobos mailing list
>> phobos at puremagic.com
>> http://lists.puremagic.com/mailman/listinfo/phobos
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos