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

linkers: basic support for the 'zig cc' linker #12293

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Akaricchi
Copy link
Contributor

@Akaricchi Akaricchi commented Sep 27, 2023

Zig has a C/C++ compiler frontend that may be useful for cross-compiling. The compiler presents as clang (because it actually is), but since zig 0.11.0 the linker reports itself as zig ld, which confuses meson. It seems mostly compatible with lld, except lacking support for --thinlto-cache-dir (for now?).

I haven't tested this too thoroughly, but I was able to build the latest Taisei with this patch and a trivial cross file, for linux and windows.

@Akaricchi Akaricchi requested a review from jpakkane as a code owner September 27, 2023 09:51
@tristan957
Copy link
Contributor

Mind adding a release notes snippet? Just something simple with an example on how to us it would be good enough

@Akaricchi
Copy link
Contributor Author

I'll add a release snippet but I'm sure if a usage example is necessary. You use it like any other compiler; zig cc/zig c++ are drop-in replacements for clang/clang++ respectively. This already worked with zig 0.10, because it just used the lld linker. Now it just works with zig 0.11 too.

@eli-schwartz
Copy link
Member

Previously, #11918

This PR seems to be better as it resolves an outstanding review comment from that PR about adding a unique linker class, and even smooths over some incompatibilities. ;)

I would still like to see zig installed in CI and tested, in addition to the release notes. Please also see the comment there about -fuse-ld.

@tristan957
Copy link
Contributor

You can squash the commits. Thanks!

@Akaricchi
Copy link
Contributor Author

Now that I think about it, the linker id should probably be ld.zig, to be consistent with other GNU-style linkers. That way there should be less confusion if e.g. we ever support Zig as a language in Meson — the linker will probably have a different interface there.

@Akaricchi
Copy link
Contributor Author

Akaricchi commented Sep 27, 2023

Please also see the comment there about -fuse-ld

@eli-schwartz
Zig seems to completely ignore -fuse-ld. What should we do about it?

@Akaricchi
Copy link
Contributor Author

Akaricchi commented Sep 27, 2023

Some of the comments in the other PR seem inaccurate.

That just means that if users try to configure a linker for zig, meson will error out when trying to pass that linker as -fuse-ld.

There's no error or even a warning; as I said above, -fuse-ld is just ignored.

It depends on the target platform. In linux, it's compatible to clang. It will probably be a different story on MacOS or Windows, which I did not test.

zig cc has the same clang/lld-compatible interface on all platforms. The article I linked even shows this by running it in Wine.
EDIT: or so it seems; I'm actually not sure about the linker part anymore. One thing I know is I did cross-compile a moderately complex project for windows with this patch.

@dcbaker
Copy link
Member

dcbaker commented Sep 27, 2023

Now that I think about it, the linker id should probably be ld.zig, to be consistent with other GNU-style linkers. That way there should be less confusion if e.g. we ever support Zig as a language in Meson — the linker will probably have a different interface there.

I might be working on that. For the moment Zig looks more like Rust, generating a final output directly. Like Rust it can compile object files but getting Meson to the point it does that will be non-trivial.

I would prefer that the linker be called someting other than just zig, I don't love ld.zig, but I could live with it. what about zigcc ?

@tristan957 tristan957 added this to the 1.3.0 milestone Sep 27, 2023
@xclaesse xclaesse modified the milestones: 1.3.0, 1.4.0 Oct 17, 2023
@Akaricchi
Copy link
Contributor Author

Akaricchi commented Dec 21, 2023

I don't love ld.zig, but I could live with it. what about zigcc ?

@dcbaker
I think the ld. prefix makes perfect sense, because it's a convention that meson uses for all "ld"-style linker interfaces consistently, and I see no reason to break it. ld.zigcc might be a good idea to avoid ambiguity, because there also exists a standalone zig linker that's a drop-in replacement for ld (it reports itself as ld.zld).

@dcbaker
Copy link
Member

dcbaker commented Dec 21, 2023

The thing is (as the person who named them) I literally used the name of the file on disk. "ld.bfd", "ld.gold" and "ld.lld". I guess since then we've added ld.solaris, even though I'm pretty sure that's not the on disk name. I guess I can live with ld.zig or ld.zigcc. Just not zig itself, because that will cause confusion if I ever send out the actual zig patches (I haven't yet because it's super unreliable without depfile support and I never go around to writing those patches for zig itself)

@Akaricchi
Copy link
Contributor Author

@eli-schwartz

I would still like to see zig installed in CI and tested,

That would be certainly nice to have but I think this PR shouldn't be blocked on that. Meson is the only build system that outright refuses to work with any compiler or linker it doesn't recognize. In my opinion, this design is only acceptable if it's very easy to contribute best-effort support for unknown toolchains. Adding zig tests into CI would be 10 times more effort than writing this patch was, and frankly I would probably rather trick meson with a hacky compiler wrapper on my end than deal with that.

@Akaricchi
Copy link
Contributor Author

Rebased, squashed, and renamed the linker again to ld.zigcc. Also renamed the class to ZigCCDynamicLinker, to disambiguate from zld and/or the actual Zig compiler's linker when/if support for those is added.

@eli-schwartz
Copy link
Member

Meson is the only build system that outright refuses to work with any compiler or linker it doesn't recognize.

I'm (still) of the opinion that if someone was going to invest in writing a POSIXCCompiler class I'd like to merge such functionality.

Anyways...

Adding zig tests into CI would be 10 times more effort than writing this patch was, and frankly I would probably rather trick meson with a hacky compiler wrapper on my end than deal with that.

Then please confirm that you ran the testsuite locally with the relevant linker selection environment variable exported and the tests passed.

@dcbaker
Copy link
Member

dcbaker commented Dec 22, 2023

@Akaricchi If you can confirm that this passes the test suite with zigcc run locally than I think @eli-schwartz and I are in agreement this can land.

@eli-schwartz
Copy link
Member

To be clear, that's exactly what we do, in fact, do when for example people submit support for proprietary compilers that require a bunch of unusual tweaks. We ask them to confirm on the honor system that they ran the testsuite. Sometimes after they do that they discover a bunch of issues they were previously unaware of, and add new commits to fix those too.

@bruchar1
Copy link
Member

bruchar1 commented Jan 5, 2024

See also #8579.

@dcbaker
Copy link
Member

dcbaker commented Jan 6, 2024

@bruchar1 i have an updated version of that PR. I’m hesitant to send it out without depfile support in zig itself.

@henrytill
Copy link

Really looking forward to this landing!

Is there anything I could do to help get this merged?

@dcbaker
Copy link
Member

dcbaker commented Jan 31, 2024

@henrytill Run the Meson test suite with zig cc as the C and C++ compilers and linkers and confirm that everything is working. I think other than confirmation that it's working this is ready to land

@henrytill
Copy link

henrytill commented Jan 31, 2024

Unfortunately, I'm getting quite a few failures using Zig 0.11.0 (current stable release).

Command:

env CC="zig cc" CXX="zig c++" ./run_tests.py 2>&1 | tee zigld-test-output.log

Output:

zigld-test-output.log

Output of uname -a:

Linux thalassa 6.5.0-5-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.5.13-1 (2023-11-29) x86_64 GNU/Linux

@dcbaker
Copy link
Member

dcbaker commented Jan 31, 2024

I didn't look too deeply, but it appears that zig cc doesn't support the -rpath-link option, and there's a lot of tests tripping up on that. There's already a check there for is_sunos() that should be extended to handle zig cc with zig ld correctly

@RossComputerGuy
Copy link
Contributor

I've made a patch which should fix the test errors #13493

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 this pull request may close these issues.

8 participants