Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rustc_trans: Append to PATH instead of prepending #31131

Conversation

alexcrichton
Copy link
Member

Currently we run sub-commands with a different PATH so they can find the bundled
gcc.exe that we ship with the *-windows-gnu host compilers. The current
logic, however, prepends to PATH which means that if the system has a gcc
installed it will not be used over the bundled gcc.

This can cause problems, however, if the system gcc is used to compile native
code and the Rust compiler then links everything with the bundled gcc. The
standard library in both situations can be subtly different (the C standard
library), and this can lead to errors such as rust-lang/flate2-rs#27.

This commit switches the ordering by appending our own tools to PATH instead
of prepending, so the system tools will be favored over the bundled ones (which
are intended to only be used as a last resort anyway).

Currently we run sub-commands with a different PATH so they can find the bundled
`gcc.exe` that we ship with the `*-windows-gnu` host compilers. The current
logic, however, *prepends* to `PATH` which means that if the system has a `gcc`
installed it will not be used over the bundled `gcc`.

This can cause problems, however, if the system gcc is used to compile native
code and the Rust compiler then links everything with the bundled gcc. The
standard library in both situations can be subtly different (the C standard
library), and this can lead to errors such as rust-lang/flate2-rs#27.

This commit switches the ordering by appending our own tools to `PATH` instead
of prepending, so the system tools will be favored over the bundled ones (which
are intended to only be used as a last resort anyway).
@rust-highfive
Copy link
Collaborator

r? @nrc

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

@alexcrichton
Copy link
Member Author

cc @rphmeier

r? @vadimcn or @brson

@vadimcn
Copy link
Contributor

vadimcn commented Jan 22, 2016

Um, not sure about this one. We used to have it this way when I first implemented gcc bundling, and it caused problems for people who also had the original mingw gcc on their PATH: #17251, #17412.

@vadimcn
Copy link
Contributor

vadimcn commented Jan 22, 2016

Btw, if you want to use external gcc, -windows-gnu toolchain can be installed in "gcc-less" mode (via [Advanced]).

@rphmeier
Copy link
Contributor

@vadimcn: I was having the opposite problem: the gcc crate would use the system gcc to compile the C portion, but rustc would use its bundled gcc to link.
When discussing with Alex in IRC I saw two possible options: either submit a patch like this or have the gcc crate emulate the behavior of rustc.
Ultimately I think this is an environment issue. I think it's a severe oversight to have linker errors on a vanilla setup like mine: completely fresh MSYS2 install, with as bare a $PATH as possible, using rustc via multirust. Should we favor more complex environments over simpler ones?

@vadimcn
Copy link
Contributor

vadimcn commented Jan 22, 2016

@rphmeier: The simplest environment is actually installing just Rust, no msys or mingw; and this is what we've optimized for. As I said above, your setup can be accommodated by installing Rust without the bundled gcc subset (unselect the "Linker and platform libraries" component in the installer).

@rphmeier
Copy link
Contributor

@vadimcn: You are probably correct that using the windows installer is the simplest route to take overall. However, I am definitely not unique in my need to maintain a rustc for each release channel. Currently, the only tool to facilitate this is multirust, which doesn't allow setting the option you recommend (yet).

I think my point still stands -- if there's no msys/mingw, appending rather than prepending will still work. Ideally, there should be a solution that provides the best of both options.

As I understand it, the issue with using the system MinGW GCC is that it isn't necessarily compatible with the bundled MinGW libraries stored in lib/rustlib/TARGET/. Can we change the problem by moving mingw-specific libraries into their own subdirectory, adding that to the linker path iff the bundled toolchain is being used?

I'm not the most knowledgeable about linking at a low level, so please fill me in on anything that I'm overlooking. I wanted to see if the other various DLLs stored there linked against mingw, but doing
/~/.multirust/.../rustlib/x86_64-pc-windows-gnu/lib $ find . -name "*.dll" | xargs ldd indicated that everything was linking only against libraries in system32.

@retep998
Copy link
Member

@rphmeier x86_64-pc-windows-gnu links statically to any mingw bits. As such you wouldn't see any mingw dependencies in the DLL dependencies.

@vadimcn
Copy link
Contributor

vadimcn commented Jan 23, 2016

