Bug 15832 – Crashing program when a helper template function is used to create a template struct

The code below crashes when compiled and run. The seemingly equivalent code obtained by commenting out `auto m = mock(dg)` and using `auto m = Mock!(typeof(dg)(dg)` instead works as intended. I can't get what's going on - it seems to have to do with the `_returns` variable. import std.traits; int delegate(int) dg; static this() { dg = (i) => i * 2; } struct MockScope(T) { this(T)(ref T oldFunc, T newFunc) { _oldFuncPtr = &oldFunc; _oldFunc = oldFunc; oldFunc = newFunc; } ~this() { *_oldFuncPtr = _oldFunc; } private: T* _oldFuncPtr; T _oldFunc; } struct Mock(T) { this(ref T func) { _returns = [ReturnType!T.init]; ReturnType!T inner(ParameterTypeTuple!T values) { auto ret = _returns[0]; return ret; } func = &inner; _scope = MockScope!(T)(func, &inner); } void returnValue(V...)(V value) { _returns.length = 0; foreach(v; value) _returns ~= v; } MockScope!T _scope; ReturnType!T[] _returns; } auto mock(T)(T f) { return Mock!T(f); } void main() { //auto m = Mock!(typeof(dg))(dg); //this works auto m = mock(dg); // this should be equivalent but crashes assert(dg(3) == 0); m.returnValue(42); assert(dg(3) == 42); }

(In reply to Atila Neves from comment #0) > struct MockScope(T) { > this(T)(ref T oldFunc, T newFunc) { > _oldFuncPtr = &oldFunc; [...] > } [...] > } > > struct Mock(T) { > this(ref T func) { [...] > _scope = MockScope!(T)(func, &inner); > } [...] > } > > auto mock(T)(T f) { > return Mock!T(f); > } > > void main() { > //auto m = Mock!(typeof(dg))(dg); //this works > auto m = mock(dg); // this should be equivalent but crashes [...] > } mock's parameter f is not ref. So it's constructing a Mock/MockScope that references its local f. Once mock returns, the local f ceases to exists. Dereferencing the pointer to it is bound to fail then. When you change mock's parameter to ref, it no longer crashes in that way. But it doesn't work either. It then boils down to this: ---- struct Mock { this(ref int delegate() func) { func = {return _return;}; } int _return = 41; } Mock mock(ref int delegate() f) { return Mock(f); } void main() { int delegate() dg; version (good) auto m = Mock(dg); // works else auto m = mock(dg); // fails assert(dg() == 41); m._return = 42; assert(dg() == 42); } ---- I think I can see the problem here, and it's similar to be above: When mock returns, the Mock is copied. But the delegate that's created in the constructor references the old, temporary location. When mock returns, the delegate gets invalidated. It references garbage from then on. As far as I see, everything works as expected here, so I'm closing this as invalid. Please reopen if I have missed something, or if you think that something should work differently.

You're right, it's not a bug. I should probably not file things late at night when I can't think anymore. The ref thing should've been obvious. However, even with ref, the explanation isn't quite right: the delegate created in the constructor doesn't reference garbage - the GC should keep the memory alive. It just references the old slice. The issue is that the delegate references a slice that gets copied when `mock` exits. It's fine as long as the pointer in the copied slice gets reallocated, but that's exactly what I was doing. I guess I was also surprised (because of C++) that the Mock struct got copied at all. I'm so used to RVO I don't think about it anymore.

Ok, I've looked into it a bit more and I think it's a bug. I @disabled this(this) and it still happens. The worse part is the length of the slice seems to be garbage as well: this(ref T func) { _returns = [ReturnType!T.init]; ReturnType!T inner(ParameterTypeTuple!T values) { auto ret = _returns[0]; import std.conv: to; assert(_returns.length < 10, "Weird length: " ~ _returns.length.to!string); // the next line causes length to be incredibly large, i.e. gargabe if(_returns.length > 1) _returns = _returns[1..$]; return ret; } func = &inner; _scope = MockScope!(T)(func, &inner); }

(In reply to Atila Neves from comment #2) > However, even with ref, the explanation isn't quite right: the delegate > created in the constructor doesn't reference garbage - the GC should keep > the memory alive. I think I was a tiny bit off, but not by much. I did some more testing and now I think this is what happens: A closure is allocated, but that closure references the field of the struct on the stack. And that field is part of a local variable, of course. So, the delegate references a local variable through two indirections. That way, stuff like this works: ---- struct S { int field; int delegate() makeFieldGetter() { return {return field;}; } } void main() { S s; s.field = 1; auto getter = s.makeFieldGetter(); assert(getter() == 1); /* passes */ s.field = 2; assert(getter() == 2); /* passes, too */ } ---- If the closure would make a copy the field on the heap, then the second assert wouldn't pass. I don't know if this is the best way to handle the situation, or if it's horribly broken, or something in between. (In reply to Atila Neves from comment #3) > Ok, I've looked into it a bit more and I think it's a bug. I @disabled > this(this) and it still happens. I think "moving" from one location to another is still allowed with a disabled this(this). And "moving" means: make a copy, don't run the postblit, destroy the source. So dmd is probably complying with the spec when it moves a non-copyable struct instead of constructing it at the destination site. I'm not sure, though, and dmd seems to be quite fickle about when it makes a copy and when not: When I change `mock`'s implementation to `auto m = Mock!T(f); return m;`, then dmd seems to not make a copy. So, yeah, this may well be a bug in the implementation, but I'm not sure if the current behavior is against the spec.

At least there's a workaround: auto m = Mock!T(...); return m; I believe that the fact that this behaviour is different from the original implementation is a compiler bug and would ask for this to reopened. Also, the memory corruption that resulted from my original implementation goes well beyong a delegate referencing gargabe. It was really weird. Atila

(In reply to Atila Neves from comment #5) > At least there's a workaround: > > auto m = Mock!T(...); > return m; > > I believe that the fact that this behaviour is different from the original > implementation is a compiler bug and would ask for this to reopened. I'm not sure if it's strictly a bug (against the spec), or if you're relying on an implementation detail when you assume construction at the destination. Either way, I agree that it's weird how adding a temporary makes the copy go away. I see a reasonable enhancement request, at least. You can reopen issues yourself. However, in this case I think we better open a new one. Did that here: issue 15842. I've added you to the CC list. > Also, the memory corruption that resulted from my original implementation > goes well beyong a delegate referencing gargabe. It was really weird. If there's anything else going on, could you provide a test case where that's more obvious? Or maybe point out how the additional corruption is visible in the code samples we have so far.

In the following code, the length is garbage, and uncommenting an assert makes the problem move! import std.traits; import std.conv; int delegate(int) dg; static this() { dg = i => i * 2; } struct MockScope(T) { this(T)(ref T oldFunc, T newFunc) { _oldFuncPtr = &oldFunc; _oldFunc = oldFunc; oldFunc = newFunc; } ~this() { *_oldFuncPtr = _oldFunc; } private: T* _oldFuncPtr; T _oldFunc; } struct Mock(T) { this(ref T func) { _returns = [ReturnType!T.init]; ReturnType!T inner(ParameterTypeTuple!T values) { auto ret = _returns[0]; assert(_returns.length < 10, "Weird length1: " ~ _returns.length.to!string); if(_returns.length > 1) _returns = _returns[1..$]; assert(_returns.length < 10, "Weird length2: " ~ _returns.length.to!string); return ret; } func = &inner; _scope = MockScope!(T)(func, &inner); } void returnValue(V...)(V value) { _returns.length = 0; foreach(v; value) _returns ~= v; // uncomment below and things work assert(_returns.length < 10, "Weird length3: " ~ _returns.length.to!string); } MockScope!T _scope; ReturnType!T[] _returns; } auto mock(T)(ref T f) { return Mock!T(f); } void main() { //auto m = Mock!(typeof(dg))(dg); auto m = mock(dg); // this should be equivalent but crashes assert(dg(3) == 0); m.returnValue(42); assert(dg(3) == 42); }

(In reply to Atila Neves from comment #7) > In the following code, the length is garbage, and uncommenting an assert > makes the problem move! I think a garbage reference to the stack explains this well. Any change to the stack between the Mock construction and a call to dg affects what dg sees when it dereferences the garbage pointer. Adding/removing almost anything, including asserts, is going to change what's on the stack. You can also make the first assert in main fail. Or you can make the second one pass when trying hard enough: ---- /* ... as above ... */ void messWithTheStack() {int[][4] x = void; x[0][0] = 42;} void main() { //auto m = Mock!(typeof(dg))(dg); auto m = mock(dg); // this should be equivalent but crashes assert(dg(3) == 0); m.returnValue(42); messWithTheStack(); assert(dg(3) == 42); } ----