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

honor the needed flag of SystemLibs in the macos linker code #10192

Closed
andrewrk opened this issue Nov 21, 2021 · 7 comments · Fixed by #11941
Closed

honor the needed flag of SystemLibs in the macos linker code #10192

andrewrk opened this issue Nov 21, 2021 · 7 comments · Fixed by #11941
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. linking os-macos
Milestone

Comments

@andrewrk
Copy link
Member

Extracted from #10164. The Mach-O linking code does not currently inspect the needed flag of dynamic libraries before linking against a dynamic library. In order to support this feature, it must be possible to do something like this:

$ cat test.c
int main(int argc, char **argv) {
    return 0;
}

$ zig build-exe test.c  -lSDL2

$ objdump -p test | grep NEEDED
  NEEDED               libc.so.6

$ zig build-exe test.c -needed-lSDL2

$ objdump -p test | grep NEEDED
  NEEDED               libSDL2-2.0.so.0
  NEEDED               libc.so.6

Excuse my use of objdump - I'm not sure what the equivalent tool is for macho.

@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. os-macos linking labels Nov 21, 2021
@andrewrk andrewrk added this to the 0.10.0 milestone Nov 21, 2021
@kubkon
Copy link
Member

kubkon commented Nov 23, 2021

@andrewrk So this one is interesting. AFAIK Apple's ld64 by default always links every library and framework from the linker line even if no symbols are imported from it. In the past, I believe it would output a warning if no symbols were actually imported which you could suppress via -needed prefix flag such as -needed-lSDL2. Upstream zig currently matches ld64's behaviour which means it also by default links every dylib from the linker line even if no symbols are imported.

With upstream zig:

$ zig build-exe test.c -lSDL2
$ otool -L test
test:
        /usr/local/opt/sdl2/lib/libSDL2-2.0.0.dylib (compatibility version 17.0.0, current version 17.0.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1311.0.0)

With Apple's clang (and ld64):

$ clang test.c -lSDL2
$ otool -L a.out
a.out:
        /usr/local/opt/sdl2/lib/libSDL2-2.0.0.dylib (compatibility version 17.0.0, current version 17.0.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1311.0.0)

@andrewrk
Copy link
Member Author

andrewrk commented Nov 23, 2021

I'd like to make it consistent across platforms if possible, meaning that we would take the "as needed" as the default. Unless this imposes too much of a perf cost from a linker perspective, in which case I think that is enough to change the default back.

If you're on board with this, it's done from the frontend perspective; all that needs to happen is the MachO linker code honor the "needed" field of SystemLib.

@kubkon
Copy link
Member

kubkon commented Nov 23, 2021

I'd like to make it consistent across platforms if possible, meaning that we would take the "as needed" as the default. Unless this imposes too much of a perf cost from a linker perspective, in which case I think that is enough to change the default back.

Hmm, I don't think we should change the default behaviour of zig ld on macOS which matches ld64. However, we can and should unpack -needed-lSDL2 to mean -lSDL2. Similarly for -needed_framework etc.

@andrewrk
Copy link
Member Author

andrewrk commented Nov 23, 2021

As it stands the frontend code that collects CLI arguments decides whether a SystemLib has the needed flag set, and regardless this issue is for the MachO linker code to honor the needed flag.

If we want to make the default different for MachO, that still goes into the frontend code that collects CLI arguments, and will need to do conditional logic based on the target OS.

Can you form an argument about why to not make it consistent across platforms? My proposal is that the default would be to effectively add -dead_strip_dylibs to the linker line for MachO, just as --as-needed is now default added to the linker line for ELF. To be clear, it is not default on ELF either, when using ld. This is a difference that Zig would be providing, to dead strip dylibs by default on all platforms. I think it's a more useful default.

@kubkon
Copy link
Member

kubkon commented Nov 23, 2021

As it stands the frontend code that collects CLI arguments decides whether a SystemLib has the needed flag set, and regardless this issue is for the MachO linker code to honor the needed flag.

If we want to make the default different for MachO, that still goes into the frontend code that collects CLI arguments, and will need to do conditional logic based on the target OS.

Can you form an argument about why to not make it consistent across platforms? My proposal is that the default would be to effectively add -dead_strip_dylibs to the linker line for MachO, just as --as-needed is now default added to the linker line for ELF. To be clear, it is not default on ELF either, when using ld. This is a difference that Zig would be providing, to dead strip dylibs by default on all platforms. I think it's a more useful default.

My main argument is linking in Objective-C runtime which sometimes is required even if no symbols are physically imported. This was already reported as a bug to us and fixed in #9542. My main argument is that macOS users won't expect this as this is the status quo on macOS.

@kubkon
Copy link
Member

kubkon commented Nov 23, 2021

Also, from man ld:

     -dead_strip_dylibs
                 Remove dylibs that are unreachable by the entry point or exported symbols. That is, suppresses the generation of load command commands for dylibs which supplied no symbols during the link. This option should not be used when linking against a dylib
                 which is required at runtime for some indirect reason such as the dylib has an important initializer.

which is effectively a warning not to do this if your program depends on a runtime such as Objective-C runtime as in the linked PR.

@andrewrk
Copy link
Member Author

andrewrk commented Nov 23, 2021

Understood. In this case let us change the default back to --no-as-needed for ELF as well. This will be a frontend CLI change in addition to this issue which is to honor the needed flag of SystemLibs in the macOS linker code.

I will make the frontend CLI change separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. linking os-macos
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants