migrate compiler, bootstrap and compiletest to windows-rs #106610

euclio · 2023-01-09T00:50:26Z

This PR migrates the compiler, bootstrap, and compiletest to use windows-rs instead of winapi-rs. windows-rs is the bindings crate provided by Microsoft, and is actively maintained compared to winapi-rs. Not all ecosystem crates have migrated over yet, so there will be a period of time where both crates are used.

windows-rs also provides some nice ergonomics over winapi-rs to convert return values to Results (which found a case where we forgot to check the return value of CreateFileW).

rustbot · 2023-01-09T00:50:32Z

r? @davidtwco

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

davidtwco · 2023-01-09T09:04:43Z

r? @jyn514

klensy · 2023-01-09T11:19:05Z

Shouldn't windows-sys be used instead?

https://github.com/microsoft/windows-rs#windows-sys: The windows-sys crate is a zero-overhead fallback for the most demanding situations

ChrisDenton · 2023-01-09T11:56:11Z

Unless it makes compiling rustc significantly slower, I think the windows crate is better for this. The "overhead", such as returning Results, is helpful and is something we'd want to do anyway.

klensy · 2023-01-09T11:58:55Z

Unless it makes compiling rustc significantly slower

But there no windows perf runs, yet.

ChrisDenton · 2023-01-09T14:30:06Z

Sure. I wouldn't expect the compile time of one crate to have a significant impact here though. And if it does we should see slower CI times. We can always switch to windows-sys if it does cause problems.

ChrisDenton

Looks good. I just have a few comments, mostly nits.

compiler/rustc_data_structures/src/profiling.rs

compiler/rustc_errors/src/lock.rs

compiler/rustc_session/src/filesearch.rs

ChrisDenton · 2023-01-09T14:02:05Z

src/bootstrap/bin/rustc.rs

+    unsafe { FileTimeToSystemTime(&user_filetime, &mut user_time) }.ok().ok()?;
+    unsafe { FileTimeToSystemTime(&kernel_filetime, &mut kernel_time) }.ok().ok()?;


Rather than using unsafe on almost every line, why not just keep the big unsafe block?

Personally I prefer limiting the scope as much as possible, to just the FFI calls.

src/bootstrap/util.rs

compiler/rustc_errors/src/lock.rs

jyn514 · 2023-01-10T00:02:59Z

The idea seems fine, but I'm short on review time and not familiar with Windows.

r? @ChrisDenton (thank you for your reviews so far ❤️)

bors · 2023-01-14T17:46:40Z

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

rustbot · 2023-01-15T18:41:35Z

Some changes occurred in src/tools/cargo

cc @ehuss

euclio · 2023-01-15T18:43:57Z

Sorry for the ping, accidentally included the cargo submodule in the previous rebase.

ChrisDenton · 2023-01-16T16:19:57Z

I'm happy to review this; I'd mostly be keen to make sure the API calls end up the same. However, I'm not sure that I'm actually authorized to sign off on this change (I'm more std/libs). cc @Mark-Simulacrum maybe?

Moving on from winapi (which is no longer updated) makes sense to do at some point. @klensy brings up the point that maybe we should be using windows-sys instead of windows due to compile times (see comment). But I would be keen to at least try windows for correctness reasons (e.g. its use of Result helps avoid at least one source of potential mistakes).

jyn514 · 2023-01-16T16:23:37Z

@ChrisDenton consider this permission to sign off of the change :) I trust your judgement.

@bors delegate=ChrisDenton

bors · 2023-01-16T16:23:39Z

✌️ @ChrisDenton can now approve this pull request

ChrisDenton · 2023-01-16T16:33:14Z

Oh ok! In that case I'm fine with this as long as CI is also ok.

@bors r+ rollup=never

bors · 2023-01-16T16:33:16Z

📌 Commit 2c691ae6036bab3dbd06ca0f0cd8c7e28f3b473f has been approved by ChrisDenton

It is now in the queue for this repository.

riverar · 2023-03-20T14:51:07Z

@euclio Hey Andy, mind resolving the conflict here so we can give bors another whack at this?

ChrisDenton · 2023-03-20T18:04:36Z

Ok, lets try again @bors r+

bors · 2023-03-20T18:04:38Z

📌 Commit bb7c373 has been approved by ChrisDenton

It is now in the queue for this repository.

bors · 2023-03-20T20:05:49Z

