Correctly infer ranlib/ar from cross-gcc #735

jonhoo · 2022-10-26T17:05:32Z

The GCC convention is that if the toolchain is named $target-gnu-gcc, then ar will be available as $target-gnu-gcc-ar. The code as written will infer it to be $target-gnu-ar, which won't work.

I'm not sure why the code that existed was written the way it was -- I don't know of any GCC toolchains where the -gcc is stripped out in the name of ar. The file listing on Debian's GCC 6.x and GCC 10.x all show the binaries using the $target-gnu-gcc-ar format, as does Arch Linux's GCC 12.x.

It may seem odd that the code always adds -gcc for that match arm, but this matches what we do for finding $CC and $CXX where we always inject the GNU prefix when target != host.

Also note that there isn't a special ar for C++, so even when self.cpp == true, we should use -gcc-ar.

Our saving grace, and likely the reason bug reports haven't come in about this, is that we also checks if the resulting binary name is executable, and if it isn't we fall back to the default AR instead. This means the bad heuristic is likely often masked by the presence of another working default AR.

See also alexcrichton/openssl-src-rs#163.

The GCC convention is that if the toolchain is named `$target-gnu-gcc`, then ar will be available as `$target-gnu-gcc-ar`. The code as written will infer it to be `$target-gnu-ar`, which won't work. I'm not sure why the code that existed was written the way it was -- I don't know of any GCC toolchains where the `-gcc` is stripped out in the name of ar. The file listing on Debian's [GCC 6.x] and [GCC 10.x] all show the binaries using the `$target-gnu-gcc-ar` format, as does Arch Linux's [GCC 12.x]. It may seem odd that the code always adds `-gcc` for that match arm, but this matches what we do for finding `$CC` and `$CXX` where we always inject the GNU prefix when target != host. Also note that there isn't a special `ar` for C++, so even when `self.cpp == true`, we should use `-gcc-ar`. Our saving grace, and likely the reason bug reports haven't come in about this, is that we also checks if the resulting binary name is executable, and if it isn't we fall back to the default AR instead. This means the bad heuristic is likely often masked by the presence of another working default AR. See also alexcrichton/openssl-src-rs#163. [GCC 6.x]: https://packages.debian.org/stretch/gcc [GCC 10.x]: https://packages.debian.org/stable/devel/gcc [GCC 12.x]: https://archlinux.org/packages/core/x86_64/gcc/

thomcc

Thanks for the detailed description, that made it much easier to review.

This will break anybody who named their tool this in order to match what cc set. It's ofc more important to do the right thing here WRT the platform, but I'll have to include it in the release notes for the next release (not a big deal, just noting this so I remember).

jonhoo · 2022-10-26T20:28:39Z

Ah, hold on @thomcc, looks like binutils provides ar without -gcc: alexcrichton/openssl-src-rs#163 (comment)

We may need a bit more smarts here to try both.

jonhoo · 2022-10-26T20:31:10Z

I think it's probably reasonable to try both with and without gcc here, and prefer whichever one exists (and gcc if they both do since that's what we infer for CC)

thomcc · 2022-10-26T20:36:07Z

Ah, mind submitting a PR with a fix? I'll push a revert after work (or just do the thing) if you can't get to it.

sfackler [observed] that the `binutils` package [provides] binaries named `$target-ar` (with no `-gcc`). This patch augments the change made in rust-lang#735 to continue picking those up too. [observed]: alexcrichton/openssl-src-rs#163 (comment) [provides]: https://packages.debian.org/bullseye/amd64/binutils-aarch64-linux-gnu/filelist

jonhoo · 2022-10-26T21:31:22Z

Just to leave breadcrumbs: #736

sfackler [observed] that the `binutils` package [provides] binaries named `$target-ar` (with no `-gcc`). This patch augments the change made in #735 to continue picking those up too. [observed]: alexcrichton/openssl-src-rs#163 (comment) [provides]: https://packages.debian.org/bullseye/amd64/binutils-aarch64-linux-gnu/filelist

