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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Congratulations and question about linker #1349

Closed
frederikhors opened this issue Feb 4, 2023 · 13 comments 路 Fixed by #1363
Closed

Congratulations and question about linker #1349

frederikhors opened this issue Feb 4, 2023 · 13 comments 路 Fixed by #1363

Comments

@frederikhors
Copy link

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

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
Copy link
Member

bjorn3 commented Feb 4, 2023

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 #1249 (comment)

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

gimli-rs/object@ef9c666 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. bytecodealliance/wasmtime#5619 will update it to 0.30 and also includes a windows correctness bug fix. @afonso360 what is the status of that PR?

@frederikhors
Copy link
Author

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

@afonso360
Copy link
Contributor

afonso360 commented Feb 4, 2023

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 bytecodealliance/wasmtime#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
Copy link
Author

I think bytecodealliance/wasmtime#5550 is closed, roght?

@afonso360
Copy link
Contributor

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
Copy link
Author

I'll try in a few hours.

@frederikhors
Copy link
Author

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
Copy link
Member

bjorn3 commented Mar 9, 2023

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
Copy link
Author

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
Copy link
Member

bjorn3 commented Mar 21, 2023

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 #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 rust-lang/rust#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
Copy link
Author

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

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

@bjorn3
Copy link
Member

bjorn3 commented Mar 21, 2023

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
Copy link
Member

bjorn3 commented Mar 21, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants