Subject: [PATCH bazel-zig-cc 0/2] Ignore undefined symbols when building dynamic libraries on darwin

From: ~jvolkman <[email-masked]>
Date: Wed, 01 Jun 2022 01:58:18 +0000

Hi,

Here's an updated version of this patchset that fixes an operator
precedence issue in zig_toolchain.bzl when building flag_sets.
//test/gorace:gorace_test was failing before, but now passes.

I haven't been able to get the arm64 tests to pass locally. Here's
hoping they work too!

Jeremy Volkman (2):
  Adds dynamic_library_linkopts to zig_cc_toolchain_config
  Pass "-Wl,-undefined=dynamic_lookup" for dynamic libraries on darwin.

 toolchain/private/cc_toolchains.bzl |  2 ++
 toolchain/private/defs.bzl          |  3 +++
 toolchain/zig_toolchain.bzl         | 22 +++++++++++++++++++---
 3 files changed, 24 insertions(+), 3 deletions(-)

-- 
2.34.2

---

From: ~jvolkman <[email-masked]>
Date: Tue, 24 May 2022 17:14:35 -0700

From: Jeremy Volkman <[email-masked]>

If specified, the flags in dynamic_library_linkopts are included in
cpp_link_dynamic_library and cpp_link_nodeps_dynamic_library actions.
---
 toolchain/private/cc_toolchains.bzl |  2 ++
 toolchain/private/defs.bzl          |  3 +++
 toolchain/zig_toolchain.bzl         | 22 +++++++++++++++++++---
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/toolchain/private/cc_toolchains.bzl b/toolchain/private/cc_toolchains.bzl
index 64e99f1..3a1e1cb 100644
--- a/toolchain/private/cc_toolchains.bzl
+++ b/toolchain/private/cc_toolchains.bzl
@@ -32,6 +32,7 @@ def declare_cc_toolchains(absolute_path, zig_include_root):
             absolute_tool_paths[name] = "%s/%s" % (absolute_path, tool_path)
 
         linkopts = target_config.linkopts
+        dynamic_library_linkopts = target_config.dynamic_library_linkopts
         copts = target_config.copts
         for s in getattr(target_config, "linker_version_scripts", []):
             linkopts = linkopts + ["-Wl,--version-script,%s/%s" % (absolute_path, s)]
@@ -45,6 +46,7 @@ def declare_cc_toolchains(absolute_path, zig_include_root):
             cxx_builtin_include_directories = cxx_builtin_include_directories,
             copts = copts,
             linkopts = linkopts,
+            dynamic_library_linkopts = dynamic_library_linkopts,
             target_cpu = target_config.bazel_target_cpu,
             target_system_name = "unknown",
             target_libc = "unknown",
diff --git a/toolchain/private/defs.bzl b/toolchain/private/defs.bzl
index 6059065..053641c 100644
--- a/toolchain/private/defs.bzl
+++ b/toolchain/private/defs.bzl
@@ -54,6 +54,7 @@ def _target_darwin(gocpu, zigcpu):
             "libc/include/any-macos-any",
         ],
         linkopts = [],
