Skip to content

Commit a90c408

Browse files
agluszakcopybara-github
agluszak
authored and
copybara-github
committed
Make linking the same shared library twice a rule error
> As more and more people start using the power of Starlark transitions, we are seeing more people bumping into a precondition baked deep in the C++ rules implementation which checks that a shared library being linked is in the same configuration directory. The check is there to make sure that the same library built in different configurations is not linked more than once. > Aside from the fact that it should be a proper rule error and not a crash, people are hitting this check when writing cc_import targets that have a transition applied to them. There are other ways to guarantee that the same library is not linked more than once without completely restricting the ability to link a library built in a different configuration. @oquenchil This PR makes linking the same shared library twice a rule error instead of a crash and it makes it possible to link a library built in a different configuration. A map from library identifiers to library paths is introduced, making sure that each shared library is linked at most once. Only dynamic linking is taken into account, for static linking see #11727 Fixes #11167 (the check which was causing the NPE was not directly related to transitions, but because it was removed, that NPE should no longer happen) Closes #11721. PiperOrigin-RevId: 322563028
1 parent c1a4f91 commit a90c408

5 files changed

Lines changed: 227 additions & 51 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,8 @@ private ImmutableSet<LinkerInput> computeLtoIndexingObjectFileInputs() {
251251
LinkerInputs.simpleLinkerInput(
252252
this.ltoCompilationContext.getMinimizedBitcodeOrSelf(objectFile),
253253
ArtifactCategory.OBJECT_FILE,
254-
/* disableWholeArchive= */ false));
254+
/* disableWholeArchive= */ false,
255+
objectFile.getRootRelativePathString()));
255256
}
256257
return objectFileInputsBuilder.build();
257258
}
@@ -843,7 +844,8 @@ public CppLinkAction build() throws InterruptedException, RuleErrorException {
843844
thinltoParamFile,
844845
allowLtoIndexing,
845846
nonExpandedLinkerInputs,
846-
needWholeArchive);
847+
needWholeArchive,
848+
ruleErrorConsumer);
847849
CollectedLibrariesToLink collectedLibrariesToLink =
848850
librariesToLinkCollector.collectLibrariesToLink();
849851

@@ -1286,16 +1288,17 @@ private void addObjectFile(LinkerInput input) {
12861288
public CppLinkActionBuilder addObjectFile(Artifact input) {
12871289
addObjectFile(
12881290
LinkerInputs.simpleLinkerInput(
1289-
input, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive= */ false));
1291+
input,
1292+
ArtifactCategory.OBJECT_FILE,
1293+
/* disableWholeArchive= */ false,
1294+
input.getRootRelativePathString()));
12901295
return this;
12911296
}
12921297

12931298
/** Adds object files to the linker action. */
12941299
public CppLinkActionBuilder addObjectFiles(Iterable<Artifact> inputs) {
12951300
for (Artifact input : inputs) {
1296-
addObjectFile(
1297-
LinkerInputs.simpleLinkerInput(
1298-
input, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive= */ false));
1301+
addObjectFile(input);
12991302
}
13001303
return this;
13011304
}

‎src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java‎

