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

Revert "Fixes incorrect install names on darwin platforms" #15261

Closed
wants to merge 2 commits into from

Conversation

det
Copy link
Contributor

@det det commented Apr 14, 2022

This reverts commit b06f495.

Also had to add a missing mnemonic function call parameter to getDynamicLibrarySoname. Copy-pasted the parameter from the other use of that function in the codebase.

Tested manually with a custom toolchain and there is also a test from the reverted code.

Fixes #15214

@det det requested a review from lberki as a code owner April 14, 2022 19:00
@google-cla
Copy link

google-cla bot commented Apr 14, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@det det force-pushed the det/fix_runtime_solib_name branch from 6f7b174 to 5b88a98 Compare April 14, 2022 19:02
@det det marked this pull request as draft April 14, 2022 19:06
@det det force-pushed the det/fix_runtime_solib_name branch from 5b88a98 to 98efcca Compare April 14, 2022 21:24
@det det marked this pull request as ready for review April 14, 2022 21:29
@sgowroji sgowroji added the team-Rules-CPP Issues for C++ rules label Apr 16, 2022
@lberki lberki requested review from oquenchil and removed request for lberki April 19, 2022 09:57
@oquenchil
Copy link
Contributor

@keith could you please take a look?

@keith
Copy link
Member

keith commented Apr 20, 2022

should we get input from @csmulhern here who originally implemented this in case there's an easy way to fix this foward?

@det
Copy link
Contributor Author

det commented Apr 20, 2022

should we get input from @csmulhern here who originally implemented this in case there's an easy way to fix this foward?

FYI, I pinged him on #15214 9 days ago. Maybe someone has an internal Google way of contacting him?

@sgowroji sgowroji added the awaiting-review PR is awaiting review from an assigned reviewer label Apr 21, 2022
@lberki
Copy link
Contributor

lberki commented Apr 21, 2022

Friendly ping sent to @csmulhern .

@csmulhern
Copy link
Contributor

Sorry for the delayed response.

I'm not actually a bazel engineer, so I'm sure there are people with better context here than me. My motivation for the original change was to fix issues I was experiencing with --incompatible_macos_set_install_name, as detailed in b06f495. TL;DR: When using linkshared = 1 or wrapping a dylib in cc_library, the library name was not mangled, and so the RPATH was being set incorrectly. The former seems like it might have been a behavioral change in bazel in the recent past (i.e. between the initial commit and my "fix"). Regardless, in my view the updated test case seems like it should stay, as it actually exercises the behavior of using a shared library. It appears there are still cases in which mangled library names are used, and the output object in CppLinkAction.build is not yet properly configured to have its path set as mangled vs unmangled.

In that case, it seems the correct fix would be to basically use the original code:

SolibSymlinkAction.getDynamicLibrarySoname(
                   output.getRootRelativePath(),
                   /* preserveName= */ false,
                   actionConstructionContext.getConfiguration().getMnemonic())

but to set preserveName based on whether the filename will be mangled or not (which doesn't seem straightforward to figure out). It would be good to get input from a bazel engineer who knows when this would or would not be the case.

@csmulhern
Copy link
Contributor

For reference, here is what I observe when running the existing test at HEAD today.

> otool -L <test_binary>
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1300.23.0)
	@rpath/libbar.so (compatibility version 0.0.0, current version 0.0.0)
	@rpath/libbaz.dylib (compatibility version 0.0.0, current version 0.0.0)
	/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation (compatibility version 300.0.0, current version 1858.112.0)
	/usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1311.100.3)
> otool -l <test_binary>| rg RPATH -A 2
          cmd LC_RPATH
      cmdsize 80
         path @loader_path/../_solib_darwin_arm64/_U_S_Stest_Ctest___Utest (offset 12)
> ls <test_binary>/../_solib_darwin_arm64/_U_S_Stest_Ctest___Utest
libbar.so    libbaz.dylib

As you can see, the test binary has its RPATH set to the solib symlink directory. The dynamic libraries are located by searching the RPATH, and the library names in the binary (and the symlink names) are unmangled.

@csmulhern
Copy link
Contributor

And with this change:

> otool -L <test_binary>
   /usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1300.23.0)
   @rpath/libtest_Slibbar.so (compatibility version 0.0.0, current version 0.0.0)
   @rpath/libtest_Slibbaz.dylib (compatibility version 0.0.0, current version 0.0.0)
   /System/Library/Frameworks/Foundation.framework/Versions/C/Foundation (compatibility version 300.0.0, current version 1858.112.0)
   /usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)
   /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1311.100.3)
> otool -l <test_binary>| rg RPATH -A 2
          cmd LC_RPATH
      cmdsize 80
         path @loader_path/../_solib_darwin_arm64/_U_S_Stest_Ctest___Utest (offset 12)
> ls <test_binary>/../_solib_darwin_arm64/_U_S_Stest_Ctest___Utest
libbar.so    libbaz.dylib

