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

Allow rust staticlib to work with MSVC's /WHOLEARCHIVE #129257

Merged
merged 2 commits into from
Aug 22, 2024

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented Aug 18, 2024

This fixes #129020 by renaming the __NULL_IMPORT_DESCRIPTOR to prevent conflicts.

try-job: dist-i686-msvc

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 18, 2024
@ChrisDenton
Copy link
Member Author

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 18, 2024
…r=<try>

Allow rust staticlib to work with MSVC's /WHOLEARCHIVE

This renames the `__NULL_IMPORT_DESCRIPTOR` to prevent conflicts.

try-job: x86_64-msvc
try-job: i686-msvc

r? ghost
@bors
Copy link
Contributor

bors commented Aug 18, 2024

⌛ Trying commit f64ecf1 with merge 04f2fce...

@bors
Copy link
Contributor

bors commented Aug 19, 2024

☀️ Try build successful - checks-actions
Build commit: 04f2fce (04f2fcee5f890f2ff221d91b27754fb2fb2ba779)

@ChrisDenton
Copy link
Member Author

The ar_archive_writer crate has been update so this should be ready for review. Hopefully the rmake test is at least a bit nicer now,

r? jieyouxu

@ChrisDenton ChrisDenton marked this pull request as ready for review August 19, 2024 17:52
@rustbot
Copy link
Collaborator

rustbot commented Aug 19, 2024

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this LGTM, feel free to r=me with or without addressing the nit.

tests/run-make/msvc-wholearchive/rmake.rs Outdated Show resolved Hide resolved
tests/run-make/msvc-wholearchive/rmake.rs Outdated Show resolved Hide resolved
@jieyouxu
Copy link
Member

@bors rollup=iffy (funny linker things)

@ChrisDenton ChrisDenton force-pushed the rename-null-descriptor branch from 50a28af to d4a14b1 Compare August 19, 2024 18:27
@ChrisDenton
Copy link
Member Author

Since the changes are cosmetic and check passed...

@bors r=jieyouxu

@bors
Copy link
Contributor

bors commented Aug 19, 2024

📌 Commit d4a14b1 has been approved by jieyouxu

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 Aug 19, 2024
//! It ensures we can use `/WHOLEARCHIVE` to link a rust staticlib into DLL
//! using the MSVC linker

//@ only-msvc
Copy link
Contributor

@mati865 mati865 Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice (but not required) to also cover windows-gnullvm that has the same issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not tried gnullvm specifically but the llvm linker has another issue which I think needs solving on their end first (which is why even the msvc test skips lld-link).

.idata$4 should not refer to special section 0

LLVM seems to not properly recognise .idata$4 and .idata$5 as special unless they're specifically in short import libraries. incidentally this also makes it break on MSVC long import libraries. Though since they haven't been in use since the 90's I guess there's not been much pressure to support it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should open a new issue to track this case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? How do we usually track upstream issues?

Copy link
Member

@jieyouxu jieyouxu Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a special label for waiting on LLVM: S-waiting-for-llvm for describing the dragon is eepy (and better if it links to a llvm upstream issue)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though since they haven't been in use since the 90's I guess there's not been much pressure to support it.

ld.bfd used by windows-gnu still uses them as it still cannot output short import libraries 😉

jieyouxu added a commit to jieyouxu/rust that referenced this pull request Aug 20, 2024
…, r=jieyouxu

Allow rust staticlib to work with MSVC's /WHOLEARCHIVE

This fixes rust-lang#129020 by renaming the `__NULL_IMPORT_DESCRIPTOR` to prevent conflicts.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 20, 2024
Rollup of 6 pull requests

Successful merges:

 - rust-lang#127623 (fix: fs::remove_dir_all: treat internal ENOENT as success)
 - rust-lang#128627 (Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms.)
 - rust-lang#129187 (bootstrap: fix clean's remove_dir_all implementation)
 - rust-lang#129190 (Added f16 and f128 to tests/ui/consts/const-float-bits-conv.rs)
 - rust-lang#129231 (improve submodule updates)
 - rust-lang#129257 (Allow rust staticlib to work with MSVC's /WHOLEARCHIVE)

r? `@ghost`
`@rustbot` modify labels: rollup
@jieyouxu
Copy link
Member

jieyouxu commented Aug 20, 2024

I'm suspecting this PR caused the failure in #129300 (comment):

