Skip to content

Commit

Permalink
C++: Fix crash reported in #7721
Browse files Browse the repository at this point in the history
objc_libraries used to always take the static libraries of C++ dependencies even if dynamic libraries were available. This is because support for dynamic linking of C++ dependencies hasn't been added to objective-C rules yet.

With 3e727b1#diff-033df3f3a834149658486b8a5754baefL438, we started propagating both static libraries and dynamic libraries in the new implementation of LibraryToLink. Since cc_test by default will always try to link dynamically, the behavior changed and if we had the dependency chain cc_test -> objc_library -> cc_library; the cc_test would start using the .so file from the cc_library, whereas before it used the .a file. Linking the .so file gives an error since as mentioned, support for this hasn't been added to Objective-C.

The fix has been to nullify the dynamic library fields of LibraryToLink as long as a static library was present. This makes the behavior the same as before.

RELNOTES:none
PiperOrigin-RevId: 239226003
  • Loading branch information
oquenchil authored and Copybara-Service committed Mar 19, 2019
1 parent 044a5f9 commit ea1703b
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,8 @@ public static List<Artifact> getDynamicLibrariesForLinking(Iterable<LibraryToLin
return dynamicLibrariesForLinkingBuilder.build();
}

public abstract Builder toBuilder();

/** Builder for LibraryToLink. */
@AutoValue.Builder
public abstract static class Builder {
Expand All @@ -592,11 +594,13 @@ abstract Builder setPicSharedNonLtoBackends(

public abstract Builder setDynamicLibrary(Artifact dynamicLibrary);

abstract Builder setResolvedSymlinkDynamicLibrary(Artifact resolvedSymlinkDynamicLibrary);
public abstract Builder setResolvedSymlinkDynamicLibrary(
Artifact resolvedSymlinkDynamicLibrary);

abstract Builder setInterfaceLibrary(Artifact interfaceLibrary);
public abstract Builder setInterfaceLibrary(Artifact interfaceLibrary);

abstract Builder setResolvedSymlinkInterfaceLibrary(Artifact resolvedSymlinkInterfaceLibrary);
public abstract Builder setResolvedSymlinkInterfaceLibrary(
Artifact resolvedSymlinkInterfaceLibrary);

public abstract Builder setAlwayslink(boolean alwayslink);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,8 @@ private CcLinkingContext buildCcLinkingContext(
FileSystemUtils.removeExtension(library.getRootRelativePathString()))
.build());
}
libraries.addAll(objcProvider.get(ObjcProvider.CC_LIBRARY));

libraries.addAll(convertLibrariesToStaticLibraries(objcProvider.get(ObjcProvider.CC_LIBRARY)));

CcLinkingContext.Builder ccLinkingContext =
CcLinkingContext.builder()
Expand All @@ -153,6 +154,27 @@ private CcLinkingContext buildCcLinkingContext(
return ccLinkingContext.build();
}

/**
* This method removes dynamic libraries from LibraryToLink objects coming from C++ dependencies.
* The reason for this is that objective-C rules do not support linking the dynamic version of the
* libraries.
*/
private ImmutableList<LibraryToLink> convertLibrariesToStaticLibraries(
Iterable<LibraryToLink> librariesToLink) {
ImmutableList.Builder<LibraryToLink> libraries = ImmutableList.builder();
for (LibraryToLink libraryToLink : librariesToLink) {
LibraryToLink.Builder staticLibraryToLink = libraryToLink.toBuilder();
if (libraryToLink.getPicStaticLibrary() != null || libraryToLink.getStaticLibrary() != null) {
staticLibraryToLink.setDynamicLibrary(null);
staticLibraryToLink.setResolvedSymlinkDynamicLibrary(null);
staticLibraryToLink.setInterfaceLibrary(null);
staticLibraryToLink.setResolvedSymlinkInterfaceLibrary(null);
}
libraries.add(staticLibraryToLink.build());
}
return libraries.build();
}

/** Throws errors or warnings for bad attribute state. */
private static void validateAttributes(RuleContext ruleContext) throws RuleErrorException {
if (ObjcRuleClasses.objcConfiguration(ruleContext).disableObjcLibraryResources()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import com.google.devtools.build.lib.analysis.config.CompilationMode;
import com.google.devtools.build.lib.analysis.util.ScratchAttributeWriter;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.util.MockCcSupport;
import com.google.devtools.build.lib.packages.util.MockObjcSupport;
import com.google.devtools.build.lib.rules.apple.ApplePlatform;
import com.google.devtools.build.lib.rules.apple.AppleToolchain;
Expand Down Expand Up @@ -2134,4 +2135,20 @@ public void testIncompatibleResourceAttributeFlag_resourcesEmpty() throws Except
"objc_library resource attributes are not allowed. Please use the 'data' attribute instead",
"objc_library(name = 'x', resources = [])");
}

/** Regression test for https://github.com/bazelbuild/bazel/issues/7721. */
@Test
public void testToolchainRuntimeLibrariesSolibDir() throws Exception {
MockObjcSupport.setup(
mockToolsConfig,
MockCcSupport.SUPPORTS_INTERFACE_SHARED_LIBRARIES_FEATURE,
MockCcSupport.SUPPORTS_DYNAMIC_LINKER_FEATURE);
scratch.file(
"foo/BUILD",
"cc_test(name = 'd', deps = [':b'])",
"objc_library(name = 'b', deps = [':a'])",
"cc_library(name = 'a', srcs = ['a.c'])");
ConfiguredTarget configuredTarget = getConfiguredTarget("//foo:d");
assertThat(configuredTarget).isNotNull();
}
}
37 changes: 37 additions & 0 deletions src/test/shell/bazel/apple/bazel_objc_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -159,4 +159,41 @@ EOF
|| fail "should fail to find symbol addOne"
}

function test_cc_test_depending_on_objc() {
setup_objc_test_support

rm -rf foo
mkdir -p foo

cat >foo/a.cc <<EOF
#include <iostream>
int main(int argc, char** argv) {
std::cout << "Hello! I'm a test!\n";
return 0;
}
EOF

cat >foo/BUILD <<EOF
cc_library(
name = "a",
srcs = ["a.cc"],
)
objc_library(
name = "b",
deps = [
":a",
],
)
cc_test(
name = "d",
deps = [":b"],
)
EOF

bazel test --verbose_failures \
//foo:d>$TEST_log 2>&1 || fail "should pass"
}

run_suite "objc/ios test suite"

0 comments on commit ea1703b

Please sign in to comment.