⌛ Testing commit bb7c373 with merge 44f5180...

bors · 2023-03-20T22:36:58Z

☀️ Test successful - checks-actions
Approved by: ChrisDenton
Pushing 44f5180 to master...

rust-timer · 2023-03-20T23:55:05Z

Finished benchmarking commit (44f5180): 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)	2.2%	[1.0%, 3.4%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.1%	[-3.4%, -0.8%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-3.4%, 3.4%]	4

Cycles

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.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.7%, 0.5%]	2

kennykerr · 2023-03-21T00:58:32Z

Sweet - thanks all - that was some serious collaboration. 🎉

ChrisDenton · 2023-03-21T02:52:50Z

Yay! 🎉

It's in nightly now so we'll see how this goes...

klensy · 2023-03-21T12:14:06Z

Hm, export table size of rustc_driver changed from 18104 to 25790 items (some nearby nightly), plus it looks like there imported few unused fn from windows libs.

kennykerr · 2023-03-21T12:51:34Z

@dpaoliello could this be related to raw-dylib? This PR uses the windows crate with the windows_raw_dylib cfg flag which means the windows crate won't include a dependency on the windows-targets crate containing the libs.

ChrisDenton · 2023-03-21T13:27:57Z

Just for future reference, the two relevant nightly versions are:

rustc 1.70.0-nightly (44f5180 2023-03-20)
rustc 1.70.0-nightly (da7c50c 2023-03-19)

Though in rustup you'll need to +1 to those dates. E.g.:

> rustup toolchain install nightly-2023-03-20-x86_64-pc-windows-msvc
> rustup toolchain install nightly-2023-03-21-x86_64-pc-windows-msvc

dpaoliello · 2023-03-22T21:46:46Z

@kennykerr I don't think it's raw-dylib, look like a lot of the various windows-rs wrappers seem to be being exported.

Using dumpbin /exports I'm seeing things like:
1860 743 04E57BD0 _RNvMNtNtNtNtNtCse7B3NxVOzTG_7windows7Windows5Win326System11Diagnostics5DebugNtB2_32AsyncIDebugApplicationNodeEvents14Begin_onDetach = _RNvMNtNtNtNtNtCse7B3NxVOzTG_7windows7Windows5Win326System11Diagnostics5DebugNtB2_32AsyncIDebugApplicationNodeEvents14Begin_onDetach

When dump that function via WinDbg:

