Skip to content

Commit

Permalink
Remove use of -undefined dynamic_lookup on darwin
Browse files Browse the repository at this point in the history 
This flag ignores undefined symbols at link time and forces them to be looked up dynamically. This is incompatible with the newer fixup chains macho format and theoretically shouldn't be necessary assuming your dependencies are well defined.

Fixes #16413

Closes #16414.

PiperOrigin-RevId: 501275674
Change-Id: I9240b2596ffea329ef55d3ad15e98635ba8839b6
  • Loading branch information
@keith @copybara-github
keith authored and copybara-github committed Jan 11, 2023
1 parent 27510e0 commit 4853dfd
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 109 deletions.
5 changes: 4 additions & 1 deletion src/main/native/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,10 @@ cc_binary(
],
includes = ["."], # For jni headers.
linkopts = select({
"//src/conditions:darwin": ["-framework CoreServices"],
"//src/conditions:darwin": [
"-Wl,-framework,CoreServices",
"-Wl,-framework,IOKit",
],
"//conditions:default": [],
}),
linkshared = 1,
Expand Down
48 changes: 0 additions & 48 deletions src/test/shell/bazel/cc_integration_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,54 +234,6 @@ EOF
"Second build failed, tree artifact was not invalidated."
}

# This test tests that Bazel can produce dynamic libraries that have undefined
# symbols on Mac and Linux. Not sure it is a sane default to allow undefined
# symbols, but it's the default we had historically. This test creates
# an executable (main) that defines bar(), and a shared library (plugin) that
# calls bar(). When linking the libplugin.so, symbol 'bar' is undefined.
# +-----------------------------+ +----------------------------------+
# | main | | libplugin.so |
# | | | |
# | main() { return foo(); } +---------> foo() { return bar() - 42; } |
# | | | + |
# | | | | |
# | bar() { return 42; } <------------------+ |
# | | | |
# +-----------------------------+ +----------------------------------+
function test_undefined_dynamic_lookup() {
if is_windows; then
# Windows doesn't allow undefined symbols in shared libraries.
return 0
fi
mkdir -p "dynamic_lookup"
cat > "dynamic_lookup/BUILD" <<EOF
cc_binary(
name = "libplugin.so",
srcs = ["plugin.cc"],
linkshared = 1,
)
cc_binary(
name = "main",
srcs = ["main.cc", "libplugin.so"],
)
EOF

cat > "dynamic_lookup/plugin.cc" <<EOF
int bar();
int foo() { return bar() - 42; }
EOF

cat > "dynamic_lookup/main.cc" <<EOF
int foo();
int bar() { return 42; }
int main() { return foo(); }
EOF

bazel build //dynamic_lookup:main || fail "Bazel couldn't build the binary."
bazel run //dynamic_lookup:main || fail "Run of the binary failed."
}

function test_save_feature_state() {
mkdir -p ea
cat > ea/BUILD <<EOF
Expand Down
2 changes: 0 additions & 2 deletions tools/cpp/unix_cc_configure.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -593,8 +593,6 @@ def configure_unix_toolchain(repository_ctx, cpu_value, overriden_tools):
"-z",
) + (
[
"-undefined",
"dynamic_lookup",
"-headerpad_max_install_names",
] if darwin else bin_search_flags + [
# Gold linker only? Can we enable this by default?
Expand Down
77 changes: 19 additions & 58 deletions tools/osx/crosstool/cc_toolchain_config.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1039,64 +1039,25 @@ def _impl(ctx):
requires = [feature_set(features = ["coverage"])],
)

if (ctx.attr.cpu == "darwin_x86_64" or
ctx.attr.cpu == "darwin_arm64" or
ctx.attr.cpu == "darwin_arm64e"):
default_link_flags_feature = feature(
name = "default_link_flags",
enabled = True,
flag_sets = [
flag_set(
actions = all_link_actions +
["objc-executable", "objc++-executable"],
flag_groups = [
flag_group(
flags = [
"-no-canonical-prefixes",
"-target",
target_system_name,
],
),
],
),
flag_set(
actions = [
ACTION_NAMES.cpp_link_dynamic_library,
ACTION_NAMES.cpp_link_nodeps_dynamic_library,
],
flag_groups = [flag_group(flags = ["-undefined", "dynamic_lookup"])],
),
flag_set(
actions = [
ACTION_NAMES.cpp_link_executable,
"objc-executable",
"objc++-executable",
],
flag_groups = [flag_group(flags = ["-undefined", "dynamic_lookup"])],
with_features = [with_feature_set(features = ["dynamic_linking_mode"])],
),
],
)
else:
default_link_flags_feature = feature(
name = "default_link_flags",
enabled = True,
flag_sets = [
flag_set(
actions = all_link_actions +
["objc-executable", "objc++-executable"],
flag_groups = [
flag_group(
flags = [
"-no-canonical-prefixes",
"-target",
target_system_name,
],
),
],
),
],
)
default_link_flags_feature = feature(
name = "default_link_flags",
enabled = True,
flag_sets = [
flag_set(
actions = all_link_actions +
["objc-executable", "objc++-executable"],
flag_groups = [
flag_group(
flags = [
"-no-canonical-prefixes",
"-target",
target_system_name,
],
),
],
),
],
)

no_deduplicate_feature = feature(
name = "no_deduplicate",
Expand Down

3 comments on commit 4853dfd

@comius
Copy link
Contributor

@comius comius commented on 4853dfd Jan 17, 2023

Choose a reason for hiding this comment

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

This seem to have broken Stardoc on Mac https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/2824#0185bd7e-6fed-4c16-ad52-256b600d4ae3

@brentleyjones
Copy link
Contributor

Choose a reason for hiding this comment

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

@keith

@keith
Copy link
Member Author

@keith keith commented on 4853dfd Jan 17, 2023

Choose a reason for hiding this comment

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

@comius bazelbuild/stardoc#146

Please sign in to comment.