Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Distribute libntdll.a with windows-gnu toolchains #108262

Merged
merged 4 commits into from
Mar 21, 2023

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented Feb 20, 2023

This allows the OS loader to load essential functions (e.g. read/write file) at load time instead of lazily doing so at runtime.

r? libs

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 20, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2023

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

This allows loading some essential functions (e.g. read/write file) at load time instead of lazily.
@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2023

The Miri subtree was changed

cc @rust-lang/miri

@Mark-Simulacrum
Copy link
Member

I'm not sure how to evaluate whether this is a good idea or not -- maybe we can reassign to someone else? It seems like a comment in the dist.rs code where we list the dll list that says something about what we should think about when extending or shrinking the list would be good.

@Mark-Simulacrum Mark-Simulacrum 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 Mar 4, 2023
@ChrisDenton ChrisDenton added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 4, 2023
@ChrisDenton
Copy link
Member Author

In #68519 a similar change was rejected because libstd didn't use ntdll at the time. That has changed. Issue #47359 aims to audit the list but it doesn't look like it's actively maintained or been followed up on. We should probably try to clean up the list at some point, although it is possible some people are relying on specific libraries being distributed. Judging from past history, it seems we tend to accidentally break windows-gnu builds first then fix them by adding to this list when people report the errors. Which isn't great.

Feel free to reassign this PR if you'd prefer.

@Mark-Simulacrum
Copy link
Member

OK. That seems like reasonable rationale for me to sign off here, we can figure out how to do auditing/better CI in the future. Seems like a hard problem.

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Mar 18, 2023

📌 Commit b44b50a has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors 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 18, 2023
@bors
Copy link
Contributor

bors commented Mar 18, 2023

⌛ Testing commit b44b50a with merge 6fa6a5f76bc57e1495fb975024495f5e5488188f...

@bors
Copy link
Contributor

bors commented Mar 18, 2023

💔 Test failed - checks-actions

@bors 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 Mar 18, 2023
@ehuss
Copy link
Contributor

ehuss commented Mar 18, 2023

@bors retry
wasm SIGSEGV

@bors 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 18, 2023
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 21, 2023
@bors
Copy link
Contributor

bors commented Mar 21, 2023

⌛ Testing commit 154f5d7 with merge 3ff4d56...

@bors
Copy link
Contributor

