Only register `WSACleanup` if `WSAStartup` is actually ever called by CDirkx · Pull Request #85595 · rust-lang/rust

CDirkx · 2021-05-23T02:40:17Z

Fix for #85441.

Because WSACleanup appears in cleanup currently WS2_32.dll is always linked, even if no network functionality is ever used.
To prevent this, WSACleanup has to only appear in init, hence the workaround of registering it in a static.

If anyone knows a cleaner solution, let me know.

rust-highfive · 2021-05-23T02:40:20Z

r? @joshtriplett

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

nagisa · 2021-05-23T11:58:50Z

Consider using SyncLazy/SyncOnceCell for the INIT.

I combined the two statics in a SyncOnceCell.

nagisa · 2021-05-23T14:05:24Z

This seems okay to me. Is there any way a test could be written for this?

klensy · 2021-05-23T14:36:06Z

This seems okay to me. Is there any way a test could be written for this?

dumpbin.exe /dependents empty.exe

For example from issue, output will be:

Microsoft (R) COFF/PE Dumper Version 14.28.29914.0
Copyright (C) Microsoft Corporation. All rights reserved.
Dump of file empty.exe
File Type: EXECUTABLE IMAGE
Image has the following dependencies:
WS2_32.dll
KERNEL32.dll
VCRUNTIME140.dll
api-ms-win-crt-runtime-l1-1-0.dll
api-ms-win-crt-math-l1-1-0.dll
api-ms-win-crt-stdio-l1-1-0.dll
api-ms-win-crt-locale-l1-1-0.dll
api-ms-win-crt-heap-l1-1-0.dll
Summary
1000 .data
1000 .pdata
7000 .rdata
1000 .reloc
19000 .text

we can grep output, but this is ugly.

CDirkx · 2021-05-23T14:57:39Z

I could add a run-make test to inspect a produced binary, however I don't know what tool to do that with: Is dumbbin.exe available on CI? Is there an already available tool that can show loaded dlls?

klensy · 2021-05-23T15:01:06Z

dumpbin is visual studio component, which included in github runner, i guess, but maybe there already some tools used for checking linked libraries\symbols in tests.

klensy · 2021-05-23T15:11:20Z

Test no-dllimport-w-cross-lang-lto looks promising, perhaps we can check that ws2_32 symbols not exist in exe.

CDirkx · 2021-05-24T22:08:15Z

I added a run-make test using objdump which I believe should be available on CI as well.

klensy · 2021-06-06T04:45:50Z

Works, at least under x86_64-pc-windows-msvc.

nagisa · 2021-06-06T15:06:58Z

r? @nagisa
@bors r+

Thanks!

bors · 2021-06-06T15:07:00Z

📌 Commit f798596 has been approved by nagisa

Only register `WSACleanup` if `WSAStartup` is actually ever called Fix for rust-lang#85441. Because `WSACleanup` appears in `cleanup` currently `WS2_32.dll` is always linked, even if no network functionality is ever used. To prevent this, `WSACleanup` has to only appear in `init`, hence the workaround of registering it in a static. If anyone knows a cleaner solution, let me know.

GuillaumeGomez · 2021-06-06T23:05:44Z

Failed in #86079.

@bors: r-

CDirkx · 2021-06-07T00:14:31Z

Good news, using objdump works on CI. Still figuring out how to reproduce the failure under x86_64-pc-windows-gnu.

crlf0710 · 2021-07-09T11:07:26Z

@CDirkx Ping from triage, any updates on this?

CDirkx · 2021-07-09T12:25:29Z

I had trouble getting the tests to run under x86_64-pc-windows-gnu on my machine in order to debug the problem on that platform. I'll try again this weekend though.

eddyb · 2021-08-08T12:20:50Z

 pub struct Socket(c::SOCKET);

-static INIT: Once = Once::new();
+static WSA: SyncOnceCell<unsafe extern "system" fn() -> i32> = SyncOnceCell::new();


Could this be named WSA_CLEANUP? Or otherwise wrap the fn pointer in a struct - as it stands, the WSA name doesn't make it obvious what the fn pointer is supposed to be.