@rphmeier: I stand corrected: on a mingw-clean system it indeed doesn't matter whether to prepend or append to the path. However, if there is an incompatible mingw on the PATH, Rust linking breaks, even for helloworld.rs, which leads to people declaring Rust on Windows as "broken" (probably rightly so). So we opted for always using the bundled gcc as this ensures that at least pure-Rust projects compile normally.

Regarding this patch: the standard Rust libraries include a few C objects which are supposed to be linked with a specific version of gcc runtime (the one they've been built with). Using a random external gcc installation for linking Rust binaries by default does not seem like a good idea.

@alexcrichton
Copy link
Member Author

@vadimcn ah yes right I thought I had seen this in the past, thanks for pointing that out! I think, however, that the issue is moot today with the gcc directory, right? The problem there was that we pass a -L argument that had a bunch of gcc libs in it and then a system gcc used some newer gcc libs, causing a mismatch. Today, however, we don't -L the gcc/lib directory, the gcc we bundle should just pick it up if it gets used, right?

Along those lines, I think we're in a situation where:

  • If there is no system gcc, we're fine to use the bundled one.
  • If there is a system gcc, we'll use it but we won't accidentally use the bundled gcc libs

It's a very good point about startup files, however, I always forget about those. That seems to put us in quite a bad situation, however, no? That means that we are only guaranteed to work with the bundled gcc which basically means that *-pc-windows-gnu can't work with C code (which also seems very important).

If that's the case, then once #30448 I would like to aggressively move towards SEH unwinding for the *-pc-windows-gnu targets which I believe would mean we can cease distribution our own startup files and start being much more compatible with "whatever gcc we find" on Windows, right?

Also out of curiosity, I would be curious to see what happens with a mingw32 gcc linking Rust code today. Does it actually explode? Do the startup objects use relatively standard symbols?

@vadimcn
Copy link
Contributor

vadimcn commented Jan 23, 2016

As far as I understand this stuff, the crux of the problem is that different ports of GCC for Windows are not particularly compatible between themselves. As long as we have any C code in Rust libs (jemalloc, miniz, etc), there is a chance that it won't work when linked with some other GCC version's runtime.

@alexcrichton
Copy link
Member Author

Hm that's definitely a good point about jemalloc, yeah. Recently jemalloc has been turned off and this actually seems like a somewhat motivating example to keep it that way. It seems like we should strive to not tie our distributed artifacts to one particular version of gcc as much as possible, we're basically just shipping it for the linker and that's it.

So to the best of my knowledge the only gcc-compiled code we distribute is:

  • libbacktrace
  • startup object files

We can get rid of the startup object files once we move to SEH unwinding, and libbacktrace will likely also get much less interesting output once we do that as well, so we can likely jettison that. At that point, if we truly do have no gcc code in the standard library, I believe this patch should be uncontroversial to land, right?

For the shorter term I would personally think that this patch is still OK to land, however. The recommended solution to this problem is to not install the mingw tools, which can also be worded as "pray your system gcc is compatible with the one we built Rust with". It seems like the only downside is that if you have an incompatible gcc and you don't include any C code (e.g. miniz via flate2) then you'll start failing to link when you could otherwise succeed.

What do you think?

@retep998
Copy link
Member

libbacktrace has to do with parsing the dwarf debug info, not the dwarf exception mechanism. I'm pretty sure -gnu isn't going to be emitting codeview/PDB debug info for a while.

@vadimcn
Copy link
Contributor

vadimcn commented Jan 25, 2016

@alexcrichton:
Our startup files are pure-Rust, so they shouldn't cause problems with C runtime (once we remove callouts to DWARF unwinder registration from them).

At that point, if we truly do have no gcc code in the standard library, I believe this patch should be uncontroversial to land, right?

But do we want to commit to never having any C code in Rust stdlib? Even if jemalloc is disabled for now, aren't we going to re-enable it once lazy locking problems are worked out?

What do you think?

I think I'd be more in favor of asking users to install a compatible version of mingw. Empirically, mingw-w64 is pretty good about cross-version compatibility.
We should also consider adding a compiler version check and a warning into the gcc crate, so it wouldn't be so confusing to users when linking fails.

@alexcrichton
Copy link
Member Author

Aren't crt2.o and dllcrt2.o slurped up from gcc? In theory they'd require things from like libgcc or something like that? The rsend.o and rsbegin.o files yeah shouldn't have any C dependencies.

I'd actually personally be ok committing to never having C code in Rust on MinGW at least, in theory we can add jemalloc but if it adds distribution and/or portability problems it seems worth it tonot do so. I figured it'd be pretty hard for us to ask for users to have a compatible MinGW installation, as we tend to prefer to work in as many environment as possible. This also means that we'd inevitably get bug reports about upgrading MinGW requirements and keeping up to date.

Overall I'd personally prefer to optimize for working with any MinGW toolchain rather than allowing us the future flexibility to include C code.

I do agree, though, that we should likely include a warning in the gcc crate no matter what in the short-term.

@vadimcn
Copy link
Contributor

vadimcn commented Jan 27, 2016

Aren't crt2.o and dllcrt2.o slurped up from gcc?

Yes, but presumably they could be slurped from the same gcc installation as the gcc runtime.

in theory we can add jemalloc but if it adds distribution and/or portability problems it seems not worth it to do so

Why would jemalloc be worth it on other platforms, but not on Windows?

I figured it'd be pretty hard for us to ask for users to have a compatible MinGW installation, as we tend to prefer to work in as many environment as possible. This also means that we'd inevitably get bug reports about upgrading MinGW requirements and keeping up to date.

If we ask them to install MinGW at all, it might as well be the right one. I am skeptical about our ability to work with every MinGW version under the sun.

aside:
I'd say that If we want to truly smoothen the path for crate users, we should support distribution of binary artifacts from crates.io. No C++ compiler installation, no library source downloads, no fiddling with whatever build system the library uses. The crate author figures out all this stuff once, everybody else can just download crate binaries and use it without jumping through all the hoops.
(...assuming the author does Windows builds. But if not, the chances of a random unix library just compiling on Windows are pretty slim, in my experience.)

@alexcrichton
Copy link
Member Author

Ok, so how does this sound for a course of action here:

  1. Land SEH unwinding for pc-windows-gnu, would involve removing rsend/rsbegin and CRT objects
  2. Remove libbacktrace as it's likely no longer much better than the system APIs that MSVC uses anyway
  3. At this point, we have no C dependencies, so land this PR

I believe this means that we can no longer mix up gcc compiled objects among gcc versions (as we're not shipping any except compiler-rt which already has no deps) and we'll favor the system gcc wherever possible.

Why would jemalloc be worth it on other platforms, but not on Windows?

In my opinion, jemalloc is not worth it on other platforms, so I'm looking for reasons to promote that opinion :). More concretely, though, we've had numerous problems which are traced back to jemalloc on Windows, and it just seems like it's far less battle-tested on Windows than it is on Linux, for example, so shipping it to all Rust programs may be a bit overzealous of us.

I am skeptical about our ability to work with every MinGW version under the sun.

True, but this failure mode of "I installed Rust and a relatively recent MinGW and my compile failed" seems quite bad today.


I actually figured distribution of binary artifacts would make this harder because each artifact is compiled with some particular gcc version (and can't go across them), so it seems like it'd be even moreso "tied to one mingw version". This is probably a discussion for another PR, however.

@vadimcn
Copy link
Contributor

vadimcn commented Jan 27, 2016

I actually figured distribution of binary artifacts would make this harder because each artifact is compiled with some particular gcc version (and can't go across them), so it seems like it'd be even moreso "tied to one mingw version"

I think crate authors can be reasonably expected to compile with a compatible gcc. It doesn't even need to be that exact version, - cross-version compat between minor or even major releases of mingw-w64 has been pretty good in my experience. The problems start cropping up when you try to mix different distros/flavors.

I forgot about another major issue however - Rust itself has pretty much no cross-version compat at all. So distributing crates in the binary form is out of question right now :(

@alexcrichton
Copy link
Member Author

Now that #30448 has landed, once #31313 has landed as well I think we'll be able to merge this.

@alexcrichton
Copy link
Member Author

Overall, it seems that this is intentional, and reasonably so, and now that I closed #31313 it looks like we may not be able to pursue this, so closing.

@alexcrichton alexcrichton deleted the swap-order-of-rustc-tools-path branch February 9, 2016 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants