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

Remove use of -undefined dynamic_lookup on darwin #16414

Conversation

keith
Copy link
Member

@keith keith commented Oct 6, 2022

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

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 bazelbuild#16413
@keith keith force-pushed the ks/remove-use-of-undefined-dynamic_lookup-on-darwin branch from 65bc2a9 to f0b850f Compare October 6, 2022 23:04
@keith
Copy link
Member Author

keith commented Oct 6, 2022

The second commit here shows a potential issue making this change could introduce, that library had incorrect dependencies but just happened to work at runtime because the library it needed must have been transitively loaded anyways. I also found this #7607

@sgowroji sgowroji added team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels Oct 7, 2022
@keith
Copy link
Member Author

keith commented Jan 6, 2023

@oquenchil can you take a look?

@oquenchil oquenchil added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jan 9, 2023
@keith keith deleted the ks/remove-use-of-undefined-dynamic_lookup-on-darwin branch January 11, 2023 15:44
@brentleyjones
Copy link
Contributor

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jan 11, 2023
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jan 11, 2023
@keith
Copy link
Member Author

keith commented Jan 12, 2023

@meteorcloudy
Copy link
Member

This looks like a breaking change:
https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/2819#0185a94e-46ce-4a78-aa61-e658115dac01

Undefined symbols for architecture x86_64:
  "_google_protobuf_MessageOptions_msg_init", referenced from:
      _upb_test_my_option_ext in proto3_test.upb.o
ld: symbol(s) not found for architecture x86_64

Detected by culprit finder: https://buildkite.com/bazel/culprit-finder/builds/4178#0185a952-34c1-46ae-99c8-9b40d67508ec

@meteorcloudy
Copy link
Member

Since this is likely a breaking change, I'll remove the "potential release blocker" label.

@meteorcloudy meteorcloudy removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jan 13, 2023
hvadehra pushed a commit that referenced this pull request Feb 14, 2023
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
keith added a commit to bazel-contrib/toolchains_llvm that referenced this pull request Feb 14, 2023
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.

Done in bazel's toolchain here bazelbuild/bazel#16414
@garymm
Copy link

garymm commented Feb 28, 2023

@keith Is googletest doing something wrong?
It seems this change breaks using gtest_main.

git clone [email protected]:google/googletest.git
cd googletest

Before this change:

USE_BAZEL_VERSION=7.0.0-pre.20221212.2 bazelisk test --java_runtime_version=remotejdk_11 --copt='-std=c++14' //googletest/test:gtest_all_test

Succeeds

After this change:

USE_BAZEL_VERSION=7.0.0-pre.20230118.2 bazelisk test --java_runtime_version=remotejdk_11 --copt='-std=c++14' //googletest/test:gtest_all_test

ERROR: /Users/garymm/src/google/googletest/BUILD.bazel:152:11: Linking libgtest_main.dylib failed: (Exit 1): cc_wrapper.sh failed: error executing command (from target //:gtest_main) external/local_config_cc/cc_wrapper.sh @bazel-out/darwin_arm64-fastbuild/bin/libgtest_main.dylib-2.params

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
Undefined symbols for architecture arm64:
  "testing::InitGoogleMock(int*, char**)", referenced from:
      _main in gmock_main.pic.o
  "testing::UnitTest::GetInstance()", referenced from:
      RUN_ALL_TESTS() in gmock_main.pic.o
  "testing::UnitTest::Run()", referenced from:
      RUN_ALL_TESTS() in gmock_main.pic.o
ld: symbol(s) not found for architecture arm64

keith added a commit to keith/bazel that referenced this pull request Feb 28, 2023
This feature isn't supported on macOS, and it isn't part of the Xcode CC
toolchain, but it is left over here from when it was supported in the
past.

bazelbuild@ec55533

Fixes: bazelbuild#16414 (comment)
@keith
Copy link
Member Author

keith commented Feb 28, 2023

@garymm thanks I have a fix for this in #16619 but also submitted #17624 with just that so it might be merged sooner. In the meantime you can workaround by using the full Xcode toolchain, or you can pass --features=-supports_dynamic_linker on macOS

keith added a commit to bazel-contrib/toolchains_llvm that referenced this pull request Jul 17, 2023
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.

Done in bazel's toolchain here bazelbuild/bazel#16414
keith added a commit to bazel-contrib/toolchains_llvm that referenced this pull request Jul 17, 2023
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.

Done in bazel's toolchain here bazelbuild/bazel#16414
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

linker warnings on macOS XCode 14: ld: warning: -undefined dynamic_lookup may not work with chained fixups
7 participants