rustc_target: Refactor target specifications related to Windows and UEFI #71030

petrochenkov · 2020-04-11T15:18:13Z

LLD support is improved.
Code is cleaned up.
Target specs are organized in a more hierarchical way.
Possible issues in UWP and UEFI platforms are identified (see FIXMEs).

The code is better read per-commit.

rust-highfive · 2020-04-11T15:18:16Z

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

petrochenkov · 2020-04-11T15:18:34Z

r? @nagisa
cc @mati865 @Amanieu because Windows/GNU
cc @retep998 because Windows/MSVC
cc @chouquette because UWP
cc @dvdhrm because UEFI

mati865

Gnu part looks OK.

src/librustc_target/spec/windows_uwp_gnu_base.rs

mati865 · 2020-04-11T16:06:33Z

src/librustc_target/spec/windows_uwp_gnu_base.rs

@@ -1,6 +1,9 @@
 use crate::spec::{LinkArgs, LinkerFlavor, TargetOptions};

 pub fn opts() -> TargetOptions {
+    let base = super::windows_gnu_base::opts();
+
+    // FIXME: Consider adding `-nostdlib` and inheriting from `windows_gnu_base`.


Yeah, this probably doesn't work great right now.
There is only one prebuilt C toolchain that advertises UWP support but it cannot be easily tested until LLD can link Rust...

dvdhrm · 2020-04-11T17:49:09Z

src/librustc_target/spec/uefi_msvc_base.rs

+        // FIXME: This should likely be `true` inherited from `msvc_base`
+        // because UEFI follows Windows ABI and uses PE/COFF.
+        // The `false` is probably causing ABI bugs right now.
+        is_like_msvc: false,


I am not even sure what these two are supposed to mean. They show up in the rustc code all over the place, but there is no real explanation to what they exactly control. When I wrote the UEFI target, I checked all their usages and was quite sure they do not effect any behavior on UEFI. However, checking again now, either their usage has increased or I missed some places back them.

I guess we should set them to true? I am quite certain that either way we will end up with code not meant for UEFI (either now or in the future when someone picks that option to toggle one more knob).

Regardless, I think it is independent of your PR. So I am definitely fine with adding these 2 comments. Thanks a lot for digging through this!

They show up in the rustc code all over the place, but there is no real explanation to what they exactly control.

is_like_msvc and is_like_windows are some ad-hoc flags controlling target-dependent behavior (introduced in #16156, #25350).
Sometimes it may mean "uses Windows ABI", sometimes "the produced executable is PE/COFF", sometimes "search libraries in directories specific to visual studio", or something else like "uses PDB as debuginfo".

I'm not sure all of these aspects apply to UEFI, so enabling them may both fix something and break something.

These flags likely need to be split into multiple more specific flags, but nobody bothered to do it yet because we had only windows-msvc and windows-gnu targets until recently.

src/librustc_target/spec/msvc_base.rs

dvdhrm · 2020-04-11T17:58:10Z

src/librustc_target/spec/uefi_base.rs

-        ],
-    );
+    pre_link_args.insert(LinkerFlavor::Msvc, pre_link_args_msvc.clone());
+    pre_link_args.insert(LinkerFlavor::Lld(LldFlavor::Link), pre_link_args_msvc);


I am really unsure what this commit does. Can you explain? Your commit message says Make sure lld-link is treated as link.exe by default, which I don't understand. What is wrong with lld-link? You can compile with lld-link on linux machines just fine, no need for link.exe.

Also, here you add support for MSVC-linking, right? Can you explain how this is related to this commit? I think it looks correct, but I am quite a bit lost with these changes. Sorry.