JohnCSimon · 2021-09-13T02:00:33Z

Ping from triage:
@CDirkx Can you please post your status on this PR?

nagisa · 2021-10-02T18:39:03Z

I looked at the build failure. I believe this is likely related to the fact that the gnu targets have function_sections disabled

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

Lines 66 to 67 in 21e1cd9

// FIXME(#13846) this should be enabled for windows
function_sections:false,

thus preventing the linker from removing the references to various networking calls, which then pull in the undesirable dll.

I don't think re-enabling the function sections is going to be feasible, so we should only run that test on -msvc targets...

Since Christiaan seems to be away since at least July, I'm going to close this and open a fixed PR.

mati865 · 2021-10-02T20:40:17Z

FWIW I think it's not an issue with -ffunction-sections but rather issue with -Wl,--gc-sections which is buggy in binutils when targetting MinGW.
function_sections: true works fine when using LLVM toolchain for MinGW.

Only register `WSACleanup` if `WSAStartup` is actually ever called See rust-lang#85595Fixesrust-lang#85441

rust-highfive assigned joshtriplett May 23, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2021

nagisa reviewed May 23, 2021
View reviewed changes

CDirkx force-pushed the win-linkage branch from fb56643 to b469f20Compare May 23, 2021 13:59

CDirkx added 2 commits May 24, 2021 23:58

Only register WSACleanup if WSAStartup is actually ever called
c3140e7

Add test for checking if WS2_32.dll is linked
f798596

CDirkx force-pushed the win-linkage branch from b469f20 to f798596Compare May 24, 2021 22:06

rust-highfive unassigned joshtriplett Jun 6, 2021

rust-highfive assigned nagisa Jun 6, 2021

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 Jun 6, 2021

GuillaumeGomez mentioned this pull request Jun 6, 2021
Rollup of 7 pull requests #86079
Closed

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 Jun 6, 2021

This comment has been minimized.
Sign in to view

rustbot added the O-windows Operating system: Windows label Jun 23, 2021

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 23, 2021

JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 25, 2021

eddyb reviewed Aug 8, 2021
View reviewed changes

camelid 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 29, 2021

nagisa closed this Oct 2, 2021

nagisa mentioned this pull request Oct 2, 2021
Only register WSACleanup if WSAStartup is actually ever called #89472
Merged

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 4, 2021
Rollup merge ofrust-lang#89472- nagisa:nagisa/wsa-cleanup, r=dtolnay
43c4355
Only register `WSACleanup` if `WSAStartup` is actually ever called See rust-lang#85595Fixesrust-lang#85441

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 4, 2021
Rollup merge ofrust-lang#89472- nagisa:nagisa/wsa-cleanup, r=dtolnay
e021a10
Only register `WSACleanup` if `WSAStartup` is actually ever called See rust-lang#85595Fixesrust-lang#85441

Uh oh!

Uh oh!

Conversation

CDirkx commented May 23, 2021

Uh oh!

rust-highfive commented May 23, 2021

Uh oh!

nagisaMay 23, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CDirkxMay 23, 2021

Choose a reason for hiding this comment

Uh oh!

nagisa commented May 23, 2021

Uh oh!

klensy commented May 23, 2021

Uh oh!

CDirkx commented May 23, 2021

Uh oh!

klensy commented May 23, 2021

Uh oh!

klensy commented May 23, 2021

Uh oh!

CDirkx commented May 24, 2021

Uh oh!

klensy commented Jun 6, 2021

Uh oh!

nagisa commented Jun 6, 2021

Uh oh!

bors commented Jun 6, 2021

Uh oh!

GuillaumeGomez commented Jun 6, 2021

Uh oh!

CDirkx commented Jun 7, 2021

Uh oh!

This comment has been minimized.

crlf0710 commented Jul 9, 2021

Uh oh!

CDirkx commented Jul 9, 2021

Uh oh!

eddybAug 8, 2021

Choose a reason for hiding this comment

Uh oh!

JohnCSimon commented Sep 13, 2021

Uh oh!

nagisa commented Oct 2, 2021

Uh oh!

mati865 commented Oct 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

nagisaMay 23, 2021•
edited
Loading