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

Re-do the FreeBSD cross-builds to use Clang and libc++. Fixes #44433 #46941

Merged
merged 2 commits into from
Dec 26, 2017
Merged

Re-do the FreeBSD cross-builds to use Clang and libc++. Fixes #44433 #46941

merged 2 commits into from
Dec 26, 2017

Conversation

ScottAbbey
Copy link
Contributor

Reviving #45077, from @jld:

The main goal here is to use FreeBSD's normal libc++, instead of
statically linking the libstdc++ packaged with GCC, because that
libstdc++ has bugs that cause rustc to deadlock inside LLVM.

But the easiest way to use libc++ is to switch the build from GCC to
Clang, and the Clang package in the Ubuntu image already knows how to
cross-compile (given a sysroot and preferably cross-binutils), so the
toolchain script now uses that instead of building a custom compiler.

This also de-duplicates the build-toolchain.sh script.

#45077 was close but didn't quite make it. I rebased @jld's work off the current master and started with that.

I was able to determine that this Travis error (#45077 (comment)) was ultimately caused by src/librustc_llvm/build.rs attempting to follow a wrong value in LLVM_STATIC_STDCPP (#45077 (comment)).

I looked at the downstream port for FreeBSD (https://svnweb.freebsd.org/ports/head/lang/rust/) and it seems like they do not use --enable-llvm-static-stdcpp.

Since libc++ is included in the FreeBSD 10+ base system, we don't need to statically link it either?

So in b989428 I have set the FreeBSD build to not actually use LLVM_STATIC_STDCPP.

I was able to run ./src/ci/docker/run.sh with both dist-i686-freebsd and dist-x86_64-freebsd successfully and in about 1 minute of testing it seemed like the dist-x86_64-freebsd results worked on a FreeBSD 11 system.

It should fix #44433, which seems to be affecting many potential users. Also FreeBSD users should be able to ./x.py build which should help anyone who wants to upstream fixes for FreeBSD.

Questions:

Does this approach seem to be the right way to go? Do we actually really want to statically link libc++? (I tried that here, but it ultimately ran into a roadblock on x86_64: #45077 (comment))

Can we rewrite the comment here to be more clear about why some systems aren't going to actually use this option:

// Building with a static libstdc++ is only supported on linux right now,
// not for MSVC or macOS
if build.config.llvm_static_stdcpp &&
!target.contains("freebsd") &&

How does this affect users of older FreeBSD systems? It seemed like no one was complaining about using a 10.3 base version in the thread for #45077. FreeBSD seems to only officially support 10.3, 10.4, and 11.x right now, do we have to consider older users? The libc++ stuff came in for FreeBSD 10, older FreeBSD used libstdc++.

Looks like @alexcrichton was leading the discussion on the previous issue:

r? @alexcrichton

Let me know what I can do to help get this through.

jld and others added 2 commits December 22, 2017 02:34
The main goal here is to use FreeBSD's normal libc++, instead of
statically linking the libstdc++ packaged with GCC, because that
libstdc++ has bugs that cause rustc to deadlock inside LLVM.

But the easiest way to use libc++ is to switch the build from GCC to
Clang, and the Clang package in the Ubuntu image already knows how to
cross-compile (given a sysroot and preferably cross-binutils), so the
toolchain script now uses that instead of building a custom compiler.

This also de-duplicates the `build-toolchain.sh` script.
The code inside this conditional will not work on FreeBSD 10+ because
those versions use clang and libc++ rather than libstdc++.

Since FreeBSD comes with libc++ in the base, presumably all 10+ systems
will have it present.

Searching for libstdc++.a will not work if it is not present.  As a
result, this would previously have set `LLVM_STATIC_STDCPP=libstdc++.a`,
which isn't a valid path and caused problems later on when building
`librustc_llvm`.

This could possibly be updated in the future to look for `libc++.a` on
FreeBSD, by expanding the code inside the conditional.  In one attempt
to run this on x86_64-freebsd, I found that libc++ was not compiled with
PIC, so it failed anyway.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Member

Thanks! The main reason for linking the C++ standard library was more ABI compatibility in the sense that we can work with any number of Linux systems without worrying about the version of the C++ standard library they have installed (aka one less dependency for rustc-the-executable).

Maybe that's not an issue with FreeBSD though? We're already not binary-compatible across major releases and it could be that within a release there's only one version of libc++?

@ScottAbbey
Copy link
Contributor Author

ScottAbbey commented Dec 23, 2017

I very much appreciate any guidance, thank you.

Maybe that's not an issue with FreeBSD though?

Since libc++ is part of the base, it is present on any FreeBSD 10+ system. So the important question is whether or not it is compatible across versions.

We're already not binary-compatible across major releases and it could be that within a release there's only one version of libc++?

I really don't know much about FreeBSD, so I tried to investigate this.

It looks like within a major release of FreeBSD, compatibility is important. So 10.3 to 10.4 should be fine. But jumping to 11.x would not be.

So as-is, the results of this PR would presumably work fine with FreeBSD 10.{3+}. But it might not work for FreeBSD 11+?


So with the strategy here of NOT using LLVM_STATIC_STDCPP, we would need to build more separate targets, like dist-x86_64-freebsd10 and dist-x86_64-freebsd11. I'm guessing that's not the path you want.


Alternatively, we can figure out how to statically link libc++, which presumably would produce a result that would work across major versions?

So the desire would be to statically link libc++. The current FreeBSD builds statically link libstdc++, which results in the deadlock issue (#44433). Since FreeBSD itself uses libc++, we probably want to use that too.

As noted earlier, I attempted statically linking libc++ and it seemed to work fine for dist-i686-freebsd but failed for dist-x86_64-freebsd. Notes from this attempt are at #45077 (comment).

The important bits of the error were:

 = note: /usr/local/x86_64-unknown-freebsd10/bin/ld: /usr/local/x86_64-unknown-freebsd10/usr/lib/libc++.a(valarray.o): relocation R_X86_64_32 against `__gxx_personality_v0' can not be used when making a shared object; recompile with -fPIC
          /usr/local/x86_64-unknown-freebsd10/usr/lib/libc++.a(valarray.o): error adding symbols: Bad value
          clang: error: linker command failed with exit code 1 (use -v to see invocation)

This seems to indicate that the libc++.a downloaded with the base in this cross-compilation is not compiled with -fPIC. So it can't be statically linked.

So we need to also compile libc++ with -fPIC so that it can be used instead of the one downloaded with FreeBSD 10 base. Presumably we use clang and the FreeBSD sources to do this.

Does this sound correct? I don't immediately know how to do this but I can continue to work on it. Mainly I'd like to find some confidence that I am at least on the right track.

@ScottAbbey
Copy link
Contributor Author

So we need to also compile libc++ with -fPIC so that it can be used instead of the one downloaded with FreeBSD 10 base. Presumably we use clang and the FreeBSD sources to do this.

After looking into this a bit, it seems like it would be very difficult to compile anything from FreeBSD sources on the Ubuntu build machine without doing it in a virtual machine.

Maybe first we should try testing the dist artifacts this current PR produces?

I have a rust-nightly-i686-unknown-freebsd.tar.xz and a rust-nightly-x86_64-unknown-freebsd.tar.xz produced from this by running the ./src/ci/docker/run.sh. Can you describe what process I would use if I wanted to test whether these work on various FreeBSD systems?

@alexcrichton
Copy link
Member

@ScottAbbey oh I wouldn't worry too much about compatibility, the ABI of functions like stat broke between 10 and 11 so we're already not compatible which means we'd already have to deal with questions like multiple targets anyway!

In that case though sounds like we should get this in to test it, and then we can evaluate what to do next!

@bors: r+

@bors
Copy link
Contributor

bors commented Dec 25, 2017

📌 Commit b989428 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Dec 25, 2017

⌛ Testing commit b989428 with merge 8944069f8e3f0e26228d5bc3895700c22afd466b...

@bors
Copy link
Contributor

bors commented Dec 25, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Dec 25, 2017

@bors retry #47002

@bors
Copy link
Contributor

bors commented Dec 26, 2017

⌛ Testing commit b989428 with merge f177bd0b40458df86eb249781f098a8a89efaab5...

@bors
Copy link
Contributor

bors commented Dec 26, 2017

💔 Test failed - status-travis

@bors
Copy link
Contributor

bors commented Dec 26, 2017

⌛ Testing commit b989428 with merge f84089f6757cd5104b731c8a813eafcac4257ae2...

@bors
Copy link
Contributor

bors commented Dec 26, 2017

💔 Test failed - status-travis

@bors
Copy link
Contributor

bors commented Dec 26, 2017

⌛ Testing commit b989428 with merge 0efdfa1...

bors added a commit that referenced this pull request Dec 26, 2017
Re-do the FreeBSD cross-builds to use Clang and libc++. Fixes #44433

Reviving #45077, from @jld:

> The main goal here is to use FreeBSD's normal libc++, instead of
> statically linking the libstdc++ packaged with GCC, because that
> libstdc++ has bugs that cause rustc to deadlock inside LLVM.
>
> But the easiest way to use libc++ is to switch the build from GCC to
> Clang, and the Clang package in the Ubuntu image already knows how to
> cross-compile (given a sysroot and preferably cross-binutils), so the
> toolchain script now uses that instead of building a custom compiler.
>
> This also de-duplicates the build-toolchain.sh script.

#45077 was close but didn't quite make it.  I rebased @jld's work off the current `master` and started with that.

I was able to determine that this Travis error (#45077 (comment)) was ultimately caused by `src/librustc_llvm/build.rs` attempting to follow a wrong value in `LLVM_STATIC_STDCPP` (#45077 (comment)).

I looked at the downstream port for FreeBSD (https://svnweb.freebsd.org/ports/head/lang/rust/) and it seems like they do not use `--enable-llvm-static-stdcpp`.

Since `libc++` is included in the FreeBSD 10+ base system, we don't need to statically link it either?

So in b989428 I have set the FreeBSD build to not actually use `LLVM_STATIC_STDCPP`.

I was able to run `./src/ci/docker/run.sh` with both `dist-i686-freebsd` and `dist-x86_64-freebsd` successfully and in about 1 minute of testing it seemed like the dist-x86_64-freebsd results worked on a FreeBSD 11 system.

It should fix #44433, which seems to be affecting many potential users.  Also FreeBSD users should be able to `./x.py build` which should help anyone who wants to upstream fixes for FreeBSD.

Questions:

Does this approach seem to be the right way to go? Do we actually really want to statically link `libc++`? (I tried that here, but it ultimately ran into a roadblock on x86_64: #45077 (comment))

Can we rewrite the comment here to be more clear about why some systems aren't going to actually use this option:
https://github.com/rust-lang/rust/blob/b989428f7dec7b52d68bed6a21e9b5b0a8086267/src/bootstrap/compile.rs#L550-L553

How does this affect users of older FreeBSD systems? It seemed like no one was complaining about using a 10.3 base version in the thread for #45077.  FreeBSD seems to only officially support 10.3, 10.4, and 11.x right now, do we have to consider older users? The `libc++` stuff came in for FreeBSD 10, older FreeBSD used `libstdc++`.

Looks like @alexcrichton was leading the discussion on the previous issue:

r? @alexcrichton

Let me know what I can do to help get this through.
@bors
Copy link
Contributor

bors commented Dec 26, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 0efdfa1 to master...

@bors bors merged commit b989428 into rust-lang:master Dec 26, 2017
@ScottAbbey ScottAbbey deleted the freebsd-build-update branch December 26, 2017 23:11
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.

rustc deadlocks on FreeBSD
6 participants