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

           Summary: critical_.d and critical.c use double check locking
                    the wrong way
           Product: D
           Version: D2
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: major
          Priority: P2
         Component: druntime
        AssignedTo: nobody@puremagic.com
        ReportedBy: deadalnix@gmail.com


--- Comment #0 from deadalnix <deadalnix@gmail.com> 2011-09-05 18:12:19 PDT ---
First of all, I'm quite new here and hope I'm not plein wrong. If this is please forgive me.

I was studying the druntime to know more about what's going on behing the hood of D2, and I found out that synchronized may have been implemented the wrong way.

Consider this piece of code from critical_.d :

        if (!dcs.next)
        {
            pthread_mutex_lock(&critical_section.cs);
            if (!dcs.next) // if, in the meantime, another thread didn't set it
            {
                dcs.next = dcs_list;
                dcs_list = dcs;
                pthread_mutex_init(&dcs.cs, &_criticals_attr);
            }
            pthread_mutex_unlock(&critical_section.cs);
        }

The double check locking patern is used the wrong way here. dcs.next may have been set before the whole dcs struct has been initialized, thus leading to undefined behaviour. Reordering instruction within the second if isn't suffiscient as long as compiler, or CPU may reorder instruction and memory read/write.

The same goes for window code, which is very similar, and c code within critical.c . I'm not sure which one is used and when). This code duplication should eventually be reduced/suppressed in the process to avoid having a problem poping in several place like we can see now.

I would be happy to help to resolve that problem, so don't hesitate to contact me. Please be indulgent if the problem is irrevevelant because I didn't understood the way druntime worked. I'm currently learn, and, even if I have a strong background in devellopement even at low level, druntime is really new to me and I'm still at the stage where I'm learning it.

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


Sean Kelly <sean@invisibleduck.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |sean@invisibleduck.org


--- Comment #1 from Sean Kelly <sean@invisibleduck.org> 2011-09-06 11:27:42 PDT ---
I've always hoped that because Walter implemented this module, he did so knowing how DMD would generate the involved code.  I do agree that this isn't a safe long-term assumption though, since the compiler may change, leaving the need to update this code as a forgotten detail.

What should really happen is for all the .c modules to be transitioned to .d modules, and for any questionable practices like these to be changed to something more universally reliable.  Between new language features and core.atomic I think this can be improved without actually sacrificing performance.

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



--- Comment #2 from deadalnix <deadalnix@gmail.com> 2011-09-06 14:33:22 PDT ---
Actually, even if the compiler do not reorder that, modern CPU can, and this is out of control, unless specific precautions are used. It's even more problematic now that almost everybody has a multicore CPU.

We need stronger guaranties to make the double check locking pattern valid, for exemple guaranties that java's 5+ volatile keyword provide.

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