Skip to content

Commit

Permalink
Fix versioned shared libraries for macOS toolchain (bazelbuild#20847)
Browse files Browse the repository at this point in the history 
The toolchain was passing -l:name. This mechanism doesn' t exist on
macOS, instead the full path to the shared library should be passed.

Mainline commit:
bazelbuild@f0ade80

As it says on that mainline commit, it cannot be checked in with a test
unlike this PR, because the macOS toolchain was moved outside of Bazel.
Therefore, the change must go in, the macOS toolchain needs to be
updated, then the test can be added. This change does go in with a test
because the toolchain is still embedded in Bazel for 6.5.

Fixes bazelbuild#20487
  • Loading branch information
@oquenchil
oquenchil authored Jan 12, 2024
1 parent 0cc7601 commit 29bbe31
Show file tree
Hide file tree
Showing 10 changed files with 108 additions and 24 deletions.
39 changes: 36 additions & 3 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,7 @@ public StructureValue build() {
public abstract static class LibraryToLinkValue extends VariableValueAdapter {
public static final String OBJECT_FILES_FIELD_NAME = "object_files";
public static final String NAME_FIELD_NAME = "name";
public static final String PATH_FIELD_NAME = "path";
public static final String TYPE_FIELD_NAME = "type";
public static final String IS_WHOLE_ARCHIVE_FIELD_NAME = "is_whole_archive";

Expand All @@ -680,8 +681,8 @@ public static LibraryToLinkValue forDynamicLibrary(String name) {
return new ForDynamicLibrary(name);
}

public static LibraryToLinkValue forVersionedDynamicLibrary(String name) {
return new ForVersionedDynamicLibrary(name);
public static LibraryToLinkValue forVersionedDynamicLibrary(String name, String path) {
return new ForVersionedDynamicLibrary(name, path);
}

public static LibraryToLinkValue forInterfaceLibrary(String name) {
Expand Down Expand Up @@ -802,8 +803,40 @@ protected String getTypeName() {
}

private static final class ForVersionedDynamicLibrary extends LibraryToLinkValueWithName {
private ForVersionedDynamicLibrary(String name) {
private String path;

private ForVersionedDynamicLibrary(String name, String path) {
super(name);
this.path = path;
}

@Override
public VariableValue getFieldValue(
String variableName,
String field,
@Nullable ArtifactExpander expander,
boolean throwOnMissingVariable) {
if (PATH_FIELD_NAME.equals(field)) {
return new StringValue(path);
}
return super.getFieldValue(variableName, field, expander, throwOnMissingVariable);
}

@Override
public boolean equals(Object obj) {
if (!(obj instanceof ForVersionedDynamicLibrary)) {
return false;
}
if (this == obj) {
return true;
}
ForVersionedDynamicLibrary other = (ForVersionedDynamicLibrary) obj;
return this.path.equals(other.path) && super.equals(other);
}

@Override
public int hashCode() {
return 31 * super.hashCode() + path.hashCode();
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -822,7 +822,7 @@ public static ImmutableList<CToolchain.Feature> getLegacyFeatures(
" variable: 'libraries_to_link.type'",
" value: 'versioned_dynamic_library'",
" }",
" flag: '-l:%{libraries_to_link.name}'",
" flag: '%{libraries_to_link.path}'",
" }"),
" flag_group {",
" expand_if_equal: {",
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,7 @@ private void addDynamicInputLinkOptions(
librariesToLink.addValue(LibraryToLinkValue.forDynamicLibrary(libName));
} else if (CppFileTypes.SHARED_LIBRARY.matches(name)
|| CppFileTypes.VERSIONED_SHARED_LIBRARY.matches(name)) {
librariesToLink.addValue(LibraryToLinkValue.forVersionedDynamicLibrary(name));
librariesToLink.addValue(LibraryToLinkValue.forVersionedDynamicLibrary(name, inputArtifact.getExecPathString()));
} else {
// Interface shared objects have a non-standard extension
// that the linker won't be able to find. So use the
Expand Down
15 changes: 15 additions & 0 deletions ...tarlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ cc_shared_library(
"bar_so",
"//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3:diff_pkg_so",
"private_lib_so",
"renamed_so_file_2",
],
features = ["windows_export_all_symbols"],
exports_filter = [
Expand Down Expand Up @@ -385,6 +386,11 @@ cc_library(
srcs = [":private_cc_library.cc"]
)

cc_library(
name = "private_lib_2",
srcs = [":private_cc_library.cc"]
)

build_failure_test(
name = "link_once_repeated_test_binary",
messages = [
Expand Down Expand Up @@ -447,6 +453,15 @@ cc_shared_library(
shared_lib_name = "renamed_so_file.so",
)

cc_shared_library(
name = "renamed_so_file_2",
features = ["windows_export_all_symbols"],
deps = [
":private_lib_2",
],
shared_lib_name = "renamed_so_file_2.so",
)

cc_library(
name = "direct_so_file_cc_lib",
srcs = [
Expand Down
1 change: 1 addition & 0 deletions ...tarlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/starlark_tests.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ def _runfiles_test_impl(ctx):
"libbar_so.so",
"libdiff_pkg_so.so",
"libprivate_lib_so.so",
"renamed_so_file_2.so",
"Smain_Sstarlark_Stests_Sbuiltins_Ubzl_Scc_Scc_Ushared_Ulibrary_Stest_Ucc_Ushared_Ulibrary_Slibfoo_Uso.so",
"Smain_Sstarlark_Stests_Sbuiltins_Ubzl_Scc_Scc_Ushared_Ulibrary_Stest_Ucc_Ushared_Ulibrary_Slibbar_Uso.so",
"Smain_Sstarlark_Stests_Sbuiltins_Ubzl_Scc_Scc_Ushared_Ulibrary_Stest_Ucc_Ushared_Ulibrary3_Slibdiff_Upkg_Uso.so",
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/com/google/devtools/build/lib/packages/util/mock/osx_cc_toolchain_config.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7540,11 +7540,11 @@ def _impl(ctx):
flag_group(
flag_groups = [
flag_group(
flags = ["-l:%{libraries_to_link.name}"],
flags = ["%{libraries_to_link.path}"],
expand_if_false = "libraries_to_link.is_whole_archive",
),
flag_group(
flags = ["-Wl,-force_load,-l:%{libraries_to_link.name}"],
flags = ["-Wl,-force_load,%{libraries_to_link.path}"],
expand_if_true = "libraries_to_link.is_whole_archive",
),
],
Expand Down
16 changes: 12 additions & 4 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/LibraryToLinkValueTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ public void equalsAndHashCode() {

// #forVersionedDynamicLibrary
equalsTester.addEqualityGroup(
LibraryToLinkValue.forVersionedDynamicLibrary("foo"),
LibraryToLinkValue.forVersionedDynamicLibrary("foo"));
equalsTester.addEqualityGroup(LibraryToLinkValue.forVersionedDynamicLibrary("bar"));
LibraryToLinkValue.forVersionedDynamicLibrary("foo", /* path= */ ""),
LibraryToLinkValue.forVersionedDynamicLibrary("foo", /* path= */ ""));
equalsTester.addEqualityGroup(LibraryToLinkValue.forVersionedDynamicLibrary("bar", /* path= */ ""));

// #forInterfaceLibrary
equalsTester.addEqualityGroup(
Expand Down Expand Up @@ -135,14 +135,22 @@ public void getFieldValue_forDynamicLibrary() {

@Test
public void getFieldValue_forVersionedDynamicLibrary() {
LibraryToLinkValue libraryToLinkValue = LibraryToLinkValue.forVersionedDynamicLibrary("foo");
LibraryToLinkValue libraryToLinkValue = LibraryToLinkValue.forVersionedDynamicLibrary("foo", "foo/bar.so");
assertThat(
libraryToLinkValue.getFieldValue(
/*variableName=*/ "variable name doesn't matter",
/*field=*/ "type",
/*expander=*/ null,
/*throwOnMissingVariable=*/ false))
.isEqualTo(new StringValue("versioned_dynamic_library"));
assertThat(
libraryToLinkValue
.getFieldValue(
/* variableName= */ "variable name doesn't matter",
/* field= */ "path",
/* expander= */ null,
/* throwOnMissingVariable= */ false))
.isEqualTo(new StringValue("foo/bar.so"));
assertThat(
libraryToLinkValue.getFieldValue(
/*variableName=*/ "variable name doesn't matter",
Expand Down
22 changes: 19 additions & 3 deletions tools/cpp/osx_cc_wrapper.sh.tpl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
set -eu

LIBS=
LIB_PATHS=
LIB_DIRS=
RPATHS=
OUTPUT=
Expand All @@ -38,6 +39,10 @@ function parse_option() {
OUTPUT=$opt
elif [[ "$opt" =~ ^-l(.*)$ ]]; then
LIBS="${BASH_REMATCH[1]} $LIBS"
elif [[ "$opt" =~ ^(.*)\.so$ ]]; then
LIB_PATHS="${opt} $LIB_PATHS"
elif [[ "$opt" =~ ^(.*)\.dylib$ ]]; then
LIB_PATHS="${opt} $LIB_PATHS"
elif [[ "$opt" =~ ^-L(.*)$ ]]; then
LIB_DIRS="${BASH_REMATCH[1]} $LIB_DIRS"
elif [[ "$opt" =~ ^\@loader_path/(.*)$ ]]; then
Expand Down Expand Up @@ -94,6 +99,11 @@ function get_otool_path() {
get_realpath $1 | sed 's|^.*/bazel-out/|bazel-out/|'
}

function call_install_name() {
/usr/bin/xcrun install_name_tool -change $(get_otool_path "$1") \
"@loader_path/$2/$3" "${OUTPUT}"
}

# Do replacements in the output
for rpath in ${RPATHS}; do
for lib in ${LIBS}; do
Expand All @@ -108,10 +118,16 @@ for rpath in ${RPATHS}; do
if [[ -n "${libname-}" ]]; then
libpath=$(get_library_path ${lib})
if [ -n "${libpath}" ]; then
/usr/bin/xcrun install_name_tool -change $(get_otool_path "${libpath}") \
"@loader_path/${rpath}/${libname}" "${OUTPUT}"
call_install_name "${libpath}" "${rpath}" "${libname}"
fi
fi
done
for libpath in ${LIB_PATHS}; do
if [ -f "$libpath" ]; then
libname=$(basename "$libpath")
if [ -f "$(dirname ${OUTPUT})/${rpath}/${libname}" ]; then
call_install_name "${libpath}" "${rpath}" "${libname}"
fi
fi
done
done

27 changes: 19 additions & 8 deletions tools/cpp/unix_cc_toolchain_config.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,24 @@ def _impl(ctx):
],
)

is_linux = ctx.attr.target_libc != "macosx"
if is_linux:
versioned_library_flag_group = flag_group(
flags = ["-l:%{libraries_to_link.name}"],
expand_if_equal = variable_with_value(
name = "libraries_to_link.type",
value = "versioned_dynamic_library",
),
)
else:
versioned_library_flag_group = flag_group(
flags = ["%{libraries_to_link.path}"],
expand_if_equal = variable_with_value(
name = "libraries_to_link.type",
value = "versioned_dynamic_library",
),
)

libraries_to_link_feature = feature(
name = "libraries_to_link",
flag_sets = [
Expand Down Expand Up @@ -868,13 +886,7 @@ def _impl(ctx):
value = "dynamic_library",
),
),
flag_group(
flags = ["-l:%{libraries_to_link.name}"],
expand_if_equal = variable_with_value(
name = "libraries_to_link.type",
value = "versioned_dynamic_library",
),
),
versioned_library_flag_group,
flag_group(
flags = ["-Wl,-no-whole-archive"],
expand_if_true = "libraries_to_link.is_whole_archive",
Expand Down Expand Up @@ -1283,7 +1295,6 @@ def _impl(ctx):
],
)

is_linux = ctx.attr.target_libc != "macosx"
libtool_feature = feature(
name = "libtool",
enabled = not is_linux,
Expand Down
4 changes: 2 additions & 2 deletions tools/osx/crosstool/cc_toolchain_config.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1478,11 +1478,11 @@ def _impl(ctx):
flag_group(
flag_groups = [
flag_group(
flags = ["-l:%{libraries_to_link.name}"],
flags = ["%{libraries_to_link.path}"],
expand_if_false = "libraries_to_link.is_whole_archive",
),
flag_group(
flags = ["-Wl,-force_load,-l:%{libraries_to_link.name}"],
flags = ["-Wl,-force_load,%{libraries_to_link.path}"],
expand_if_true = "libraries_to_link.is_whole_archive",
),
],
Expand Down

0 comments on commit 29bbe31

Please sign in to comment.