0:000> uf 0000000184E57BD0
rustc_driver_fcf43a7b54edf3a8!RNvMsj_NtNtNtNtNtCse7B3NxVOzTG_7windows7Windows5Win326System11Diagnostics5DebugNtB5_13IActiveScript5Close:
00000001`84e57bd0 56              push    rsi
00000001`84e57bd1 4883ec20        sub     rsp,20h
00000001`84e57bd5 4889ce          mov     rsi,rcx
00000001`84e57bd8 488b0a          mov     rcx,qword ptr [rdx]
00000001`84e57bdb 488b01          mov     rax,qword ptr [rcx]
00000001`84e57bde ff5038          call    qword ptr [rax+38h]
00000001`84e57be1 85c0            test    eax,eax
00000001`84e57be3 780e            js      rustc_driver_fcf43a7b54edf3a8!RNvMsj_NtNtNtNtNtCse7B3NxVOzTG_7windows7Windows5Win326System11Diagnostics5DebugNtB5_13IActiveScript5Close+0x23 (00000001`84e57bf3)  Branch

rustc_driver_fcf43a7b54edf3a8!RNvMsj_NtNtNtNtNtCse7B3NxVOzTG_7windows7Windows5Win326System11Diagnostics5DebugNtB5_13IActiveScript5Close+0x15:
00000001`84e57be5 31c0            xor     eax,eax

rustc_driver_fcf43a7b54edf3a8!RNvMsj_NtNtNtNtNtCse7B3NxVOzTG_7windows7Windows5Win326System11Diagnostics5DebugNtB5_13IActiveScript5Close+0x17:
00000001`84e57be7 488906          mov     qword ptr [rsi],rax
00000001`84e57bea 4889f0          mov     rax,rsi
00000001`84e57bed 4883c420        add     rsp,20h
00000001`84e57bf1 5e              pop     rsi
00000001`84e57bf2 c3              ret

rustc_driver_fcf43a7b54edf3a8!RNvMsj_NtNtNtNtNtCse7B3NxVOzTG_7windows7Windows5Win326System11Diagnostics5DebugNtB5_13IActiveScript5Close+0x23:
00000001`84e57bf3 89c1            mov     ecx,eax
00000001`84e57bf5 e89685ffff      call    rustc_driver_fcf43a7b54edf3a8!RNvXs4_NtNtCse7B3NxVOzTG_7windows4core5errorNtB5_5ErrorINtNtCs3tdp6Z6tc58_4core7convert4FromNtNtB7_7hresult7HRESULTE4from (00000001`84e50190)
00000001`84e57bfa 48894608        mov     qword ptr [rsi+8],rax
00000001`84e57bfe 895610          mov     dword ptr [rsi+10h],edx
00000001`84e57c01 b801000000      mov     eax,1
00000001`84e57c06 ebdf            jmp     rustc_driver_fcf43a7b54edf3a8!RNvMsj_NtNtNtNtNtCse7B3NxVOzTG_7windows7Windows5Win326System11Diagnostics5DebugNtB5_13IActiveScript5Close+0x17 (00000001`84e57be7)  Branch

That looks like a COM or WinRT call wrapper to me...

I'm guessing that rustc_driver is a Rust dylib and so all pub Rust functions are exported?

kennykerr · 2023-03-22T21:52:36Z

Thanks, yeah if it's a lib then that's not too surprising.

dpaoliello · 2023-03-22T21:56:55Z

Thanks, yeah if it's a lib then that's not too surprising.

It's not a lib, it's a dll that is exporting Rust functions (rather than C functions). If I understand Rust's model for this, then all pub Rust functions must be exported from that dll in case a dependent of rustc_driver attempts to use a function in one rustc_driver's dependencies.

Would it be possible to trim down the API exposed by windows-rs to reduce the number of pub functions?

kennykerr · 2023-03-22T22:03:06Z

Would it be possible to trim down the API exposed by windows-rs to reduce the number of pub functions?

Just using Cargo features - there's a discussion here about reducing the size of the largest namespaces where I mentioned the Windows.Win32.System.Diagnostics.Debug namespace listed in your dump.

Reducing the size of that namespace would go a long way toward reducing the number of pub functions in the scenario.

Alternatively, you could use the windows-sys crate, which has a much smaller surface area and omits all of those COM interface functions.

kennykerr · 2023-03-29T12:11:45Z

Would it be possible to trim down the API exposed by windows-rs to reduce the number of pub functions?

https://github.com/microsoft/windows-rs/releases/tag/0.47.0 has been published and includes an update to the metadata resulting in a much smaller Win32_System_Diagnostics_Debug feature. This should go a long way toward reducing the number of pub functions.

Update windows-sys This updates the windows-sys dependency from 0.45 to 0.48. This shouldn't add or remove any duplicate dependencies (since there are other dependencies still using 0.45 and 0.42). The intent is to move it along the direction towards unifying in the future (though it seems like a moving target that will be difficult to ever hit). This also bumps the home crate version. I think it should be OK to make the migration from winapi to windows-sys a patch version, though there seems to be some issues with the way windows-sys works that could introduce some build-time problems in some situations (such as those encountered in rust-lang/rust#108665 and rust-lang/rust#106610). However, I don't expect too much of an issue.

rustbot assigned davidtwco Jan 9, 2023

rustbot assigned jyn514 and unassigned davidtwco Jan 9, 2023

ChrisDenton reviewed Jan 9, 2023

View reviewed changes

klensy reviewed Jan 9, 2023

View reviewed changes

compiler/rustc_errors/src/lock.rs Outdated Show resolved Hide resolved

rustbot assigned ChrisDenton and unassigned jyn514 Jan 10, 2023

euclio force-pushed the windows-rs branch from ee44d1f to fea9b7b Compare January 11, 2023 21:16

euclio force-pushed the windows-rs branch from fea9b7b to 0964a04 Compare January 15, 2023 18:41

euclio force-pushed the windows-rs branch from 0964a04 to 2c691ae Compare January 15, 2023 18:43

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 16, 2023

migrate compiler, bootstrap, and compiletest to windows-rs

bb7c373

euclio force-pushed the windows-rs branch from f9ca6cb to bb7c373 Compare March 20, 2023 17:21

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 Mar 20, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 20, 2023

bors merged commit 44f5180 into rust-lang:master Mar 20, 2023

rustbot added this to the 1.70.0 milestone Mar 20, 2023

This was referenced Mar 20, 2023

Add support for SpanTrace capture in ICE reports #103993

Closed

Add OpenHarmony targets #108792

Merged

dpaoliello mentioned this pull request Mar 27, 2023

Tracking issue for RFC 2627: #[link(kind="raw-dylib")] #58713

Open

15 tasks

euclio deleted the windows-rs branch March 29, 2023 13:54

ehuss mentioned this pull request Apr 23, 2023

Update windows-sys rust-lang/cargo#12021

Merged

matthiaskrgr mentioned this pull request Feb 13, 2024

ICE: downgrade_to_delayed_bug: cannot downgrade Warning to DelayedBug: not an error #121006

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate compiler, bootstrap and compiletest to windows-rs #106610

migrate compiler, bootstrap and compiletest to windows-rs #106610

euclio commented Jan 9, 2023

rustbot commented Jan 9, 2023

davidtwco commented Jan 9, 2023

klensy commented Jan 9, 2023

ChrisDenton commented Jan 9, 2023

klensy commented Jan 9, 2023

ChrisDenton commented Jan 9, 2023

ChrisDenton left a comment

ChrisDenton Jan 9, 2023

euclio Jan 11, 2023

jyn514 commented Jan 10, 2023

bors commented Jan 14, 2023

rustbot commented Jan 15, 2023

euclio commented Jan 15, 2023

ChrisDenton commented Jan 16, 2023

jyn514 commented Jan 16, 2023

bors commented Jan 16, 2023

ChrisDenton commented Jan 16, 2023

bors commented Jan 16, 2023

riverar commented Mar 20, 2023

ChrisDenton commented Mar 20, 2023

bors commented Mar 20, 2023

bors commented Mar 20, 2023

bors commented Mar 20, 2023

rust-timer commented Mar 20, 2023

kennykerr commented Mar 21, 2023

ChrisDenton commented Mar 21, 2023

klensy commented Mar 21, 2023

kennykerr commented Mar 21, 2023

ChrisDenton commented Mar 21, 2023

dpaoliello commented Mar 22, 2023

kennykerr commented Mar 22, 2023

dpaoliello commented Mar 22, 2023

kennykerr commented Mar 22, 2023 •

edited

Loading

kennykerr commented Mar 29, 2023

		unsafe { FileTimeToSystemTime(&user_filetime, &mut user_time) }.ok().ok()?;
		unsafe { FileTimeToSystemTime(&kernel_filetime, &mut kernel_time) }.ok().ok()?;

migrate compiler, bootstrap and compiletest to windows-rs #106610

migrate compiler, bootstrap and compiletest to windows-rs #106610

Conversation

euclio commented Jan 9, 2023

rustbot commented Jan 9, 2023

davidtwco commented Jan 9, 2023

klensy commented Jan 9, 2023

ChrisDenton commented Jan 9, 2023

klensy commented Jan 9, 2023

ChrisDenton commented Jan 9, 2023

ChrisDenton left a comment

Choose a reason for hiding this comment

ChrisDenton Jan 9, 2023

Choose a reason for hiding this comment

euclio Jan 11, 2023

Choose a reason for hiding this comment

jyn514 commented Jan 10, 2023

bors commented Jan 14, 2023

rustbot commented Jan 15, 2023

euclio commented Jan 15, 2023

ChrisDenton commented Jan 16, 2023

jyn514 commented Jan 16, 2023

bors commented Jan 16, 2023

ChrisDenton commented Jan 16, 2023

bors commented Jan 16, 2023

riverar commented Mar 20, 2023

ChrisDenton commented Mar 20, 2023

bors commented Mar 20, 2023

bors commented Mar 20, 2023

bors commented Mar 20, 2023

rust-timer commented Mar 20, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

kennykerr commented Mar 21, 2023

ChrisDenton commented Mar 21, 2023

klensy commented Mar 21, 2023

kennykerr commented Mar 21, 2023

ChrisDenton commented Mar 21, 2023

dpaoliello commented Mar 22, 2023

kennykerr commented Mar 22, 2023

dpaoliello commented Mar 22, 2023

kennykerr commented Mar 22, 2023 • edited Loading

kennykerr commented Mar 29, 2023

kennykerr commented Mar 22, 2023 •

edited

Loading