From 4b161d5771b458916cc1503969d592c89cc5768b Mon Sep 17 00:00:00 2001 From: Steven Schveighoffer Date: Tue, 28 Jun 2016 16:29:20 -0400 Subject: [PATCH 01/11] Fixes issue 16211 - Fix cycle detection algorithm. Left the old algorithm there to make the diff uncomplicated. --- src/rt/minfo.d | 418 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 384 insertions(+), 34 deletions(-) diff --git a/src/rt/minfo.d b/src/rt/minfo.d index 94aa32a806..bf58c90185 100644 --- a/src/rt/minfo.d +++ b/src/rt/minfo.d @@ -60,6 +60,339 @@ struct ModuleGroup * Throws: * Exception if it fails. */ + version(all) + { + void sortCtors() + { + debug(printModuleDependencies) + { + import core.stdc.stdio : printf; + foreach(_m; _modules) + { + printf("%s%s%s:", _m.name.ptr, (_m.flags & MIstandalone) ? "+".ptr : "".ptr, (_m.flags & (MIctor | MIdtor)) ? "*".ptr : "".ptr); + foreach(_i; _m.importedModules) + printf(" %s", _i.name.ptr); + printf("\n"); + } + } + + immutable len = _modules.length; + + // This is ugly to say the least. Would be nice to use a binary + // search at least instead of linear... + int findModule(in ModuleInfo* mi) + { + foreach (i, m; _modules) + if (m is mi) return cast(int)i; + return -1; + } + + // allocate the two constructor lists. These will be stored for life of + // the program, so use malloc. + _ctors = (cast(immutable(ModuleInfo)**).malloc(len * (void*).sizeof))[0 .. len]; + _tlsctors = (cast(immutable(ModuleInfo)**).malloc(len * (void*).sizeof))[0 .. len]; + + // Start out doing the shared ctors. The destruction is done in reverse. + auto ctors = _ctors; + // current element being inserted into ctors list. + size_t ctoridx = 0; + + // allocate some stack arrays that will be used throughout the process. + ubyte* p = cast(ubyte *)alloca(len * ubyte.sizeof); + auto reachable = p[0 .. len]; + + p = cast(ubyte *)alloca(len * ubyte.sizeof); + auto flags = p[0 .. len]; + + + // use this to hold the error message. + string errmsg = null; + + void print(string[] msgs...) + { + foreach (m; msgs) + { + // save to the error message. Note that if we are throwing an + // exception, we don't care about being careful with using + // stack memory. Just use the GC/runtime. + errmsg ~= m; + } + } + + void println(string[] msgs...) + { + print(msgs); + version(Windows) + print("\r\n"); + else + print("\n"); + } + + // this set of functions helps create a valid path between two + // interdependent modules. This is only used if a cycle is found, to + // print the cycle to the user. Therefore, we don't initialize the data + // until we have to. + int[] cyclePath; + int[] distance; + int[][] edges; + + // determine the shortest path between two modules. Uses dijkstra + // without a priority queue. (we can be a bit slow here, in order to + // get a better printout). + void shortest(int start, int target) + { + // initial setup + distance[] = int.max; + int curdist = 0; + distance[start] = 0; + while(true) + { + bool done = true; + foreach(i, x; distance) + { + if(x == curdist) + { + if(i == target) + { + done = true; + break; + } + foreach(n; edges[i]) + { + if(distance[n] == int.max) + { + distance[n] = curdist + 1; + done = false; + } + } + } + } + if(done) + break; + ++curdist; + } + // it should be impossible to not get to target, this is just a + // sanity check. Not an assert, because druntime is compiled in + // release mode. + if(distance[target] != curdist) + { + throw new Error("internal error printing module cycle"); + } + + // determine the path. This is tricky, because we have to + // follow the edges in reverse to get back to the original. We + // don't have a reverse mapping, so it takes a bit of looping. + cyclePath.length += curdist; + auto subpath = cyclePath[$-curdist .. $]; + while(true) + { + --curdist; + subpath[curdist] = target; + if(curdist == 0) + break; +distloop: + // search for next (previous) module in cycle. + foreach(int m, d; distance) + { + if(d == curdist) + { + // determine if m can reach target + foreach(e; edges[m]) + { + if(e == target) + { + // recurse + target = m; + break distloop; + } + } + } + } + } + } + + // this function initializes the bookeeping necessary to create the + // cycle path, and then creates it. It is a precondition that src and + // target modules are involved in a cycle + void genPath(int srcidx, int targetidx) + { + assert(srcidx != -1); + assert(targetidx != -1); + + // set up all the arrays. Use the GC, we are going to exit anyway. + distance.length = len; + edges.length = len; + foreach(i, m; _modules) + { + // use reachable, because an import can appear more than once. + // https://issues.dlang.org/show_bug.cgi?id=16208 + reachable[] = 0; + foreach(e; m.importedModules) + { + auto impidx = findModule(e); + if(impidx != -1 && impidx != i) + reachable[impidx] = 1; + } + + foreach(int j, r; reachable) + { + if(r) + edges[i] ~= j; + } + } + + // a cycle starts with the source. + cyclePath ~= srcidx; + // first get to the target + shortest(srcidx, targetidx); + // now get back. + shortest(targetidx, srcidx); + } + + // find all the non-trivial dependencies (that is, dependencies that have a + // ctor or dtor) of a given module. Doing this, we can 'skip over' the + // trivial modules to get at the non-trivial ones. + size_t _findDependencies(int idx, bool orig = true) + { + if(reachable[idx]) + return 0; + auto current = _modules[idx]; + size_t result = 0; + reachable[idx] = 1; + if(!orig && (flags[idx] & (MIctor | MIdtor)) && !(flags[idx] & MIstandalone)) + // non-trivial, stop here + return result + 1; + foreach (m; current.importedModules) + { + auto midx = findModule(m); + if(midx != -1) + // not part of this DSO, don't consider it. + result += _findDependencies(midx, false); + } + return result; + } + + // This function will determine the order of construction/destruction and + // check for cycles. If a cycle is found, the cycle path is transformed + // into a string and thrown as an error. + // + // Each call into this function is given a module that has static + // ctor/dtors that must be dealt with. It recurses only when it finds + // dependencies that also have static ctor/dtors. + void _checkModCtors2(int curidx) + { + assert(curidx != -1); + immutable ModuleInfo* current = _modules[curidx]; + + // we only get called if current has a dtor or a ctor, so no need to + // check that. First, determine what non-trivial elements are + // reachable. + reachable[] = 0; + auto nmodules = _findDependencies(curidx); + + // allocate the dependencies on the stack + auto p = cast(int *)alloca(nmodules * int.sizeof); + auto dependencies = p[0 .. nmodules]; + uint depidx = 0; + // fill in the dependencies + foreach (int i, r; reachable) + { + if(r && i != curidx) + { + if((flags[i] & (MIctor | MIdtor)) && !(flags[i] & MIstandalone)) + { + dependencies[depidx++] = i; + } + } + } + assert(depidx == nmodules); + + // ok, now perform cycle detection + flags[curidx] |= MIctorstart; + foreach (m; dependencies) + { + auto mflags = flags[m]; + if(mflags & MIctorstart) + { + // found a cycle + println("Cyclic dependency between module ", _modules[m].name, " and ", current.name); + genPath(m, curidx); + + foreach(midx; cyclePath[0 .. $-1]) + { + println(_modules[midx].name, (flags[midx] & (MIctor | MIdtor)) ? "* ->" : " ->"); + } + println(_modules[m].name, "*"); + throw new Error(errmsg); + } + else if(!(mflags & MIctordone)) + { + _checkModCtors2(m); + } + } + flags[curidx] = (flags[curidx] & ~MIctorstart) | MIctordone; + // add this module to the construction order list + ctors[ctoridx++] = current; + } + + void _checkModCtors3() + { + foreach (int idx, m; _modules) + { + // TODO: Should null ModuleInfo be allowed? + if (m is null) continue; + auto flag = flags[idx]; + if((flag & (MIctor | MIdtor)) && !(flag & MIctordone)) + { + if(flag & MIstandalone) + { + // no need to run a check on this one, but we do need to call its ctor/dtor + ctors[ctoridx++] = m; + } + else + _checkModCtors2(idx); + } + } + } + + // initialize the flags for the first run (shared ctors). + foreach (uint i, m; _modules) + { + // TODO: Should null ModuleInfo be allowed? + if (m is null) continue; + ubyte flag = m.flags & MIstandalone; + if(m.dtor) + flag |= MIdtor; + if(m.ctor) + flag |= MIctor; + flags[i] = flag; + } + + _checkModCtors3(); + + // _ctors is now valid up to ctoridx + _ctors = _ctors[0 .. ctoridx]; + + // tls ctors/dtors + ctors = _tlsctors; + ctoridx = 0; + foreach (i, m; modules) + { + // TODO: Should null ModuleInfo be allowed? + if (m is null) continue; + ubyte flag = m.flags & MIstandalone; + if(m.tlsdtor) + flag |= MIdtor; + if(m.tlsctor) + flag |= MIctor; + flags[i] = flag; + } + _checkModCtors3(); + _tlsctors = _tlsctors[0 .. ctoridx]; + } + } + else + { void sortCtors() { immutable len = _modules.length; @@ -198,7 +531,7 @@ struct ModuleGroup { /* Internal runtime error, recursion exceeds number of modules. */ - (stackidx < _modules.length) || assert(0); + (stackidx < stack.length) || assert(0); // recurse stack[stackidx++] = StackRec(mods, idx); @@ -231,6 +564,7 @@ struct ModuleGroup sort(_ctors, MIctor | MIdtor); sort(_tlsctors, MItlsctor | MItlsdtor); } + } void runCtors() { @@ -382,13 +716,13 @@ void runModuleFuncsRev(alias getfp)(const(immutable(ModuleInfo)*)[] modules) unittest { - static void assertThrown(T : Throwable, E)(lazy E expr) + static void assertThrown(T : Throwable, E)(lazy E expr, string msg) { try expr; catch (T) return; - assert(0); + assert(0, msg); } static void stub() @@ -435,52 +769,54 @@ unittest return mi; } - static void checkExp( + static void checkExp(string testname, bool shouldThrow, immutable(ModuleInfo*)[] modules, immutable(ModuleInfo*)[] dtors=null, immutable(ModuleInfo*)[] tlsdtors=null) { auto mgroup = ModuleGroup(modules); mgroup.sortCtors(); - foreach (m; mgroup._modules) - assert(!(m.flags & (MIctorstart | MIctordone))); - assert(mgroup._ctors == dtors); - assert(mgroup._tlsctors == tlsdtors); + + // if we are expecting sort to throw, don't throw because of unexpected + // success! + if(!shouldThrow) + { + foreach (m; mgroup._modules) + assert(!(m.flags & (MIctorstart | MIctordone)), testname); + assert(mgroup._ctors == dtors, testname); + assert(mgroup._tlsctors == tlsdtors, testname); + } } - // no ctors { auto m0 = mockMI(0); auto m1 = mockMI(0); auto m2 = mockMI(0); - checkExp([&m0.mi, &m1.mi, &m2.mi]); + checkExp("no ctors", false, [&m0.mi, &m1.mi, &m2.mi]); } - // independent ctors { auto m0 = mockMI(MIictor); auto m1 = mockMI(0); auto m2 = mockMI(MIictor); auto mgroup = ModuleGroup([&m0.mi, &m1.mi, &m2.mi]); - checkExp([&m0.mi, &m1.mi, &m2.mi]); + checkExp("independent ctors", false, [&m0.mi, &m1.mi, &m2.mi]); } - // standalone ctor { auto m0 = mockMI(MIstandalone | MIctor); auto m1 = mockMI(0); auto m2 = mockMI(0); auto mgroup = ModuleGroup([&m0.mi, &m1.mi, &m2.mi]); - checkExp([&m0.mi, &m1.mi, &m2.mi], [&m0.mi]); + checkExp("standalone ctor", false, [&m0.mi, &m1.mi, &m2.mi], [&m0.mi]); } - // imported standalone => no dependency { auto m0 = mockMI(MIstandalone | MIctor); auto m1 = mockMI(MIstandalone | MIctor); auto m2 = mockMI(0); m1.setImports(&m0.mi); - checkExp([&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi]); + checkExp("imported standalone => no dependency", false, [&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi]); } { @@ -488,26 +824,24 @@ unittest auto m1 = mockMI(MIstandalone | MIctor); auto m2 = mockMI(0); m0.setImports(&m1.mi); - checkExp([&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi]); + checkExp("imported standalone => no dependency (2)", false, [&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi]); } - // standalone may have cycle { auto m0 = mockMI(MIstandalone | MIctor); auto m1 = mockMI(MIstandalone | MIctor); auto m2 = mockMI(0); m0.setImports(&m1.mi); m1.setImports(&m0.mi); - checkExp([&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi]); + checkExp("standalone may have cycle", false, [&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi]); } - // imported ctor => ordered ctors { auto m0 = mockMI(MIctor); auto m1 = mockMI(MIctor); auto m2 = mockMI(0); m1.setImports(&m0.mi); - checkExp([&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi], []); + checkExp("imported ctor => ordered ctors", false, [&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi], []); } { @@ -515,27 +849,34 @@ unittest auto m1 = mockMI(MIctor); auto m2 = mockMI(0); m0.setImports(&m1.mi); - assert(m0.importedModules == [&m1.mi]); - checkExp([&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m0.mi], []); + checkExp("imported ctor => ordered ctors (2)", false, [&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m0.mi], []); } - // detects ctors cycles { auto m0 = mockMI(MIctor); auto m1 = mockMI(MIctor); auto m2 = mockMI(0); m0.setImports(&m1.mi); m1.setImports(&m0.mi); - assertThrown!Throwable(checkExp([&m0.mi, &m1.mi, &m2.mi])); + assertThrown!Throwable(checkExp("", true, [&m0.mi, &m1.mi, &m2.mi]), "detects ctors cycles"); + } + + { + auto m0 = mockMI(MIctor); + auto m1 = mockMI(MIctor); + auto m2 = mockMI(0); + m0.setImports(&m2.mi); + m1.setImports(&m2.mi); + m2.setImports(&m0.mi, &m1.mi); + assertThrown!Throwable(checkExp("", true, [&m0.mi, &m1.mi, &m2.mi]), "detects cycle with repeats"); } - // imported ctor/tlsctor => ordered ctors/tlsctors { auto m0 = mockMI(MIctor); auto m1 = mockMI(MIctor); auto m2 = mockMI(MItlsctor); m0.setImports(&m1.mi, &m2.mi); - checkExp([&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m0.mi], [&m2.mi]); + checkExp("imported ctor/tlsctor => ordered ctors/tlsctors", false, [&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m0.mi], [&m2.mi]); } { @@ -543,30 +884,37 @@ unittest auto m1 = mockMI(MIctor); auto m2 = mockMI(MItlsctor); m0.setImports(&m1.mi, &m2.mi); - checkExp([&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m0.mi], [&m2.mi, &m0.mi]); + checkExp("imported ctor/tlsctor => ordered ctors/tlsctors (2)", false, [&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m0.mi], [&m2.mi, &m0.mi]); } - // no cycle between ctors/tlsctors { auto m0 = mockMI(MIctor); auto m1 = mockMI(MIctor); auto m2 = mockMI(MItlsctor); m0.setImports(&m1.mi, &m2.mi); m2.setImports(&m0.mi); - checkExp([&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m0.mi], [&m2.mi]); + checkExp("no cycle between ctors/tlsctors", false, [&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m0.mi], [&m2.mi]); } - // detects tlsctors cycle { auto m0 = mockMI(MItlsctor); auto m1 = mockMI(MIctor); auto m2 = mockMI(MItlsctor); m0.setImports(&m2.mi); m2.setImports(&m0.mi); - assertThrown!Throwable(checkExp([&m0.mi, &m1.mi, &m2.mi])); + assertThrown!Throwable(checkExp("", true, [&m0.mi, &m1.mi, &m2.mi]), "detects tlsctors cycle"); + } + + { + auto m0 = mockMI(MItlsctor); + auto m1 = mockMI(MIctor); + auto m2 = mockMI(MItlsctor); + m0.setImports(&m1.mi); + m1.setImports(&m0.mi, &m2.mi); + m2.setImports(&m1.mi); + assertThrown!Throwable(checkExp("", true, [&m0.mi, &m1.mi, &m2.mi]), "detects tlsctors cycle with repeats"); } - // closed ctors cycle { auto m0 = mockMI(MIctor); auto m1 = mockMI(MIstandalone | MIctor); @@ -574,7 +922,9 @@ unittest m0.setImports(&m1.mi); m1.setImports(&m2.mi); m2.setImports(&m0.mi); - checkExp([&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m2.mi, &m0.mi]); + // NOTE: this is implementation dependent, sorted order shouldn't be tested. + //checkExp("closed ctors cycle", false, [&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m2.mi, &m0.mi]); + checkExp("closed ctors cycle", false, [&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi, &m2.mi]); } } From 96cfbe03df65e1d1a87bc24d694eaa1be8d66420 Mon Sep 17 00:00:00 2001 From: Steven Schveighoffer Date: Tue, 28 Jun 2016 16:40:36 -0400 Subject: [PATCH 02/11] remove old constructor sorting algorithm. --- src/rt/minfo.d | 177 ------------------------------------------------- 1 file changed, 177 deletions(-) diff --git a/src/rt/minfo.d b/src/rt/minfo.d index bf58c90185..ddd728020c 100644 --- a/src/rt/minfo.d +++ b/src/rt/minfo.d @@ -60,8 +60,6 @@ struct ModuleGroup * Throws: * Exception if it fails. */ - version(all) - { void sortCtors() { debug(printModuleDependencies) @@ -390,181 +388,6 @@ distloop: _checkModCtors3(); _tlsctors = _tlsctors[0 .. ctoridx]; } - } - else - { - void sortCtors() - { - immutable len = _modules.length; - if (!len) - return; - - static struct StackRec - { - @property immutable(ModuleInfo)* mod() - { - return _mods[_idx]; - } - - immutable(ModuleInfo*)[] _mods; - size_t _idx; - } - - auto stack = (cast(StackRec*).calloc(len, StackRec.sizeof))[0 .. len]; - // TODO: reuse GCBits by moving it to rt.util.container or core.internal - immutable nwords = (len + 8 * size_t.sizeof - 1) / (8 * size_t.sizeof); - auto ctorstart = cast(size_t*).malloc(nwords * size_t.sizeof); - auto ctordone = cast(size_t*).malloc(nwords * size_t.sizeof); - if (!stack.ptr || ctorstart is null || ctordone is null) - assert(0); - scope (exit) { .free(stack.ptr); .free(ctorstart); .free(ctordone); } - - int findModule(in ModuleInfo* mi) - { - foreach (i, m; _modules) - if (m is mi) return cast(int)i; - return -1; - } - - void sort(ref immutable(ModuleInfo)*[] ctors, uint mask) - { - import core.bitop; - - ctors = (cast(immutable(ModuleInfo)**).malloc(len * size_t.sizeof))[0 .. len]; - if (!ctors.ptr) - assert(0); - - // clean flags - memset(ctorstart, 0, nwords * size_t.sizeof); - memset(ctordone, 0, nwords * size_t.sizeof); - size_t stackidx = 0; - size_t cidx; - - immutable(ModuleInfo*)[] mods = _modules; - size_t idx; - while (true) - { - while (idx < mods.length) - { - auto m = mods[idx]; - - immutable bitnum = findModule(m); - - if (bitnum < 0 || bt(ctordone, bitnum)) - { - /* If the module can't be found among the ones to be - * sorted it's an imported module from another DSO. - * Those don't need to be considered during sorting as - * the OS is responsible for the DSO load order and - * module construction is done during DSO loading. - */ - ++idx; - continue; - } - else if (bt(ctorstart, bitnum)) - { - /* Trace back to the begin of the cycle. - */ - bool ctorInCycle; - size_t start = stackidx; - while (start--) - { - auto sm = stack[start].mod; - if (sm == m) - break; - immutable sbitnum = findModule(sm); - assert(sbitnum >= 0); - if (bt(ctorstart, sbitnum)) - ctorInCycle = true; - } - assert(stack[start].mod == m); - if (ctorInCycle) - { - /* This is an illegal cycle, no partial order can be established - * because the import chain have contradicting ctor/dtor - * constraints. - */ - string msg = "Aborting: Cycle detected between modules with "; - if (mask & (MIctor | MIdtor)) - msg ~= "shared "; - msg ~= "ctors/dtors:\n"; - foreach (e; stack[start .. stackidx]) - { - msg ~= e.mod.name; - if (e.mod.flags & mask) - msg ~= '*'; - msg ~= " ->\n"; - } - msg ~= stack[start].mod.name; - free(); - throw new Exception(msg); - } - else - { - /* This is also a cycle, but the import chain does not constrain - * the order of initialization, either because the imported - * modules have no ctors or the ctors are standalone. - */ - ++idx; - } - } - else - { - if (m.flags & mask) - { - if (m.flags & MIstandalone || !m.importedModules.length) - { // trivial ctor => sort in - ctors[cidx++] = m; - bts(ctordone, bitnum); - } - else - { // non-trivial ctor => defer - bts(ctorstart, bitnum); - } - } - else // no ctor => mark as visited - { - bts(ctordone, bitnum); - } - - if (m.importedModules.length) - { - /* Internal runtime error, recursion exceeds number of modules. - */ - (stackidx < stack.length) || assert(0); - - // recurse - stack[stackidx++] = StackRec(mods, idx); - idx = 0; - mods = m.importedModules; - } - } - } - - if (stackidx) - { // pop old value from stack - --stackidx; - mods = stack[stackidx]._mods; - idx = stack[stackidx]._idx; - auto m = mods[idx++]; - immutable bitnum = findModule(m); - assert(bitnum >= 0); - if (m.flags & mask && !bts(ctordone, bitnum)) - ctors[cidx++] = m; - } - else // done - break; - } - // store final number and shrink array - ctors = (cast(immutable(ModuleInfo)**).realloc(ctors.ptr, cidx * size_t.sizeof))[0 .. cidx]; - } - - /* Do two passes: ctor/dtor, tlsctor/tlsdtor - */ - sort(_ctors, MIctor | MIdtor); - sort(_tlsctors, MItlsctor | MItlsdtor); - } - } void runCtors() { From a3c7270a976620496ef65154084996ef2cf67d62 Mon Sep 17 00:00:00 2001 From: Steven Schveighoffer Date: Tue, 19 Jul 2016 17:32:22 -0400 Subject: [PATCH 03/11] More improvements. --- src/rt/minfo.d | 200 ++++++++++++++++++++++++------------------------- 1 file changed, 99 insertions(+), 101 deletions(-) diff --git a/src/rt/minfo.d b/src/rt/minfo.d index ddd728020c..966d21b3ae 100644 --- a/src/rt/minfo.d +++ b/src/rt/minfo.d @@ -62,6 +62,7 @@ struct ModuleGroup */ void sortCtors() { + import core.bitop: bts, btr, bt; debug(printModuleDependencies) { import core.stdc.stdio : printf; @@ -74,34 +75,49 @@ struct ModuleGroup } } - immutable len = _modules.length; + immutable uint len = cast(uint)_modules.length; + if(!len) + return; // nothing to do. + + immutable(ModuleInfo)* minMod = _modules[0]; + immutable(ModuleInfo)* maxMod = _modules[0]; + foreach(m; _modules) + { + if(m < minMod) + minMod = m; + if(m > maxMod) + maxMod = m; + } - // This is ugly to say the least. Would be nice to use a binary - // search at least instead of linear... int findModule(in ModuleInfo* mi) { + // short circuit linear search if possible. + if(mi < minMod || mi > maxMod) + return -1; + // binary search would be nice... foreach (i, m; _modules) if (m is mi) return cast(int)i; return -1; } - // allocate the two constructor lists. These will be stored for life of - // the program, so use malloc. - _ctors = (cast(immutable(ModuleInfo)**).malloc(len * (void*).sizeof))[0 .. len]; - _tlsctors = (cast(immutable(ModuleInfo)**).malloc(len * (void*).sizeof))[0 .. len]; - // Start out doing the shared ctors. The destruction is done in reverse. - auto ctors = _ctors; + // The list of constructors that will be returned by the sorting. + immutable(ModuleInfo)*[] ctors; // current element being inserted into ctors list. size_t ctoridx = 0; // allocate some stack arrays that will be used throughout the process. - ubyte* p = cast(ubyte *)alloca(len * ubyte.sizeof); - auto reachable = p[0 .. len]; - - p = cast(ubyte *)alloca(len * ubyte.sizeof); - auto flags = p[0 .. len]; - + immutable nwords = (len + 8 * size_t.sizeof - 1) / (8 * size_t.sizeof); + immutable flagbytes = nwords * size_t.sizeof; + auto ctorstart = cast(size_t*)alloca(flagbytes);// ctor/dtor seen + auto ctordone = cast(size_t*)alloca(flagbytes);// ctor/dtor processed + auto relevant = cast(size_t*)alloca(flagbytes);// has ctors/dtors + auto reachable = cast(size_t*)alloca(flagbytes);// used for reachability check + + void clearFlags(size_t *flags) + { + memset(flags, 0, flagbytes); + } // use this to hold the error message. string errmsg = null; @@ -224,18 +240,13 @@ distloop: { // use reachable, because an import can appear more than once. // https://issues.dlang.org/show_bug.cgi?id=16208 - reachable[] = 0; + clearFlags(reachable); foreach(e; m.importedModules) { auto impidx = findModule(e); if(impidx != -1 && impidx != i) - reachable[impidx] = 1; - } - - foreach(int j, r; reachable) - { - if(r) - edges[i] ~= j; + if(!bts(reachable, impidx)) + edges[i] ~= impidx; } } @@ -252,20 +263,23 @@ distloop: // trivial modules to get at the non-trivial ones. size_t _findDependencies(int idx, bool orig = true) { - if(reachable[idx]) - return 0; + // if it was already set, then we've already examined it. auto current = _modules[idx]; size_t result = 0; - reachable[idx] = 1; - if(!orig && (flags[idx] & (MIctor | MIdtor)) && !(flags[idx] & MIstandalone)) - // non-trivial, stop here - return result + 1; foreach (m; current.importedModules) { auto midx = findModule(m); + // if midx is -1, then this isn't part of this DSO. if(midx != -1) - // not part of this DSO, don't consider it. - result += _findDependencies(midx, false); + if(!bts(reachable, midx)) + { + if(bt(relevant, midx)) + // non-trivial, stop here + result += 1; + else + // non-relevant, recurse + result += _findDependencies(midx, false); + } } return result; } @@ -282,111 +296,95 @@ distloop: assert(curidx != -1); immutable ModuleInfo* current = _modules[curidx]; - // we only get called if current has a dtor or a ctor, so no need to - // check that. First, determine what non-trivial elements are - // reachable. - reachable[] = 0; + // First, determine what non-trivial elements are reachable. + clearFlags(reachable); + bts(reachable, curidx); auto nmodules = _findDependencies(curidx); - // allocate the dependencies on the stack + // allocate the dependencies on the stack, because reachable will + // be reused as we recurse. auto p = cast(int *)alloca(nmodules * int.sizeof); auto dependencies = p[0 .. nmodules]; uint depidx = 0; // fill in the dependencies - foreach (int i, r; reachable) + foreach (int i; 0 .. len) { - if(r && i != curidx) + if(i != curidx && bt(reachable, i) && bt(relevant, i)) { - if((flags[i] & (MIctor | MIdtor)) && !(flags[i] & MIstandalone)) + if(bt(ctorstart, i)) { - dependencies[depidx++] = i; + // found a cycle + println("Cyclic dependency between module ", _modules[i].name, " and ", current.name); + genPath(i, curidx); + + foreach(midx; cyclePath[0 .. $-1]) + { + println(_modules[midx].name, bt(relevant, midx) ? "* ->" : " ->"); + } + println(_modules[i].name, "*"); + throw new Error(errmsg); } + dependencies[depidx++] = i; } } assert(depidx == nmodules); - // ok, now perform cycle detection - flags[curidx] |= MIctorstart; + // process all the dependencies + bts(ctorstart, curidx); foreach (m; dependencies) - { - auto mflags = flags[m]; - if(mflags & MIctorstart) - { - // found a cycle - println("Cyclic dependency between module ", _modules[m].name, " and ", current.name); - genPath(m, curidx); - - foreach(midx; cyclePath[0 .. $-1]) - { - println(_modules[midx].name, (flags[midx] & (MIctor | MIdtor)) ? "* ->" : " ->"); - } - println(_modules[m].name, "*"); - throw new Error(errmsg); - } - else if(!(mflags & MIctordone)) - { + if(!bt(ctordone, m)) _checkModCtors2(m); - } - } - flags[curidx] = (flags[curidx] & ~MIctorstart) | MIctordone; + bts(ctordone, curidx); + btr(ctorstart, curidx); // add this module to the construction order list ctors[ctoridx++] = current; } - void _checkModCtors3() + immutable(ModuleInfo)*[] + _checkModCtors3(int function(immutable ModuleInfo *) getf) { + clearFlags(relevant); + clearFlags(ctorstart); + clearFlags(ctordone); + + // pre-allocate enough space to hold all modules. + ctors = (cast(immutable(ModuleInfo)**).malloc(len * (void*).sizeof))[0 .. len]; + ctoridx = 0; foreach (int idx, m; _modules) { // TODO: Should null ModuleInfo be allowed? if (m is null) continue; - auto flag = flags[idx]; - if((flag & (MIctor | MIdtor)) && !(flag & MIctordone)) + auto flag = getf(m); + if(flag & MIctor) { if(flag & MIstandalone) { - // no need to run a check on this one, but we do need to call its ctor/dtor + // can run at any time. Just run it first. ctors[ctoridx++] = m; } else + { + bts(relevant, idx); + } + } + } + + // now run the search in the relevant ones + foreach (int idx, m; _modules) + { + if(bt(relevant, idx)) + { + if(!bt(ctordone, idx)) _checkModCtors2(idx); } } - } - // initialize the flags for the first run (shared ctors). - foreach (uint i, m; _modules) - { - // TODO: Should null ModuleInfo be allowed? - if (m is null) continue; - ubyte flag = m.flags & MIstandalone; - if(m.dtor) - flag |= MIdtor; - if(m.ctor) - flag |= MIctor; - flags[i] = flag; + return ctors[0 .. ctoridx]; } - _checkModCtors3(); - - // _ctors is now valid up to ctoridx - _ctors = _ctors[0 .. ctoridx]; - - // tls ctors/dtors - ctors = _tlsctors; - ctoridx = 0; - foreach (i, m; modules) - { - // TODO: Should null ModuleInfo be allowed? - if (m is null) continue; - ubyte flag = m.flags & MIstandalone; - if(m.tlsdtor) - flag |= MIdtor; - if(m.tlsctor) - flag |= MIctor; - flags[i] = flag; - } - _checkModCtors3(); - _tlsctors = _tlsctors[0 .. ctoridx]; + // finally, do the sorting for both shared and tls ctors. + _ctors = _checkModCtors3((immutable ModuleInfo * m) => (m.flags & MIstandalone) | ((m.dtor || m.ctor)? MIctor : 0)); + _tlsctors = _checkModCtors3((immutable ModuleInfo * m) => (m.flags & MIstandalone) | ((m.tlsdtor || m.tlsctor)? MIctor : 0)); } void runCtors() @@ -746,8 +744,8 @@ unittest m1.setImports(&m2.mi); m2.setImports(&m0.mi); // NOTE: this is implementation dependent, sorted order shouldn't be tested. - //checkExp("closed ctors cycle", false, [&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m2.mi, &m0.mi]); - checkExp("closed ctors cycle", false, [&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi, &m2.mi]); + checkExp("closed ctors cycle", false, [&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m2.mi, &m0.mi]); + //checkExp("closed ctors cycle", false, [&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi, &m2.mi]); } } From 277f9f22144e0b6a261c8ca180a53f0b88369676 Mon Sep 17 00:00:00 2001 From: Steven Schveighoffer Date: Tue, 19 Jul 2016 19:07:05 -0400 Subject: [PATCH 04/11] Prune branches from the search when the non-relevant nodes have have already been examined. --- src/rt/minfo.d | 83 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 56 insertions(+), 27 deletions(-) diff --git a/src/rt/minfo.d b/src/rt/minfo.d index 966d21b3ae..3d1350a7fe 100644 --- a/src/rt/minfo.d +++ b/src/rt/minfo.d @@ -101,11 +101,6 @@ struct ModuleGroup } - // The list of constructors that will be returned by the sorting. - immutable(ModuleInfo)*[] ctors; - // current element being inserted into ctors list. - size_t ctoridx = 0; - // allocate some stack arrays that will be used throughout the process. immutable nwords = (len + 8 * size_t.sizeof - 1) / (8 * size_t.sizeof); immutable flagbytes = nwords * size_t.sizeof; @@ -263,10 +258,8 @@ distloop: // trivial modules to get at the non-trivial ones. size_t _findDependencies(int idx, bool orig = true) { - // if it was already set, then we've already examined it. - auto current = _modules[idx]; size_t result = 0; - foreach (m; current.importedModules) + foreach (m; _modules[idx].importedModules) { auto midx = findModule(m); // if midx is -1, then this isn't part of this DSO. @@ -274,16 +267,27 @@ distloop: if(!bts(reachable, midx)) { if(bt(relevant, midx)) + { // non-trivial, stop here result += 1; - else - // non-relevant, recurse - result += _findDependencies(midx, false); + } + else if(!bt(ctordone, midx)) + { + // non-relevant, and hasn't been exhaustively processed, recurse. + result += (!bt(ctorstart, midx) ? 1 : 0) + + _findDependencies(midx, false); + } } } return result; } + // The list of constructors that will be returned by the sorting. + immutable(ModuleInfo)*[] ctors; + // current element being inserted into ctors list. + size_t ctoridx = 0; + + // This function will determine the order of construction/destruction and // check for cycles. If a cycle is found, the cycle path is transformed // into a string and thrown as an error. @@ -309,22 +313,34 @@ distloop: // fill in the dependencies foreach (int i; 0 .. len) { - if(i != curidx && bt(reachable, i) && bt(relevant, i)) + if(i != curidx && bt(reachable, i)) { - if(bt(ctorstart, i)) - { - // found a cycle - println("Cyclic dependency between module ", _modules[i].name, " and ", current.name); - genPath(i, curidx); - - foreach(midx; cyclePath[0 .. $-1]) - { - println(_modules[midx].name, bt(relevant, midx) ? "* ->" : " ->"); - } - println(_modules[i].name, "*"); - throw new Error(errmsg); - } - dependencies[depidx++] = i; + if(bt(relevant, i)) + { + if(bt(ctorstart, i)) + { + // found a cycle + println("Cyclic dependency between module ", _modules[i].name, " and ", current.name); + genPath(i, curidx); + + foreach(midx; cyclePath[0 .. $-1]) + { + println(_modules[midx].name, bt(relevant, midx) ? "* ->" : " ->"); + } + println(_modules[i].name, "*"); + throw new Error(errmsg); + } + dependencies[depidx++] = i; + } + else if(!bts(ctorstart, i)) + { + // this is a non-relevant module, but it has never been + // imported by any other relevant module in our search. + // Once we have processed the current relevant module, + // we can mark this non-relevant module as done, so it's + // not ever looked at again. + dependencies[depidx++] = i; + } } } assert(depidx == nmodules); @@ -332,10 +348,23 @@ distloop: // process all the dependencies bts(ctorstart, curidx); foreach (m; dependencies) - if(!bt(ctordone, m)) + { + if(bt(relevant, m) && !bt(ctordone, m)) _checkModCtors2(m); + } bts(ctordone, curidx); btr(ctorstart, curidx); + + // mark all non-relevant dependencies as done + foreach(m; dependencies) + { + if(!bt(relevant, m)) + { + bts(ctordone, m); + // no need to clear ctorstart, as we don't care about + // irrelevant modules for detecting cycles. + } + } // add this module to the construction order list ctors[ctoridx++] = current; } From 8752d18f0d30f39b70fb64d2ebe63aaae42bdeb1 Mon Sep 17 00:00:00 2001 From: Steven Schveighoffer Date: Tue, 19 Jul 2016 20:32:00 -0400 Subject: [PATCH 05/11] Switch to manual stack allocation for finding relevant dependencies. --- src/rt/minfo.d | 64 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 14 deletions(-) diff --git a/src/rt/minfo.d b/src/rt/minfo.d index 3d1350a7fe..4b7b27ac0e 100644 --- a/src/rt/minfo.d +++ b/src/rt/minfo.d @@ -256,28 +256,64 @@ distloop: // find all the non-trivial dependencies (that is, dependencies that have a // ctor or dtor) of a given module. Doing this, we can 'skip over' the // trivial modules to get at the non-trivial ones. - size_t _findDependencies(int idx, bool orig = true) + size_t _findDependencies(int idx) { + static struct stackFrame + { + int curMod; + int curDep; + } + + // initialize "stack" + auto stack = cast(stackFrame *)alloca(stackFrame.sizeof * len); + auto stacktop = stack + len; + auto sp = stack; + sp.curMod = idx; + sp.curDep = 0; size_t result = 0; - foreach (m; _modules[idx].importedModules) + + for(;;) { - auto midx = findModule(m); + auto m = _modules[sp.curMod]; + if(sp.curDep >= m.importedModules.length) + { + // return + if(sp == stack) + // finished the algorithm + break; + --sp; + ++sp.curDep; + continue; + } + auto midx = findModule(m.importedModules[sp.curDep]); // if midx is -1, then this isn't part of this DSO. - if(midx != -1) - if(!bts(reachable, midx)) + if(midx != -1 && !bts(reachable, midx)) + { + if(bt(relevant, midx)) { - if(bt(relevant, midx)) - { - // non-trivial, stop here - result += 1; - } - else if(!bt(ctordone, midx)) + // non-trivial, stop here + result += 1; + } + else if(!bt(ctordone, midx)) + { + // non-relevant, and hasn't been exhaustively processed, recurse. + if(!bt(ctorstart, midx)) + ++result; + // recurse + if(++sp >= stacktop) { - // non-relevant, and hasn't been exhaustively processed, recurse. - result += (!bt(ctorstart, midx) ? 1 : 0) + - _findDependencies(midx, false); + // stack overflow, this shouldn't happen. + import core.internal.abort : abort; + abort("stack overflow on dependency search"); } + sp.curMod = midx; + sp.curDep = 0; + continue; } + } + + // next dependency + ++sp.curDep; } return result; } From 3de899d9817fb1eb9e1c91af9f67adf314c81552 Mon Sep 17 00:00:00 2001 From: Steven Schveighoffer Date: Wed, 20 Jul 2016 10:57:17 -0400 Subject: [PATCH 06/11] Change pruning of non-relevant nodes to be at deepest level, actually simplifies a lot. Instead of reusing reachable and allocating deps, just allocate new reachable array at each recursion. --- src/rt/minfo.d | 156 +++++++++++++++++++++++-------------------------- 1 file changed, 72 insertions(+), 84 deletions(-) diff --git a/src/rt/minfo.d b/src/rt/minfo.d index 4b7b27ac0e..03b08b855c 100644 --- a/src/rt/minfo.d +++ b/src/rt/minfo.d @@ -107,7 +107,6 @@ struct ModuleGroup auto ctorstart = cast(size_t*)alloca(flagbytes);// ctor/dtor seen auto ctordone = cast(size_t*)alloca(flagbytes);// ctor/dtor processed auto relevant = cast(size_t*)alloca(flagbytes);// has ctors/dtors - auto reachable = cast(size_t*)alloca(flagbytes);// used for reachability check void clearFlags(size_t *flags) { @@ -231,9 +230,10 @@ distloop: // set up all the arrays. Use the GC, we are going to exit anyway. distance.length = len; edges.length = len; + auto reachable = (new size_t[](nwords)).ptr; foreach(i, m; _modules) { - // use reachable, because an import can appear more than once. + // use bit array to prevent duplicates // https://issues.dlang.org/show_bug.cgi?id=16208 clearFlags(reachable); foreach(e; m.importedModules) @@ -256,7 +256,9 @@ distloop: // find all the non-trivial dependencies (that is, dependencies that have a // ctor or dtor) of a given module. Doing this, we can 'skip over' the // trivial modules to get at the non-trivial ones. - size_t _findDependencies(int idx) + // + // If a cycle is detected, returns the index of the module that completes the cycle. + int findDeps(int idx, size_t *reachable) { static struct stackFrame { @@ -270,7 +272,10 @@ distloop: auto sp = stack; sp.curMod = idx; sp.curDep = 0; - size_t result = 0; + + // initialize reachable by flagging source module + clearFlags(reachable); + bts(reachable, idx); for(;;) { @@ -282,40 +287,44 @@ distloop: // finished the algorithm break; --sp; - ++sp.curDep; - continue; } - auto midx = findModule(m.importedModules[sp.curDep]); - // if midx is -1, then this isn't part of this DSO. - if(midx != -1 && !bts(reachable, midx)) + else { - if(bt(relevant, midx)) + auto midx = findModule(m.importedModules[sp.curDep]); + // if midx is -1, then this isn't part of this DSO. + if(midx != -1 && !bts(reachable, midx)) { - // non-trivial, stop here - result += 1; - } - else if(!bt(ctordone, midx)) - { - // non-relevant, and hasn't been exhaustively processed, recurse. - if(!bt(ctorstart, midx)) - ++result; - // recurse - if(++sp >= stacktop) + if(bt(relevant, midx)) + { + // need to process this node, don't recurse. + if(bt(ctorstart, midx)) + { + // was already started, this is a cycle. + return midx; + } + } + else if(!bt(ctordone, midx)) { - // stack overflow, this shouldn't happen. - import core.internal.abort : abort; - abort("stack overflow on dependency search"); + // non-relevant, and hasn't been exhaustively processed, recurse. + if(++sp >= stacktop) + { + // stack overflow, this shouldn't happen. + import core.internal.abort : abort; + abort("stack overflow on dependency search"); + } + sp.curMod = midx; + sp.curDep = 0; + continue; } - sp.curMod = midx; - sp.curDep = 0; - continue; } } // next dependency ++sp.curDep; } - return result; + + // no cycles seen + return -1; } // The list of constructors that will be returned by the sorting. @@ -331,82 +340,61 @@ distloop: // Each call into this function is given a module that has static // ctor/dtors that must be dealt with. It recurses only when it finds // dependencies that also have static ctor/dtors. - void _checkModCtors2(int curidx) + void processMod(int curidx) { assert(curidx != -1); immutable ModuleInfo* current = _modules[curidx]; - // First, determine what non-trivial elements are reachable. - clearFlags(reachable); - bts(reachable, curidx); - auto nmodules = _findDependencies(curidx); - - // allocate the dependencies on the stack, because reachable will - // be reused as we recurse. - auto p = cast(int *)alloca(nmodules * int.sizeof); - auto dependencies = p[0 .. nmodules]; - uint depidx = 0; - // fill in the dependencies - foreach (int i; 0 .. len) + // First, determine what modules are reachable. + auto reachable = cast(size_t *)alloca(flagbytes); + auto cycleIdx = findDeps(curidx, reachable); + if(cycleIdx != -1) { - if(i != curidx && bt(reachable, i)) + auto cycleMod = _modules[cycleIdx]; + // found a cycle + println("Cyclic dependency between module ", cycleMod.name, " and ", current.name); + genPath(cycleIdx, curidx); + + foreach(midx; cyclePath[0 .. $-1]) { - if(bt(relevant, i)) - { - if(bt(ctorstart, i)) - { - // found a cycle - println("Cyclic dependency between module ", _modules[i].name, " and ", current.name); - genPath(i, curidx); - - foreach(midx; cyclePath[0 .. $-1]) - { - println(_modules[midx].name, bt(relevant, midx) ? "* ->" : " ->"); - } - println(_modules[i].name, "*"); - throw new Error(errmsg); - } - dependencies[depidx++] = i; - } - else if(!bts(ctorstart, i)) - { - // this is a non-relevant module, but it has never been - // imported by any other relevant module in our search. - // Once we have processed the current relevant module, - // we can mark this non-relevant module as done, so it's - // not ever looked at again. - dependencies[depidx++] = i; - } + println(_modules[midx].name, bt(relevant, midx) ? "* ->" : " ->"); } + println(cycleMod.name, "*"); + throw new Error(errmsg); } - assert(depidx == nmodules); - // process all the dependencies + // process the dependencies. First, we process all relevant ones bts(ctorstart, curidx); - foreach (m; dependencies) + foreach (int i; 0 .. len) { - if(bt(relevant, m) && !bt(ctordone, m)) - _checkModCtors2(m); + if(i != curidx && bt(reachable, i) && + bt(relevant, i) && !bt(ctordone, i)) + { + assert(!bt(ctorstart, i)); // sanity check, this should have been flagged a cycle earlier + processMod(i); + } } + + // now mark this node, and all nodes reachable from this module as done. bts(ctordone, curidx); btr(ctorstart, curidx); - - // mark all non-relevant dependencies as done - foreach(m; dependencies) + foreach (int i; 0 .. len) { - if(!bt(relevant, m)) + if(bt(reachable, i)) { - bts(ctordone, m); - // no need to clear ctorstart, as we don't care about - // irrelevant modules for detecting cycles. + // Since relevant dependencies are already marked as done + // from recursion above, no reason to check for relevance, + // that is a wasted op. + bts(ctordone, i); } } + // add this module to the construction order list ctors[ctoridx++] = current; } immutable(ModuleInfo)*[] - _checkModCtors3(int function(immutable ModuleInfo *) getf) + doSort(int function(immutable ModuleInfo *) getf) { clearFlags(relevant); clearFlags(ctorstart); @@ -434,13 +422,13 @@ distloop: } } - // now run the search in the relevant ones + // now run the algorithm in the relevant ones foreach (int idx, m; _modules) { if(bt(relevant, idx)) { if(!bt(ctordone, idx)) - _checkModCtors2(idx); + processMod(idx); } } @@ -448,8 +436,8 @@ distloop: } // finally, do the sorting for both shared and tls ctors. - _ctors = _checkModCtors3((immutable ModuleInfo * m) => (m.flags & MIstandalone) | ((m.dtor || m.ctor)? MIctor : 0)); - _tlsctors = _checkModCtors3((immutable ModuleInfo * m) => (m.flags & MIstandalone) | ((m.tlsdtor || m.tlsctor)? MIctor : 0)); + _ctors = doSort((immutable ModuleInfo * m) => (m.flags & MIstandalone) | ((m.dtor || m.ctor)? MIctor : 0)); + _tlsctors = doSort((immutable ModuleInfo * m) => (m.flags & MIstandalone) | ((m.tlsdtor || m.tlsctor)? MIctor : 0)); } void runCtors() From e8e34a14b6d449fbf268d00ee122e7ba5ed7c145 Mon Sep 17 00:00:00 2001 From: Steven Schveighoffer Date: Sun, 7 Aug 2016 21:35:28 -0400 Subject: [PATCH 07/11] Applied dfmt --- src/rt/minfo.d | 133 +++++++++++++++++++++++++------------------------ 1 file changed, 68 insertions(+), 65 deletions(-) diff --git a/src/rt/minfo.d b/src/rt/minfo.d index 03b08b855c..bf654d1d29 100644 --- a/src/rt/minfo.d +++ b/src/rt/minfo.d @@ -62,53 +62,56 @@ struct ModuleGroup */ void sortCtors() { - import core.bitop: bts, btr, bt; - debug(printModuleDependencies) + import core.bitop : bts, btr, bt; + + debug (printModuleDependencies) { import core.stdc.stdio : printf; - foreach(_m; _modules) + + foreach (_m; _modules) { - printf("%s%s%s:", _m.name.ptr, (_m.flags & MIstandalone) ? "+".ptr : "".ptr, (_m.flags & (MIctor | MIdtor)) ? "*".ptr : "".ptr); - foreach(_i; _m.importedModules) + printf("%s%s%s:", _m.name.ptr, (_m.flags & MIstandalone) + ? "+".ptr : "".ptr, (_m.flags & (MIctor | MIdtor)) ? "*".ptr : "".ptr); + foreach (_i; _m.importedModules) printf(" %s", _i.name.ptr); printf("\n"); } } - immutable uint len = cast(uint)_modules.length; - if(!len) + immutable uint len = cast(uint) _modules.length; + if (!len) return; // nothing to do. immutable(ModuleInfo)* minMod = _modules[0]; immutable(ModuleInfo)* maxMod = _modules[0]; - foreach(m; _modules) + foreach (m; _modules) { - if(m < minMod) + if (m < minMod) minMod = m; - if(m > maxMod) + if (m > maxMod) maxMod = m; } int findModule(in ModuleInfo* mi) { // short circuit linear search if possible. - if(mi < minMod || mi > maxMod) + if (mi < minMod || mi > maxMod) return -1; // binary search would be nice... foreach (i, m; _modules) - if (m is mi) return cast(int)i; + if (m is mi) + return cast(int) i; return -1; } - // allocate some stack arrays that will be used throughout the process. immutable nwords = (len + 8 * size_t.sizeof - 1) / (8 * size_t.sizeof); immutable flagbytes = nwords * size_t.sizeof; - auto ctorstart = cast(size_t*)alloca(flagbytes);// ctor/dtor seen - auto ctordone = cast(size_t*)alloca(flagbytes);// ctor/dtor processed - auto relevant = cast(size_t*)alloca(flagbytes);// has ctors/dtors + auto ctorstart = cast(size_t*) alloca(flagbytes); // ctor/dtor seen + auto ctordone = cast(size_t*) alloca(flagbytes); // ctor/dtor processed + auto relevant = cast(size_t*) alloca(flagbytes); // has ctors/dtors - void clearFlags(size_t *flags) + void clearFlags(size_t* flags) { memset(flags, 0, flagbytes); } @@ -130,7 +133,7 @@ struct ModuleGroup void println(string[] msgs...) { print(msgs); - version(Windows) + version (Windows) print("\r\n"); else print("\n"); @@ -153,21 +156,21 @@ struct ModuleGroup distance[] = int.max; int curdist = 0; distance[start] = 0; - while(true) + while (true) { bool done = true; - foreach(i, x; distance) + foreach (i, x; distance) { - if(x == curdist) + if (x == curdist) { - if(i == target) + if (i == target) { done = true; break; } - foreach(n; edges[i]) + foreach (n; edges[i]) { - if(distance[n] == int.max) + if (distance[n] == int.max) { distance[n] = curdist + 1; done = false; @@ -175,14 +178,14 @@ struct ModuleGroup } } } - if(done) + if (done) break; ++curdist; } // it should be impossible to not get to target, this is just a // sanity check. Not an assert, because druntime is compiled in // release mode. - if(distance[target] != curdist) + if (distance[target] != curdist) { throw new Error("internal error printing module cycle"); } @@ -191,23 +194,23 @@ struct ModuleGroup // follow the edges in reverse to get back to the original. We // don't have a reverse mapping, so it takes a bit of looping. cyclePath.length += curdist; - auto subpath = cyclePath[$-curdist .. $]; - while(true) + auto subpath = cyclePath[$ - curdist .. $]; + while (true) { --curdist; subpath[curdist] = target; - if(curdist == 0) + if (curdist == 0) break; -distloop: + distloop: // search for next (previous) module in cycle. - foreach(int m, d; distance) + foreach (int m, d; distance) { - if(d == curdist) + if (d == curdist) { // determine if m can reach target - foreach(e; edges[m]) + foreach (e; edges[m]) { - if(e == target) + if (e == target) { // recurse target = m; @@ -231,16 +234,16 @@ distloop: distance.length = len; edges.length = len; auto reachable = (new size_t[](nwords)).ptr; - foreach(i, m; _modules) + foreach (i, m; _modules) { // use bit array to prevent duplicates // https://issues.dlang.org/show_bug.cgi?id=16208 clearFlags(reachable); - foreach(e; m.importedModules) + foreach (e; m.importedModules) { auto impidx = findModule(e); - if(impidx != -1 && impidx != i) - if(!bts(reachable, impidx)) + if (impidx != -1 && impidx != i) + if (!bts(reachable, impidx)) edges[i] ~= impidx; } } @@ -258,7 +261,7 @@ distloop: // trivial modules to get at the non-trivial ones. // // If a cycle is detected, returns the index of the module that completes the cycle. - int findDeps(int idx, size_t *reachable) + int findDeps(int idx, size_t* reachable) { static struct stackFrame { @@ -267,7 +270,7 @@ distloop: } // initialize "stack" - auto stack = cast(stackFrame *)alloca(stackFrame.sizeof * len); + auto stack = cast(stackFrame*) alloca(stackFrame.sizeof * len); auto stacktop = stack + len; auto sp = stack; sp.curMod = idx; @@ -277,14 +280,13 @@ distloop: clearFlags(reachable); bts(reachable, idx); - for(;;) + for (;;) { auto m = _modules[sp.curMod]; - if(sp.curDep >= m.importedModules.length) + if (sp.curDep >= m.importedModules.length) { // return - if(sp == stack) - // finished the algorithm + if (sp == stack) // finished the algorithm break; --sp; } @@ -292,24 +294,25 @@ distloop: { auto midx = findModule(m.importedModules[sp.curDep]); // if midx is -1, then this isn't part of this DSO. - if(midx != -1 && !bts(reachable, midx)) + if (midx != -1 && !bts(reachable, midx)) { - if(bt(relevant, midx)) + if (bt(relevant, midx)) { // need to process this node, don't recurse. - if(bt(ctorstart, midx)) + if (bt(ctorstart, midx)) { // was already started, this is a cycle. return midx; } } - else if(!bt(ctordone, midx)) + else if (!bt(ctordone, midx)) { // non-relevant, and hasn't been exhaustively processed, recurse. - if(++sp >= stacktop) + if (++sp >= stacktop) { // stack overflow, this shouldn't happen. import core.internal.abort : abort; + abort("stack overflow on dependency search"); } sp.curMod = midx; @@ -332,7 +335,6 @@ distloop: // current element being inserted into ctors list. size_t ctoridx = 0; - // This function will determine the order of construction/destruction and // check for cycles. If a cycle is found, the cycle path is transformed // into a string and thrown as an error. @@ -346,16 +348,16 @@ distloop: immutable ModuleInfo* current = _modules[curidx]; // First, determine what modules are reachable. - auto reachable = cast(size_t *)alloca(flagbytes); + auto reachable = cast(size_t*) alloca(flagbytes); auto cycleIdx = findDeps(curidx, reachable); - if(cycleIdx != -1) + if (cycleIdx != -1) { auto cycleMod = _modules[cycleIdx]; // found a cycle println("Cyclic dependency between module ", cycleMod.name, " and ", current.name); genPath(cycleIdx, curidx); - foreach(midx; cyclePath[0 .. $-1]) + foreach (midx; cyclePath[0 .. $ - 1]) { println(_modules[midx].name, bt(relevant, midx) ? "* ->" : " ->"); } @@ -367,8 +369,7 @@ distloop: bts(ctorstart, curidx); foreach (int i; 0 .. len) { - if(i != curidx && bt(reachable, i) && - bt(relevant, i) && !bt(ctordone, i)) + if (i != curidx && bt(reachable, i) && bt(relevant, i) && !bt(ctordone, i)) { assert(!bt(ctorstart, i)); // sanity check, this should have been flagged a cycle earlier processMod(i); @@ -380,7 +381,7 @@ distloop: btr(ctorstart, curidx); foreach (int i; 0 .. len) { - if(bt(reachable, i)) + if (bt(reachable, i)) { // Since relevant dependencies are already marked as done // from recursion above, no reason to check for relevance, @@ -393,8 +394,7 @@ distloop: ctors[ctoridx++] = current; } - immutable(ModuleInfo)*[] - doSort(int function(immutable ModuleInfo *) getf) + immutable(ModuleInfo)*[] doSort(int function(immutable ModuleInfo*) getf) { clearFlags(relevant); clearFlags(ctorstart); @@ -406,11 +406,12 @@ distloop: foreach (int idx, m; _modules) { // TODO: Should null ModuleInfo be allowed? - if (m is null) continue; + if (m is null) + continue; auto flag = getf(m); - if(flag & MIctor) + if (flag & MIctor) { - if(flag & MIstandalone) + if (flag & MIstandalone) { // can run at any time. Just run it first. ctors[ctoridx++] = m; @@ -425,9 +426,9 @@ distloop: // now run the algorithm in the relevant ones foreach (int idx, m; _modules) { - if(bt(relevant, idx)) + if (bt(relevant, idx)) { - if(!bt(ctordone, idx)) + if (!bt(ctordone, idx)) processMod(idx); } } @@ -436,8 +437,10 @@ distloop: } // finally, do the sorting for both shared and tls ctors. - _ctors = doSort((immutable ModuleInfo * m) => (m.flags & MIstandalone) | ((m.dtor || m.ctor)? MIctor : 0)); - _tlsctors = doSort((immutable ModuleInfo * m) => (m.flags & MIstandalone) | ((m.tlsdtor || m.tlsctor)? MIctor : 0)); + _ctors = doSort((immutable ModuleInfo* m) => (m.flags & MIstandalone) | ((m.dtor + || m.ctor) ? MIctor : 0)); + _tlsctors = doSort((immutable ModuleInfo* m) => (m.flags & MIstandalone) | ((m.tlsdtor + || m.tlsctor) ? MIctor : 0)); } void runCtors() From bd1ebb544831023a61b84254612c8b6f975a6104 Mon Sep 17 00:00:00 2001 From: Steven Schveighoffer Date: Mon, 8 Aug 2016 10:56:46 -0400 Subject: [PATCH 08/11] Address all critique. --- src/core/bitop.d | 137 ++++++++++++++++++++ src/rt/minfo.d | 330 +++++++++++++++++++++++------------------------ 2 files changed, 301 insertions(+), 166 deletions(-) diff --git a/src/core/bitop.d b/src/core/bitop.d index 716cc9d83f..a29aef9156 100644 --- a/src/core/bitop.d +++ b/src/core/bitop.d @@ -355,6 +355,143 @@ int bts(size_t* p, size_t bitnum) pure @system; assert(array[1] == 0x100); } +/** + * Range over bit set. Each element is the bit number that is set. + * + * This is more efficient than testing each bit in a sparsely populated bit + * set. Note that the first bit in the bit set would be bit 0. + */ +struct BitRange +{ + /// Number of bits in each size_t + enum bitsPerWord = size_t.sizeof * 8; + + private + { + const(size_t)*bits; // points at next word of bits to check + size_t cur; // needed to allow searching bits using bsf + size_t idx; // index of current set bit + size_t len; // number of bits in the bit set. + } + @nogc nothrow pure: + + /** + * Construct a BitRange. + * + * Params: + * bitarr - The array of bits to iterate over + * numBits - The total number of valid bits in the given bit array + */ + this(const(size_t)* bitarr, size_t numBits) @system + { + bits = bitarr; + len = numBits; + if (len) + { + // prime the first bit + cur = *bits++ ^ 1; + popFront(); + } + } + + /// Range functions + size_t front() + { + assert(!empty); + return idx; + } + + /// ditto + bool empty() const + { + return idx >= len; + } + + /// ditto + void popFront() @system + { + // clear the current bit + auto curbit = idx % bitsPerWord; + cur ^= size_t(1) << curbit; + if(!cur) + { + // find next size_t with set bit + idx -= curbit; + while (!cur) + { + if ((idx += bitsPerWord) >= len) + // now empty + return; + cur = *bits++; + } + idx += bsf(cur); + } + else + { + idx += bsf(cur) - curbit; + } + } +} + +/// +@system unittest +{ + import core.stdc.stdlib : malloc, free; + import core.stdc.string : memset; + + // initialize a bit array + enum nBytes = (100 + BitRange.bitsPerWord - 1) / 8; + size_t *bitArr = cast(size_t *)malloc(nBytes); + scope(exit) free(bitArr); + memset(bitArr, 0, nBytes); + + // set some bits + bts(bitArr, 48); + bts(bitArr, 24); + bts(bitArr, 95); + bts(bitArr, 78); + + enum sum = 48 + 24 + 95 + 78; + + // iterate + size_t testSum; + size_t nBits; + foreach(b; BitRange(bitArr, 100)) + { + testSum += b; + ++nBits; + } + + assert(testSum == sum); + assert(nBits == 4); +} + +@system unittest +{ + void testIt(size_t numBits, size_t[] bitsToTest...) + { + import core.stdc.stdlib : malloc, free; + import core.stdc.string : memset; + immutable numBytes = (numBits + size_t.sizeof * 8 - 1) / 8; + size_t* bitArr = cast(size_t *)malloc(numBytes); + scope(exit) free(bitArr); + memset(bitArr, 0, numBytes); + foreach(b; bitsToTest) + bts(bitArr, b); + auto br = BitRange(bitArr, numBits); + foreach(b; bitsToTest) + { + assert(!br.empty); + assert(b == br.front); + br.popFront(); + } + assert(br.empty); + } + + testIt(100, 0, 1, 31, 63, 85); + testIt(100, 6, 45, 89, 92, 99); +} + /** * Swaps bytes in a 4 byte uint end-to-end, i.e. byte 0 becomes * byte 3, byte 1 becomes byte 2, byte 2 becomes byte 1, byte 3 diff --git a/src/rt/minfo.d b/src/rt/minfo.d index bf654d1d29..bda5a11538 100644 --- a/src/rt/minfo.d +++ b/src/rt/minfo.d @@ -53,100 +53,41 @@ struct ModuleGroup return _modules; } - /****************************** - * Allocate and fill in _ctors[] and _tlsctors[]. - * Modules are inserted into the arrays in the order in which the constructors - * need to be run. - * Throws: - * Exception if it fails. - */ - void sortCtors() + // this function initializes the bookeeping necessary to create the + // cycle path, and then creates it. It is a precondition that src and + // target modules are involved in a cycle. + // + // The delegate is a helper to map module info pointers to index into the modules array + private void genCyclePath(ref int[] cyclePath, int srcidx, int targetidx, + scope int delegate(immutable(ModuleInfo)*) findModule) { - import core.bitop : bts, btr, bt; - - debug (printModuleDependencies) - { - import core.stdc.stdio : printf; - - foreach (_m; _modules) - { - printf("%s%s%s:", _m.name.ptr, (_m.flags & MIstandalone) - ? "+".ptr : "".ptr, (_m.flags & (MIctor | MIdtor)) ? "*".ptr : "".ptr); - foreach (_i; _m.importedModules) - printf(" %s", _i.name.ptr); - printf("\n"); - } - } - - immutable uint len = cast(uint) _modules.length; - if (!len) - return; // nothing to do. + import core.bitop : bt, btc, bts; - immutable(ModuleInfo)* minMod = _modules[0]; - immutable(ModuleInfo)* maxMod = _modules[0]; - foreach (m; _modules) - { - if (m < minMod) - minMod = m; - if (m > maxMod) - maxMod = m; - } - - int findModule(in ModuleInfo* mi) - { - // short circuit linear search if possible. - if (mi < minMod || mi > maxMod) - return -1; - // binary search would be nice... - foreach (i, m; _modules) - if (m is mi) - return cast(int) i; - return -1; - } - - // allocate some stack arrays that will be used throughout the process. - immutable nwords = (len + 8 * size_t.sizeof - 1) / (8 * size_t.sizeof); + // set up all the arrays. Use the GC, we are going to exit anyway. + int[] distance; + int[][] edges; + distance.length = _modules.length; + edges.length = _modules.length; + immutable nwords = (_modules.length + 8 * size_t.sizeof - 1) / (8 * size_t.sizeof); immutable flagbytes = nwords * size_t.sizeof; - auto ctorstart = cast(size_t*) alloca(flagbytes); // ctor/dtor seen - auto ctordone = cast(size_t*) alloca(flagbytes); // ctor/dtor processed - auto relevant = cast(size_t*) alloca(flagbytes); // has ctors/dtors + auto reachable = cast(size_t*) malloc(flagbytes); + scope (exit) + .free(reachable); - void clearFlags(size_t* flags) + foreach (i, m; _modules) { - memset(flags, 0, flagbytes); - } - - // use this to hold the error message. - string errmsg = null; - - void print(string[] msgs...) - { - foreach (m; msgs) + // use bit array to prevent duplicates + // https://issues.dlang.org/show_bug.cgi?id=16208 + memset(reachable, 0, flagbytes); + foreach (e; m.importedModules) { - // save to the error message. Note that if we are throwing an - // exception, we don't care about being careful with using - // stack memory. Just use the GC/runtime. - errmsg ~= m; + auto impidx = findModule(e); + if (impidx != -1 && impidx != i) + if (!bts(reachable, impidx)) + edges[i] ~= impidx; } } - void println(string[] msgs...) - { - print(msgs); - version (Windows) - print("\r\n"); - else - print("\n"); - } - - // this set of functions helps create a valid path between two - // interdependent modules. This is only used if a cycle is found, to - // print the cycle to the user. Therefore, we don't initialize the data - // until we have to. - int[] cyclePath; - int[] distance; - int[][] edges; - // determine the shortest path between two modules. Uses dijkstra // without a priority queue. (we can be a bit slow here, in order to // get a better printout). @@ -222,38 +163,74 @@ struct ModuleGroup } } - // this function initializes the bookeeping necessary to create the - // cycle path, and then creates it. It is a precondition that src and - // target modules are involved in a cycle - void genPath(int srcidx, int targetidx) + // a cycle starts with the source. + cyclePath ~= srcidx; + + // first get to the target + shortest(srcidx, targetidx); + // now get back. + shortest(targetidx, srcidx); + + } + + /****************************** + * Allocate and fill in _ctors[] and _tlsctors[]. + * Modules are inserted into the arrays in the order in which the constructors + * need to be run. + * Throws: + * Exception if it fails. + */ + void sortCtors() + { + import core.bitop : bts, btr, bt, BitRange; + import rt.util.container.hashtab; + + debug (printModuleDependencies) { - assert(srcidx != -1); - assert(targetidx != -1); - - // set up all the arrays. Use the GC, we are going to exit anyway. - distance.length = len; - edges.length = len; - auto reachable = (new size_t[](nwords)).ptr; - foreach (i, m; _modules) + import core.stdc.stdio : printf; + + foreach (_m; _modules) { - // use bit array to prevent duplicates - // https://issues.dlang.org/show_bug.cgi?id=16208 - clearFlags(reachable); - foreach (e; m.importedModules) - { - auto impidx = findModule(e); - if (impidx != -1 && impidx != i) - if (!bts(reachable, impidx)) - edges[i] ~= impidx; - } + printf("%s%s%s:", _m.name.ptr, (_m.flags & MIstandalone) + ? "+".ptr : "".ptr, (_m.flags & (MIctor | MIdtor)) ? "*".ptr : "".ptr); + foreach (_i; _m.importedModules) + printf(" %s", _i.name.ptr); + printf("\n"); } + } + + immutable uint len = cast(uint) _modules.length; + if (!len) + return; // nothing to do. + + // used for mapping module info to indexes. + HashTab!(immutable(ModuleInfo)*, int) modIndexes; + foreach (i, m; _modules) + modIndexes[m] = cast(int) i; - // a cycle starts with the source. - cyclePath ~= srcidx; - // first get to the target - shortest(srcidx, targetidx); - // now get back. - shortest(targetidx, srcidx); + int findModule(immutable(ModuleInfo)* mi) + { + if (auto idx = mi in modIndexes) + return *idx; + return -1; + } + + // allocate some stack arrays that will be used throughout the process. + immutable nwords = (len + 8 * size_t.sizeof - 1) / (8 * size_t.sizeof); + immutable flagbytes = nwords * size_t.sizeof; + auto ctorstart = cast(size_t*) malloc(flagbytes); // ctor/dtor seen + auto ctordone = cast(size_t*) malloc(flagbytes); // ctor/dtor processed + auto relevant = cast(size_t*) malloc(flagbytes); // has ctors/dtors + scope (exit) + { + .free(ctorstart); + .free(ctordone); + .free(relevant); + } + + void clearFlags(size_t* flags) + { + memset(flags, 0, flagbytes); } // find all the non-trivial dependencies (that is, dependencies that have a @@ -261,19 +238,21 @@ struct ModuleGroup // trivial modules to get at the non-trivial ones. // // If a cycle is detected, returns the index of the module that completes the cycle. - int findDeps(int idx, size_t* reachable) + int findDeps(size_t idx, size_t* reachable) { static struct stackFrame { - int curMod; - int curDep; + size_t curMod; + size_t curDep; } // initialize "stack" - auto stack = cast(stackFrame*) alloca(stackFrame.sizeof * len); + auto stack = cast(stackFrame*) malloc(stackFrame.sizeof * len); + scope (exit) + .free(stack); auto stacktop = stack + len; auto sp = stack; - sp.curMod = idx; + sp.curMod = cast(int) idx; sp.curDep = 0; // initialize reachable by flagging source module @@ -331,7 +310,7 @@ struct ModuleGroup } // The list of constructors that will be returned by the sorting. - immutable(ModuleInfo)*[] ctors; + immutable(ModuleInfo)** ctors; // current element being inserted into ctors list. size_t ctoridx = 0; @@ -342,34 +321,46 @@ struct ModuleGroup // Each call into this function is given a module that has static // ctor/dtors that must be dealt with. It recurses only when it finds // dependencies that also have static ctor/dtors. - void processMod(int curidx) + void processMod(size_t curidx) { - assert(curidx != -1); immutable ModuleInfo* current = _modules[curidx]; // First, determine what modules are reachable. - auto reachable = cast(size_t*) alloca(flagbytes); + auto reachable = cast(size_t*) malloc(flagbytes); + scope (exit) + .free(reachable); auto cycleIdx = findDeps(curidx, reachable); if (cycleIdx != -1) { auto cycleMod = _modules[cycleIdx]; // found a cycle - println("Cyclic dependency between module ", cycleMod.name, " and ", current.name); - genPath(cycleIdx, curidx); + + version (Windows) + enum EOL = "\r\n"; + else + enum EOL = "\n"; + + string errmsg = "Cyclic dependency between module " + ~ cycleMod.name ~ " and " ~ current.name ~ EOL; + int[] cyclePath; + genCyclePath(cyclePath, cycleIdx, cast(int) curidx, &findModule); foreach (midx; cyclePath[0 .. $ - 1]) { - println(_modules[midx].name, bt(relevant, midx) ? "* ->" : " ->"); + errmsg ~= _modules[midx].name; + errmsg ~= bt(relevant, midx) ? "* ->" ~ EOL : " ->" ~ EOL; } - println(cycleMod.name, "*"); - throw new Error(errmsg); + errmsg ~= cycleMod.name; + errmsg ~= "*" ~ EOL; + throw new Error(errmsg, __FILE__, __LINE__); } // process the dependencies. First, we process all relevant ones bts(ctorstart, curidx); - foreach (int i; 0 .. len) + auto brange = BitRange(reachable, len); + foreach (i; brange) { - if (i != curidx && bt(reachable, i) && bt(relevant, i) && !bt(ctordone, i)) + if (i != curidx && bt(relevant, i) && !bt(ctordone, i)) { assert(!bt(ctorstart, i)); // sanity check, this should have been flagged a cycle earlier processMod(i); @@ -379,39 +370,35 @@ struct ModuleGroup // now mark this node, and all nodes reachable from this module as done. bts(ctordone, curidx); btr(ctorstart, curidx); - foreach (int i; 0 .. len) + foreach (i; brange) { - if (bt(reachable, i)) - { - // Since relevant dependencies are already marked as done - // from recursion above, no reason to check for relevance, - // that is a wasted op. - bts(ctordone, i); - } + // Since relevant dependencies are already marked as done + // from recursion above, no reason to check for relevance, + // that is a wasted op. + bts(ctordone, i); } // add this module to the construction order list ctors[ctoridx++] = current; } - immutable(ModuleInfo)*[] doSort(int function(immutable ModuleInfo*) getf) + immutable(ModuleInfo)*[] doSort(size_t relevantFlags) { clearFlags(relevant); clearFlags(ctorstart); clearFlags(ctordone); // pre-allocate enough space to hold all modules. - ctors = (cast(immutable(ModuleInfo)**).malloc(len * (void*).sizeof))[0 .. len]; + ctors = (cast(immutable(ModuleInfo)**).malloc(len * (void*).sizeof)); ctoridx = 0; foreach (int idx, m; _modules) { // TODO: Should null ModuleInfo be allowed? if (m is null) continue; - auto flag = getf(m); - if (flag & MIctor) + if (m.flags & relevantFlags) { - if (flag & MIstandalone) + if (m.flags & MIstandalone) { // can run at any time. Just run it first. ctors[ctoridx++] = m; @@ -424,23 +411,21 @@ struct ModuleGroup } // now run the algorithm in the relevant ones - foreach (int idx, m; _modules) + foreach (idx; BitRange(relevant, len)) { - if (bt(relevant, idx)) - { - if (!bt(ctordone, idx)) - processMod(idx); - } + if (!bt(ctordone, idx)) + processMod(idx); } + ctors = cast(immutable(ModuleInfo)**).realloc(ctors, ctoridx * (void*).sizeof); + if (ctors is null) + assert(0); return ctors[0 .. ctoridx]; } // finally, do the sorting for both shared and tls ctors. - _ctors = doSort((immutable ModuleInfo* m) => (m.flags & MIstandalone) | ((m.dtor - || m.ctor) ? MIctor : 0)); - _tlsctors = doSort((immutable ModuleInfo* m) => (m.flags & MIstandalone) | ((m.tlsdtor - || m.tlsctor) ? MIctor : 0)); + _ctors = doSort(MIctor | MIdtor); + _tlsctors = doSort(MItlsctor | MItlsdtor); } void runCtors() @@ -656,7 +641,7 @@ unittest // if we are expecting sort to throw, don't throw because of unexpected // success! - if(!shouldThrow) + if (!shouldThrow) { foreach (m; mgroup._modules) assert(!(m.flags & (MIctorstart | MIctordone)), testname); @@ -693,7 +678,8 @@ unittest auto m1 = mockMI(MIstandalone | MIctor); auto m2 = mockMI(0); m1.setImports(&m0.mi); - checkExp("imported standalone => no dependency", false, [&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi]); + checkExp("imported standalone => no dependency", false, + [&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi]); } { @@ -701,7 +687,8 @@ unittest auto m1 = mockMI(MIstandalone | MIctor); auto m2 = mockMI(0); m0.setImports(&m1.mi); - checkExp("imported standalone => no dependency (2)", false, [&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi]); + checkExp("imported standalone => no dependency (2)", false, + [&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi]); } { @@ -710,7 +697,8 @@ unittest auto m2 = mockMI(0); m0.setImports(&m1.mi); m1.setImports(&m0.mi); - checkExp("standalone may have cycle", false, [&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi]); + checkExp("standalone may have cycle", false, + [&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi]); } { @@ -718,7 +706,8 @@ unittest auto m1 = mockMI(MIctor); auto m2 = mockMI(0); m1.setImports(&m0.mi); - checkExp("imported ctor => ordered ctors", false, [&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi], []); + checkExp("imported ctor => ordered ctors", false, + [&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi], []); } { @@ -726,7 +715,8 @@ unittest auto m1 = mockMI(MIctor); auto m2 = mockMI(0); m0.setImports(&m1.mi); - checkExp("imported ctor => ordered ctors (2)", false, [&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m0.mi], []); + checkExp("imported ctor => ordered ctors (2)", false, + [&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m0.mi], []); } { @@ -735,7 +725,8 @@ unittest auto m2 = mockMI(0); m0.setImports(&m1.mi); m1.setImports(&m0.mi); - assertThrown!Throwable(checkExp("", true, [&m0.mi, &m1.mi, &m2.mi]), "detects ctors cycles"); + assertThrown!Throwable(checkExp("", true, [&m0.mi, &m1.mi, &m2.mi]), + "detects ctors cycles"); } { @@ -745,7 +736,8 @@ unittest m0.setImports(&m2.mi); m1.setImports(&m2.mi); m2.setImports(&m0.mi, &m1.mi); - assertThrown!Throwable(checkExp("", true, [&m0.mi, &m1.mi, &m2.mi]), "detects cycle with repeats"); + assertThrown!Throwable(checkExp("", true, [&m0.mi, &m1.mi, &m2.mi]), + "detects cycle with repeats"); } { @@ -753,7 +745,8 @@ unittest auto m1 = mockMI(MIctor); auto m2 = mockMI(MItlsctor); m0.setImports(&m1.mi, &m2.mi); - checkExp("imported ctor/tlsctor => ordered ctors/tlsctors", false, [&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m0.mi], [&m2.mi]); + checkExp("imported ctor/tlsctor => ordered ctors/tlsctors", false, + [&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m0.mi], [&m2.mi]); } { @@ -761,7 +754,8 @@ unittest auto m1 = mockMI(MIctor); auto m2 = mockMI(MItlsctor); m0.setImports(&m1.mi, &m2.mi); - checkExp("imported ctor/tlsctor => ordered ctors/tlsctors (2)", false, [&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m0.mi], [&m2.mi, &m0.mi]); + checkExp("imported ctor/tlsctor => ordered ctors/tlsctors (2)", false, + [&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m0.mi], [&m2.mi, &m0.mi]); } { @@ -770,7 +764,8 @@ unittest auto m2 = mockMI(MItlsctor); m0.setImports(&m1.mi, &m2.mi); m2.setImports(&m0.mi); - checkExp("no cycle between ctors/tlsctors", false, [&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m0.mi], [&m2.mi]); + checkExp("no cycle between ctors/tlsctors", false, + [&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m0.mi], [&m2.mi]); } { @@ -779,7 +774,8 @@ unittest auto m2 = mockMI(MItlsctor); m0.setImports(&m2.mi); m2.setImports(&m0.mi); - assertThrown!Throwable(checkExp("", true, [&m0.mi, &m1.mi, &m2.mi]), "detects tlsctors cycle"); + assertThrown!Throwable(checkExp("", true, [&m0.mi, &m1.mi, &m2.mi]), + "detects tlsctors cycle"); } { @@ -789,7 +785,8 @@ unittest m0.setImports(&m1.mi); m1.setImports(&m0.mi, &m2.mi); m2.setImports(&m1.mi); - assertThrown!Throwable(checkExp("", true, [&m0.mi, &m1.mi, &m2.mi]), "detects tlsctors cycle with repeats"); + assertThrown!Throwable(checkExp("", true, [&m0.mi, &m1.mi, &m2.mi]), + "detects tlsctors cycle with repeats"); } { @@ -800,7 +797,8 @@ unittest m1.setImports(&m2.mi); m2.setImports(&m0.mi); // NOTE: this is implementation dependent, sorted order shouldn't be tested. - checkExp("closed ctors cycle", false, [&m0.mi, &m1.mi, &m2.mi], [&m1.mi, &m2.mi, &m0.mi]); + checkExp("closed ctors cycle", false, [&m0.mi, &m1.mi, &m2.mi], + [&m1.mi, &m2.mi, &m0.mi]); //checkExp("closed ctors cycle", false, [&m0.mi, &m1.mi, &m2.mi], [&m0.mi, &m1.mi, &m2.mi]); } } From 35ccbc1d3e90fb30602eb12315da65d36e248d17 Mon Sep 17 00:00:00 2001 From: Steven Schveighoffer Date: Mon, 8 Aug 2016 11:39:50 -0400 Subject: [PATCH 09/11] Realloc to size 0 data returns null, so avoid that situation. --- src/rt/minfo.d | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/rt/minfo.d b/src/rt/minfo.d index bda5a11538..6d0ff613fa 100644 --- a/src/rt/minfo.d +++ b/src/rt/minfo.d @@ -417,6 +417,13 @@ struct ModuleGroup processMod(idx); } + if (ctoridx == 0) + { + // no ctors in the list. + .free(ctors); + return null; + } + ctors = cast(immutable(ModuleInfo)**).realloc(ctors, ctoridx * (void*).sizeof); if (ctors is null) assert(0); From 89b1ef393bfe8c9bc8f2d6dee92fecfb899ed15e Mon Sep 17 00:00:00 2001 From: Steven Schveighoffer Date: Mon, 8 Aug 2016 11:42:42 -0400 Subject: [PATCH 10/11] Remove check for null module info -- this can't happen anymore. --- src/rt/minfo.d | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/rt/minfo.d b/src/rt/minfo.d index 6d0ff613fa..5a886fc5cf 100644 --- a/src/rt/minfo.d +++ b/src/rt/minfo.d @@ -393,9 +393,6 @@ struct ModuleGroup ctoridx = 0; foreach (int idx, m; _modules) { - // TODO: Should null ModuleInfo be allowed? - if (m is null) - continue; if (m.flags & relevantFlags) { if (m.flags & MIstandalone) From 50a40f7f5801abfc76793c320fe1433a1ff42afd Mon Sep 17 00:00:00 2001 From: Steven Schveighoffer Date: Thu, 11 Aug 2016 09:16:56 -0400 Subject: [PATCH 11/11] Realized there's no reason to pass cyclePath by reference. --- src/rt/minfo.d | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/rt/minfo.d b/src/rt/minfo.d index 5a886fc5cf..b30bc0847a 100644 --- a/src/rt/minfo.d +++ b/src/rt/minfo.d @@ -58,12 +58,13 @@ struct ModuleGroup // target modules are involved in a cycle. // // The delegate is a helper to map module info pointers to index into the modules array - private void genCyclePath(ref int[] cyclePath, int srcidx, int targetidx, + private int[] genCyclePath(int srcidx, int targetidx, scope int delegate(immutable(ModuleInfo)*) findModule) { import core.bitop : bt, btc, bts; // set up all the arrays. Use the GC, we are going to exit anyway. + int[] cyclePath; int[] distance; int[][] edges; distance.length = _modules.length; @@ -171,6 +172,7 @@ struct ModuleGroup // now get back. shortest(targetidx, srcidx); + return cyclePath; } /****************************** @@ -342,8 +344,7 @@ struct ModuleGroup string errmsg = "Cyclic dependency between module " ~ cycleMod.name ~ " and " ~ current.name ~ EOL; - int[] cyclePath; - genCyclePath(cyclePath, cycleIdx, cast(int) curidx, &findModule); + auto cyclePath = genCyclePath(cycleIdx, cast(int) curidx, &findModule); foreach (midx; cyclePath[0 .. $ - 1]) {