-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Conversation
* 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)
src/Compilation.zig
Outdated
if (options.target.isDarwin()) { | ||
// TODO ability of zig ld MachO backend to link bitcode files | ||
break :blk false; | ||
} else if (options.want_lto) |explicit| { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
src/Compilation.zig
Outdated
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
closes #8727
closes #8728
closes #8499
Changes:
zig ld
gains ability to create dylibsdisable LTO when targeting macOS sincezig ld
doesn't yet understand how to link bitcode filesld64.lld
hooks and mentions from the project (this includes removing ofld64.lld
as a punting target inmain.zig
punt_to_lld
- lemme know @andrewrk if this acceptable in your opinion!)