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

Use -windows-gnu for all UEFI targets #1264

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

nicholasbishop
Copy link
Contributor

@nicholasbishop nicholasbishop commented Nov 3, 2024

This restores the behavior prior to #1252, when the UEFI targets were hardcoded in src/lib.rs.

Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @madsmtm can you take a look at this please?

@madsmtm
Copy link
Collaborator

madsmtm commented Nov 3, 2024

Huh, I seemed to remember explicitly verifying that the code did the same thing, but I must have missed this, sorry about that :/

I'm not against doing this fix in cc if that's where it's needed, and the code looks fine. That said, I would really like some documentation on it!

I found this code comment in the rustc sources talking a bit about why -gnu is necessary, but I would expect a comment in similar regard in cc, covering at least:

  • Why cc differs from rustc here (more linking failures?).
  • Reasoning why it is sound for cc to use a different target than rustc (are there calling convention concerns?).
  • A link to an LLVM issue or something about this, where we could work on getting proper support for Windows UEFI targets.

@nicholasbishop
Copy link
Contributor Author

nicholasbishop commented Nov 3, 2024

That makes sense. Unfortunately I don't have answers to most of those questions yet. The specific motivation for this PR was trying to build https://github.com/Amanieu/minicov for x86_64-unknown-uefi; that used to work, but stopped working with cc-v1.1.32. When the LLVM target is x86_64-unknown-windows, I get an error from a missing malloc.h.

Possibly the Rust UEFI aarch64/x86_64 targets should be switched to -windows-gnu, as i686 already is.

@madsmtm
Copy link
Collaborator

madsmtm commented Nov 3, 2024

Fair enough, then perhaps a comment that we don't know why this hack is needed, and link to this PR ;)

Alternatively, if you're feeling venturous, I found https://krinkinmu.github.io/2020/10/11/efi-getting-started.html#bulding, which seems to suggest that the actual fix is to pass the -ffreestanding flag?

@madsmtm
Copy link
Collaborator

madsmtm commented Nov 3, 2024

Actually, seems like LLVM should have support for UEFI nowadays: https://discourse.llvm.org/t/rfc-uefi-driver-support-uefi-target/73261.

So maybe pass those targets (*-unknown-uefi) to Clang instead? (Though I wouldn't block a quickfix on that).

@madsmtm
Copy link
Collaborator

madsmtm commented Nov 3, 2024

Opened rust-lang/rust#132570

This restores the behavior prior to
rust-lang#1252, when the UEFI targets were
hardcoded in src/lib.rs.
@nicholasbishop
Copy link
Contributor Author

Thanks for opening that PR! I wasn't aware clang had added these targets.

I've updated this PR to add a comment describing the history, the current state, and a TODO for the future.

Also tweaked the way the override is set, so that it won't immediately change to follow rustc if the rustc targets are changed, since I think we would want to be intentional about changing cc-rs to match.

Copy link
Collaborator

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now, thanks for writing things out!

@NobodyXu NobodyXu merged commit 140f595 into rust-lang:main Nov 4, 2024
27 checks passed
@github-actions github-actions bot mentioned this pull request Nov 4, 2024
@nicholasbishop nicholasbishop deleted the bishop-fix-uefi-target branch November 4, 2024 02:40
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.

3 participants