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

Windows builds fail to link C++ static library #107162

Closed
jdm opened this issue Jan 21, 2023 · 23 comments · Fixed by #107360 or #128936
Closed

Windows builds fail to link C++ static library #107162

jdm opened this issue Jan 21, 2023 · 23 comments · Fixed by #107360 or #128936
Assignees
Labels
C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. O-windows-msvc Toolchain: MSVC, Operating system: Windows P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jdm
Copy link
Contributor

jdm commented Jan 21, 2023

Code

Building the mozjs crate for any Windows target stopped working after #97485. mozjs has a build script that compiles the C++ library SpiderMonkey into js_static.a, which is then linked. Before #97485, these builds worked successfully. Since that PR was merged, CI using nightly and beta toolsets targeting MSVC x86_64 and UWP x86_64 now fail to build with the following error:

error: failed to add native library D:\a\mozjs\mozjs\target\debug\build\mozjs_sys-4062d5cd7cbc7110\out\build/js/src/build\js_static.lib: Unsupported archive identifier

This was first reported in servo/mozjs#327 and servo/mozjs#334.

Version it worked on

2022-12-03 nightly
Rust 1.66 beta

Version with regression

2022-12-04 nightly
Rust 1.67 beta

If you know when this regression occurred, please add a line like below, replacing {channel} with one of stable, beta, or nightly.

@rustbot modify labels: +regression-from-stable-to-beta -regression-untriaged

@jdm jdm added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Jan 21, 2023
@rustbot rustbot added regression-from-stable-to-beta Performance or correctness regression from stable to beta. I-prioritize Issue: Indicates that prioritization has been requested for this issue. and removed regression-untriaged Untriaged performance or correctness regression. labels Jan 21, 2023
@jyn514
Copy link
Member

jyn514 commented Jan 22, 2023

cc @bjorn3

@bjorn3
Copy link
Member

bjorn3 commented Jan 22, 2023

As workaround adding windows to

