fix Issue 15985 - [REG2.068/2.069] Code doesn't link unless compiled …

[WalterBright]

…with -debug

Can it really be this simple? Let's see!

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	15985	regression	[REG2.068/2.069] Code doesn't link unless compiled with -debug

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

1. Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

2. Change the base branch of your PR to stable

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 "master + dmd#14855"

[ibuclaw]

Needs explanation in comment above.

[WalterBright]

comment fixed

[ibuclaw]

Still, needs an explanation. :-)

comment fixed

[ibuclaw]

I'd prefer @kinke to have a review of this as well.

added -allinst

[kinke]

Before Iain's recent refactoring of this function, it was absolutely crucial that both fields were cleared, so that subsequent invocations for the same instance just returned the cached result based on minfo, which had been finalized after the 1st call.

Now after the refactoring, there should be way less invocations. But clearly the result shouldn't change for later invocations, regardless of whether the tinst parent was cleared after the first invocation or not. So something seems wrong, and this seemingly just patches over some cracks - ascending repeatedly up to the ancestors shouldn't be needed and just hurt performance.

[WalterBright]

The problem was that clearing the tinst meant the compiler lost track of the context that indicated that an instantiation needed to have code generated for it.

People have been struggling for years attempting to figure out why sometimes templates were not emitted to the code generator. This fix looks like the solution.

@kinke if you have a better fix, by all means submit it!

[WalterBright]

which had been finalized after the 1st call

The bug with that is the 1st call did not need code generated, and the second call did. Hence the h function did not get code generated for it.

[ibuclaw]

Before Iain's recent refactoring of this function, it was absolutely crucial that both fields were cleared, so that subsequent invocations for the same instance just returned the cached result based on minfo, which had been finalized after the 1st call.

I don't think my refactoring has anything to do with a bug that has been around since 2.068. The only logic change was the enforcement of going through the primary instance. By and large the rest tried to stay true to the original (minus redundant checking).

Besides, only the tnext descending was put in a loop, the tinsts are still recursive.

[kinke]

The bug with that is the 1st call did not need code generated, and the second call did.

And that's the bug right there. A unique template instantiation's needsCodegen() should definitely not change with time. It's supposed to be called from the codegen layer only, after everything has been analyzed, and the full template instantiation graph has been constructed (tnext chain for the primary instance itself and the respective chains of all ancestors).

[kinke]

What I'm trying to say is that the original idea was that minfo is finalized after the first invocation, in a way that subsequent calls don't need to check the siblings (tnext) and the ancestors (tinst) anymore, but can derive the single stable needsCodegen result from minfo from the first invocation.

[ibuclaw]

Now after the refactoring, there should be way less invocations. But clearly the result shouldn't change for later invocations, regardless of whether the tinst parent was cleared after the first invocation or not. So something seems wrong, and this seemingly just patches over some cracks - ascending repeatedly up to the ancestors shouldn't be needed and just hurt performance.

There seems to be some cyclic link happening between tinst and text, where for example the 3rd tinst in this->tinst->tnext->tinst->tinst leads us back to this; returns false because it's still speculative as far as it knows; propagates false to 2nd tinst; however 1st tinst decides after inspecting some more siblings that yes it does need codegen; propagates true back to this. Later 2nd tinst is inspected again and returns false... But if we don't sever the tinst link it will instead return true because its ancestor this is now true. Ughhhh if I understand the flow correctly.

[kinke]

Ugh, ouch... okay for such cycles in the graph, unlinking tnext should at least prevent infinite needsCodegen recursions / compiler stack overflows, but keeping tinst intact might improve the result wrt. wrongly culled instantiations. I guess such cycles account for a good deal of the template-emission bugs. Would ideally probably be detected (inprogress) and handled more conservatively (do codegen if in doubt)...

[ibuclaw]

An inprogress flag seems reasonable, and returning true if set. That might trip the minst assertions however.

[WalterBright]

/private/var/folders/hy/gdmd645x1ws410vx266t7py00000gn/T/cirrus-ci-build/generated/osx/release/64/dmd -conf= -m64 -Irunnable  -fPIC -release -g -odtest_results/runnable -oftest_results/runnable/testabi_3.o  -c runnable/testabi.d 
runnable/testabi.d(661): Error: bad type/size of operands `cmp`
runnable/testabi.d(675): Error: template instance `testabi.test1_asm!(sls, 49)` error instantiating

this error makes no sense at all

[WalterBright]

I guess that error was a heisenbug or cleared up with a rebase.

[ibuclaw]

@JohanEngelen @kinke testing this against industry would be appreciated.

[JohanEngelen]

@JohanEngelen @kinke testing this against industry would be appreciated.

Tested on weka's default build process, worked without linking issues. (sorry for the delay)