Limit dylib symbols

[Zoxc]

This makes windows-gnu match the behavior of windows-msvc. It probably doesn't make sense to export these symbols on other platforms either.

[rust-highfive]

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

[matthewjasper]

r? @michaelwoerister

[Zoxc]

cc @alexcrichton

[michaelwoerister]

Note: This should fix issue #53014. Before I approve, I want to discuss the problem some more in that issue in order to make sure we know what exactly is going on.

[Zoxc]

@bors try

[bors]

⌛ Trying commit 2f948ea with merge d8873bfe3191043214a8a2630a01ac602be96a01...

[Zoxc]

@michaelwoerister That issue is for msvc, which isn't affected by this PR.

[pnkfelix]

Note: This should fix issue #53014. Before I approve, I want to discuss the problem some more in that issue in order to make sure we know what exactly is going on.

I'm confused: This code only changes a method in impl<'a> Linker for GccLinker<'a>; but the reports in issue #53014 say that the problem is arising under MSVC. I would have thought that the former would have nothing to do with the latter.

Am I misunderstanding the relationship between the GccLinker type and the MSVC target?

Update: Ah, @Zoxc pointed out the same detail.

[michaelwoerister]

Ah OK. So this is just a drive-by bug fix?

[Zoxc]

Ran into it when reviving #56987.

[michaelwoerister]

Well, I guess there's no harm in passing the list of exported symbols to the linker explicitly then. Although there's still the question why it makes a difference (as mentioned in #53014 (comment)).

[michaelwoerister]

And I wonder if we can also remove the early exit for proc-macros. IIRC, they should only export no_mangle functions.

[bors]

☀️ Try build successful - checks-travis
Build commit: d8873bfe3191043214a8a2630a01ac602be96a01

[Zoxc]

@rust-timer build d8873bfe3191043214a8a2630a01ac602be96a01

[rust-timer]

Success: Queued d8873bfe3191043214a8a2630a01ac602be96a01 with parent 3750348, comparison URL.

[rust-timer]

Finished benchmarking try commit d8873bfe3191043214a8a2630a01ac602be96a01

[michaelwoerister]

Looks pretty good! I know @alexcrichton already found in the past that dynamic linking can take up quite a bit of time when compiling small crates.

You can r=me in the current state or try out things with proc-macro early exit also removed.

[rust-highfive]

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:03d0faf2:start=1554735105860439897,finish=1554735209490245048,duration=103629805151
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
Setting environment variables from .travis.yml

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[Zoxc]

I wasn't expecting that error, maybe we ignore a proc macro without the right symbols exported?

Anyway, @bors r=michaelwoerister

[bors]

📌 Commit 2f948ea has been approved by michaelwoerister

[tesuji]

Is this beta-nominated? #59800 would need them.

[pnkfelix]

discussed in T-compiler meeting; accepting for beta backport.

[Centril]

Discussed at the T-release meeting. @Mark-Simulacrum would be happy to r+ a PR that adds a note to the "known issues" section of the relnotes.

[alexkornitzer]

If I understand correctly this PR appears to be the cause of multiple dylib regressions introduced from 1.37.0 forcing people to pin to 1.36.0 if struck by these errors. Furthermore this issue has been present for a rather long time now, if there is no easy solution is it worth fielding the idea of reverting the change? 185dceb The still affected issues appear to be: #64340, #66265 and #65610

[michaelwoerister]

Thanks for bringing that up, @alexkornitzer.

Nominating for discussing in T-compiler: It looks like we should do something. It's not clear to me, however, if the reported breakage is due to code relying on unspecified implementation details or if this should be considered a bug.

[alexkornitzer]

Hi @michaelwoerister, did the compiler team manage to reach a consensus on this? I tried looking for the minutes from the last meeting but they don't appear to be online yet.

[michaelwoerister]

@alexkornitzer This has not been discussed yet, as far as I know.

[alexkornitzer]

@michaelwoerister that is disappointing but thank you for the update.

[michaelwoerister]

@Zoxc, did you ever get a chance to take a look at the bug reports above (esp. #64340 and #66265)? It looks to me like the use cases rely on accidental, unspecified behavior with regards to how linking works.

[Zoxc]

#66265 seems like it should result in a linker error even if exit was exported from Rust since there's multiple definitions of exit. It's not really something that should be supported without explicit support for shadowing symbols (which we don't have).

I haven't look closely at #64340, but it seems to have a problem linking Rust symbols now (not linking to C stuff), which might be a bug with dylibs in rustc.

[solb]

@Zoxc The reason there's no duplicate-definition error in #66265 is that the definitions of the exit symbol are in separate shared object files. It's not a type of shadowing that needs compiler support; rather, this is a concept known as library interposition, which is supported by the dynamic linker itself and is key to how shared libraries work on POSIX systems. (For example, it permits use cases such as runtime heap checking and library call tracing a la ltrace.)

[solb]

@michaelwoerister I've been working with gritty details of shared libraries and the dynamic linker a lot recently, and would be happy to be involved in discussions of this and similar issues when they arise. Any idea when this will come up in triage?

[eddyb]

It's not a type of shadowing that needs compiler support;

This is not entirely accurate, as the compiler does need to let the linker see the exports (which is why it doesn't work anymore after this PR was merged, IIUC the discussion here).

I think @Zoxc's point is that we never advertised this as a feature, and so our official position could be that any situation where this worked previously, was merely coincidental and does not constitute "permission" to rely on shadowing.

There could be several reasons to take this stance:

• supporting shadowing could be at odds with some other goal
  • performance wins of this PR, for example
• it might not be possible to support it to the same extent on all platforms
• there may be footgun/safety/soundness concerns around "just letting it happen"

[michaelwoerister]

@solb There is some discussion going on here: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/dylib.20linking.20breakage/near/182674053

[pnkfelix]

Here is a link to the public archive of the discussion that @michaelwoerister linked above: https://zulip-archive.rust-lang.org/131828tcompiler/28755dyliblinkingbreakage.html

I'm removing the nomination label from this now because I think that discussion essentially covered our thinking at that time, and I don't think much of the thinking has changed since then.

Notably, that discussion did end with:

we should try to write-up an RFC with a plan and decide that way (personally). Making arbitrary changes at this point without having had public discussion feels not great.

(That's all still true, but I don't think anyone has been actually tasked with creating such an RFC or similar public proposal.)