+        dynamic_library_linkopts = [],
         copts = [],
         bazel_target_cpu = "darwin",
         constraint_values = [
@@ -84,6 +85,7 @@ def _target_linux_gnu(gocpu, zigcpu, glibc_version):
         compiler_extra_includes = ["glibc-hacks/glibchack-fcntl.h"] if fcntl_hack else [],
         linker_version_scripts = ["glibc-hacks/fcntl.map"] if fcntl_hack else [],
         linkopts = ["-lc++", "-lc++abi"],
+        dynamic_library_linkopts = [],
         copts = [],
         bazel_target_cpu = "k8",
         constraint_values = [
@@ -105,6 +107,7 @@ def _target_linux_musl(gocpu, zigcpu):
             "libc/include/{}-linux-any".format(zigcpu),
         ] + (["libc/include/x86-linux-any"] if zigcpu == "x86_64" else []),
         linkopts = ["-s", "-w"],
+        dynamic_library_linkopts = [],
         copts = ["-D_LIBCPP_HAS_MUSL_LIBC", "-D_LIBCPP_HAS_THREAD_API_PTHREAD"],
         bazel_target_cpu = "k8",
         constraint_values = [
diff --git a/toolchain/zig_toolchain.bzl b/toolchain/zig_toolchain.bzl
index aa14ec1..f22bc4c 100644
--- a/toolchain/zig_toolchain.bzl
+++ b/toolchain/zig_toolchain.bzl
@@ -14,6 +14,11 @@ all_link_actions = [
     ACTION_NAMES.cpp_link_nodeps_dynamic_library,
 ]
 
+dynamic_library_link_actions = [
+    ACTION_NAMES.cpp_link_dynamic_library,
+    ACTION_NAMES.cpp_link_nodeps_dynamic_library,
+]
+
 compile_and_link_actions = [
     ACTION_NAMES.c_compile,
     ACTION_NAMES.cpp_compile,
@@ -73,21 +78,31 @@ def _zig_cc_toolchain_config_impl(ctx):
         ],
     )
 
+    if ctx.attr.dynamic_library_linkopts:
+        dynamic_library_flag_sets = [
+            flag_set(
+                actions = dynamic_library_link_actions,
+                flag_groups = [flag_group(flags = ctx.attr.dynamic_library_linkopts)],
+            ),
+        ]
+    else:
+        dynamic_library_flag_sets = []
+
     default_linker_flags = feature(
         name = "default_linker_flags",
         enabled = True,
         flag_sets = [
             flag_set(
                 actions = all_link_actions,
-                flag_groups = ([
+                flag_groups = [
                     flag_group(
                         flags = ["-target", ctx.attr.target] +
                                 no_gc_sections +
                                 ctx.attr.linkopts,
                     ),
-                ]),
+                ],
             ),
-        ],
+        ] + dynamic_library_flag_sets,
     )
 
     features = [
@@ -119,6 +134,7 @@ zig_cc_toolchain_config = rule(
     attrs = {
         "cxx_builtin_include_directories": attr.string_list(),
         "linkopts": attr.string_list(),
+        "dynamic_library_linkopts": attr.string_list(),
         "copts": attr.string_list(),
         "tool_paths": attr.string_dict(),
         "target": attr.string(),
-- 
2.34.2

---

From: ~jvolkman <[email-masked]>
Date: Tue, 24 May 2022 17:17:33 -0700

This linker flag causes undefined symbols to be ignored, which is the
default behavior on linux but not macos. This is required when building
shared libraries to use as e.g. Python modules. This also matches what
Bazel's built-in macos cc toolchain does.
---
 toolchain/private/defs.bzl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/toolchain/private/defs.bzl b/toolchain/private/defs.bzl
index 053641c..42aaaeb 100644
--- a/toolchain/private/defs.bzl
+++ b/toolchain/private/defs.bzl
@@ -54,7 +54,7 @@ def _target_darwin(gocpu, zigcpu):
             "libc/include/any-macos-any",
         ],
         linkopts = [],
-        dynamic_library_linkopts = [],
+        dynamic_library_linkopts = ["-Wl,-undefined=dynamic_lookup"],
         copts = [],
         bazel_target_cpu = "darwin",
         constraint_values = [
-- 
2.34.2

---

From: "builds.sr.ht" <[email-masked]>
Date: Wed, 01 Jun 2022 02:12:16 +0000

bazel-zig-cc/patches/.build.yml: SUCCESS in 13m55s

[Ignore undefined symbols when building dynamic libraries on darwin][0] from [~jvolkman][1]

[0]: https://lists.sr.ht/~motiejus/bazel-zig-cc/patches/32674
[1]: mailto:[email-masked]

✓ #771513 SUCCESS bazel-zig-cc/patches/.build.yml https://builds.sr.ht/~motiejus/job/771513

---

From: Motiejus Jakštys <[email-masked]>
Date: Wed, 1 Jun 2022 12:02:06 +0300

> //test/gorace:gorace_test was failing before, but now passes.

As I have no idea how this works on Darwin, and we don't use Darwin
targets internally yet, my acceptance criteria for Darwin is:
- CI passes.
- after reading the commit messages it seems like you know what you
are doing. Though for next time please add more links where those are
discussed.
- I trust people on internet by default (in other words, I don't have
a reason to not trust you).

Applied.

> I haven't been able to get the arm64 tests to pass locally. Here's

I assume you mean linux_arm64. That fails for me too, because it is
relying on system's arm64 libc. The real resolution would be to use a
hermetic libc, which I haven't gotten around doing yet. But as long as
CI passes, we're good.

Motiejus

---

From: Motiejus Jakštys <[email-masked]>
Date: Wed, 1 Jun 2022 12:36:30 +0300

On Wed, Jun 1, 2022 at 12:30 PM Jeremy Volkman <[email-masked]> wrote:
>
> Hi Motiejus,
>
> Thanks for accepting. I'm not sure if you saw, but I had submitted this patch a week ago (broken) with more discussion and links in the overview. I forgot to increment the version number, so it wasn't obvious.

I had a vague memory of looking into this, but wasn't sure where.
Turns out that wasn't that long ago. :)

Anyhow, reason I am asking this: I try to apply patches as-is, so all
re-posts should be self-contained, as if earlier of them didn't
happen, because they are not going to the git history. Let's err on
the side of more verbosity than less, always.

Thanks,
Motiejus

---

From: Jeremy Volkman <[email-masked]>
Date: Wed, 1 Jun 2022 02:42:53 -0700

Sounds good. Thanks!

-Jeremy