Lines changed: 36 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import com.google.devtools.build.lib.actions.Artifact;
2222
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
2323
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
24+
import com.google.devtools.build.lib.packages.RuleErrorConsumer;
2425
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
2526
import com.google.devtools.build.lib.rules.cpp.CcToolchainVariables.LibraryToLinkValue;
2627
import com.google.devtools.build.lib.rules.cpp.CcToolchainVariables.SequenceBuilder;
@@ -50,6 +51,7 @@ public class LibrariesToLinkCollector {
5051
private final String rpathRoot;
5152
private final boolean needToolchainLibrariesRpath;
5253
private final Map<Artifact, Artifact> ltoMap;
54+
private final RuleErrorConsumer ruleErrorConsumer;
5355

5456
public LibrariesToLinkCollector(
5557
boolean isNativeDeps,
@@ -66,7 +68,8 @@ public LibrariesToLinkCollector(
6668
Artifact thinltoParamFile,
6769
boolean allowLtoIndexing,
6870
Iterable<LinkerInput> linkerInputs,
69-
boolean needWholeArchive) {
71+
boolean needWholeArchive,
72+
RuleErrorConsumer ruleErrorConsumer) {
7073
this.isNativeDeps = isNativeDeps;
7174
this.cppConfiguration = cppConfiguration;
7275
this.ccToolchainProvider = toolchain;
@@ -80,6 +83,7 @@ public LibrariesToLinkCollector(
8083
this.allowLtoIndexing = allowLtoIndexing;
8184
this.linkerInputs = linkerInputs;
8285
this.needWholeArchive = needWholeArchive;
86+
this.ruleErrorConsumer = ruleErrorConsumer;
8387

8488
needToolchainLibrariesRpath =
8589
toolchainLibrariesSolibDir != null
@@ -236,20 +240,30 @@ private Pair<Boolean, Boolean> addLinkerInputs(
236240
NestedSetBuilder<LinkerInput> expandedLinkerInputsBuilder) {
237241
boolean includeSolibDir = false;
238242
boolean includeToolchainLibrariesSolibDir = false;
243+
Map<String, PathFragment> linkedLibrariesPaths = new HashMap<>();
244+
239245
for (LinkerInput input : linkerInputs) {
240246
if (input.getArtifactCategory() == ArtifactCategory.DYNAMIC_LIBRARY
241247
|| input.getArtifactCategory() == ArtifactCategory.INTERFACE_LIBRARY) {
242248
PathFragment libDir = input.getArtifact().getExecPath().getParentDirectory();
249+
Preconditions.checkNotNull(libDir);
250+
String libraryIdentifier = input.getLibraryIdentifier();
251+
PathFragment previousLibDir = linkedLibrariesPaths.get(libraryIdentifier);
252+
253+
if (previousLibDir == null) {
254+
linkedLibrariesPaths.put(libraryIdentifier, libDir);
255+
} else if (!previousLibDir.equals(libDir)) {
256+
ruleErrorConsumer.ruleError(
257+
String.format(
258+
"You are trying to link the same dynamic library %s built in a different"
259+
+ " configuration. Previously registered instance had path %s, current one"
260+
+ " has path %s",
261+
libraryIdentifier, previousLibDir, libDir));
262+
}
263+
243264
// When COPY_DYNAMIC_LIBRARIES_TO_BINARY is enabled, dynamic libraries are not symlinked
244265
// under solibDir, so don't check it and don't include solibDir.
245266
if (!featureConfiguration.isEnabled(CppRuleClasses.COPY_DYNAMIC_LIBRARIES_TO_BINARY)) {
246-
Preconditions.checkState(
247-
libDir.startsWith(solibDir) || libDir.startsWith(toolchainLibrariesSolibDir),
248-
"Artifact '%s' is not under directory expected '%s',"
249-
+ " neither it is in directory for toolchain libraries '%s'.",
250-
input.getArtifact(),
251-
solibDir,
252-
toolchainLibrariesSolibDir);
253267
if (libDir.equals(solibDir)) {
254268
includeSolibDir = true;
255269
}
@@ -392,7 +406,10 @@ private void addStaticInputLinkOptions(
392406
// still an input to this action.
393407
expandedLinkerInputsBuilder.add(
394408
LinkerInputs.simpleLinkerInput(
395-
a, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive= */ false));
409+
a,
410+
ArtifactCategory.OBJECT_FILE,
411+
/* disableWholeArchive= */ false,
412+
a.getRootRelativePathString()));
396413
continue;
397414
}
398415
// No LTO indexing step, so use the LTO backend's generated artifact directly
@@ -402,7 +419,10 @@ private void addStaticInputLinkOptions(
402419
nonLtoArchiveMembersBuilder.add(member);
403420
expandedLinkerInputsBuilder.add(
404421
LinkerInputs.simpleLinkerInput(
405-
member, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive = */ false));
422+
member,
423+
ArtifactCategory.OBJECT_FILE,
424+
/* disableWholeArchive = */ false,
425+
member.getRootRelativePathString()));
406426
}
407427
ImmutableList<Artifact> nonLtoArchiveMembers = nonLtoArchiveMembersBuilder.build();
408428
if (!nonLtoArchiveMembers.isEmpty()) {
@@ -443,7 +463,10 @@ private void addStaticInputLinkOptions(
443463
// still an input to this action.
444464
expandedLinkerInputsBuilder.add(
445465
LinkerInputs.simpleLinkerInput(
446-
a, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive= */ false));
466+
a,
467+
ArtifactCategory.OBJECT_FILE,
468+
/* disableWholeArchive= */ false,
469+
a.getRootRelativePathString()));
447470
return;
448471
}
449472
// No LTO indexing step, so use the LTO backend's generated artifact directly
@@ -487,8 +510,8 @@ private static boolean handledByLtoIndexing(Artifact a, boolean allowLtoIndexing
487510
// LTO indexing because we are linking a test, to improve scalability when linking many tests.
488511
return allowLtoIndexing
489512
&& !a.getRootRelativePath()
490-
.startsWith(
491-
PathFragment.create(CppLinkActionBuilder.SHARED_NONLTO_BACKEND_ROOT_PREFIX));
513+
.startsWith(
514+
PathFragment.create(CppLinkActionBuilder.SHARED_NONLTO_BACKEND_ROOT_PREFIX));
492515
}
493516

494517
private Map<Artifact, Artifact> generateLtoMap() {

‎src/main/java/com/google/devtools/build/lib/rules/cpp/LinkerInput.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,4 +59,11 @@ default boolean isLinkstamp() {
5959

6060
/** If true, Bazel will not wrap this input in whole-archive block. */
6161
boolean disableWholeArchive();
62+
63+
/**
64+
* Return the identifier for the library. This is used for de-duplication of linker inputs: two
65+
* libraries should have the same identifier iff they are in fact the same library but linked in a
66+
* different way (e.g. static/dynamic, PIC/no-PIC)
67+
*/
68+
String getLibraryIdentifier();
6269
}

‎src/main/java/com/google/devtools/build/lib/rules/cpp/LinkerInputs.java‎

Lines changed: 40 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,7 @@
2424
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
2525
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
2626

27-
/**
28-
* Factory for creating new {@link LinkerInput} objects.
29-
*/
27+
/** Factory for creating new {@link LinkerInput} objects. */
3028
public abstract class LinkerInputs {
3129
/**
3230
* An opaque linker input that is not a library, for example a linker script or an individual
@@ -38,10 +36,15 @@ public static class SimpleLinkerInput implements LinkerInput {
3836
private final Artifact artifact;
3937
private final ArtifactCategory category;
4038
private final boolean disableWholeArchive;
39+
private final String libraryIdentifier;
4140

4241
@AutoCodec.Instantiator
4342
public SimpleLinkerInput(
44-
Artifact artifact, ArtifactCategory category, boolean disableWholeArchive) {
43+
Artifact artifact,
44+
ArtifactCategory category,
45+
boolean disableWholeArchive,
46+
String libraryIdentifier) {
47+
Preconditions.checkNotNull(libraryIdentifier);
4548
String basename = artifact.getFilename();
4649
switch (category) {
4750
case STATIC_LIBRARY:
@@ -65,6 +68,7 @@ public SimpleLinkerInput(
6568
this.artifact = Preconditions.checkNotNull(artifact);
6669
this.category = category;
6770
this.disableWholeArchive = disableWholeArchive;
71+
this.libraryIdentifier = libraryIdentifier;
6872
}
6973

7074
@Override
@@ -125,12 +129,21 @@ public boolean isMustKeepDebug() {
125129
public boolean disableWholeArchive() {
126130
return disableWholeArchive;
127131
}
132+
133+
@Override
134+
public String getLibraryIdentifier() {
135+
return libraryIdentifier;
136+
}
128137
}
129138

130139
@ThreadSafety.Immutable
131140
private static class LinkstampLinkerInput extends SimpleLinkerInput {
132-
private LinkstampLinkerInput(Artifact artifact) {
133-
super(artifact, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive= */ false);
141+
private LinkstampLinkerInput(Artifact artifact, String libraryIdentifier) {
142+
super(
143+
artifact,
144+
ArtifactCategory.OBJECT_FILE,
145+
/* disableWholeArchive= */ false,
146+
libraryIdentifier);
134147
Preconditions.checkState(Link.OBJECT_FILETYPES.matches(artifact.getFilename()));
135148
}
136149

@@ -155,13 +168,6 @@ public interface LibraryToLink extends LinkerInput {
155168
* number of LTO backends that can be generated for a single blaze test invocation.
156169
*/
157170
ImmutableMap<Artifact, LtoBackendArtifacts> getSharedNonLtoBackends();
158-
159-
/**
160-
* Return the identifier for the library. This is used for de-duplication of linker inputs: two
161-
* libraries should have the same identifier iff they are in fact the same library but linked
162-
* in a different way (e.g. static/dynamic, PIC/no-PIC)
163-
*/
164-
String getLibraryIdentifier();
165171
}
166172

167173
/**
@@ -188,8 +194,7 @@ public static class SolibLibraryToLink implements LibraryToLink {
188194

189195
@Override
190196
public String toString() {
191-
return String.format("SolibLibraryToLink(%s -> %s",
192-
solibSymlinkArtifact.toString(), libraryArtifact.toString());
197+
return String.format("SolibLibraryToLink(%s -> %s", solibSymlinkArtifact, libraryArtifact);
193198
}
194199

195200
@Override
@@ -245,9 +250,8 @@ public boolean equals(Object that) {
245250
}
246251

247252
SolibLibraryToLink thatSolib = (SolibLibraryToLink) that;
248-
return
249-
solibSymlinkArtifact.equals(thatSolib.solibSymlinkArtifact) &&
250-
libraryArtifact.equals(thatSolib.libraryArtifact);
253+
return solibSymlinkArtifact.equals(thatSolib.solibSymlinkArtifact)
254+
&& libraryArtifact.equals(thatSolib.libraryArtifact);
251255
}
252256

253257
@Override
@@ -426,20 +430,28 @@ public boolean disableWholeArchive() {
426430
public static Iterable<LinkerInput> simpleLinkerInputs(
427431
Iterable<Artifact> input, final ArtifactCategory category, boolean disableWholeArchive) {
428432
return Iterables.transform(
429-
input, artifact -> simpleLinkerInput(artifact, category, disableWholeArchive));
433+
input,
434+
artifact ->
435+
simpleLinkerInput(
436+
artifact, category, disableWholeArchive, artifact.getRootRelativePathString()));
430437
}
431438

432439
public static Iterable<LinkerInput> linkstampLinkerInputs(Iterable<Artifact> input) {
433-
return Iterables.transform(input, artifact -> new LinkstampLinkerInput(artifact));
440+
return Iterables.transform(
441+
input,
442+
artifact -> new LinkstampLinkerInput(artifact, artifact.getRootRelativePathString()));
434443
}
435444

436445
/** Creates a linker input for which we do not know what objects files it consists of. */
437446
public static LinkerInput simpleLinkerInput(
438-
Artifact artifact, ArtifactCategory category, boolean disableWholeArchive) {
447+
Artifact artifact,
448+
ArtifactCategory category,
449+
boolean disableWholeArchive,
450+
String libraryIdentifier) {
439451
// This precondition check was in place and *most* of the tests passed with them; the only
440452
// exception is when you mention a generated .a file in the srcs of a cc_* rule.
441453
// Preconditions.checkArgument(!ARCHIVE_LIBRARY_FILETYPES.contains(artifact.getFileType()));
442-
return new SimpleLinkerInput(artifact, category, disableWholeArchive);
454+
return new SimpleLinkerInput(artifact, category, disableWholeArchive, libraryIdentifier);
443455
}
444456

445457
/**
@@ -450,17 +462,13 @@ public static Iterable<LibraryToLink> opaqueLibrariesToLink(
450462
return Iterables.transform(input, artifact -> precompiledLibraryToLink(artifact, category));
451463
}
452464

453-
/**
454-
* Creates a solib library symlink from the given artifact.
455-
*/
465+
/** Creates a solib library symlink from the given artifact. */
456466
public static LibraryToLink solibLibraryToLink(
457467
Artifact solibSymlink, Artifact original, String libraryIdentifier) {
458468
return new SolibLibraryToLink(solibSymlink, original, libraryIdentifier);
459469
}
460470

461-
/**
462-
* Creates an input library for which we do not know what objects files it consists of.
463-
*/
471+
/** Creates an input library for which we do not know what objects files it consists of. */
464472
public static LibraryToLink precompiledLibraryToLink(
465473
Artifact artifact, ArtifactCategory category) {
466474
// This precondition check was in place and *most* of the tests passed with them; the only
@@ -497,7 +505,9 @@ public static LibraryToLink opaqueLibraryToLink(
497505
}
498506

499507
public static LibraryToLink opaqueLibraryToLink(
500-
Artifact artifact, ArtifactCategory category, String libraryIdentifier,
508+
Artifact artifact,
509+
ArtifactCategory category,
510+
String libraryIdentifier,
501511
CppConfiguration.StripMode stripMode) {
502512
return new CompoundLibraryToLink(
503513
artifact,
@@ -558,9 +568,7 @@ public static Iterable<Artifact> toNonSolibArtifacts(Iterable<LibraryToLink> lib
558568
return Iterables.transform(libraries, LibraryToLink::getOriginalLibraryArtifact);
559569
}
560570

561-
/**
562-
* Returns the linker input artifacts from a collection of {@link LinkerInput} objects.
563-
*/
571+
/** Returns the linker input artifacts from a collection of {@link LinkerInput} objects. */
564572
public static Iterable<Artifact> toLibraryArtifacts(Iterable<? extends LinkerInput> artifacts) {
565573
return Iterables.transform(artifacts, LinkerInput::getArtifact);
566574
}

0 commit comments

Comments
 (0)