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

[all linkers] fail hard on unsupported flags #11906

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

motiejus
Copy link
Contributor

@motiejus motiejus commented Jun 22, 2022

Currently zig cc, when confronted with a linker argument it does
not understand, skips the flag and emits a warning.

This has been causing headaches for people that build third-party
software (including me). Zig seemingly builds and links the final
executable, only to segfault when running it. Emiting a warning and
"successfully" compiling a binary breaks autoconf assumptions.
See https://github.com/zigchroot/zig-chroot/issues/1 for a recent
painful example.

If there are linker warnings when compiling software, the first thing we
have to do is add support for ones linker is complaining, and only then
go file issues. If zig "successfully" (i.e. status code = 0) compiles a
binary, there is instead a tendency to blaim "zig doing something
weird". (I am guilty of this.) In my experience, adding the unsupported
arguments has been quite easy; see #11679, #11875, #11874 for recent
examples.

With the "soft prerequisites" below I was able to build all of
the CGo programs that I am encountering at $dayjob.

Harder prerequisites — some production code in my dayjob will cease to build unless we merge this first:

Soft prerequisites — some things I've observed in other issues/PRs:

Andrew's comment in favor of failing early instead of emitting a warning: https://github.com/zigchroot/zig-chroot/issues/1#issuecomment-1054710190 . (NOTE: the repo no longer exists, I would be grateful if someone has a copy of the comment.)

@marler8997
Copy link
Contributor

You'd be able to get this behavior now without breaking anyone if you made it an opt-in compiler switch.

@motiejus
Copy link
Contributor Author

You'd be able to get this behavior now without breaking anyone if you made it an opt-in compiler switch.

I can, however, I am unsure of its value and am quite sure of its disadvantages. There are a few examples of such users:

  1. Users who want their software to compile with zig cc.
  2. Users who are copy-pasting others' instructions and will leave in their build scripts whatever "works".

Users in category (1) can revert this commit and see how far the build can progress for their software. If they want to ship scripts that compile their software with zig, the only reasonable way forward is to implement the missing flags (and then ship). If we, however, offer a clutch, the incentive -- and venue for error -- will be to use that flag, but not implement the necessary bits in zig. At some point all linker options will be understood, so we will have to leave that flag.. for backwards compatibility?

I especially explicitly want to make (2) difficult. A linker flag "errors-as-warnings" is a source of pain and misery especially when something emits a "warning" 5 layers down in whatever you happen to be using.

@marler8997
Copy link
Contributor

In order to use "zig cc" with as many projects as possible, I don't think we have a choice whether or not we want to allow unrecognized command line arguments. I suggest we add a command-line option to enable/disable this behavior (i.e. --allow-unrecognized-flags vs --deny-unrecognized-flags) and plan to keep this option permanently. This would give users the ability to turn it on or off according to their needs and a non-breaking path forward to change the behavior from "opt-in" to "opt-out". This means you could have the feature now without breaking anyone, and after some time has passed and projects that need it have started using --allow-unrecignized-flags, then we can make the safer option to deny unrecognized flags the default.

@kubkon
Copy link
Member

kubkon commented Jun 22, 2022

I don't have a strong opinion on this either way - on one hand, I agree with @motiejus that we really should do more than just (almost) silently ignore the unsupported flags. On the other hand, I agree with @marler8997 that there are so many flags out there it will be a pain trying to support them all and/or might take a long time to implement. If you don't mind I'd like to defer the final decision to @andrewrk once he's back at work next week.

Changing topic slightly, @motiejus dunno if you noticed, with this change building arm64 Zig binary fails on macos - it complains about unknown -search_paths_first flag. This one is an easy fix: we should ignore it since this is the default behaviour in both ld64 and zld.

@kubkon kubkon added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label Jun 22, 2022
@marler8997
Copy link
Contributor

I suppose one way people could add this feature back in is they could create a wrapper script around zig that throws away any arguments that they know Zig doesn't recognize. I don't anticipate this to be a very common use case and I think this amount of work to accommodate esoteric projects is OK. Having to rebuild Zig to allow unrecognized arguments seems like too much, but with this wrapper script idea I think I've changed my mind and am now in favor of this :)

@motiejus
Copy link
Contributor Author

I found Andrew's comment in favor of this in https://github.com/zigchroot/zig-chroot/issues/1#issuecomment-1054710190 ; now I am adding support to get rid of those warnings (you can see PRs slowly trickling in):

