D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 2935 - ICE(out.c) using struct with constructor as function default argument
Summary: ICE(out.c) using struct with constructor as function default argument
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: x86 Windows
: P2 normal
Assignee: No Owner
URL:
Keywords: ice-on-valid-code, patch
: 3399 3648 (view as issue list)
Depends on:
Blocks:
 
Reported: 2009-05-04 09:12 UTC by Don
Modified: 2015-06-09 01:26 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Don 2009-05-04 09:12:52 UTC
This is a variation of bug 2437, but ICEs in a completely different place.
---
struct Foo{
   int z;
   this(int a){z=a;}
}
void bar(Foo a = Foo(1)){ }
void foo() { bar(); }
---
Internal error: ..\ztc\out.c 1199
Comment 1 Don 2009-09-02 12:15:57 UTC
Same root cause as bug 2437, and the same simple patch fixes it.
Comment 2 Don 2009-09-03 00:29:40 UTC
Not patched yet, see bug 2437 for details.
Comment 3 Rob Jacques 2009-10-05 21:03:51 UTC
Note that struct defaults using either opAssign or static opCall do not seem to be affected by this bug.
Comment 4 Don 2009-10-14 07:45:46 UTC
*** Issue 3399 has been marked as a duplicate of this issue. ***
Comment 5 Koroskin Denis 2009-12-25 07:15:59 UTC
*** Issue 3648 has been marked as a duplicate of this issue. ***
Comment 6 Don 2010-04-12 13:01:45 UTC
PATCH:
I think it's enough to change tocsym.c, VarDeclaration::toSymbol(),
around line 201. If it's a CTFE variable, it's shouldn't be marked as an extern.


        t->Tcount++;

-        if (isDataseg())
+        if (isDataseg() && !isCTFE())
        {
            if (isThreadlocal())
            {   /* Thread local storage
                 */
                TYPE *ts = t;
                ts->Tcount++;   // make sure a different t is allocated
Comment 7 Walter Bright 2010-04-27 08:49:55 UTC
The problem is a semantic one. Default arguments are evaluated in the context of the function declaration, not where it's used. So, the temporary generated by the constructor is created in global space!

The patch tries to force it back into the function scope, but that doesn't work as you can see if you change foo() to:

  void foo() { bar(); bar(); }

as it fails trying to allocate the same symbol twice. Not sure what the correct solution is at the moment.
Comment 8 Walter Bright 2010-04-27 09:04:17 UTC
The problem is the code here in expression.c funcParameters():

                arg = p->defaultArg;
                arg = arg->copy();      <-- Danger, Will Robinson!
                arg = arg->resolveLoc(loc, sc);
                arguments->push(arg);

The arg->copy() is the problem, as it will copy any DeclarationExp's resulting in multiple declarations with the same name. A correct fix will be to do what DeclarationExp::doInline() does, which is for any non-static declarations, create another declaration.

A new expression tree walker has to be built to accomplish this. Perhaps a good approach is to create a generic walker that accepts a lambda function to operate on each node.
Comment 9 Walter Bright 2010-04-27 11:37:45 UTC
changeset 452
Comment 10 Don 2010-05-05 19:04:44 UTC
Fixed DMD2.044