www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 6607] New: critical_.d and critical.c use double check locking the wrong way

reply d-bugmail puremagic.com writes:
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: -------
Sep 05 2011
next sibling parent d-bugmail puremagic.com writes:
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: -------
Sep 06 2011
prev sibling parent d-bugmail puremagic.com writes:
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: -------
Sep 06 2011