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

[ibuclaw]

⚠️ Please think about this change very carefully (i.e: more than 15 minutes) as it touches a pretty critical routine. ⚠️

This is best reviewed with whitespace changes hidden. https://github.com/dlang/dmd/pull/14787/files?w=1

Problem: needsCodegen calls itself recursively, this results in a stack overflow when checking an instance that has a sufficient number of siblings, or is lexically nested deep enough in other template instances, or a quadratic combination of both.

To solve: I have isolated the meat of the needsCodegen algorithm (there are two; (1) for -allinst, (2) for the default emission strategy). Then, the recursive call for instance siblings has been replaced with a for loop.

How was the "meat" determined?

1. The first check is done on the "main" template instance (stored in the tempdecl.instances AA), which is the same for every sibling. No need to check this repeatedly, so we leave this block alone and save some processing time as well.
   

   dmd/compiler/src/dmd/dtemplate.d

   Lines 6258 to 6262 in fc695ff

   	if (errors || (inst && inst.isDiscardable()))
   	{
   	minst = null; // mark as speculative
   	return false;
   	}

2. The tnext branch in both the default and allinst emission strategies are there for caching the result of needsCodegen only. They can be part of the for loop body, no need to include them in the nested function.
   

   dmd/compiler/src/dmd/dtemplate.d

   Lines 6282 to 6296 in fc695ff

   	if (tnext)
   	{
   	if (tnext.needsCodegen())
   	{
   	minst = tnext.minst; // cache result
   	assert(minst);
   	assert(minst.isRoot());
   	return true;
   	}
   	else if (!minst && tnext.minst)
   	{
   	minst = tnext.minst; // cache result from non-speculative sibling
   	return false;
   	}
   	}

   
   

   dmd/compiler/src/dmd/dtemplate.d

   Lines 6339 to 6356 in fc695ff

   	if (tnext)
   	{
   	const needsCodegen = tnext.needsCodegen(); // sets tnext.minst
   	if (tnext.minst) // not speculative
   	{
   	if (!needsCodegen)
   	{
   	minst = tnext.minst; // cache result
   	assert(!minst.isRoot() && !minst.rootImports());
   	return false;
   	}
   	else if (!minst)
   	{
   	minst = tnext.minst; // cache result from non-speculative sibling
   	return true;
   	}
   	}
   	}

3. Everything between (1) and (2) then is the part of the algorithm used to detect whether codegen is required. These have been moved into static nested functions named needsCodegenAllInst (returns false if no condition hit).
   

   dmd/compiler/src/dmd/dtemplate.d

   Lines 6268 to 6279 in fc695ff

   	// 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())
   	{
   	minst = tinst.minst; // cache result
   	assert(minst);
   	assert(minst.isRoot());
   	return true;
   	}

   
   and needsCodegenRootOnly (returns true if no condition hit).
   

   dmd/compiler/src/dmd/dtemplate.d

   Lines 6322 to 6336 in fc695ff

   	// 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)
   	{
   	const needsCodegen = tinst.needsCodegen(); // sets tinst.minst
   	if (tinst.minst) // not speculative
   	{
   	minst = tinst.minst; // cache result
   	return needsCodegen;
   	}
   	}
   	// Elide codegen if `this` doesn't need it.
   	if (minst && !minst.isRoot() && !minst.rootImports())
   	return false;

4. There is a case to handle in needsCodegenRootOnly for when the needsCodegen variable is true - meaning "do codegen if the ancestor needs it". For this, the tnext of the instance is set to null so that no siblings (or further siblings) are checked.

---

There is still recursive checking done for parents (tinst) of the template instance, so the possibility for a stack overflow is still present if we are handling a deeply nested instance (as in, 100,000+ levels of lexical nestedness). I'm willing to leave that risk in until someone presents a reasonable example which triggers that.

Are there any cases that I might have missed here? Do we need to check tnext.errors on every sibling too?

[dlang-bot]

