Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix Issue 18026 - Stack overflow in dmd/dtemplate.d:6248, TemplateInstance::needsCodegen() #14787

Merged
merged 5 commits into from
Jan 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 56 additions & 31 deletions compiler/src/dmd/dtemplate.d
Original file line number Diff line number Diff line change
Expand Up @@ -6255,33 +6255,46 @@ extern (C++) class TemplateInstance : ScopeDsymbol
this.tnext = null;
this.tinst = null;

if (errors || (inst && inst.isDiscardable()))
// Don't do codegen if the instance has errors,
// is a dummy instance (see evaluateConstraint),
// or is determined to be discardable.
if (errors || inst is null || inst.isDiscardable())
{
minst = null; // mark as speculative
return false;
}

// This should only be called on the primary instantiation.
assert(this is inst);

if (global.params.allInst)
{
// Do codegen if there is an instantiation from a root module, to maximize link-ability.

// Do codegen if `this` is instantiated from a root module.
if (minst && minst.isRoot())
return true;

// Do codegen if the ancestor needs it.
if (tinst && tinst.needsCodegen())
static ThreeState needsCodegenAllInst(TemplateInstance tithis, TemplateInstance tinst)
{
minst = tinst.minst; // cache result
assert(minst);
assert(minst.isRoot());
return true;
// Do codegen if `this` is instantiated from a root module.
if (tithis.minst && tithis.minst.isRoot())
return ThreeState.yes;

// Do codegen if the ancestor needs it.
if (tinst && tinst.inst && tinst.inst.needsCodegen())
{
tithis.minst = tinst.inst.minst; // cache result
assert(tithis.minst);
assert(tithis.minst.isRoot());
return ThreeState.yes;
}
return ThreeState.none;
}

if (const needsCodegen = needsCodegenAllInst(this, tinst))
return needsCodegen == ThreeState.yes ? true : false;

// Do codegen if a sibling needs it.
if (tnext)
for (; tnext; tnext = tnext.tnext)
{
if (tnext.needsCodegen())
const needsCodegen = needsCodegenAllInst(tnext, tnext.tinst);
if (needsCodegen == ThreeState.yes)
{
minst = tnext.minst; // cache result
assert(minst);
Expand All @@ -6291,8 +6304,10 @@ extern (C++) class TemplateInstance : ScopeDsymbol
else if (!minst && tnext.minst)
{
minst = tnext.minst; // cache result from non-speculative sibling
return false;
// continue searching
}
ibuclaw marked this conversation as resolved.
Show resolved Hide resolved
else if (needsCodegen != ThreeState.none)
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codecov says this is not covered, is it Codecov's issue or really not covered ?

Copy link
Member Author

@ibuclaw ibuclaw Jan 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's really not covered, because the interior function never returns ThreeState.no.

I left it in to guard us in case that were to change in the future, and also to align with the non-allinst branch.

I think these two branches could be factored because the outer shell is mostly a duplicate now, but I'll leave that for something to look at in far future - maybe one of the strategies will have to go anyway because missing symbols is one of the "big problems" at the moment, and the default strategy to prune prune prune is a massive PITA that isn't helping any cause.

}

// Elide codegen because there's no instantiation from any root modules.
Expand All @@ -6317,31 +6332,39 @@ extern (C++) class TemplateInstance : ScopeDsymbol
* => Elide codegen if there is at least one instantiation from a non-root module
* which doesn't import any root modules.
*/

// If the ancestor isn't speculative,
// 1. do codegen if the ancestor needs it
// 2. elide codegen if the ancestor doesn't need it (non-root instantiation of ancestor incl. subtree)
if (tinst)
static ThreeState needsCodegenRootOnly(TemplateInstance tithis, TemplateInstance tinst)
{
const needsCodegen = tinst.needsCodegen(); // sets tinst.minst
if (tinst.minst) // not speculative
// If the ancestor isn't speculative,
// 1. do codegen if the ancestor needs it
// 2. elide codegen if the ancestor doesn't need it (non-root instantiation of ancestor incl. subtree)
if (tinst && tinst.inst)
{
minst = tinst.minst; // cache result
return needsCodegen;
tinst = tinst.inst;
const needsCodegen = tinst.needsCodegen(); // sets tinst.minst
if (tinst.minst) // not speculative
{
tithis.minst = tinst.minst; // cache result
return needsCodegen ? ThreeState.yes : ThreeState.no;
}
}

// Elide codegen if `this` doesn't need it.
if (tithis.minst && !tithis.minst.isRoot() && !tithis.minst.rootImports())
return ThreeState.no;

return ThreeState.none;
}

// Elide codegen if `this` doesn't need it.
if (minst && !minst.isRoot() && !minst.rootImports())
return false;
if (const needsCodegen = needsCodegenRootOnly(this, tinst))
return needsCodegen == ThreeState.yes ? true : false;

// Elide codegen if a (non-speculative) sibling doesn't need it.
if (tnext)
for (; tnext; tnext = tnext.tnext)
{
const needsCodegen = tnext.needsCodegen(); // sets tnext.minst
const needsCodegen = needsCodegenRootOnly(tnext, tnext.tinst); // sets tnext.minst
if (tnext.minst) // not speculative
{
if (!needsCodegen)
if (needsCodegen == ThreeState.no)
{
minst = tnext.minst; // cache result
assert(!minst.isRoot() && !minst.rootImports());
Expand All @@ -6350,8 +6373,10 @@ extern (C++) class TemplateInstance : ScopeDsymbol
else if (!minst)
{
minst = tnext.minst; // cache result from non-speculative sibling
return true;
// continue searching
}
else if (needsCodegen != ThreeState.none)
break;
}
}

Expand Down
12 changes: 12 additions & 0 deletions compiler/test/compilable/test18026.d
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// https://issues.dlang.org/show_bug.cgi?id=18026
bool f(T)(T x)
{
return false;
}

static foreach(i; 0..60000)
{
static if(f(i))
{
}
}