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.

Sign up for GitHub

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

Jump to bottom

Windows GNU Targets Failing with LTO #141

Closed
coltonhurst opened this issue Sep 17, 2024 · 5 comments
Closed

Windows GNU Targets Failing with LTO #141

coltonhurst opened this issue Sep 17, 2024 · 5 comments
Labels
discussion

Comments

@coltonhurst
Copy link

Hello 👋

I am experiencing build failures with rustls-platform-verifier when building for release with lto when targeting the x86_64-pc-windows-gnu target.

I created an example repository with the potential bug: https://github.com/coltonhurst/rustls-platform-verifier-lto-bug

My log.txt containing the errors is here:

log.txt

Thank you!
@coltonhurst coltonhurst changed the title Windows GNU Targets Failing Windows GNU Targets Failing with LTO Sep 17, 2024
@coltonhurst coltonhurst mentioned this issue Sep 17, 2024
Merged
@ctz
Copy link
Member

ctz commented Sep 17, 2024

Is this rust-lang/rust#109797?

@complexspaces
Copy link
Collaborator

complexspaces commented Sep 17, 2024 •
edited
Loading

I'm not sure if its that exact bug or not, but this looks like a compiler regression for sure. These are my reasons for thinking so:

  • We don't perform any manual linkage in this library and let winapi or windows_sys handle that part.
  • The referenced symbol missing is coming from the standard library's random generation APIs, so the linker is having a hard time finding the right parts for our library's object, not the final binary failing to find something from this crate.

I spent some time checking compiler versions and this seems like it regressed in 1.80.0 for me locally. Switching to fat LTO also fixes it, further indicating a bug in the toolchain.

This build works without any issues: cargo +1.79.0 build --target x86_64-pc-windows-gnu --release
This fails and produces the same linker error: cargo +1.80.0 build --target x86_64-pc-windows-gnu --release

@complexspaces
Copy link
Collaborator

complexspaces commented Sep 17, 2024 •
edited
Loading

FWIW, I would personally recommend not using the GNU toolchain for Rust on Windows if possible. Its full of bugs in the toolchain/runtime layer, resulting in a lot of non-ideal workarounds or features being disabled in Rust tooling/std because of it.

If Bitwarden doesn't want to use MSVC (or any MS tooling), clang and the LLVM toolchain might work instead combined with the strategy in this blog if needed.

@coltonhurst
Copy link
Author

Ah yes, looks like it could be the compiler regression rust-lang/rust#109797.

Fair point regarding GNU for Rust on Windows. Right now our Go binding is built with our Rust SDK with cgo, which doesn't seem to have great support for anything besides gcc. The CGo Wiki mentions only gcc will work 🥲 though we have tried other compilers. Maybe in the future we can find a better solution.

@ctz @complexspaces thanks for the detailed investigation, as usual!

coltonhurst added a commit to bitwarden/sdk-sm that referenced this issue Sep 19, 2024
@coltonhurst
[SM-1444] Fix Windows GNU Builds (#1053)
7641717 
## 🎟️ Tracking

https://bitwarden.atlassian.net/browse/SM-1444

## 📔 Objective

Windows GNU builds are currently broken due to a bug in
[rustls-platform-verifier](https://github.com/rustls/rustls-platform-verifier)
when LTO is turned on.

I have submitted a GH Issue for this here:
rustls/rustls-platform-verifier#141

## ⏰ Reminders before review

- Contributor guidelines followed
- All formatters and local linters executed and passed
- Written new unit and / or integration tests where applicable
- Protected functional changes with optionality (feature flags)
- Used internationalization (i18n) for all UI strings
- CI builds passed
- Communicated to DevOps any deployment requirements
- Updated any necessary documentation (Confluence, contributing docs) or
informed the documentation
  team

## 🦮 Reviewer guidelines

<!-- Suggested interactions but feel free to use (or not) as you desire!
-->

- 👍 (`:+1:`) or similar for great changes
- 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info
- ❓ (`:question:`) for questions
- 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry
that's not quite a confirmed
  issue and could potentially benefit from discussion
- 🎨 (`:art:`) for suggestions / improvements
- ❌ (`:x:`) or ⚠️ (`:warning:`) for more significant problems or
concerns needing attention
- 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or
indications of technical debt
- ⛏ (`:pick:`) for minor or nitpick changes
@cpu
Copy link
Member

cpu commented Oct 15, 2024

I think this can probably be closed since there's no action to take in this crate, and there's a documented workaround for the compiler regression.

(If I'm wrong and there's a reason to keep this open feel free to flip it back!)

@cpu cpu closed this as not planned Won't fix, can't repro, duplicate, stale Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
discussion
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@cpu @ctz @coltonhurst @complexspaces