D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 2437 - ICE(tocsym.c, !needThis()) - default struct argument
Summary: ICE(tocsym.c, !needThis()) - default struct 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
: 2440 (view as issue list)
Depends on:
Blocks:
 
Reported: 2008-10-31 17:19 UTC by simon
Modified: 2015-06-09 01:20 UTC (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description simon 2008-10-31 17:19:08 UTC
Compiling the following code with 2.019, 2.020

struct aStruct{
	int	m_Int;

	this(int a){
		m_Int = a;
	}
}

class aClass{
	void aFunc(aStruct a = aStruct(44)){
	}
}

int main(string argv[]) {
	aClass a = new aClass();
	a.aFunc();
	return 0;
}

Gives:

ICE - Assertion failure: '!needThis()' on line 166 in file 'tocsym.c'

Not sure if this code is valid or invalid.
Comment 1 Don 2009-05-04 07:50:05 UTC
*** Bug 2440 has been marked as a duplicate of this bug. ***
Comment 2 Don 2009-09-02 12:11:01 UTC
The root cause is in func.c, FuncDeclaration::semantic(Scope *sc)

Around line 240, we read:

    sd = parent->isStructDeclaration();
    if (sd)
    {
	if (isCtorDeclaration())
	{
	    return;    // <============== this is a problem!
	}

Returning at this point means that the scope isn't saved. This prevents the later semantic passes from running, in certain cases. Commenting out the return prevents the ICE. I haven't yet checked this patch on the test suite, it could be that additional changes are required. But the problems begin here.
Comment 3 Don 2009-09-03 00:28:53 UTC
Aargh, patch is incorrect. I think the patch does fix a genuine problem (it seems to be necessary to allow you to evaluate a struct constructor in CTFE), but there's a second problem as well.

It may be because a struct constructor such as aStruct(44) turns into 
((aStruct __ctmp2;) , __ctmp2).this(44)

And that __ctmp2 is a funny beast. Where is it stored? Its parent is the *MODULE*, not the function it's called from. But it's not a static variable.
Weird.
Comment 4 Rob Jacques 2009-10-05 21:01:26 UTC
Now at line 170 in DMD 2.033.
Note, this bug only applies to struct constructors and not default arguments using static opCall or opAssign.
Comment 5 Don 2010-04-12 13:51:13 UTC
See also the closely related bug 2935.
I can patch this (against svn 433) with these two changes (and the DMD test suite still passes), which basically just remove the checks, in the case where a CTFE variable is used. I'm not certain these patches are correct, though.


-- PATCH ------------------------------------------
tocsym.c, line 170:

Symbol *VarDeclaration::toSymbol()
{
    //printf("VarDeclaration::toSymbol(%s)\n", toChars());
    //if (needThis()) *(char*)0=0;
+    if (!isCTFE())
        assert(!needThis());


and secondly, in e2ir.c, SymbolExp::toElem() line 664:

    //printf("SymbolExp::toElem('%s') %p\n", toChars(), this);
    //printf("\tparent = '%s'\n", var->parent ? var->parent->toChars() : "null");
-    if (op == TOKvar && var->needThis())
+    if (op == TOKvar && var->needThis() && !v->isCTFE())
    {
        error("need 'this' to access member %s", toChars());
        return el_long(TYint, 0);
    }

---------------------------------

A totally different (and probably better) alternative would be to fix it in expression.c, around line 6770, when the temporary variable is created. If (!sc->func), it's being created in global scope, and should be neither in STCfield nor STCextern storage class. But I'm not sure how this should be done.

            // First look for constructor
            if (ad->ctor && arguments && arguments->dim)
            {
                // Create variable that will get constructed
                Identifier *idtmp = Lexer::uniqueId("__ctmp");
                VarDeclaration *tmp = new VarDeclaration(loc, t1, idtmp, NULL);
                tmp->storage_class |= STCctfe;
+                if (!sc->func) { 
+   // it's being created in global scope. Do something!!
Comment 6 Don 2010-04-14 07:52:15 UTC
The code I've previously posted is definitely incorrect. The problem here is that the compiler's thinks that the struct is a field of the parent; and this happens because the scope is wrong.

I'm really not sure where to fix this. One possibility is to add it to CommaExp::semantic, creating a new scope whenever there's a declaration which isn't in module scope. But I think the correct solution is to change the scope for evaluating the default arguments, in mtype.c
around line 4766; 


        /* Create a scope for evaluating the default arguments for the parameters
         */
        Scope *argsc = sc->push();
        argsc->stc = 0;                 // don't inherit storage class
        argsc->protection = PROTpublic;
+        /* Compiler-generated comma expressions (such as struct constructors)
+         * can include a variable declaration. This declaration needs to go somewhere.
+         * If we're inside a function, it can become a local variable of that function;
+         * but if not (for example, if it is a function default parameter), it needs
+         * its own scope.
+         */
+        if (!argsc->func) // not a nested function
+          argsc->parent = (Dsymbol*)sc->module;

But I don't know if it's OK to set the parent in this way. The cast is needed only because mtype.c doesn't have the real definition of Module.
The patch to bug 2935 is required in addition to this one.
Comment 7 Walter Bright 2010-04-27 09:06:41 UTC
I think the correct solution is identified in 2935.
Comment 8 Walter Bright 2010-04-27 11:37:19 UTC
changeset 452
Comment 9 Don 2010-05-05 19:04:12 UTC
Fixed DMD2.044