Caused by:
  process didn't exit successfully: `C:\a\rust\rust\build\i686-pc-windows-msvc\stage1-rustc\release\build\crossbeam-utils-7d693aa43cb4fd79\build-script-build` (exit code: 0xc0000135, STATUS_DLL_NOT_FOUND)
[RUSTC-TIMING] smallvec test:false 0.460
error: failed to run custom build command for `proc-macro2 v1.0.86`

Caused by:
Caused by:
  process didn't exit successfully: `C:\a\rust\rust\build\i686-pc-windows-msvc\stage1-rustc\release\build\proc-macro2-6de43c34e6077ea1\build-script-build` (exit code: 0xc0000135, STATUS_DLL_NOT_FOUND)

Temporarily kicking this out of the queue to check if it's this or another bootstrap PR.

@bors r-

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 20, 2024
@bors
Copy link
Contributor

bors commented Aug 21, 2024

💔 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 Aug 21, 2024
@ChrisDenton
Copy link
Member Author

@bors retry spurious "failed to remove file" on x86_64-msvc-ext again

@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 Aug 21, 2024
@jieyouxu
Copy link
Member

pinging back #127883

@bors
Copy link
Contributor

bors commented Aug 22, 2024

⌛ Testing commit 40af214 with merge 5dbffc5...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 22, 2024
…r=jieyouxu

Allow rust staticlib to work with MSVC's /WHOLEARCHIVE

This fixes rust-lang#129020 by renaming the `__NULL_IMPORT_DESCRIPTOR` to prevent conflicts.

try-job: dist-i686-msvc
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] miri test:false 4.390
error: failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe`

Caused by:
  Access is denied. (os error 5)
Command has failed. Rerun with -v to see more details.
  local time: Thu, Aug 22, 2024  3:16:53 AM
  network time: Thu, 22 Aug 2024 03:16:54 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

@bors
Copy link
Contributor

bors commented Aug 22, 2024

💔 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 Aug 22, 2024
@jieyouxu
Copy link
Member

@bors retry (#127883 windows ci failure 2: electric boogaloo)

@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 Aug 22, 2024
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Aug 22, 2024
…, r=jieyouxu

Allow rust staticlib to work with MSVC's /WHOLEARCHIVE

This fixes rust-lang#129020 by renaming the `__NULL_IMPORT_DESCRIPTOR` to prevent conflicts.

try-job: dist-i686-msvc
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 22, 2024
…, r=jieyouxu

Allow rust staticlib to work with MSVC's /WHOLEARCHIVE

This fixes rust-lang#129020 by renaming the `__NULL_IMPORT_DESCRIPTOR` to prevent conflicts.

try-job: dist-i686-msvc
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 22, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#126985 (Implement `-Z embed-source` (DWARFv5 source code embedding extension))
 - rust-lang#128349 (Enable `f16` on x86 and x86-64)
 - rust-lang#128876 (Ship MinGW-w64 runtime DLLs along with `rust-lld.exe` for `-pc-windows-gnu` targets)
 - rust-lang#129190 (Add f16 and f128 to tests/ui/consts/const-float-bits-conv.rs)
 - rust-lang#129257 (Allow rust staticlib to work with MSVC's /WHOLEARCHIVE)
 - rust-lang#129386 (Use a LocalDefId in ResolvedArg.)
 - rust-lang#129400 (Update `compiler_builtins` to `0.1.120`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Aug 22, 2024

⌛ Testing commit 40af214 with merge 5ad98b4...

@bors
Copy link
Contributor

bors commented Aug 22, 2024

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 5ad98b4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 22, 2024
@bors bors merged commit 5ad98b4 into rust-lang:master Aug 22, 2024
7 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 22, 2024
@ChrisDenton ChrisDenton deleted the rename-null-descriptor branch August 22, 2024 18:18
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5ad98b4): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.7% [-1.7%, -1.7%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 2.7%)

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

Cycles

Results (secondary 9.6%)

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)
9.6% [9.6%, 9.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

Results (secondary -0.0%)

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)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.0% [-0.0%, -0.0%] 1
All ❌✅ (primary) - - 0

Bootstrap: 749.851s -> 749.145s (-0.09%)
Artifact size: 338.94 MiB -> 338.93 MiB (-0.00%)

@jieyouxu jieyouxu added the CI-spurious-fail-msvc CI spurious failure: target env msvc label Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc CI-spurious-fail-msvc CI spurious failure: target env msvc 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Rust 1.79+ stdlib is unusable with MSVC when creating static libraries
7 participants