The goal of this commit is to make sure that if something can be linked with link.exe then it can be linked with lld-link as well, and vice versa (assuming good enough LLD's feature-parity with link.exe).

UEFI was one of the targets that supported linking with lld-link but not with link.exe for no good reason.

Right, that makes sense. Looks good! Thanks!

bors · 2020-04-12T00:44:30Z

☔ The latest upstream changes (presumably #71031) made this pull request unmergeable. Please resolve the merge conflicts.

nagisa · 2020-04-12T23:38:02Z

src/librustc_target/spec/uefi_msvc_base.rs

+        // Non-standard subsystems have no default entry-point in PE+ files. We have to define
+        // one. "efi_main" seems to be a common choice amongst other implementations and the
+        // spec.
+        "/entry:efi_main".to_string(),


Hmm, FWIW the #[start] function still generates main symbol for the entry point.

But appears to be pre-existing, so nvmd.

/entry specifies the binary entry point which main is not.

nagisa · 2020-04-12T23:49:29Z

@bors r+

bors · 2020-04-12T23:49:30Z

📌 Commit 1c04d33b7505bff06bbf12313865f903c7982498 has been approved by nagisa

bors · 2020-04-13T04:28:29Z

☔ The latest upstream changes (presumably #71023) made this pull request unmergeable. Please resolve the merge conflicts.

The differences if they are discovered will need to be explicitly documented

as much as possible.

The old naming is from ancient times when there was no MSVC support. Also `uefi_base` -> `uefi_msvc_base`. It will inherit from `msvc_base` in a future commit, plus a GNU UEFI target is also potentially possible.

and inherit both `windows_msvc_base` and `uefi_msvc_base` from it.

petrochenkov · 2020-04-13T15:55:44Z

@bors r=nagisa

bors · 2020-04-13T15:55:45Z

📌 Commit 8392e47 has been approved by nagisa

rustc_target: Refactor target specifications related to Windows and UEFI - LLD support is improved. - Code is cleaned up. - Target specs are organized in a more hierarchical way. - Possible issues in UWP and UEFI platforms are identified (see FIXMEs). The code is better read per-commit.

@ghost

Rollup of 4 pull requests Successful merges: - rust-lang#70891 (unit rvalue, use constant `()` instead of tuple) - rust-lang#71030 (rustc_target: Refactor target specifications related to Windows and UEFI) - rust-lang#71100 (Miri: expand frame hooks) - rust-lang#71116 (Entirely remove `DUMMY_HIR_ID`) Failed merges: r? @ghost

rustc_target: Mark UEFI targets as `is_like_windows`/`is_like_msvc` And document what `is_like_windows` and `is_like_msvc` actually mean in more detail. Addresses FIXMEs left from rust-lang#71030. r? `@nagisa`

rustc_target: Mark UEFI targets as `is_like_windows`/`is_like_msvc` And document what `is_like_windows` and `is_like_msvc` actually mean in more detail. Addresses FIXMEs left from rust-lang#71030. r? ``@nagisa``

rustc_target: Mark UEFI targets as `is_like_windows`/`is_like_msvc` And document what `is_like_windows` and `is_like_msvc` actually mean in more detail. Addresses FIXMEs left from rust-lang#71030. r? ```@nagisa```

rustc_target: Mark UEFI targets as `is_like_windows`/`is_like_msvc` And document what `is_like_windows` and `is_like_msvc` actually mean in more detail. Addresses FIXMEs left from rust-lang#71030. r? `@nagisa`

rust-highfive assigned varkor Apr 11, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 11, 2020

rust-highfive assigned nagisa and unassigned varkor Apr 11, 2020

mati865 approved these changes Apr 11, 2020

View reviewed changes

dvdhrm reviewed Apr 11, 2020

View reviewed changes

petrochenkov force-pushed the linkorder2 branch from ec5383b to 1c04d33 Compare April 12, 2020 20:50

petrochenkov mentioned this pull request Apr 12, 2020

[WIP] Encode more info about order-dependence of linker arguments #71081

Closed

nagisa reviewed Apr 12, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 12, 2020

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 13, 2020

petrochenkov added 9 commits April 13, 2020 18:37

rustc_target: Make sure lld-link is treated as link.exe by default

3ad8c88

The differences if they are discovered will need to be explicitly documented

rustc_target: Move tests into a separate unconfigured file

7a4f059

as much as possible.

linker: Pass /NODEFAULTLIB in a more regular way

cfe90eb

rustc_target: windows(_uwp)_base -> windows(_uwp)_gnu_base

6580ceb

The old naming is from ancient times when there was no MSVC support. Also `uefi_base` -> `uefi_msvc_base`. It will inherit from `msvc_base` in a future commit, plus a GNU UEFI target is also potentially possible.

rustc_target: Remove some useless imports

9c9db3a

rustc_target: Inherit windows_uwp_msvc_base from windows_msvc_base

fd0e298

rustc_target: Inherit windows_uwp_gnu_base from windows_gnu_base

88c4802

rustc_target: Introduce msvc_base

57870ea

and inherit both `windows_msvc_base` and `uefi_msvc_base` from it.

Address review comments

8392e47

petrochenkov force-pushed the linkorder2 branch from 1c04d33 to 8392e47 Compare April 13, 2020 15:54

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 13, 2020

Centril mentioned this pull request Apr 15, 2020

Rollup of 5 pull requests #71161

Closed

Centril mentioned this pull request Apr 15, 2020

Rollup of 4 pull requests #71162

Merged

bors merged commit ffafd15 into rust-lang:master Apr 15, 2020

petrochenkov mentioned this pull request Apr 15, 2020

Add linker arguments from i686-pc-windows-msvc to LLD too #68646

Closed

petrochenkov mentioned this pull request Nov 11, 2020

rustc_target: Mark UEFI targets as is_like_windows/is_like_msvc #78959

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc_target: Refactor target specifications related to Windows and UEFI #71030

rustc_target: Refactor target specifications related to Windows and UEFI #71030

petrochenkov commented Apr 11, 2020

rust-highfive commented Apr 11, 2020

petrochenkov commented Apr 11, 2020

mati865 left a comment

mati865 Apr 11, 2020

dvdhrm Apr 11, 2020

petrochenkov Apr 12, 2020 •

edited

Loading

dvdhrm Apr 11, 2020

petrochenkov Apr 12, 2020

dvdhrm Apr 13, 2020

bors commented Apr 12, 2020

nagisa Apr 12, 2020

nagisa Apr 12, 2020

retep998 Apr 13, 2020

nagisa commented Apr 12, 2020

bors commented Apr 12, 2020

bors commented Apr 13, 2020

petrochenkov commented Apr 13, 2020

bors commented Apr 13, 2020

rustc_target: Refactor target specifications related to Windows and UEFI #71030

rustc_target: Refactor target specifications related to Windows and UEFI #71030

Conversation

petrochenkov commented Apr 11, 2020

rust-highfive commented Apr 11, 2020

petrochenkov commented Apr 11, 2020

mati865 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov Apr 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa commented Apr 12, 2020

bors commented Apr 12, 2020

bors commented Apr 13, 2020

petrochenkov commented Apr 13, 2020

bors commented Apr 13, 2020

petrochenkov Apr 12, 2020 •

edited

Loading