Use raw-dylib in the std for non-x86 Windows

[ChrisDenton]

The raw_dylib feature was recently stabilized for non-x86 Windows targets. This is now in beta so std start to use it without feature flags. Eventually this may allowing linking Rust programs without needing import libraries.

I think the standard library is best placed to dog food before the beta becomes the stable the release. There hopefully shouldn't be any issues but if there are it'd be good to catch them early.

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

r? @thomcc

(rust-highfive has picked a reviewer for you, use r? to override)

[ChrisDenton]

This PR does not use raw-dylib for kernel32 because the vcruntime needs it to be linked in some way. Also note that some ntdll functions we depend on are now linked using raw-dylib. The only reason these were dynamically loaded is that some old versions of the Windows SDK may not have the required import library. Using raw-dylib means it's no longer necessary (except on x86).

[thomcc]

This is really nice. I kind of expect that this might hit some issues for some users use-cases, but I do think we ultimately want to do this, and this seems like the best way to root out such issues.

@bors r+

[bors]

📌 Commit 60a8ff71de494e7c93c5691b79498374c9c33d64 has been approved by thomcc

It is now in the queue for this repository.

[bors]

⌛ Testing commit 60a8ff71de494e7c93c5691b79498374c9c33d64 with merge 8701c58da2bf4e39241e596a4791da07b343617b...

[bors]

💔 Test failed - checks-actions

[ChrisDenton]

The error is:

   = note: a.tcp_stress.8a6846bf-cgu.0.rcgu.o : error LNK2019: unresolved external symbol __imp_closesocket referenced in function _ZN3std10sys_common9backtrace28__rust_begin_short_backtrace17he99d99762636b925E

Hm, so this is weird because it's only closesocket (from ws2_32) that fails with raw-dylib. What makes it special is that this is used in a small drop function that's #[inline] so it's being inlined into __rust_begin_short_backtrace:

rust/library/std/src/os/windows/io/socket.rs

Lines 202 to 209 in 8b70583

	impl Drop for OwnedSocket {
	#[inline]
	fn drop(&mut self) {
	unsafe {
	let _ = c::closesocket(self.socket);
	}
	}
	}

So the test framework loads the std dynamically but I guess closesocket is inlined statically. And unlike with normal links, it's not picking up that it also needs the import.

cc @dpaoliello

[ChrisDenton]

I'm just confirming that I'm right about the issue and checking if there are any other problems. Obviously this commit should not be merged.

[ChrisDenton]

Test successful. Reverting test commit.

[dpaoliello]

The error is:

   = note: a.tcp_stress.8a6846bf-cgu.0.rcgu.o : error LNK2019: unresolved external symbol __imp_closesocket referenced in function _ZN3std10sys_common9backtrace28__rust_begin_short_backtrace17he99d99762636b925E

Hm, so this is weird because it's only closesocket (from ws2_32) that fails with raw-dylib. What makes it special is that this is used in a small drop function that's #[inline] so it's being inlined into __rust_begin_short_backtrace:

rust/library/std/src/os/windows/io/socket.rs

Lines 202 to 209 in 8b70583

	impl Drop for OwnedSocket {
	#[inline]
	fn drop(&mut self) {
	unsafe {
	let _ = c::closesocket(self.socket);
	}
	}
	}

So the test framework loads the std dynamically but I guess closesocket is inlined statically. And unlike with normal links, it's not picking up that it also needs the import.

cc @dpaoliello

Interesting - can you please file and issue and assign it to me? I'm on vacation next week but will look into it the week after that.

I'm not sure if the best approach will be to have the declaring binary (in this case, std) include the symbol in its normal import library, or if the consuming binary should generate the symbol instead...

[thomcc]

I don't think this is waiting on review, so I'm punting it to the author.

That said, maybe S-blocked is more accurate...

@rustbot author

[ChrisDenton]

Issue created. This PR is blocked on #102714 so I'll set it to draft.

[ChrisDenton]

#102988 merged.

@rustbot ready

[thomcc]

This looks great! Thrilled to see that the underlying issue was fixable too!

@bors r+

[bors]

📌 Commit 4c05e3a has been approved by thomcc

It is now in the queue for this repository.

[notriddle]

Failed in rollup. #103777 (comment)

@bors r-

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[ChrisDenton]

Ok so the miri issue was that we're now importing NtWriteFile directly instead of loading dynamically so the shim had to be moved. Hopefully I did it right.

[RalfJung]

Looks good, just a comment nit!

[RalfJung]

r=me on the Miri changes.

[bors]

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

[ChrisDenton]

Ok, I'm closing this for now. It seems it would be wise to let the raw_dylib feature bake for longer before using it in std. There's a PR up which will use it in the compiler. Once that is merged and has been around for awhile I'll look to revisit this.

I'll keep this branch around for that reason.