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

zig ld can create dylibs now; remove system linker hack and any mention of ld64.lld from the codebase #9266

Merged
merged 4 commits into from Jun 29, 2021

Conversation

kubkon
Copy link
Member

@kubkon kubkon commented Jun 29, 2021

closes #8727
closes #8728
closes #8499

Changes:

  • zig ld gains ability to create dylibs
  • disable LTO when targeting macOS since zig ld doesn't yet understand how to link bitcode files
  • remove system linker hack from the project
  • remove all ld64.lld hooks and mentions from the project (this includes removing of ld64.lld as a punting target in main.zig punt_to_lld - lemme know @andrewrk if this acceptable in your opinion!)

* sort the symbols by name to optimise the export trie first
* insert only symbols with global linkage (private exts or weak should
  not make the cut)
Comment on lines 903 to 906
if (options.target.isDarwin()) {
// TODO ability of zig ld MachO backend to link bitcode files
break :blk false;
} else if (options.want_lto) |explicit| {
Copy link
Member

Choose a reason for hiding this comment

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

This should probably stay how it was previously, so that if you request -fLTO explicitly, you get an error, rather than silently doing the wrong thing. I believe the if (!use_lld) below already does the correct logic in the meantime.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, but then, if this is set to true, front-end will generate bitcode instead of standard object file leading to weird errors. So you're suggesting checking this and throwing a tailored error instead in the linker?

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted back to what it was!

if (options.target.isDarwin()) {
// TODO ability of zig ld MachO backend to link bitcode files
break :blk false;
} else if (options.want_lto) |explicit| {
if (!use_lld)
Copy link
Member

Choose a reason for hiding this comment

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

If you scroll up to line 845, you can see where use_lld is computed. It looks like the logic for computing use_lld should have an additional if (options.target.isDarwin()) then set use_lld=false, since we don't even have the ability to invoke lld for darwin targets.

Copy link
Member Author

Choose a reason for hiding this comment

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

We still use use_lld to select between incremental and Zld. I will be working on converging the two implementations but it's a lot of work and not included in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Understood. Would it be too much for this PR to split a link_incrementally flag out from use_lld in the meantime?

Copy link
Member Author

Choose a reason for hiding this comment

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

If that's ok, I'd leave it as is for now and split it in the PR merging the codepaths which I'll be working on next. Does that sound OK?

Copy link
Member

Choose a reason for hiding this comment

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

No problem 👍

@@ -42,7 +42,6 @@ else()
FIND_AND_ADD_LLD_LIB(lldMinGW)
FIND_AND_ADD_LLD_LIB(lldELF)
FIND_AND_ADD_LLD_LIB(lldCOFF)
FIND_AND_ADD_LLD_LIB(lldMachO)
Copy link
Member

Choose a reason for hiding this comment

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

this deletion is just so beautiful, it brings a tear to my eyes 🥲

including:
* finding lldMachO in CMake config
* punting `ld64.lld` to LLD linker
* providing bindings to LLD linker
@kubkon kubkon merged commit 81bf05b into master Jun 29, 2021
@kubkon kubkon deleted the zld-dylibs branch June 29, 2021 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants