Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

fixup for cycle detection #1720

Merged
MartinNowak merged 2 commits into from
Dec 23, 2016
Merged

fixup for cycle detection #1720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ad304a
fixup for issue 16211 - Add deprecate feature for cycle detection as the
schveiguy Dec 22, 2016
7978252
change API to not require string oncycle handling (sortCtors is called
schveiguy Dec 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
266 changes: 255 additions & 11 deletions src/rt/minfo.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,26 +156,40 @@ struct ModuleGroup
* Allocate and fill in _ctors[] and _tlsctors[].
* Modules are inserted into the arrays in the order in which the constructors
* need to be run.
*
* Params:
* cycleHandling - string indicating option for cycle handling
* Throws:
* Exception if it fails.
*/
void sortCtors()
void sortCtors(string cycleHandling)
{
import core.bitop : bts, btr, bt, BitRange;
import rt.util.container.hashtab;
import rt.config : rt_configOption;

// used to unwind stack for printing deprecation message.
static class DeprecatedCycleException : Exception
{
this() { super(""); }
}
scope deprecation = new DeprecatedCycleException();

enum OnCycle
{
deprecate,
abort,
print,
ignore
}

auto onCycle = OnCycle.abort;
// Change default to .abort in 2.073
auto onCycle = OnCycle.deprecate;

switch(rt_configOption("oncycle")) with(OnCycle)
switch(cycleHandling) with(OnCycle)
{
case "deprecate":
onCycle = deprecate;
break;
case "abort":
onCycle = abort;
break;
Expand All @@ -190,7 +204,7 @@ struct ModuleGroup
break;
default:
// invalid cycle handling option.
throw new Error("DRT invalid cycle handling option: " ~ rt_configOption("oncycle"));
throw new Error("DRT invalid cycle handling option: " ~ cycleHandling);
}

debug (printModuleDependencies)
Expand Down Expand Up @@ -339,8 +353,7 @@ struct ModuleGroup
else
{
auto midx = edges[sp.curMod][sp.curDep];
// if midx is -1, then this isn't part of this DSO.
if (midx != -1 && !bts(reachable, midx))
if (!bts(reachable, midx))
Copy link
Member

@MartinNowak MartinNowak Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems correct but slightly unrelated (though you prolly double checked for the old implementation).
That means we could change the type of edges to uint, right?

Copy link
Member Author

@schveiguy schveiguy Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I realized this was a remnant from the implementation that still used the findModule function. I don't put cross-module dependencies as -1 in the edges array, so no point in checking for -1 here.

Copy link
Member Author

@schveiguy schveiguy Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That means we could change the type of edges to uint, right?

Hm... probably could. I don't think it's too important though, no chance of having over 2 billion modules to sort. I'm leery of unsigned math, too.

{
if (bt(relevant, midx))
{
Expand All @@ -350,7 +363,16 @@ struct ModuleGroup
// was already started, this is a cycle.
final switch(onCycle) with(OnCycle)
{
case deprecate:
// check with old algorithm
if(sortCtorsOld(edges))
{
// unwind to print deprecation message.
throw deprecation;
}
goto case abort; // fall through
case abort:

string errmsg = "";
buildCycleMessage(idx, midx, (string x) {errmsg ~= x;});
throw new Error(errmsg, __FILE__, __LINE__);
Expand Down Expand Up @@ -484,8 +506,215 @@ struct ModuleGroup
}

// finally, do the sorting for both shared and tls ctors.
_ctors = doSort(MIctor | MIdtor);
_tlsctors = doSort(MItlsctor | MItlsdtor);
try
{
_ctors = doSort(MIctor | MIdtor);
_tlsctors = doSort(MItlsctor | MItlsdtor);
}
catch(DeprecatedCycleException)
{
// print a warning
import core.stdc.stdio : fprintf, stderr;
fprintf(stderr, "Deprecation 16211 warning:\n"
~ "A cycle has been detected in your program that was undetected prior to DMD\n"
~ "2.072. This program will continue, but will not operate when using DMD 2.073\n"
~ "to compile. Use runtime option --DRT-oncycle=print to see the cycle details.\n");

}
}

/// ditto
void sortCtors()
{
import rt.config : rt_configOption;
sortCtors(rt_configOption("oncycle"));
}

/******************************
* This is the old ctor sorting algorithm that does not find all cycles.
*
* It is here to allow the deprecated behavior from the original algorithm
* until people have fixed their code.
*
* If no cycles are found, the _ctors and _tlsctors are replaced with the
* ones generated by this algorithm to preserve the old incorrect ordering
* behavior.
*
* Params:
* edges - The module edges as found in the `importedModules` member of
* each ModuleInfo. Generated in sortCtors.
* Returns:
* true if no cycle is found, false if one was.
*/
bool sortCtorsOld(int[][] edges)
{
immutable len = edges.length;
assert(len == _modules.length);

static struct StackRec
{
@property int mod()
{
return _mods[_idx];
}

int[] _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);
int[] initialEdges = (cast(int *)malloc(int.sizeof * len))[0 .. len];
if (!stack.ptr || ctorstart is null || ctordone is null || !initialEdges.ptr)
assert(0);
scope (exit)
{
.free(stack.ptr);
.free(ctorstart);
.free(ctordone);
.free(initialEdges.ptr);
}

// initialize the initial edges
foreach (int i, ref v; initialEdges)
v = i;

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

int[] mods = initialEdges;

size_t idx;
while (true)
{
while (idx < mods.length)
{
auto m = mods[idx];

if (bt(ctordone, m))
{
// this module has already been processed, skip
++idx;
continue;
}
else if (bt(ctorstart, m))
{
/* 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;
assert(sm >= 0);
if (bt(ctorstart, sm))
ctorInCycle = true;
}
assert(stack[start].mod == m);
if (ctorInCycle)
{
return false;
}
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
{
auto curmod = _modules[m];
if (curmod.flags & mask)
{
if (curmod.flags & MIstandalone || !edges[m].length)
{ // trivial ctor => sort in
ctors[cidx++] = curmod;
bts(ctordone, m);
}
else
{ // non-trivial ctor => defer
bts(ctorstart, m);
}
}
else // no ctor => mark as visited
{
bts(ctordone, m);
}

if (edges[m].length)
{
/* Internal runtime error, recursion exceeds number of modules.
*/
(stackidx < len) || assert(0);

// recurse
stack[stackidx++] = StackRec(mods, idx);
idx = 0;
mods = edges[m];
}
}
}

if (stackidx)
{ // pop old value from stack
--stackidx;
mods = stack[stackidx]._mods;
idx = stack[stackidx]._idx;
auto m = mods[idx++];
if (bt(ctorstart, m) && !bts(ctordone, m))
ctors[cidx++] = _modules[m];
}
else // done
break;
}
// store final number and shrink array
ctors = (cast(immutable(ModuleInfo)**).realloc(ctors.ptr, cidx * size_t.sizeof))[0 .. cidx];
return true;
}

/* Do two passes: ctor/dtor, tlsctor/tlsdtor
*/
immutable(ModuleInfo)*[] _ctors2;
immutable(ModuleInfo)*[] _tlsctors2;
auto result = sort(_ctors2, MIctor | MIdtor) && sort(_tlsctors2, MItlsctor | MItlsdtor);
if (result) // no cycle
{
// fall back to original ordering as part of the deprecation.
if(_ctors.ptr)
.free(_ctors.ptr);
_ctors = _ctors2;
if(_tlsctors.ptr)
.free(_tlsctors.ptr);
_tlsctors = _tlsctors2;
}
else
{
// free any allocated memory that will be forgotten
if (_ctors2.ptr)
.free(_ctors2.ptr);
if (_tlsctors2.ptr)
.free(_tlsctors2.ptr);
}
return result;
}

void runCtors()
Expand Down Expand Up @@ -691,13 +920,13 @@ unittest
return mi;
}

static void checkExp(string testname, bool shouldThrow,
static void checkExp2(string testname, bool shouldThrow, string oncycle,
immutable(ModuleInfo*)[] modules,
immutable(ModuleInfo*)[] dtors=null,
immutable(ModuleInfo*)[] tlsdtors=null)
{
auto mgroup = ModuleGroup(modules);
mgroup.sortCtors();
mgroup.sortCtors(oncycle);

// if we are expecting sort to throw, don't throw because of unexpected
// success!
Expand All @@ -710,6 +939,15 @@ unittest
}
}

static void checkExp(string testname, bool shouldThrow,
immutable(ModuleInfo*)[] modules,
immutable(ModuleInfo*)[] dtors=null,
immutable(ModuleInfo*)[] tlsdtors=null)
{
checkExp2(testname, shouldThrow, "abort", modules, dtors, tlsdtors);
}


{
auto m0 = mockMI(0);
auto m1 = mockMI(0);
Expand Down Expand Up @@ -787,6 +1025,9 @@ unittest
m1.setImports(&m0.mi);
assertThrown!Throwable(checkExp("", true, [&m0.mi, &m1.mi, &m2.mi]),
"detects ctors cycles");
assertThrown!Throwable(checkExp2("", true, "deprecate",
[&m0.mi, &m1.mi, &m2.mi]),
"detects ctors cycles (dep)");
}

{
Expand Down Expand Up @@ -836,6 +1077,9 @@ unittest
m2.setImports(&m0.mi);
assertThrown!Throwable(checkExp("", true, [&m0.mi, &m1.mi, &m2.mi]),
"detects tlsctors cycle");
assertThrown!Throwable(checkExp2("", true, "deprecate",
[&m0.mi, &m1.mi, &m2.mi]),
"detects tlsctors cycle (dep)");
}

{
Expand Down
8 changes: 5 additions & 3 deletions test/cycles/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
include ../common.mak

TESTS:=cycle_ignore cycle_abort cycle_print
TESTS:=cycle_ignore cycle_abort cycle_print cycle_deprecate

DIFF:=diff
SED:=sed
Expand All @@ -11,9 +11,11 @@ all: $(addprefix $(ROOT)/,$(addsuffix .done,$(TESTS)))
$(ROOT)/cycle_ignore.done: RETCODE=0
$(ROOT)/cycle_ignore.done: LINES=0
$(ROOT)/cycle_abort.done: RETCODE=1
$(ROOT)/cycle_abort.done: LINES=5
$(ROOT)/cycle_abort.done: LINES=7
$(ROOT)/cycle_print.done: RETCODE=0
$(ROOT)/cycle_print.done: LINES=8
$(ROOT)/cycle_print.done: LINES=6
$(ROOT)/cycle_deprecate.done: RETCODE=0
$(ROOT)/cycle_deprecate.done: LINES=4
$(ROOT)/%.done: $(ROOT)/test_cycles
@echo Testing $*
$(QUIET)$(TIMELIMIT)$(ROOT)/test_cycles --DRT-oncycle=$(patsubst cycle_%.done,%, $(notdir $@)) > $@ 2>&1; test $$? -eq $(RETCODE)
Expand Down
6 changes: 0 additions & 6 deletions test/cycles/src/mod2.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,6 @@ module mod2;
import mod1;
import mod3;

shared int x;
shared static this()
{
x = 2;
}

void main()
{
// do nothing
Expand Down