rustc_target: Refactor internal linker flavors #101988

petrochenkov · 2022-09-18T17:31:07Z

In accordance with the design from #96827 (comment)

lld_flavor and linker_is_gnu fields are removed from internal target specs, but still parsed from JSON specs using compatibility layer introduced in #100552.
r? @lqd

rust-highfive · 2022-09-18T17:31:11Z

⚠️ Warning ⚠️

These commits modify compiler targets. (See the Target Tier Policy.)

lqd · 2022-09-22T18:03:39Z

(I've already started looking at this and will finish this weekend)

luqmana

Woo! Thanks for starting to untangle the mess! Looks good overall, a couple small comments

compiler/rustc_target/src/spec/mod.rs

luqmana · 2022-09-23T23:00:19Z

compiler/rustc_target/src/spec/mod.rs

+            | LinkerFlavor::Darwin(Cc::Yes, _)
+            | LinkerFlavor::WasmLld(Cc::Yes)
+            | LinkerFlavor::Unix(Cc::Yes) => LinkerFlavorCli::Gcc,
+            LinkerFlavor::Gnu(_, Lld::Yes) => LinkerFlavorCli::Lld(LldFlavor::Ld),


Explicit Cc::No here since that's what we want here semantically vs "anything"? Makes it a little more resilient in-case the match arms get moved around in the future. Similarly for Darwin.

That applies to Lld::No too, so you'd have to fill in all the redundant info in variants below to stay consistent. My initial thought was to not fill the redundant info to make it shorter and do not distract attention from the "important" part.

compiler/rustc_target/src/spec/mod.rs

luqmana · 2022-09-23T23:41:57Z

compiler/rustc_codegen_ssa/src/back/link.rs

                } else if stem == "lld" || stem == "rust-lld" {
-                    LinkerFlavor::Lld(sess.target.lld_flavor)
+                    let lld_flavor = sess.target.linker_flavor.lld_flavor();
+                    LinkerFlavor::from_cli(LinkerFlavorCli::Lld(lld_flavor), &sess.target)
                } else {


Given we already have cases for ld.lld, lld-link and wasm-ld is there any reason not to have one for ld64.lld as well?

I just didn't change any logic here, the linker name heuristic can be improved in multiple ways, e.g. accounting for cc and c++/g++/clang++ as well.

Yea, that was just a small thing I noticed while reading through that bit. More for completeness sake since I don't think i've seen any complaints about that case missing

luqmana · 2022-09-23T23:45:52Z

compiler/rustc_codegen_ssa/src/back/link.rs

+                    LinkerFlavor::WasmLld(Cc::No)
+                } else if stem == "ld" || stem.ends_with("-ld") {
+                    LinkerFlavor::from_cli(LinkerFlavorCli::Ld, &sess.target)
+                } else if stem == "ld.lld" {


Come to think of it, does this case even work as expected? file_stem on ld.lld would return ld meaning we never take this branch.

It's reachable if it full file name was ld.lld.exe :)

True but you'd think ld.lld is a lot more common than ld.lld.exe 😛

Not impossible though!

we@we-pc MINGW64 ~ $ where ld.lld C:\msys64\mingw64\bin\ld.lld.exe

compiler/rustc_target/src/spec/wasm32_wasi.rs

compiler/rustc_target/src/spec/wasm64_unknown_unknown.rs

lqd

OK that was a mouthful. It's unfortunate that lld is forcing our hand to make things more complex, but they're also sometimes simpler/clearer than today with the new explicitness (albeit also more verbose).

I left a couple comments in addition to Luqman's (and an if that needs clarification or fixing), and with these done: r=me after rebase.

compiler/rustc_target/src/spec/mod.rs

compiler/rustc_codegen_ssa/src/back/link.rs

compiler/rustc_target/src/spec/wasm32_wasi.rs

lqd · 2022-09-28T21:29:40Z

(Note that Github duplicated comments added to existing conversations when it posted the review, so they lost the context of what they're replying to, but that's present in the original conversations...)

lqd · 2022-09-28T21:30:14Z

@rustbot author

petrochenkov · 2022-10-05T20:18:41Z

@bors r=lqd

bors · 2022-10-05T20:18:43Z

📌 Commit 708874a7f704c5f6e85cb65a4ba488d03ea5603d has been approved by lqd

It is now in the queue for this repository.

bors · 2022-10-05T23:42:09Z

⌛ Testing commit 708874a7f704c5f6e85cb65a4ba488d03ea5603d with merge 2a0993ab9e0963429b6f47804e74f8975ef0ea08...

In accordance with the design from rust-lang#96827 (comment)

petrochenkov · 2022-10-06T09:58:33Z

Fixed an access to unsynchronized json fields in LinkerFlavor::from_cli.
@bors r=lqd

bors · 2022-10-06T09:58:35Z

📌 Commit 572b6a9 has been approved by lqd

It is now in the queue for this repository.

bors · 2022-10-07T03:35:06Z

⌛ Testing commit 572b6a9 with merge cf0fa76...

bors · 2022-10-07T07:31:46Z

☀️ Test successful - checks-actions
Approved by: lqd
Pushing cf0fa76 to master...

rust-timer · 2022-10-07T08:53:40Z

Finished benchmarking commit (cf0fa76): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.3%	[2.1%, 2.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-4.4%, -2.2%]	3
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

the arithmetic mean of the percent change ↩
number of relevant changes ↩

yuriks · 2022-10-08T22:30:10Z

I believe that this change has broken the parsing of pre-link-args in custom json targets:

PS C:\Users\yuriks\projects\mkos> rustc +nightly-2022-10-07 -Z unstable-options --target ..\rust-bootloader\x86_64-bootloader.json --print target-spec-json
{
  "arch": "x86_64",
  "data-layout": "e-m:e-i64:64-f80:128-n8:16:32:64-S128",
  "disable-redzone": true,
  "features": "-mmx,-sse,+soft-float",
  "linker": "rust-lld",
  "linker-flavor": "ld.lld",
  "llvm-target": "x86_64-unknown-none-gnu",
  "panic-strategy": "abort",
  "pre-link-args": {
    "ld.lld": [
      "--script=linker.ld",
      "--gc-sections",
      "--some-invalid-argument"
    ]
  },
  "relocation-model": "static",
  "target-pointer-width": "64"
}
PS C:\Users\yuriks\projects\mkos> rustc +nightly-2022-10-08 -Z unstable-options --target ..\rust-bootloader\x86_64-bootloader.json --print target-spec-json
{
  "arch": "x86_64",
  "data-layout": "e-m:e-i64:64-f80:128-n8:16:32:64-S128",
  "disable-redzone": true,
  "features": "-mmx,-sse,+soft-float",
  "linker": "rust-lld",
  "linker-flavor": "ld.lld",
  "llvm-target": "x86_64-unknown-none-gnu",
  "panic-strategy": "abort",
  "relocation-model": "static",
  "target-pointer-width": "64"
}

I am unable to bisect the breakage down to a specific commit, but this is the only that changed relevant parts of the compiler between the two nightlies, and it seems that this change in particular would cause this breakage:

rust/compiler/rustc_target/src/spec/mod.rs

Lines 1743 to 1745 in cf0fa76

    
           LinkerFlavor::Gnu(_, Lld::Yes) 
        
           | LinkerFlavor::Darwin(_, Lld::Yes) 
        
           | LinkerFlavor::Msvc(Lld::Yes) => {}

cf0fa76#diff-aa810a3be0834da891b171a8b04b09d0d3bbb76ac27c757cd232235099e062d2R1742

Is this intentional? If so, what's the new way of passing custom linker arguments via the target?

petrochenkov · 2022-10-09T05:51:27Z

@yuriks
No, the breakage is not intentional, this PR was supposed to keep backward compatibility with old target specs, I'll check what happens.

petrochenkov · 2022-10-09T09:36:17Z

@yuriks
Fixed in #102836.

rustc_target: Fix json target specs using LLD linker flavors in link args Fixes rust-lang#101988 (comment) (a regression introduced by rust-lang#101988).

Revert to using latest nightly as the rust toolchain since the blocker issue (rust-lang/rust#101988 (comment)) has been resolved. Signed-off-by: Andy-Python-Programmer <[email protected]>

rust-highfive assigned lqd Sep 18, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 18, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2022

petrochenkov force-pushed the flavor2 branch from db8e12d to 9df08f0 Compare September 18, 2022 17:52

This comment was marked as resolved.

Sign in to view

luqmana reviewed Sep 24, 2022

View reviewed changes

lqd approved these changes Sep 28, 2022

View reviewed changes

rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 28, 2022

petrochenkov force-pushed the flavor2 branch from 9df08f0 to 708874a Compare October 5, 2022 20:17

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 Oct 5, 2022

This comment was marked as resolved.

Sign in to view

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

This comment was marked as resolved.

Sign in to view

rustc_target: Refactor internal linker flavors

572b6a9

In accordance with the design from rust-lang#96827 (comment)

petrochenkov force-pushed the flavor2 branch from 708874a to 572b6a9 Compare October 6, 2022 09:57

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 Oct 6, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 7, 2022

bors merged commit cf0fa76 into rust-lang:master Oct 7, 2022

rustbot added this to the 1.66.0 milestone Oct 7, 2022

This was referenced Oct 7, 2022

Migrate codegen_ssa to diagnostics structs - [Part 1] #102612

Merged

Only specify --target by default for -Zgcc-ld=lld on wasm #101792

Merged

Add kernel-address sanitizer support for freestanding targets #99679

Merged

petrochenkov mentioned this pull request Oct 9, 2022

rustc_target: Fix json target specs using LLD linker flavors in link args #102836

Merged

This was referenced Oct 9, 2022

linking fails on Rust nightly-2022-10-08 rust-osdev/bootloader#271

Closed

chore: roll nightly to 2022-10-07 hawkw/mycelium#335

Merged

phil-opp mentioned this pull request Oct 10, 2022

linking with rust-lld failed: exit status: 1 rust-osdev/bootimage#87

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc_target: Refactor internal linker flavors #101988

rustc_target: Refactor internal linker flavors #101988

petrochenkov commented Sep 18, 2022

rust-highfive commented Sep 18, 2022

This comment was marked as resolved.

lqd commented Sep 22, 2022

luqmana left a comment

luqmana Sep 23, 2022

petrochenkov Sep 24, 2022

luqmana Sep 23, 2022

petrochenkov Sep 24, 2022

luqmana Sep 24, 2022

luqmana Sep 23, 2022

petrochenkov Sep 24, 2022

luqmana Sep 24, 2022

petrochenkov Sep 24, 2022 •

edited

Loading

lqd left a comment

lqd commented Sep 28, 2022

lqd commented Sep 28, 2022

petrochenkov commented Oct 5, 2022

bors commented Oct 5, 2022

bors commented Oct 5, 2022

This comment was marked as resolved.

This comment was marked as resolved.

petrochenkov commented Oct 6, 2022

bors commented Oct 6, 2022

bors commented Oct 7, 2022

bors commented Oct 7, 2022

rust-timer commented Oct 7, 2022

yuriks commented Oct 8, 2022 •

edited

Loading

petrochenkov commented Oct 9, 2022

petrochenkov commented Oct 9, 2022

rustc_target: Refactor internal linker flavors #101988

rustc_target: Refactor internal linker flavors #101988

Conversation

petrochenkov commented Sep 18, 2022

rust-highfive commented Sep 18, 2022

This comment was marked as resolved.

lqd commented Sep 22, 2022

luqmana left a comment

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

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

Choose a reason for hiding this comment

petrochenkov Sep 24, 2022 • edited Loading

Choose a reason for hiding this comment

lqd left a comment

Choose a reason for hiding this comment

lqd commented Sep 28, 2022

lqd commented Sep 28, 2022

petrochenkov commented Oct 5, 2022

bors commented Oct 5, 2022

bors commented Oct 5, 2022

This comment was marked as resolved.

This comment was marked as resolved.

petrochenkov commented Oct 6, 2022

bors commented Oct 6, 2022

bors commented Oct 7, 2022

bors commented Oct 7, 2022

rust-timer commented Oct 7, 2022

Overall result: no relevant changes - no action needed

Footnotes

yuriks commented Oct 8, 2022 • edited Loading

petrochenkov commented Oct 9, 2022

petrochenkov commented Oct 9, 2022

petrochenkov Sep 24, 2022 •

edited

Loading

yuriks commented Oct 8, 2022 •

edited

Loading