The test fails as the library names are wrong (e.g. libtest_Slibbar.so in the binary, but libbar.so in the solib symlink directory).

@csmulhern
Copy link
Contributor

Note: the above output is when using --incompatible_macos_set_install_name, as that is the only time the argument being modified in CppLinkAction.build is used.

When not using this flag, the binary locates the shared libraries using @loader_path, and this behavior is the same with or without this change.

> otool -L <test_binary>
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1300.23.0)
	@loader_path/../_solib_darwin_arm64/_U_S_Stest_Ctest___Utest/libbar.so (compatibility version 0.0.0, current version 0.0.0)
	@loader_path/../_solib_darwin_arm64/_U_S_Stest_Ctest___Utest/libbaz.dylib (compatibility version 0.0.0, current version 0.0.0)
	/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation (compatibility version 300.0.0, current version 1858.112.0)
	/usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1311.100.3)

@pcjanzen
Copy link
Contributor

The test that was introduced in b06f495 is IMO a little borderline. The docs for cc_binary.linkshared explicitly state

However, for build purposes it will never be linked into the dependent binary, as it is assumed that shared libraries built with a cc_binary rule are only loaded manually by other programs, so it should not be considered a substitute for the cc_library

so since you are technically not supposed to link to such a binary, only dlopen it, it does not matter what its install_name is.

The whole point of runtime_solib_name is to allow cc_toolchains that have the supports_dynamic_linker feature to create dynamic cc_librarys with the correct soname, and #13427 breaks that.

Unfortunately, between the time that #12304 was committed and the time that #13427 was submitted, the default darwin toolchain stopped using supports_dynamic_linker (ec55533), so in the existing test, //cpp:foo is now linked statically, never dynamically, and there is no install_name to test.

@csmulhern
Copy link
Contributor

Thanks for bringing that to my attention. What would be the appropriate test case then? Is there existing infrastructure for testing with a custom toolchain that has supports_dynamic_linker set to true?

If I have a prebuilt dylib that I bring in to bazel using cc_import, I believe that will also result in an unmangled name, and that this should be a supported use case. As such, we would still want to support mangled and unmangled names within runtime_solib_name.

@pcjanzen
Copy link
Contributor

If I have a prebuilt dylib that I bring in to bazel using cc_import, I believe that will also result in an unmangled name, and that this should be a supported use case. As such, we would still want to support mangled and unmangled names within runtime_solib_name.

There are no link actions for a prebuilt dylib, so I'm not sure how you would use runtime_solib_name in that case.

@csmulhern
Copy link
Contributor

There are no link actions for a prebuilt dylib, so I'm not sure how you would use runtime_solib_name in that case.

You're right. I was under the impression that --incompatible_macos_set_install_name was affecting the install name in this case, but it appears that's not the case. The test I had in mind that was failing is in the rules_rust repository, but it appears that that is using cc_import on a cc_binary rule: https://github.com/bazelbuild/rules_rust/blob/main/examples/ffi/rust_calling_c/c/BUILD.bazel#L30. It sounds like that is just not a supported use case for cc_binary.

I think going ahead and reverting sounds like the right course of action here then, which should fix cc toolchains using supports_dynamic_linker. Ideally the test case should be updated to actually test --incompatible_macos_set_install_name.

@pcjanzen
Copy link
Contributor

Don't get me wrong, I think that the ideal scenario is for runtime_solib_name to return the basename of the file in the specific case of cc_binary(linkshared = True), and the mangled name for a cc_library, but if that is difficult or impossible then we should prefer to get the cc_library case correct.

Copy link
Contributor

@oquenchil oquenchil left a comment

Choose a reason for hiding this comment

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

LGTM

@bazel-io bazel-io closed this in 6b21b77 May 5, 2022
@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 May 5, 2022
@ckolli5
Copy link

ckolli5 commented May 9, 2022

@bazel-io fork 5.2.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label May 9, 2022
ckolli5 added a commit that referenced this pull request May 10, 2022
This reverts commit b06f495.

Also had to add a missing mnemonic function call parameter to `getDynamicLibrarySoname`. Copy-pasted the parameter from the other use of that function in the codebase.

Tested manually with a custom toolchain and there is also a test from the reverted code.

Fixes #15214

Closes #15261.

PiperOrigin-RevId: 446662219

Co-authored-by: Chris Clearwater <chris@clearwater.dev>
meteorcloudy pushed a commit that referenced this pull request May 10, 2022
This reverts commit b06f495.

Also had to add a missing mnemonic function call parameter to `getDynamicLibrarySoname`. Copy-pasted the parameter from the other use of that function in the codebase.

Tested manually with a custom toolchain and there is also a test from the reverted code.

Fixes #15214

Closes #15261.

PiperOrigin-RevId: 446662219

Co-authored-by: Chris Clearwater <chris@clearwater.dev>
@ShreeM01 ShreeM01 removed the awaiting-review PR is awaiting review from an assigned reviewer label Sep 15, 2022
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.

#13427 Broke mac C++ toolchains