C++: Fix crash with repeated precompiled library in srcs
With a precompiled library repeated in the srcs of a cc_library we got a crash
when trying to add them both to an immutable map because they had the same
identifier. Since it's probably undesired behavior to link the same library
twice built in different configurations which is what happened as originally
reported in b/156775573, the behavior will still be an error, a proper error
though instead of a crash.
RELNOTES:none
PiperOrigin-RevId: 314680827
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
index 91de1be..e4ed503 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
@@ -45,6 +45,7 @@
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.ImplicitOutputsFunction;
import com.google.devtools.build.lib.packages.RawAttributeMapper;
+import com.google.devtools.build.lib.packages.RuleErrorConsumer;
import com.google.devtools.build.lib.packages.TargetUtils;
import com.google.devtools.build.lib.packages.Type;
import com.google.devtools.build.lib.rules.cpp.CcCommon.CcFlagsSupplier;
@@ -56,6 +57,7 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.ArrayList;
import java.util.HashSet;
+import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -352,7 +354,10 @@
}
List<LibraryToLink> precompiledLibraries =
convertPrecompiledLibrariesToLibraryToLink(
- common, ruleContext.getFragment(CppConfiguration.class).forcePic(), precompiledFiles);
+ ruleContext,
+ common,
+ ruleContext.getFragment(CppConfiguration.class).forcePic(),
+ precompiledFiles);
if (!ccCompilationOutputs.isEmpty()) {
checkIfLinkOutputsCollidingWithPrecompiledFiles(
@@ -674,12 +679,21 @@
return ccCompilationOutputsIsEmpty && !ccLinkingOutputsIsEmpty;
}
- private static Map<String, Artifact> buildMapIdentifierToArtifact(Iterable<Artifact> artifacts) {
- ImmutableMap.Builder<String, Artifact> libraries = ImmutableMap.builder();
+ private static ImmutableMap<String, Artifact> buildMapIdentifierToArtifact(
+ RuleErrorConsumer ruleErrorConsumer, Iterable<Artifact> artifacts) {
+ Map<String, Artifact> libraries = new LinkedHashMap<>();
for (Artifact artifact : artifacts) {
- libraries.put(CcLinkingOutputs.libraryIdentifierOf(artifact), artifact);
+ String identifier = CcLinkingOutputs.libraryIdentifierOf(artifact);
+ if (libraries.containsKey(identifier)) {
+ ruleErrorConsumer.attributeError(
+ "srcs",
+ String.format(
+ "Trying to link twice a library with the same identifier '%s', files: %s and %s",
+ identifier, artifact.toDetailString(), libraries.get(identifier).toDetailString()));
+ }
+ libraries.put(identifier, artifact);
}
- return libraries.build();
+ return ImmutableMap.copyOf(libraries);
}
/*
@@ -697,19 +711,24 @@
* Note that some target platforms do not require shared library code to be PIC.
*/
private static List<LibraryToLink> convertPrecompiledLibrariesToLibraryToLink(
- CcCommon common, boolean forcePic, PrecompiledFiles precompiledFiles) {
+ RuleErrorConsumer ruleErrorConsumer,
+ CcCommon common,
+ boolean forcePic,
+ PrecompiledFiles precompiledFiles) {
ImmutableList.Builder<LibraryToLink> librariesToLink = ImmutableList.builder();
Map<String, Artifact> staticLibraries =
- buildMapIdentifierToArtifact(precompiledFiles.getStaticLibraries());
+ buildMapIdentifierToArtifact(ruleErrorConsumer, precompiledFiles.getStaticLibraries());
Map<String, Artifact> picStaticLibraries =
- buildMapIdentifierToArtifact(precompiledFiles.getPicStaticLibraries());
+ buildMapIdentifierToArtifact(ruleErrorConsumer, precompiledFiles.getPicStaticLibraries());
Map<String, Artifact> alwayslinkStaticLibraries =
- buildMapIdentifierToArtifact(precompiledFiles.getAlwayslinkStaticLibraries());
+ buildMapIdentifierToArtifact(
+ ruleErrorConsumer, precompiledFiles.getAlwayslinkStaticLibraries());
Map<String, Artifact> alwayslinkPicStaticLibraries =
- buildMapIdentifierToArtifact(precompiledFiles.getPicAlwayslinkLibraries());
+ buildMapIdentifierToArtifact(
+ ruleErrorConsumer, precompiledFiles.getPicAlwayslinkLibraries());
Map<String, Artifact> dynamicLibraries =
- buildMapIdentifierToArtifact(precompiledFiles.getSharedLibraries());
+ buildMapIdentifierToArtifact(ruleErrorConsumer, precompiledFiles.getSharedLibraries());
Set<String> identifiersUsed = new HashSet<>();
for (Map.Entry<String, Artifact> staticLibraryEntry :
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/PrecompiledFiles.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/PrecompiledFiles.java
index e8f6d48..a96da74 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/PrecompiledFiles.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/PrecompiledFiles.java
@@ -30,8 +30,8 @@
private final ImmutableList<Artifact> files;
/**
- * Initializes this object with the artifacts obtained from the "srcs" attribute of the given
- * rule (this is the most common usage for this class).
+ * Initializes this object with the artifacts obtained from the "srcs" attribute of the given rule
+ * (this is the most common usage for this class).
*/
public PrecompiledFiles(RuleContext ruleContext) {
if (ruleContext.attributes().has("srcs", BuildType.LABEL_LIST)) {
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
index f9a1a30..d570c2c 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
@@ -1571,4 +1571,66 @@
.containsExactly("//foo:foo", "//foo:bar", "//foo:baz")
.inOrder();
}
+
+ @Test
+ public void testPrecompiledFilesFromDifferentConfigs() throws Exception {
+ scratch.file(
+ "foo/BUILD",
+ "load(':example_transition.bzl', 'transitioned_file')",
+ "genrule(",
+ " name = 'generated',",
+ " outs = ['libbar.so'],",
+ " cmd = 'echo foo > @',",
+ ")",
+ "transitioned_file(",
+ " name = 'transitioned_libbar',",
+ " src = 'generated',",
+ ")",
+ "cc_library(",
+ " name = 'foo',",
+ " srcs = [",
+ " 'generated',",
+ " 'transitioned_libbar',",
+ " ],",
+ ")");
+ scratch.file(
+ "foo/example_transition.bzl",
+ "def _impl(settings, attr):",
+ " _ignore = (settings, attr)",
+ " return [",
+ " {'//command_line_option:cpu': 'k8'},",
+ " ]",
+ "cpu_transition = transition(",
+ " implementation = _impl,",
+ " inputs = [],",
+ " outputs = ['//command_line_option:cpu'],",
+ ")",
+ "def _transitioned_file_impl(ctx):",
+ " return DefaultInfo(files = depset([ctx.file.src]))",
+ "",
+ "transitioned_file = rule(",
+ " implementation = _transitioned_file_impl,",
+ " attrs = {",
+ " 'src': attr.label(",
+ " allow_single_file = True,",
+ " cfg = cpu_transition,",
+ " ),",
+ " '_whitelist_function_transition': attr.label(",
+ " default = '//tools/whitelists/function_transition_whitelist',",
+ " ),",
+ " },",
+ ")");
+ scratch.overwriteFile(
+ "tools/whitelists/function_transition_whitelist/BUILD",
+ "package_group(",
+ " name = 'function_transition_whitelist',",
+ " packages = ['//...'],",
+ ")",
+ "filegroup(",
+ " name = 'srcs',",
+ " srcs = glob(['**']),",
+ " visibility = ['//tools/whitelists:__pkg__'],",
+ ")");
+ checkError("//foo", "Trying to link twice");
+ }
}