LTO is enabled by default for .deb packages starting in Ubuntu 21.04[1], causing build failures on Ubuntu 22.04 when we use GCC ranlib (used by default with `cc` crate version >= 1.0.74[2]). LTO is incompatible with `__asm__(".symver ..")` because LTO uses the linker-script-provided exported symbols to determine what is safe to optimize out, and the linker script is naturally unaware of manually-exported symbols. We can work around this by adding `__attribute__((used))` to the functions we want to keep in the final object. Another option would be to use GCC's `__attribute__((symver("..@")))`[3] directive, but this relies on too new of a toolchain version (GCC 10). Addendum 1: The fundamental reason for why LTO is a problem for `aziot-key-openssl-engine-shared` in the first place is that this crate uses what is, in effect, a "symbol stub" to hook Rust code into OpenSSL's engine macros. First, `build/engine.c` declares the engine function signature and uses OpenSSL's macros to expand the dynamic engine binding. This file is then compiled (but not linked) into an object that will become the dynamic library's public interface. The way this is accomplished is by linking the whole object into the `cdylib` (as opposed to only linking referenced functions). LTO requires us to go one step further by preventing the linker from optimizing out symbols not declared as globally-exported in `rustc`'s linker script, which does not know of the symbol declaration in the stub object. There is an open RFC request for allowing re-export of C symbols from `cdylib` crates: rust-lang/rfcs#2771. Addendum 2: When our supported platforms start shipping with GNU as >= 2.35 and/or Clang 13, we may want to add `,remove` to the `.symver` directive arguments to lift the restriction that `aziot-key-openssl-engine-shared` cannot be included in tests[4]. [1]: https://wiki.ubuntu.com/ToolChain/LTO [2]: Binutils ranlib was used before, see discussion in rust-lang/cc-rs#735 for full details. [3]: `..@` instead of `..@@` since we do not define a version node name, meaning double-at leads to symbol duplication. [4]: https://maskray.me/blog/2020-11-26-all-about-symbol-versioning

LTO is enabled by default for .deb packages starting in Ubuntu 21.04[1], causing build failures on Ubuntu 22.04 when we use GCC ranlib (used by default with `cc` crate version >= 1.0.74[2]). LTO is incompatible with `__asm__(".symver ..")` because it uses the linker-script-provided exported symbols to determine what is safe to optimize out, and the linker script is naturally unaware of manually-exported symbols. We can work around this by adding `__attribute__((used))` to the functions we want to keep in the final object. Another option would be to use GCC's `__attribute__((symver("..@")))`[3] directive, but this relies on too new of a toolchain version (GCC 10). Addendum 1: The fundamental reason for why LTO is a problem for `aziot-key-openssl-engine-shared` in the first place is that this crate uses what is, in effect, a "symbol stub" to hook Rust code into OpenSSL's engine macros. First, `build/engine.c` declares the engine function signature and uses OpenSSL's macros to expand the dynamic engine binding. This file is then compiled (but not linked) into an object that will become the dynamic library's public interface. The way this is accomplished is by linking the whole object into the `cdylib` (as opposed to only linking referenced functions). LTO requires us to go one step further by preventing the linker from optimizing out symbols not declared as globally-exported in `rustc`'s linker script, which does not know of the symbol declaration in the stub object. There is an open RFC request for allowing re-export of C symbols from `cdylib` crates: rust-lang/rfcs#2771. Addendum 2: When our supported platforms start shipping with GNU as >= 2.35 and/or Clang 13, we may want to add `,remove` to the `.symver` directive arguments to lift the restriction that `aziot-key-openssl-engine-shared` cannot be included in tests[4]. [1]: https://wiki.ubuntu.com/ToolChain/LTO [2]: Binutils ranlib was used before, see discussion in rust-lang/cc-rs#735 for full details. [3]: `..@` instead of `..@@` since we do not define a version node name, meaning double-at leads to symbol duplication. [4]: https://maskray.me/blog/2020-11-26-all-about-symbol-versioning

thomcc approved these changes Oct 26, 2022

View reviewed changes

thomcc merged commit 4796486 into rust-lang:main Oct 26, 2022

jonhoo mentioned this pull request Oct 26, 2022

Continue to support binutils ar #736

Merged

jonhoo deleted the fix-ar-detection branch October 26, 2022 20:50

nagisa mentioned this pull request Oct 30, 2022

archiver in freebsd images appears to be broken cross-rs/cross#1100

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly infer ranlib/ar from cross-gcc #735

Correctly infer ranlib/ar from cross-gcc #735

jonhoo commented Oct 26, 2022

thomcc left a comment

jonhoo commented Oct 26, 2022

jonhoo commented Oct 26, 2022

thomcc commented Oct 26, 2022 •

edited

jonhoo commented Oct 26, 2022

Correctly infer ranlib/ar from cross-gcc #735

Correctly infer ranlib/ar from cross-gcc #735

Conversation

jonhoo commented Oct 26, 2022

thomcc left a comment

Choose a reason for hiding this comment

jonhoo commented Oct 26, 2022

jonhoo commented Oct 26, 2022

thomcc commented Oct 26, 2022 • edited

jonhoo commented Oct 26, 2022

thomcc commented Oct 26, 2022 •

edited