bors commented Mar 21, 2023

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 3ff4d56 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 21, 2023
@bors bors merged commit 3ff4d56 into rust-lang:master Mar 21, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 21, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3ff4d56): 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.7% [0.7%, 0.7%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.7% [-0.7%, -0.7%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.3% [-0.7%, 0.7%] 3

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

@ChrisDenton ChrisDenton deleted the libntdll branch March 21, 2023 13:28
Kryptos-FR added a commit to fragcolor-xyz/shards that referenced this pull request Mar 23, 2023
Kryptos-FR added a commit to fragcolor-xyz/shards that referenced this pull request Mar 23, 2023
gnomesysadmins pushed a commit to GNOME/librsvg that referenced this pull request Jun 7, 2023
Link also to ntdll.lib which became required since Rust 1.70.0 or
nightly-2023-03-21 [1] (which was sadly not clearly documented as such) when
linking the resulting rsvg DLL.  So, we extend the batch script that is used to
formerly query the default Rust toolchain to also check whether it requires
linking to ntdll.lib by checking against the Rust version.

[1]: rust-lang/rust#108262

Fixes issue #968.
gnomesysadmins pushed a commit to GNOME/librsvg that referenced this pull request Jun 7, 2023
Link also to ntdll.lib which became required since Rust 1.70.0 or
nightly-2023-03-21 [1] (which was sadly not clearly documented as such) when
linking the resulting rsvg DLL.  So, we extend the batch script that is used to
formerly query the default Rust toolchain to also check whether it requires
linking to ntdll.lib by checking against the Rust version.

[1]: rust-lang/rust#108262

Fixes issue #968.
gnomesysadmins pushed a commit to GNOME/librsvg that referenced this pull request Jun 7, 2023
Link also to ntdll.lib which became required since Rust 1.70.0 or
nightly-2023-03-21 [1] (which was sadly not clearly documented as such) when
linking the resulting rsvg DLL.  So, we extend the batch script that is used to
formerly query the default Rust toolchain to also check whether it requires
linking to ntdll.lib by checking against the Rust version.

[1]: rust-lang/rust#108262

Fixes issue #968.
gnomesysadmins pushed a commit to GNOME/librsvg that referenced this pull request Jun 7, 2023
Link also to ntdll.lib which became required since Rust 1.70.0 or
nightly-2023-03-21 [1] (which was sadly not clearly documented as such) when
linking the resulting rsvg DLL.  So, we extend the batch script that is used to
formerly query the default Rust toolchain to also check whether it requires
linking to ntdll.lib by checking against the Rust version.

[1]: rust-lang/rust#108262

Fixes issue #968.
gnomesysadmins pushed a commit to GNOME/librsvg that referenced this pull request Jun 7, 2023
Link also to ntdll.lib which became required since Rust 1.70.0 or
nightly-2023-03-21 [1] (which was sadly not clearly documented as such) when
linking the resulting rsvg DLL.  So, we extend the batch script that is used to
formerly query the default Rust toolchain to also check whether it requires
linking to ntdll.lib by checking against the Rust version.

[1]: rust-lang/rust#108262

Fixes issue #968.
gnomesysadmins pushed a commit to GNOME/librsvg that referenced this pull request Jun 7, 2023
Link also to ntdll.lib which became required since Rust 1.70.0 or
nightly-2023-03-21 [1] (which was sadly not clearly documented as such) when
linking the resulting rsvg DLL.  So, we extend the batch script that is used to
formerly query the default Rust toolchain to also check whether it requires
linking to ntdll.lib by checking against the Rust version.

[1]: rust-lang/rust#108262

Fixes issue #968.
gnomesysadmins pushed a commit to GNOME/librsvg that referenced this pull request Jun 8, 2023
Extend the helper batch file that we use to query the default Rust toolchain so
that we can use the rustc that corresponds to the Rust toolchain that we are
using to query the system libraries (i.e. from the Windows SDK), in order to
ensure that the final librvg DLL links, as Rust might involve changes that
require more system libraries to be required for the build, such as in issue

[1]: rust-lang/rust#108262

Fixes issue #968.
gnomesysadmins pushed a commit to GNOME/librsvg that referenced this pull request Jun 8, 2023
Extend the helper batch file that we use to query the default Rust toolchain so
that we can use the rustc that corresponds to the Rust toolchain that we are
using to query the system libraries (i.e. from the Windows SDK), in order to
ensure that the final librvg DLL links, as Rust might involve changes that
require more system libraries to be required for the build, such as in issue

[1]: rust-lang/rust#108262

Fixes issue #968.
gnomesysadmins pushed a commit to GNOME/librsvg that referenced this pull request Jun 8, 2023
Extend the helper batch file that we use to query the default Rust toolchain so
that we can use the rustc that corresponds to the Rust toolchain that we are
using to query the system libraries (i.e. from the Windows SDK), in order to
ensure that the final librvg DLL links, as Rust might involve changes that
require more system libraries to be required for the build, such as in issue

[1]: rust-lang/rust#108262

Fixes issue #968.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 10, 2023
Fix ntdll linkage issues on Windows UWP platforms

See discussion: rust-lang#112265 (comment)

Static loading `ntdll` functions does not work for UWP programs, which will end up link errors complaining about missing symbols, or failure to pass the WACK tests. The breakage was introduced in rust-lang#108262.

This PR basically reverts part of the changes in rust-lang#108262 for UWP only, and fixes some lint suggestions.
gnomesysadmins pushed a commit to GNOME/librsvg that referenced this pull request Jun 13, 2023
Extend the helper batch file that we use to query the default Rust toolchain so
that we can use the rustc that corresponds to the Rust toolchain that we are
using to query the system libraries (i.e. from the Windows SDK), in order to
ensure that the final librvg DLL links, as Rust might involve changes that
require more system libraries to be required for the build, such as in issue

[1]: rust-lang/rust#108262

Fixes issue #968.

Part-of: <https://gitlab.gnome.org/GNOME/librsvg/-/merge_requests/842>
gnomesysadmins pushed a commit to GNOME/librsvg that referenced this pull request Jun 14, 2023
Extend the helper batch file that we use to query the default Rust toolchain so
that we can use the rustc that corresponds to the Rust toolchain that we are
using to query the system libraries (i.e. from the Windows SDK), in order to
ensure that the final librvg DLL links, as Rust might involve changes that
require more system libraries to be required for the build, such as in issue

[1]: rust-lang/rust#108262

Fixes issue #968.
gnomesysadmins pushed a commit to GNOME/librsvg that referenced this pull request Jun 14, 2023
Extend the helper batch file that we use to query the default Rust toolchain so
that we can use the rustc that corresponds to the Rust toolchain that we are
using to query the system libraries (i.e. from the Windows SDK), in order to
ensure that the final librvg DLL links, as Rust might involve changes that
require more system libraries to be required for the build, such as in the
issue below:

rust-lang/rust#108262

Fixes issue #968 for the librsvg-2.56 branch.
cpu added a commit to cpu/rustls-ffi that referenced this pull request Jun 29, 2023
Previously, building the tip of `main` in CI for Windows, Windows CMake
(Debug), and Windows CMake (Release) was failing with link-time errors
of the form:

```
rustls_ffi.lib(std-391022a4250a8b9a.std.feb3b897-cgu.0.rcgu.o) : error
LNK2019: unresolved external symbol __imp_NtWriteFile referenced in
function
_ZN3std3sys7windows6handle6Handle17synchronous_write17h5e143db420a86fa8E
[D:\a\rustls-ffi\rustls-ffi\build\tests\server.vcxproj]
```

The fix is to explicitly include `ntdll.lib` in the native static libs
that we link on Windows. Doing this fixes the builds once we also update
the `verify-static-libraries.py` script to expect this additional lib.

This may be related to an upstream `rust-lang/rust` change[0] but I'm
not 100% sure.

[0]: rust-lang/rust#108262
cpu added a commit to rustls/rustls-ffi that referenced this pull request Jun 30, 2023
Previously, building the tip of `main` in CI for Windows, Windows CMake
(Debug), and Windows CMake (Release) was failing with link-time errors
of the form:

```
rustls_ffi.lib(std-391022a4250a8b9a.std.feb3b897-cgu.0.rcgu.o) : error
LNK2019: unresolved external symbol __imp_NtWriteFile referenced in
function
_ZN3std3sys7windows6handle6Handle17synchronous_write17h5e143db420a86fa8E
[D:\a\rustls-ffi\rustls-ffi\build\tests\server.vcxproj]
```

The fix is to explicitly include `ntdll.lib` in the native static libs
that we link on Windows. Doing this fixes the builds once we also update
the `verify-static-libraries.py` script to expect this additional lib.

This may be related to an upstream `rust-lang/rust` change[0] but I'm
not 100% sure.

[0]: rust-lang/rust#108262
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants