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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce7c5df
fix Issue 18026 - Stack overflow in dmd/dtemplate.d:6248, TemplateIns…
ibuclaw Jan 4, 2023
5002893
dmd.dtemplate: Add assert(this is inst) to needsCodegen
ibuclaw Jan 9, 2023
1ad92d7
dmd.dtemplate: Don't end search after making inst non-speculative in …
ibuclaw Jan 9, 2023
2128aad
dmd.dtemplate: Break if needsCodegen is not ThreeState.none
ibuclaw Jan 10, 2023
2a993e3
dmd.dtemplate: Recurse through the primary instantiation, skip if 'in…
ibuclaw Jan 10, 2023
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
74 changes: 46 additions & 28 deletions compiler/src/dmd/dtemplate.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6264,24 +6264,31 @@ extern (C++) class TemplateInstance : ScopeDsymbol
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.needsCodegen())
{
tithis.minst = tinst.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 @@ -6293,6 +6300,8 @@ extern (C++) class TemplateInstance : ScopeDsymbol
minst = tnext.minst; // cache result from non-speculative sibling
return false;
}
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 •
edited
Loading

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 +6326,38 @@ 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)
{
minst = tinst.minst; // cache result
return needsCodegen;
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 @@ -6352,6 +6368,8 @@ extern (C++) class TemplateInstance : ScopeDsymbol
minst = tnext.minst; // cache result from non-speculative sibling
return true;
}
else if (needsCodegen != ThreeState.none)
break;
}
}

Expand Down
12 changes: 12 additions & 0 deletions compiler/test/compilable/test18026.d
View file
Open in desktop
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))
{
}
}