warning: unsupported linker arg: --sort-section
warning: unsupported linker arg: alignment
warning: unsupported linker arg: --sort-common
warning: unsupported linker arg: --no-undefined
warning: unsupported linker arg: --exclude-libs
warning: unsupported linker arg: ALL
warning: unsupported linker arg: --dynamic-list
warning: unsupported linker arg: ./dynamic.list

motiejus added a commit to motiejus/zig that referenced this pull request Jun 23, 2022
Ignore MachO-specific flag -search_paths_first, since it is the default
in zld and ld64.

Also see Jakub's comment[1]:

    Changing topic slightly, @motiejus dunno if you noticed, with this
    change building arm64 Zig binary fails on macos - it complains about
    unknown -search_paths_first flag. This one is an easy fix: we should
    ignore it since this is the default behaviour in both ld64 and zld.

[1]: ziglang#11906 (comment)
@motiejus
Copy link
Contributor Author

Changing topic slightly, @motiejus dunno if you noticed, with this change building arm64 Zig binary fails on macos - it complains about unknown -search_paths_first flag.

We don't build MacOS binaries for arm64, because we did not get to the -framework thing yet. :)

This one is an easy fix: we should ignore it since this is the default behaviour in both ld64 and zld.

#11917

kubkon pushed a commit to motiejus/zig that referenced this pull request Jun 24, 2022
Ignore MachO-specific flag -search_paths_first, since it is the default
in zld and ld64.

Also see Jakub's comment[1]:

    Changing topic slightly, @motiejus dunno if you noticed, with this
    change building arm64 Zig binary fails on macos - it complains about
    unknown -search_paths_first flag. This one is an easy fix: we should
    ignore it since this is the default behaviour in both ld64 and zld.

[1]: ziglang#11906 (comment)
@kubkon
Copy link
Member

kubkon commented Jun 25, 2022

MachO linker should now implement all flags that could lead to a fatal error when building x86_64 and arm64 in the CI. Relevant PRs: #11917 and #11929

@motiejus
Copy link
Contributor Author

I found Andrew's comment in favor of this in https://github.com/zigchroot/zig-chroot/issues/1#issuecomment-1054710190 <...>

The repository is gone now. Neither google cache nor archive.org have it; nor does my MUA (I subscribed to the thread later than this). If somebody still has a reference to the original comment, or even better, the whole thread — I would be grateful if you could put it here.

andrewrk pushed a commit that referenced this pull request Jul 19, 2022
Ignore MachO-specific flag -search_paths_first, since it is the default
in zld and ld64.

Also see Jakub's comment[1]:

    Changing topic slightly, @motiejus dunno if you noticed, with this
    change building arm64 Zig binary fails on macos - it complains about
    unknown -search_paths_first flag. This one is an easy fix: we should
    ignore it since this is the default behaviour in both ld64 and zld.

[1]: #11906 (comment)
wooster0 pushed a commit to wooster0/zig that referenced this pull request Jul 24, 2022
Ignore MachO-specific flag -search_paths_first, since it is the default
in zld and ld64.

Also see Jakub's comment[1]:

    Changing topic slightly, @motiejus dunno if you noticed, with this
    change building arm64 Zig binary fails on macos - it complains about
    unknown -search_paths_first flag. This one is an easy fix: we should
    ignore it since this is the default behaviour in both ld64 and zld.

[1]: ziglang#11906 (comment)
Currently `zig cc`, when confronted with a linker argument it does
not understand, skips the flag and emits a warning.

This has been causing headaches for people that build third-party
software (including me). Zig seemingly builds and links the final
executable, only to segfault when running it.

If there are linker warnings when compiling software, the first thing we
have to do is add support for ones linker is complaining, and only then
go file issues. If zig "successfully" (i.e. status code = 0) compiles a
binary, there is instead a tendency to blaim "zig doing something
weird". (I am guilty of this.) In my experience, adding the unsupported
arguments has been quite easy; see ziglang#11679, ziglang#11875, ziglang#11874 for recent
examples.

With the current ones (+ prerequisites below) I was able to build all of
the CGo programs that I am encountering at $dayjob. CGo is a reasonable
example, because it is exercising the unusual linker args quite a bit.

Prerequisites: ziglang#11614 and ziglang#11863.
@andrewrk
Copy link
Member

I agree with this behavior and will merge this PR- but first zig CLI parsing needs to support all the existing linker flags to avoid breaking things.

@andrewrk
Copy link
Member

I merged some other linker flag PRs, pushed d2650eb, filed #14663, and I'm going to go ahead and merge this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants