C++: Make cc_binary error out for colliding libraries
Make cc_binary give an error if two shared libraries in its dependencies or
itself link the same library statically.
Test in unknown commit. Since we are testing with last bazel green, this CL must be submitted first.
RELNOTES:none
PiperOrigin-RevId: 298372502
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
index 2386f48..7c61b38 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
@@ -16,6 +16,7 @@
import static com.google.devtools.build.lib.rules.cpp.CppRuleClasses.DYNAMIC_LINKING_MODE;
import static com.google.devtools.build.lib.rules.cpp.CppRuleClasses.STATIC_LINKING_MODE;
+import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Function;
import com.google.common.base.Preconditions;
@@ -32,6 +33,8 @@
import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
import com.google.devtools.build.lib.analysis.AnalysisUtils;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.FileProvider;
+import com.google.devtools.build.lib.analysis.FilesToRunProvider;
import com.google.devtools.build.lib.analysis.MakeVariableSupplier.MapBackedMakeVariableSupplier;
import com.google.devtools.build.lib.analysis.OutputGroupInfo;
import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
@@ -46,6 +49,8 @@
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
import com.google.devtools.build.lib.analysis.actions.SymlinkAction;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
+import com.google.devtools.build.lib.analysis.test.AnalysisFailure;
+import com.google.devtools.build.lib.analysis.test.AnalysisFailureInfo;
import com.google.devtools.build.lib.analysis.test.ExecutionInfo;
import com.google.devtools.build.lib.analysis.test.InstrumentedFilesInfo;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -273,10 +278,16 @@
@Override
public ConfiguredTarget create(RuleContext context)
throws InterruptedException, RuleErrorException, ActionConflictException {
- return CcBinary.init(semantics, context, /*fake =*/ false);
+ RuleConfiguredTargetBuilder ruleBuilder = new RuleConfiguredTargetBuilder(context);
+ CcBinary.init(semantics, ruleBuilder, context, /*fake =*/ false);
+ return ruleBuilder.build();
}
- public static ConfiguredTarget init(CppSemantics semantics, RuleContext ruleContext, boolean fake)
+ public static void init(
+ CppSemantics semantics,
+ RuleConfiguredTargetBuilder ruleBuilder,
+ RuleContext ruleContext,
+ boolean fake)
throws InterruptedException, RuleErrorException, ActionConflictException {
CcCommon.checkRuleLoadedThroughMacro(ruleContext);
semantics.validateDeps(ruleContext);
@@ -294,7 +305,8 @@
}
}
if (ruleContext.hasErrors()) {
- return null;
+ fillInRequiredProviders(ruleBuilder, ruleContext);
+ return;
}
CcCommon common = new CcCommon(ruleContext);
@@ -314,7 +326,8 @@
semantics.validateAttributes(ruleContext);
if (ruleContext.hasErrors()) {
- return null;
+ fillInRequiredProviders(ruleBuilder, ruleContext);
+ return;
}
// if cc_binary includes "linkshared=1", then gcc will be invoked with
@@ -335,7 +348,8 @@
&& !CppFileTypes.SHARED_LIBRARY.matches(binary.getFilename())
&& !CppFileTypes.VERSIONED_SHARED_LIBRARY.matches(binary.getFilename())) {
ruleContext.attributeError("linkshared", "'linkshared' used in non-shared library");
- return null;
+ fillInRequiredProviders(ruleBuilder, ruleContext);
+ return;
}
LinkingMode linkingMode = getLinkStaticness(ruleContext, cppConfiguration);
@@ -363,7 +377,8 @@
.build();
if (ruleContext.hasErrors()) {
- return null;
+ fillInRequiredProviders(ruleBuilder, ruleContext);
+ return;
}
CcCompilationHelper compilationHelper =
@@ -534,7 +549,8 @@
pdbFile,
winDefFile);
if (ruleContext.hasErrors()) {
- return null;
+ fillInRequiredProviders(ruleBuilder, ruleContext);
+ return;
}
CcLinkingOutputs ccLinkingOutputsBinary = ccLinkingOutputsAndCcLinkingInfo.first;
@@ -637,7 +653,6 @@
linkCompileOutputSeparately);
RunfilesSupport runfilesSupport = RunfilesSupport.withExecutable(ruleContext, runfiles, binary);
- RuleConfiguredTargetBuilder ruleBuilder = new RuleConfiguredTargetBuilder(ruleContext);
addTransitiveInfoProviders(
ruleContext,
ccToolchain,
@@ -693,14 +708,13 @@
}
CcSkylarkApiProvider.maybeAdd(ruleContext, ruleBuilder);
- return ruleBuilder
+ ruleBuilder
.addProvider(RunfilesProvider.class, RunfilesProvider.simple(runfiles))
.addProvider(
DebugPackageProvider.class,
new DebugPackageProvider(ruleContext.getLabel(), strippedFile, binary, explicitDwpFile))
.setRunfilesSupport(runfilesSupport, binary)
- .addNativeDeclaredProvider(ccLauncherInfo)
- .build();
+ .addNativeDeclaredProvider(ccLauncherInfo);
}
public static Pair<CcLinkingOutputs, CcLauncherInfo> createTransitiveLinkingActions(
@@ -1170,12 +1184,40 @@
}
}
- private static ImmutableList<Pair<List<String>, CcLinkingContext.LinkerInput>>
- mergeCcSharedLibraryInfos(RuleContext ruleContext, CppSemantics cppSemantics) {
- ImmutableList.Builder<Pair<List<String>, CcLinkingContext.LinkerInput>>
- directMergedCcSharedLibraryInfos = ImmutableList.builder();
- ImmutableList.Builder<Pair<List<String>, CcLinkingContext.LinkerInput>>
- transitiveMergedCcSharedLibraryInfos = ImmutableList.builder();
+ /**
+ * Class for working more easily with the fields from the Starlark CcSharedLibraryInfo provider
+ */
+ @AutoValue
+ public abstract static class CcSharedLibraryInfo {
+ abstract ImmutableList<String> getExports();
+
+ abstract CcLinkingContext.LinkerInput getLinkerInput();
+
+ abstract ImmutableList<String> getLinkOnceStaticLibs();
+
+ static CcSharedLibraryInfo.Builder builder() {
+ return new AutoValue_CcBinary_CcSharedLibraryInfo.Builder();
+ }
+
+ /** Builder for CcSharedLibraryInfo */
+ @AutoValue.Builder
+ public abstract static class Builder {
+ abstract Builder setExports(List<String> exports);
+
+ abstract Builder setLinkerInput(CcLinkingContext.LinkerInput linkerInput);
+
+ abstract Builder setLinkOnceStaticLibs(List<String> linkOnceStaticLibs);
+
+ abstract CcSharedLibraryInfo build();
+ }
+ }
+
+ private static ImmutableList<CcSharedLibraryInfo> mergeCcSharedLibraryInfos(
+ RuleContext ruleContext, CppSemantics cppSemantics) {
+ ImmutableList.Builder<CcSharedLibraryInfo> directMergedCcSharedLibraryInfos =
+ ImmutableList.builder();
+ ImmutableList.Builder<CcSharedLibraryInfo> transitiveMergedCcSharedLibraryInfos =
+ ImmutableList.builder();
for (TransitiveInfoCollection dep : ruleContext.getPrerequisites("dynamic_deps", Mode.TARGET)) {
StructImpl ccSharedLibraryInfo = cppSemantics.getCcSharedLibraryInfo(dep);
if (ccSharedLibraryInfo == null) {
@@ -1207,7 +1249,25 @@
}
CcLinkingContext.LinkerInput linkerInput = (CcLinkingContext.LinkerInput) linkerInputField;
- directMergedCcSharedLibraryInfos.add(Pair.of(exports, linkerInput));
+ Object linkOnceStaticLibsField = ccSharedLibraryInfo.getValue("link_once_static_libs");
+ if (linkOnceStaticLibsField == null) {
+ ruleContext.ruleError(
+ String.format(
+ "The cc_shared_library '%s' does not have a 'link_once_static_libs' field",
+ dep.getLabel()));
+ return null;
+ }
+ ImmutableList<String> linkOnceStaticLibs =
+ ImmutableList.copyOf(
+ Sequence.castSkylarkListOrNoneToList(
+ linkOnceStaticLibsField, String.class, "link_once_static_libs"));
+
+ directMergedCcSharedLibraryInfos.add(
+ CcSharedLibraryInfo.builder()
+ .setExports(exports)
+ .setLinkerInput(linkerInput)
+ .setLinkOnceStaticLibs(linkOnceStaticLibs)
+ .build());
Object dynamicDepsField = ccSharedLibraryInfo.getValue("dynamic_deps");
if (dynamicDepsField == null) {
@@ -1227,11 +1287,20 @@
for (Tuple<Object> exportsAndLinkerInput : dynamicDeps.toList()) {
List<String> exportsFromDynamicDep =
Sequence.castSkylarkListOrNoneToList(
- exportsAndLinkerInput.get(0), String.class, "exports_from_dynamic_deps");
+ exportsAndLinkerInput.get(0), String.class, "exports_from_dynamic_dep");
CcLinkingContext.LinkerInput linkerInputFromDynamicDep =
(CcLinkingContext.LinkerInput) exportsAndLinkerInput.get(1);
+ List<String> linkOnceStaticLibsFromDynamicDep =
+ Sequence.castSkylarkListOrNoneToList(
+ exportsAndLinkerInput.get(0),
+ String.class,
+ "link_once_static_libs_from_dynamic_dep");
transitiveMergedCcSharedLibraryInfos.add(
- Pair.of(exportsFromDynamicDep, linkerInputFromDynamicDep));
+ CcSharedLibraryInfo.builder()
+ .setExports(exportsFromDynamicDep)
+ .setLinkerInput(linkerInputFromDynamicDep)
+ .setLinkOnceStaticLibs(linkOnceStaticLibsFromDynamicDep)
+ .build());
}
} catch (EvalException e) {
ruleContext.ruleError(
@@ -1240,7 +1309,7 @@
return null;
}
}
- return ImmutableList.<Pair<List<String>, CcLinkingContext.LinkerInput>>builder()
+ return ImmutableList.<CcSharedLibraryInfo>builder()
.addAll(directMergedCcSharedLibraryInfos.build())
.addAll(transitiveMergedCcSharedLibraryInfos.build())
.build();
@@ -1248,14 +1317,10 @@
private static ImmutableMap<String, CcLinkingContext.LinkerInput>
buildExportsMapFromOnlyDynamicDeps(
- RuleContext ruleContext,
- ImmutableList<Pair<List<String>, CcLinkingContext.LinkerInput>>
- mergedCcSharedLibraryInfos) {
+ RuleContext ruleContext, ImmutableList<CcSharedLibraryInfo> mergedCcSharedLibraryInfos) {
Map<String, CcLinkingContext.LinkerInput> exportsMap = new HashMap<>();
- for (Pair<List<String>, CcLinkingContext.LinkerInput> entry : mergedCcSharedLibraryInfos) {
- List<String> exports = entry.first;
- CcLinkingContext.LinkerInput linkerInput = entry.second;
- for (String export : exports) {
+ for (CcSharedLibraryInfo entry : mergedCcSharedLibraryInfos) {
+ for (String export : entry.getExports()) {
if (exportsMap.containsKey(export)) {
ruleContext.ruleError(
"Two shared libraries in dependencies export the same symbols. Both "
@@ -1266,11 +1331,16 @@
.getDynamicLibrary()
.getExecPathString()
+ " and "
- + linkerInput.getLibraries().get(0).getDynamicLibrary().getExecPathString()
+ + entry
+ .getLinkerInput()
+ .getLibraries()
+ .get(0)
+ .getDynamicLibrary()
+ .getExecPathString()
+ " export "
+ export);
}
- exportsMap.put(export, linkerInput);
+ exportsMap.put(export, entry.getLinkerInput());
}
}
return ImmutableMap.copyOf(exportsMap);
@@ -1300,7 +1370,8 @@
private static ImmutableList<CcLinkingContext.LinkerInput> filterInputs(
RuleContext ruleContext,
CcLinkingContext ccLinkingContext,
- ImmutableMap<String, CcLinkingContext.LinkerInput> exportsMap) {
+ ImmutableMap<String, CcLinkingContext.LinkerInput> exportsMap,
+ ImmutableMap<String, String> linkOnceStaticLibsMap) {
ImmutableList.Builder<CcLinkingContext.LinkerInput> staticLinkerInputs =
ImmutableList.builder();
ImmutableList.Builder<GraphNodeInfo> graphStructureAspectNodes = ImmutableList.builder();
@@ -1335,7 +1406,15 @@
if (!linkStaticallyAndDynamicallyLabels.second.contains(owner)
&& (linkStaticallyAndDynamicallyLabels.first.contains(owner)
|| ruleContext.getLabel().toString().equals(owner))) {
- staticLinkerInputs.add(linkerInput);
+ if (linkOnceStaticLibsMap.containsKey(owner)) {
+ ruleContext.ruleError(
+ owner
+ + " is already linked statically in "
+ + linkOnceStaticLibsMap.get(owner)
+ + " but not exported.");
+ } else {
+ staticLinkerInputs.add(linkerInput);
+ }
}
}
@@ -1384,9 +1463,31 @@
return CcInfo.merge(ccInfos.build()).getCcLinkingContext().getLinkerInputs().toList();
}
+ public static ImmutableMap<String, String> buildLinkOnceStaticLibsMap(
+ RuleContext ruleContext, ImmutableList<CcSharedLibraryInfo> mergedCcSharedLibraryInfos) {
+ Map<String, String> linkOnceStaticLibsMap = new HashMap<>();
+ for (CcSharedLibraryInfo ccSharedLibraryInfo : mergedCcSharedLibraryInfos) {
+ String owner = ccSharedLibraryInfo.getLinkerInput().getOwner().toString();
+ for (String linkOnceStaticLib : ccSharedLibraryInfo.getLinkOnceStaticLibs()) {
+ if (linkOnceStaticLibsMap.containsKey(linkOnceStaticLib)) {
+ ruleContext.attributeError(
+ "dynamic_deps",
+ "Two shared libraries in dependencies link the same library statically. Both "
+ + linkOnceStaticLibsMap.get(linkOnceStaticLib)
+ + " and "
+ + owner
+ + " link statically "
+ + linkOnceStaticLib);
+ }
+ linkOnceStaticLibsMap.put(linkOnceStaticLib, owner);
+ }
+ }
+ return ImmutableMap.copyOf(linkOnceStaticLibsMap);
+ }
+
private static CcLinkingContext filterLibrariesThatAreLinkedDynamically(
RuleContext ruleContext, CcLinkingContext ccLinkingContext, CppSemantics cppSemantics) {
- ImmutableList<Pair<List<String>, CcLinkingContext.LinkerInput>> mergedCcSharedLibraryInfos =
+ ImmutableList<CcSharedLibraryInfo> mergedCcSharedLibraryInfos =
mergeCcSharedLibraryInfos(ruleContext, cppSemantics);
ImmutableList<CcLinkingContext.LinkerInput> preloadedDeps =
getPreloadedDepsFromDynamicDeps(ruleContext, cppSemantics);
@@ -1394,12 +1495,39 @@
return null;
}
+ ImmutableMap<String, String> linkOnceStaticLibsMap =
+ buildLinkOnceStaticLibsMap(ruleContext, mergedCcSharedLibraryInfos);
ImmutableMap<String, CcLinkingContext.LinkerInput> exportsMap =
buildExportsMapFromOnlyDynamicDeps(ruleContext, mergedCcSharedLibraryInfos);
ImmutableList<CcLinkingContext.LinkerInput> staticLinkerInputs =
- filterInputs(ruleContext, ccLinkingContext, exportsMap);
+ filterInputs(ruleContext, ccLinkingContext, exportsMap, linkOnceStaticLibsMap);
return createLinkingContextWithDynamicDependencies(
staticLinkerInputs, preloadedDeps, exportsMap.values());
}
+
+ /**
+ * In native rules we can return null as a configured target. However, this doesn't play nicely
+ * with the Starlark testing framework. Instead we have to return a built target even if it's
+ * empty. If we do that we hit some preconditions that make sure that certain providers are
+ * present, so we fill in those with an EMPTY instance.
+ *
+ * <p>The rest of the method makes sure the errors are present for the Starlark testing framework
+ * to see.
+ */
+ private static void fillInRequiredProviders(
+ RuleConfiguredTargetBuilder ruleBuilder, RuleContext ruleContext) {
+ ruleBuilder.addProvider(RunfilesProvider.class, RunfilesProvider.EMPTY);
+ ruleBuilder.addProvider(FileProvider.class, FileProvider.EMPTY);
+ ruleBuilder.addProvider(FilesToRunProvider.class, FilesToRunProvider.EMPTY);
+
+ if (ruleContext.getConfiguration().allowAnalysisFailures()) {
+ ImmutableList.Builder<AnalysisFailure> analysisFailures = ImmutableList.builder();
+ for (String errorMessage : ruleContext.getSuppressedErrorMessages()) {
+ analysisFailures.add(new AnalysisFailure(ruleContext.getLabel(), errorMessage));
+ }
+ ruleBuilder.addNativeDeclaredProvider(
+ AnalysisFailureInfo.forAnalysisFailures(analysisFailures.build()));
+ }
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcTest.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcTest.java
index 4478b63..f38b6a3 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcTest.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcTest.java
@@ -16,6 +16,7 @@
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
import com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory;
import com.google.devtools.build.lib.analysis.RuleContext;
@@ -33,6 +34,8 @@
@Override
public ConfiguredTarget create(RuleContext context)
throws InterruptedException, RuleErrorException, ActionConflictException {
- return CcBinary.init(semantics, context, /*fake =*/ false);
+ RuleConfiguredTargetBuilder ruleBuilder = new RuleConfiguredTargetBuilder(context);
+ CcBinary.init(semantics, ruleBuilder, context, /*fake =*/ false);
+ return ruleBuilder.build();
}
}
