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

macho: allow undefined symbols in dylibs #10314

Merged
merged 1 commit into from
Dec 11, 2021
Merged

Conversation

kubkon
Copy link
Member

@kubkon kubkon commented Dec 11, 2021

We now respect both -fallow-shlib-undefined and -Wl,"-undefined=dynamic_lookup" flags. This is the first step towards solving issues #8180. We currently do not expose any other ld64 equivalent flag for -undefined flag - we basically throw an error should the user specify a different flag. Support for those is conditional on closing #8180. As a result of this change, it is now possible to generate a valid native Node.js addon with Zig for macOS.

You can also check whether the option is on by requesting --verbose-link when building a shared object like so:

$ zig build-lib -dynamic example.zig -I$(brew --prefix node)/include/node -fallow-shlib-undefined -femit-bin=example.node --verbose-link
zig ld -dynamic -dylib -install_name @rpath/example.node zig-cache/o/f33f9be186fda290c12c65772584b1cb/example.o /Users/kubkon/.cache/zig/o/7d88bd701accf5f7b37248e431dd6be8/libcompiler_rt.a -o example.node -lSystem -lc -undefined dynamic_lookup

Fixes #3000

cc @jorangreef @ifreund

We now respect both `-fallow-shlib-undefined` and
`-Wl,"-undefined=dynamic_lookup"` flags. This is the first step
towards solving issues #8180 and #3000. We currently do not expose
any other ld64 equivalent flag for `-undefined` flag - we basically
throw an error should the user specify a different flag. Support for
those is conditional on closing #8180. As a result of this change,
it is now possible to generate a valid native Node.js addon with Zig
for macOS.
@kubkon kubkon added this to the 0.9.0 milestone Dec 11, 2021
@kubkon kubkon requested a review from andrewrk December 11, 2021 20:44
@@ -1703,6 +1703,16 @@ fn buildOutputType(
}
emit_implib = .{ .yes = linker_args.items[i] };
emit_implib_arg_provided = true;
} else if (mem.eql(u8, arg, "-undefined")) {
Copy link
Member

Choose a reason for hiding this comment

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

Quick question- you mentioned that this supports -Wl,"-undefined=dynamic_lookup", but it looks like this actually implements -Wl,-undefined,dynamic_lookup. Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Uhm, wait, did I make a typo somewhere? It should implement the former, -Wl,"-undefined=dynamic_lookup" unless I messed something up.

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, I think that's what I did. Note that there is another if statement after we match -undefined:

    5                 } else if (mem.eql(u8, arg, "-undefined")) {
     4                     i += 1;
     3                     if (i >= linker_args.items.len) {
     2                         fatal("expected linker arg after '{s}'", .{arg});
     1                     }
  1711                     if (mem.eql(u8, "dynamic_lookup", linker_args.items[i])) {
     1                         linker_allow_shlib_undefined = true;
     2                     } else {
     3                         fatal("unsupported -undefined option '{s}'", .{linker_args.items[i]});
     4                     }
     5                 } else {

Copy link
Member

Choose a reason for hiding this comment

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

Never mind- I forgot that we had this code up there:

                            // Handle nested-joined args like `-Wl,-rpath=foo`.
                            // Must be prefixed with 1 or 2 dashes.
                            if (linker_arg.len >= 3 and linker_arg[0] == '-' and linker_arg[2] != '-') {
                                if (mem.indexOfScalar(u8, linker_arg, '=')) |equals_pos| {
                                    try linker_args.append(linker_arg[0..equals_pos]);
                                    try linker_args.append(linker_arg[equals_pos + 1 ..]);
                                    continue;
                                }
                            }

all good 👍

@kubkon kubkon merged commit dbfcebf into master Dec 11, 2021
@kubkon kubkon deleted the macho-allow-shlib-undefined branch December 11, 2021 23:32
@jorangreef
Copy link
Contributor

Thanks so much @kubkon and @andrewrk ! Going to go link a cup of coffee now...

messense added a commit to messense/maturin that referenced this pull request Feb 23, 2022
messense added a commit to rust-cross/cargo-zigbuild that referenced this pull request Feb 23, 2022
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.

Zig and Node.js N-API example
3 participants