Bug 5212 – no escape analysis for typesafe variadic function arguments

With DMD 2.050 this D2 program usually asserts at run-time: class Foo { int[] args; this(int[] args_...) { args = args_; } } Foo foo() { return new Foo(1, 2, 3); // passes stack data to Foo } void main() { assert(foo().args == [1, 2, 3]); } That's the root cause of a bug found by a person in the D.learn group: http://www.digitalmars.com/webnews/newsgroups.php?art_group=digitalmars.D.learn&article_id=22697 Just adding a dup inside the this() avoids the bug. Also, the bug is avoided if Foo receives arguments like this: Foo foo() { return new Foo([1, 2, 3]); } So this nature of typesafe variadic arguments is bug-prone. There are various ways to avoid this problem. One possible solution is to modify the semantics of this kind of arguments pass, so the code inside the function foo always see an args array allocated on the heap (it may perform a run-time test, and dup the given arguments only if the data is on the stack, so in the second example it doesn't dup and saves a little of RAM and running time). To restore the original efficient semantics of the typesafe variadics a "scope" attribute may be added (as the one used to avoid a closure): class Foo { int[] args; this(scope int[] args_...) { // unsafe but fast args = args_; } } This is safer than the current semantics because, as usual in D design, the safe way is the built-in one and the faster one is on request. This is just one of the possible ways to avoid this problem.

> Just adding a dup inside the this() avoids the bug. Also, the bug is avoided if > Foo receives arguments like this: > > Foo foo() { > return new Foo([1, 2, 3]); > } Is it valid to pass them byref here? It's a nice performance trick, but... shouldn't arguments be on the stack?

Ideally, the compiler would to proper data flow analysis and raise an error if the reference to the variadic parameter array "escapes". Closures actually do something similar, although that case seems to be simpler.

(In reply to comment #1) > > Just adding a dup inside the this() avoids the bug. Also, the bug is avoided if > > Foo receives arguments like this: > > > > Foo foo() { > > return new Foo([1, 2, 3]); > > } > > Is it valid to pass them byref here? > It's a nice performance trick, but... shouldn't arguments be on the stack? Steven has pointed out that one can discern between variadic arguments and an array by creating an overload for array.

A quote from Denis: > I find a bit strange that the compiler accepts f([1,2,3]) > when the declaration reads eg void f(int[] ints...).

Extra note: It's a problem of perception: typesafe variadic arguments don't look like normal function arguments that you know are usually on the stack, they look like dynamic arrays, and in D most dynamic arrays are allocated on the heap (it's easy and useful to take a dynamic-array-slice of a stack allocated array, but in this case the code shows that the slice doesn't contain heap data). If your function has a signature similar to this one: void foo(int[3] arr...) { It's not too much hard to think that 'arr' is on the stack. But dynamic arrays don't give that image: void foo(int[] arr...) { This is why I think it's better for the compiler to test if the arr data is on the stack, and dup it otherwise (unless a 'scope' is present, in this case both the test and allocation aren't present).

Variadic arguments are arguments, and arguments are on the stack. T[] is not a dynamic array, it's a slice. Messing python idioms into D, while those idioms were not implemented in D, is a bug-prone way of thinking (misunderstanding).

One of the many posts of the relative thread: http://www.digitalmars.com/webnews/newsgroups.php?art_group=digitalmars.D&article_id=122157 (In reply to comment #6) > Variadic arguments are arguments, and arguments are on the stack. A dynamic array argument is an argument, it is on the stack, and most times its contents are on the heap. But not always. > T[] is not a dynamic array, it's a slice. This is not significant. What is significant is where the memory it points is located. > Messing python idioms into D, while > those idioms were not implemented in D, is a bug-prone way of thinking > (misunderstanding). Python wasn't part of this discussion.

At the very least, the compiler should be improved to perform proper data flow analysis for these kinds of cases, and detect the escaping reference to the on-stack array.

P.S. This bug still happens in latest git (dmd 2.061). Here's another test case: import std.conv; import std.stdio; class C { real[] val; this(real[] v...) { val = v; } override string toString() { return to!string(val); } } C f() { return new C(1.0); } void main() { auto e = f(); writeln(e); }

*** Issue 9527 has been marked as a duplicate of this issue. ***

Without escape analysis these functions are not memory @safe. Maybe we should think about this in conjunction with scope ref, http://wiki.dlang.org/DIP36#.40safe_concerns.

This topic was discussed again in D.learn: http://forum.dlang.org/thread/[email protected] An improvement of my idea is that when you use "scope": this(scope int[] args_...) { The compiler doesn't perfom the dup (and doesn't allow you to escape the data).

I don't think that implicit .dup is the way to go here. Typesafe variadic functions are about efficiently passing multiple arguments.

(In reply to comment #13) > I don't think that implicit .dup is the way to go here. > Typesafe variadic functions are about efficiently passing multiple arguments. Generally safety triumphs over efficiency. So what solution do you propose? Automatically changing all functions with typesafe variadic arguements into @system is not safe, it's a breaking change, and it doesn't solve the problem. Perhaps a solution is to attach an implicit "scope" attribute too all typesafe variadic arguments, so this: class Foo { int[] args; this(int[] args_...) { } } Is seen by the compiler as: class Foo { int[] args; this(scope int[] args_...) { } } But even if this will work, currently scope is not well implemented.

Yes, they should be made scope. The specs already say something similar. > An implementation may construct the object or array instance on the stack. Therefore, it is an error to refer to that instance after the variadic function has returned > But even if this will work, currently scope is not well implemented. Yes, we're missing escape analysis but that's no argument for implicit heap copies.

We should probably enforce (or automatically) add scope to typesafe variadic arguments.

(In reply to Martin Nowak from comment #16) > We should probably enforce (or automatically) add scope to typesafe variadic > arguments. I guess enforcing an explicit scope annotation would make the deprecation a bit simpler, but not much.

Mentioning -dip1000 so Walter finds this.

Commits pushed to master at https://github.com/dlang/dmd https://github.com/dlang/dmd/commit/be4ad4b744f1649f6b010b13a5cbf96582786c46 fix Issue 5212 - no escape analysis for typesafe variadic function arguments https://github.com/dlang/dmd/commit/de449877d0dc9ba0eb2e799d7b178ebb6fe85c18 Merge pull request #7976 from WalterBright/fix5212 fix Issue 5212 - no escape analysis for typesafe variadic function ar…

*** Issue 18980 has been marked as a duplicate of this issue. ***

The original code compiles and asserts with DMD v2.080.1

And yes, I was able to compile the code with -dip1000

Compiling: ----- @safe: class Foo { int[] args; @safe this(int[] args_...) { args = args_; } } Foo foo() { return new Foo(1, 2, 3); // passes stack data to Foo } void main() { assert(foo().args == [1, 2, 3]); } ---- with -preview=DIP1000 yields: test.d(5): Error: scope variable `args_` assigned to non-scope `this.args` meaning the compiler is working correctly.