trans: Avoid using weak_odr linkage for closures.

[michaelwoerister]

At the moment, all closures get weak_odr linkage, since that allows for easily avoiding symbol conflicts between copies of the same closure in different crates -- something that can happen when a function containing a closure is inlined into another crate.
One problem though with this approach is that the mingw toolchain does not allow for more than 2^15 weak symbols within one binary. Unfortunately, there are codebases out there run into this limit, e.g. servo/servo#12393 (comment).

So, if we want to keep supporting mingw, we need to find a way to avoid symbol conflicts for closures without using weak linkage.

For monomorphizations of generic functions the same issue exists but in that case it has been easier to solve because we could just instantiate them within each codegen unit with "internal" linkage, thus avoiding any conflicts. For closures this is currently not possible because the cross-crate inlining infrastructure does not allow for inlining a closure on demand.

I see a few possible solutions, each with its own set of disadvantages:

1. Extend the inlining-infrastructure to better support closures. Then we could handle closures just the same as monomorphizations. The downside is that this would mean an investment into code that will become obsolete once non-MIR-trans is removed.
2. Always force MIR-trans for closures from extern crates, then we don't need to inline them. The downside is that MIR-trans might not be ready for that yet and that it's kind of a strange and inconsistent behavior.
3. Make sure that the symbol-hash of a closure contains the crate-id of the instantiating crate, thus avoid cross-crate conflicts. The downside is that this makes symbol naming more complicated again, i.e. one can't just call instance.symbol_name() and get a nicely predictable result anymore.
4. Store the set of exported closure instances in crate metadata and never locally instantiate closures that already exist somewhere. The downside is that it means more stuff in metadata.
5. Drop support for mingw :P

cc @rust-lang/compiler

[retep998]

Drop support for mingw :P

I'd imagine there is a non-trivial amount of people that would complain about dropping support for MinGW.

[hanna-kruppe]

To be clear, if MIR trans is turned on across the board then the problem doesn't show up? Then "drop support for mingw" really means

Drop old-trans support for mingw, force mingw users to use MIR trans even though it's not enabled by default yet

Since the only reasons MIR isn't in orbit yet are one or two blockers and some caution about possible unknown problems, this seems like a reasonable temporary inconvenience.

[michaelwoerister]

I'd imagine there is a non-trivial amount of people that would complain about dropping support for MinGW.

Yes, I didn't really mean that to be a serious option.

[michaelwoerister]

Drop old-trans support for mingw, force mingw users to use MIR trans even though it's not enabled by default yet

The problem with this is that MIR-trans does not solve the problem, it just allows for implementing a solution, which would then not be restricted just to MinGW. If we implement this solution (making an internal copy of closures in every codegen unit), we have to use MIR-trans of closures everywhere.

We could do some special casing for MingW, where closures are treated differently on just that platform but that's kind of messy and we would have to support two different linkage schemes in parallel.

I'm not ruling this option out though. It is a good observation.

[michaelwoerister]

OK, so I have a WIP implementation of the "require MIR-trans on MinGW" option here:
https://github.com/michaelwoerister/rust/commits/internal-closures

It might be even less intrusive than expected since it only requires MIR-trans if one is compiling with more than one codegen-unit per crate. Most people probably wouldn't even notice.

[michaelwoerister]

The solution implemented in the branch mentioned above looks pretty good. It passes make check, it allows for compiling Servo with MingW, and most of its ugliness will go away once we don't have to support legacy trans anymore. I'll make a PR and we'll see if people are OK with merging it.

[nagisa]

weak_odr linkage seems to also cause inliner to miss inlining opportunities within the same crate, which is the cause of somewhat-steady influx of perf regression reports.