3560 – foreach over nested function generates wrong code

D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 3560 - foreach over nested function generates wrong code
Summary: foreach over nested function generates wrong code
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: Other All
: P2 critical
Assignee: No Owner
URL:
Keywords: patch, wrong-code
Depends on:
Blocks:
Reported: 2009年11月30日 09:44 UTC by Serg Kovrov
Modified: 2015年06月09日 05:11 UTC (History)
2 users (show)

See Also:


Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this issue.
Description Serg Kovrov 2009年11月30日 09:44:23 UTC
import std.stream: File;
import std.stdio: writefln;
void outer()
{
 auto auth_file = new File("path-to-existing-file");
 writefln("outer: 0x%X", &auth_file);
 
 int inner(int delegate(ref ubyte[]) dg)
 {
 writefln("inner: 0x%X", &auth_file);
 return 0;
 }
 foreach (entry; &inner)
 {
 //...
 }
}
void main()
{
 outer();
}
--------------------
expected output:
 outer: 0xB7D19E44
 inner: 0xB7D19E44
actual output:
outer: 0xB7D19E44
inner: 0x4
Comment 1 Don 2010年04月29日 02:50:11 UTC
Reduced test case (this example segfaults at runtime) shows it is related to varargs.
------------------------
void bug3560(int ...){ }
void main()
{
 int localvar = 7;
 bug3560(2); // this call must be made before the foreach
 int inner(int delegate(ref int) dg) {
 int k = localvar;
 return 0;
 }
 foreach (entry; &inner)
 {
 }
}
Comment 2 Don 2010年07月13日 15:12:13 UTC
Further reduction shows that it doesn't require varargs. Raising priority to critical, since it means that virtually all closures are broken.
What's happening: on the call to inner, EAX should hold a pointer to
the closure variables (on the heap). But when there's another function call, it's using EBX instead, and the EAX value is whatever garbage is left after the function call.
So it gets the calling convention wrong.
====================
void bug3560(int x){ }
void main()
{
 int localvar = 7;
 int inner(int delegate(ref int) dg) {
 int k = localvar; // BUG: &localvar == 0x24 (!)
 return 0;
 }
 bug3560(0x20);
 foreach (entry; &inner) { }
}
Comment 3 Don 2010年07月14日 12:41:39 UTC
It doesn't even need a function call, and doesn't need a closure.
Anything which modifies the EAX register before the foreach will do it.
This test case generates bad code (runtime segfault) even on D1 (even old versions, eg D1.020).
The inner function assumes that EAX contains the context pointer, but the foreach code doesn't set EAX. Instead, it's passing the context pointer in EBX.
=====================
void main()
{
 int localvar = 7;
 int inner(int delegate(ref int) dg) {
 int k = localvar;
 return 0;
 }
 int a = localvar * localvar; // This modifies the EAX register
 
 foreach (entry; &inner)
 {
 }
}
Comment 4 Don 2010年07月15日 14:42:34 UTC
This is a front-end problem.
ForeachStatement::semantic() immediately runs aggr->semantic(). 
In this case, aggr is an address of a nested function 'inner'.
AddrExp->semantic() turns it into a DelegateExp.
Later on in Foreach::semantic, if the aggregate is of delegate type, it wraps it in a CallExp, then runs CallExp::semantic. The problem is that CallExp assumes that semantic has NOT been run on its argument.
This works fine if the aggregate is a delegate variable. But if it's a delegate expression, CallExp transforms the DelegateExp for 'inner' into a DotVarExp(inner, inner). And then it's a mess. CallExp needs to be passed 'inner', not the delegateExp.
I can see two possible fixes. 
(1) Change CallExp so that changes DelegateExp(f,f) into CallExp(f), if f is a nested function, instead of changing it into a DotVarExp.
OR
(2) If it's a delegate expression for a nested function, call the function directly.
This patch implements the second method.
statement.c, Foreach::semantic(), line 1891
 else if (tab->ty == Tdelegate)
 {
 /* Call:
 * aggr(flde)
 */
 Expressions *exps = new Expressions();
 exps->push(flde);
+ if (aggr->op == TOKdelegate &&
+ ((DelegateExp *)aggr)->func->isNested())
+ e = new CallExp(loc, ((DelegateExp *)aggr)->e1, exps);
+ else
+ e = new CallExp(loc, aggr, exps);
- e = new CallExp(loc, aggr, exps);
 e = e->semantic(sc);
Comment 5 Walter Bright 2010年07月24日 16:24:48 UTC
http://www.dsource.org/projects/dmd/changeset/586 


AltStyle によって変換されたページ (->オリジナル) /