Thanks for your pull request, @ibuclaw!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	18026	normal	Stack overflow in ddmd/dtemplate.d:6241, TemplateInstance::needsCodegen()

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "stable + dmd#14787"

[JohanEngelen]

Tested on weka's codebase, no issues.

[ibuclaw]

Changed so that the nested function returns a ThreeState instead.

[kinke]

Thx for working on this. - My biggest concern is that iterating over the siblings doesn't clear their tinst and tnext fields anymore, so a full needsCodegen() call later on doesn't simply return the cached result anymore.

[kinke]

One nice effect of this, AFAICT, is that needsCodegen() should now be invoked on the primary (fully analyzed) template instantiation only, and the other duplicates (siblings) only visited in the nested functions. Reason: only primary instantiations are appended to module members (=> direct needsCodegen() calls from glue layer), and only primary instantiations get nested instantiations (=> tinst parent of these nested ones). So an assert(this is inst) should hold.

[ibuclaw]

One nice effect of this, AFAICT, is that needsCodegen() should now be invoked on the primary (fully analyzed) template instantiation only, and the other duplicates (siblings) only visited in the nested functions. Reason: only primary instantiations are appended to module members (=> direct needsCodegen() calls from glue layer), and only primary instantiations get nested instantiations (=> tinst parent of these nested ones). So an assert(this is inst) should hold.

Though calling needsCodegen() with a secondary instance would potentially generate a wrong result anyway? Assert is fine, though can instead guard the entrypoint with:

if (this !is inst)
    return inst.needsCodegen();

[ibuclaw]

Thx for working on this. - My biggest concern is that iterating over the siblings doesn't clear their tinst and tnext fields anymore, so a full needsCodegen() call later on doesn't simply return the cached result anymore.

I don't like this side effect of needsCodegen(), there should really be another way to "cache" the result that doesn't involve stamping over the semantic analysis of the primary instantiation.

[kinke]

Though calling needsCodegen() with a secondary instance would potentially generate a wrong result anyway? Assert is fine, though can instead guard the entrypoint with: …

I'd prefer the assert, as it really shouldn't happen anymore, and then makes my point wrt. missing reset for secondary siblings moot. This was an observation last time I looked at this brainfuck mess, and I was thinking about adding a 2nd bool isSiblingCheck = false param (to needsCodegen) to make things somewhat clearer (=> regular calls need to start from the primary instance to walk over all siblings, as the tnext linked list starts from the primary instance). Your version is much nicer in that regard.

Every secondary sibling should only be checked at most once then, when checking the siblings of the primary instance. And the primary one gets its tinst and tnext fields cleared during the 1st invocation, in case it is checked multiple times (e.g., when checking the tinst ancestor recursively).

[ibuclaw]

Though calling needsCodegen() with a secondary instance would potentially generate a wrong result anyway? Assert is fine, though can instead guard the entrypoint with: …

I'd prefer the assert, as it really shouldn't happen anymore, and then makes my point wrt. missing reset for secondary siblings moot.

Looks like there's no guarantee that non-primary instantiations have been set-up correctly, getting asserts from the tnext.tinst.inst of a template instantiation.

[ibuclaw]

@kinke and looks like I still need a break in place that I added on converting the internal check to a ThreeState - which confirms my understanding that halting the search on the first instance that returns yes or no to be correct.

https://buildkite.com/dlang/dmd/builds/29578#018598fd-7e4b-47c8-827a-7fb55b19597e

[kinke]

looks like I still need a break in place that I added on converting the internal check to a ThreeState - which confirms my understanding that halting the search on the first instance that returns yes or no to be correct.

Yep that should be fine. All the else if could be replaced by if. And maybe the comment (cache result from non-speculative sibling) made clearer (convert speculative primary instance to non-speculative).

[kinke]

Note that I came across this sometime later: https://github.com/dlang/dmd/pull/13731/files

[dkorpel]

Is this ready?

[ibuclaw]

How lucky do you feel?

[Geod24]

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

[ibuclaw]

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.