Fix issue #1712

[JohanEngelen]

Cross-module inlining (i.e. emitting some code from another module) is only done when a function is not fully semantic3 analysed.
The problem is that sometimes, a function has already been fully "semantic3'ed", but is not emitted anywhere. This happens for nested functions, and those are currently not cross-module inlined (see #1603).
In issue #1712, it happens for a function with template parameter __FILE__, i.e. a function that should always be inlined (#1703) exactly because of the link problem observed in #1712.

This fix for #1712 attempts to solve this for now, by emitting pragma(inline, true) functions more aggressively. It fixes the current issue, and I hope it does not create new bugs.

Currently dustmiting to create a testcase; it will take a lot of time.

[kinke]

Any progress? Otherwise I'd suggest disabling cross-module inlining by default (or only enabling it for optimization levels > 3) for now, until the issue is fully understood.

[JohanEngelen]

Found the problem (it's bad), fixing it.

[JohanEngelen]

The problem was that inside DtoDefineFunction, a call to DtoResolveFunction would end up invoking DtoDefineFunction and defining the function with available_externally, which would not be "fixed up" later because it all happened in the call trace from the DtoDefineFunction that should do the fix up. The fix is to tell DtoResolveFunction and DtoDeclareFunction that they may not end up defining the function.

[JohanEngelen]

(I'm still dustmiting the testcase btw. It takes forever)

[JohanEngelen]

Dustmiting is done, and a fairly simple testcase resulted. I've modified it such that it doesn't depend on Phobos and added it to the PR.
Further fixes were needed involving with nested functions and nested structs. The fix for this issue uncovers unimplemented nested function cross-module emitting problems.

[kinke]

With each new fix, a new failure pops up. ;/
I'd really recommend not enabling cross-module inlining by default for v1.1 yet, and exposing it via optimization levels > 3 for now.

[JohanEngelen]

Yeah, I think you're right :(

[dnadlinger]

Even apart form the issue at hand here, as long as __FILE__/… and name stability issues are not addressed, enabling cross-module inlining by default would be equal to willingly introducing regressions. I'd be uneasy shipping a change that suddenly causes unsuspecting user code to break, even if there might have been a problem in more complex situations already. After all, __FILE__ template parameters have been in Phobos for ages but nobody seems to have run into the issue until recently.

I wouldn't try enabling it at -O4 or -O5; judging from what you see on the forums or GitHub, many people just use it by default, even it it hasn't ever really done anything more than -O3. An extra command line flag is annoying, but might be the safest bet for wide-spread testing.

[dnadlinger]

This predicate looks like a good candidate for a helper function that takes a Dsymbol * or whatever – feeling compelled to copy-and-paste comments is probably a good indicator for that.

[JohanEngelen]

Definitely.

[JohanEngelen]

I opened #1737 to "fix" all cross-module inlining issues: disable the thing.

(edit: moved discussion to 1737 about stuff not directly related to this PR)