rust-lang / rustc_codegen_cranelift

Cranelift based backend for rustc
Apache License 2.0
1.59k stars 100 forks source link

Congratulations and question about linker #1349

Closed frederikhors closed 1 year ago

frederikhors commented 1 year ago

I just discovered this project: you did an amazing job @bjorn3! Wow! :smiley:

I'm creating a web app using

cargo watch -x run (now cargo-clif watch -x run)

to relaunch web server to try the changes.

Before cargo-clif the time to wait after a change was: 11/12 seconds.

Now is 5/6: amazing!

But there is a thing.

With the "original" Microsoft linker the build never completes (it hangs on the last step, the last crate, indefinitely). Can I ask you why?

I need to use llvm linker instead with global cargo config like this:

[target.x86_64-pc-windows-msvc]
linker = "lld-link.exe"
bjorn3 commented 1 year ago

you did an amazing job @bjorn3! Wow!

Thanks!

With the "original" Microsoft linker the build never completes (it hangs on the last step, the last crate, indefinitely). Can I ask you why?

To quote https://github.com/bjorn3/rustc_codegen_cranelift/issues/1249#issuecomment-1272350880

Looks like object generates a section for every GotRelative relocation rather than putting them all in the same section.

https://github.com/gimli-rs/object/commit/ef9c666f3780da38214d77e394dde72bc2b69238 which landed in object 0.30 likely fixed this and with this likely the linker performance issue, but cranelift-object is still stuck at object 0.29. https://github.com/bytecodealliance/wasmtime/pull/5619 will update it to 0.30 and also includes a windows correctness bug fix. @afonso360 what is the status of that PR?

frederikhors commented 1 year ago

Thank you. I will close this issue as soon as I verify that it works after that merge.

afonso360 commented 1 year ago

Hey,

That PR is pretty much ready to go, we just need someone to validate the remaining dependency upgrades. Although it might need a rebase.

It's looking more likely for https://github.com/bytecodealliance/wasmtime/pull/5550 to be merged first. And that one does a pretty similar dependency upgrade so it might be that the linker issue gets fixed via that one.

I'll try to ping someone to review the remaining dependencies to get these merged.

frederikhors commented 1 year ago

I think https://github.com/bytecodealliance/wasmtime/pull/5550 is closed, roght?

afonso360 commented 1 year ago

Yeah, all of the pending Windows patches were merged. They should be released in Cranelift 0.94 in about two weeks.

In the mean time, here is a build of cg-clif that already includes all of those patches if you'd like to try it sooner.

frederikhors commented 1 year ago

I'll try in a few hours.

frederikhors commented 1 year ago

If you mean run https://github.com/bjorn3/rustc_codegen_cranelift/actions/runs/4276291179, I tried with cg_clif-x86_64-pc-windows-msvc artifact and it works!

I can't tell you how grateful I am for your work!

bjorn3 commented 1 year ago

Cranelift 0.94.0 branched 3 days ago on the 5th. On the 20th it will be released. Once it is released I will update cg_clif as soon as I have time.

frederikhors commented 1 year ago

I tried cg_clif-x86_64-pc-windows-msvc from https://github.com/bjorn3/rustc_codegen_cranelift/actions/runs/4479742328.

It works amazingly!

Can I ask why you haven't enabled the releases yet?

So we can also use useful tools like Scoop.sh.

bjorn3 commented 1 year ago

Can I ask why you haven't enabled the releases yet?

The intention has been to distribute cg_clif as rustup component in the future. This has been taking longer than I intended though. I did put up https://github.com/bjorn3/rustc_codegen_cranelift/pull/1357 last month though which brings it a significant step closer to distributing cg_clif as rustup component. I just need to finish that PR, finish the rust side and once both are merged I think I can rebase https://github.com/rust-lang/rust/pull/81746 (which does the actual distribution as rustup component) and get it reviewed. There are some issues on https://github.com/bjorn3/rustc_codegen_cranelift/milestone/2 that may need to be fixed first though. In the mean time I guess I could create a release every commit (and delete the old release), but I'm a bit afraid about spamming the notifications of everyone watching this repo.

frederikhors commented 1 year ago

The "spam" is what I need if I subscribe to releases! LOL.

Anyway, what do you mean by "rustup component"?

bjorn3 commented 1 year ago

Each rust toolchain is split into a couple of components that can be individually installed using rustup. For example rustc itself, cargo, rustfmt and clippy. My intention is to make cg_clif one of those components that can be installed for nightly rustc toolchains.

bjorn3 commented 1 year ago

The "spam" is what I need if I subscribe to releases! LOL.

When I subscribe to a repo, I'm generally more interested in issue and PR activity than every single commit pushed to the default branch. But I understand that not everyone has the same preferences. I guess using a custom watch which disables releases would work for people that don't want a release notification every time I push to the master branch.