if sess.target.arch == "wasm32" || sess.target.arch == "wasm64" {
should revert back to the old archive writer. Luckily it was preserved for wasm.

Could you post a hex dump of the first 16 or so bytes of the js_static.lib archive to see the magic bytes of this file.

@sagudev
Copy link
Contributor

sagudev commented Jan 22, 2023

Here is hexdump:

0000000 3c21 6874 6e69 0a3e 202f 2020 2020 2020
0000010 2020 2020 2020 2020 2030 2020 2020 2020

Here is GitHub action of working one: https://github.com/sagudev/mozjs/actions/runs/3974783209 and here is GitHub action of defect one: https://github.com/sagudev/mozjs/actions/runs/3974683394. Artifact contains .lib file, that is produced in windows job.

@bjorn3
Copy link
Member

bjorn3 commented Jan 22, 2023

Yeah, that is indeed very much not a valid archive.

┌────────┬─────────────────────────┬────────┐
│00000000│ 3c 21 68 74 6e 69 0a 3e │<!htni_>│
│00000008│ 20 2f 20 20 20 20 20 20 │ /      │
│00000010│ 20 20 20 20 20 20 20 20 │        │
│00000018│ 20 30 20 20 20 20 20 20 │ 0      │
└────────┴─────────────────────────┴────────┘

It does look like !<thin>\n (which is a valid header for thin archives) except with every pair of bytes swapped. Rustc also shouldn't be producing thin archives.

@sagudev
Copy link
Contributor

sagudev commented Jan 22, 2023

Well hex-editor in vscode shows it right (but hexdump is showing reversed bits on same file):
slika

@mati865
Copy link
Contributor

mati865 commented Jan 22, 2023

Do you know if this affects all Windows targets or just MSVC based?
I couldn't get it to build at all with windows-gnu:

$ LANG=C.utf8 cargo t
   Compiling mozjs_sys v0.68.2 (D:\tmp\mozjs\mozjs)
error: failed to run custom build command for `mozjs_sys v0.68.2 (D:\tmp\mozjs\mozjs)`

Caused by:
  process didn't exit successfully: `D:\tmp\mozjs\target\debug\build\mozjs_sys-352d87b28c39ffac\build-script-build` (exit code: 101)
  --- stdout
  cargo:outdir=D:\tmp\mozjs\target\debug\build\mozjs_sys-27c0928751302e7b\out\build

  --- stderr
  make: invalid --jobserver-auth string '__rust_jobserver_semaphore_2019855554'
  make: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.
  D:\tmp\mozjs\mozjs\makefile.cargo:175: *** recipe commences before first target.  Stop.
  thread 'main' panicked at 'assertion failed: result.success()', mozjs\build.rs:178:5
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@jyn514 jyn514 added the O-windows-msvc Toolchain: MSVC, Operating system: Windows label Jan 22, 2023
@sagudev
Copy link
Contributor

sagudev commented Jan 22, 2023

Ideally we would want to find another case for regression, because mozjs is complicated.
@mati865 Do you have MozillaBuild 3.4 installed as error is in makefile.cargo:175 and MozBuild comes packaged with python? I suggest looking at GitHub Workflows to correctly setup for compiling.

@mati865
Copy link
Contributor

mati865 commented Jan 24, 2023

@mati865 Do you have MozillaBuild 3.4 installed as error is in makefile.cargo:175 and MozBuild comes packaged with python?

No, I had checked first step in the readme but the link is dead:

Follow the directions at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites

@sagudev
Copy link
Contributor

sagudev commented Jan 24, 2023

Well, those instruction would not be relevant anymore as we are still few SM versions behind if they would be available. Following GitHub Workflows is currently the best way.

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jan 25, 2023
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 25, 2023
@wesleywiser wesleywiser self-assigned this Jan 26, 2023
@pnkfelix
Copy link
Member

@rustbot label: -P-high +P-critical

(Upgrading priority because, until we better understand the scope of this bug, it strikes me as something that might motivate a fast-tracked point release for 1.67.)

@rustbot rustbot added P-critical Critical priority and removed P-high High priority labels Jan 26, 2023
@apiraino
Copy link
Contributor

I wonder if we have another case of this issue in #107162

@bjorn3
Copy link
Member

bjorn3 commented Jan 26, 2023

You linked this issue itself.

@lqd
Copy link
Member

lqd commented Jan 26, 2023

Could be #107334

@bjorn3
Copy link
Member

bjorn3 commented Jan 26, 2023

Oh, I completely misunderstood what the issue was. I thought this was about rustc producing a malformed archive, but it is actually about rustc not being able to read thin archives produced by an external build system. Object indeed doesn't support parsing thin archives and even if it did, it would likely need to be wired up in rustc separately.

I think the best course of action is to change new_archive_writer to always take the

Box::new(LlvmArchiveBuilder { sess, additions: Vec::new() })
branch. This will restore the old behavior for cg_llvm. #105221 will probably also need to be reverted partially or as a whole because it removed macOS fat archive reading for the LLVM archive writer.

(This would have been so much more painful to revert if I hadn't needed to keep the old LLVM archive writer for wasm.)

@bjorn3
Copy link
Member

bjorn3 commented Jan 26, 2023

And yes #107334 is very likely a duplicate of this.

@kanerogers
Copy link

kanerogers commented Jan 27, 2023

I believe I may be encountering a similar, or related, regression between 1.67 and 1.66 when attempting to build and link against shaderc on MSVC: google/shaderc-rs#133.

@bjorn3
Copy link
Member

bjorn3 commented Jan 27, 2023

Opened #107360 to revert back to LlvmArchiveWriter, which would fix this issue for now I think.

@bors bors closed this as completed in 2527416 Jan 28, 2023
@pnkfelix pnkfelix reopened this Feb 2, 2023
@pnkfelix
Copy link
Member

pnkfelix commented Feb 2, 2023

@rustbot label: E-needs-test

@rustbot rustbot added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Feb 2, 2023
@pnkfelix
Copy link
Member

pnkfelix commented Feb 2, 2023

(and any such test should probably be beta-backported; see discussion from T-compiler triage meeting)

bors added a commit to rust-lang-ci/rust that referenced this issue Feb 8, 2023
…n-archive-reading-for-1-68-beta, r=cuviper

Backport reverts to fix thin archive reading for 1 68 beta

This is a backport of PR rust-lang#107360 aimed at beta.

cc rust-lang#107162, rust-lang#107334 and google/shaderc-rs#133
@wesleywiser wesleywiser added P-medium Medium priority and removed P-critical Critical priority labels Feb 16, 2023
@wesleywiser
Copy link
Member

Downgrading to P-medium as we've shipped a point release with this fix. Leaving open as we would love a regression test to be added.

@Mark-Simulacrum Mark-Simulacrum removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. labels Mar 3, 2023
@Mark-Simulacrum
Copy link
Member

Untagging as a regression, since this has been fixed on beta + stable + nightly at this point as far as I can tell.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 12, 2024
…=jieyouxu

Support reading thin archives in ArArchiveBuilder

And switch to using ArArchiveBuilder with the LLVM backend too now that all regressions are fixed.

Fixes rust-lang#107407
Fixes rust-lang#107162
rust-lang#107495 has been fixed in a previous PR already.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 12, 2024
…=jieyouxu

Support reading thin archives in ArArchiveBuilder

And switch to using ArArchiveBuilder with the LLVM backend too now that all regressions are fixed.

Fixes rust-lang#107407
Fixes rust-lang#107162
rust-lang#107495 has been fixed in a previous PR already.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 12, 2024
…=jieyouxu

Support reading thin archives in ArArchiveBuilder

And switch to using ArArchiveBuilder with the LLVM backend too now that all regressions are fixed.

Fixes rust-lang#107407
Fixes rust-lang#107162
rust-lang#107495 has been fixed in a previous PR already.
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 14, 2024
…try>

Support reading thin archives in ArArchiveBuilder

And switch to using ArArchiveBuilder with the LLVM backend too now that all regressions are fixed.

Fixes rust-lang#107407
Fixes rust-lang#107162
rust-lang#107495 has been fixed in a previous PR already.

try-job: dist-aarch64-msvc
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 15, 2024
…ieyouxu

Support reading thin archives in ArArchiveBuilder

And switch to using ArArchiveBuilder with the LLVM backend too now that all regressions are fixed.

Fixes rust-lang#107407
Fixes rust-lang#107162
rust-lang#107495 has been fixed in a previous PR already.
@bors bors closed this as completed in d2b5aa6 Aug 15, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Aug 17, 2024
Support reading thin archives in ArArchiveBuilder

And switch to using ArArchiveBuilder with the LLVM backend too now that all regressions are fixed.

Fixes rust-lang/rust#107407
Fixes rust-lang/rust#107162
rust-lang/rust#107495 has been fixed in a previous PR already.
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Aug 29, 2024
Support reading thin archives in ArArchiveBuilder

And switch to using ArArchiveBuilder with the LLVM backend too now that all regressions are fixed.

Fixes rust-lang/rust#107407
Fixes rust-lang/rust#107162
rust-lang/rust#107495 has been fixed in a previous PR already.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. O-windows-msvc Toolchain: MSVC, Operating system: Windows P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet