Skip to content

Commit

Permalink
Make linking the same shared library twice a rule error
Browse files Browse the repository at this point in the history 
> 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
  • Loading branch information
@agluszak @copybara-github
agluszak authored and copybara-github committed Jul 22, 2020
1 parent c1a4f91 commit a90c408
Show file tree
Hide file tree
Showing 5 changed files with 227 additions and 51 deletions.
15 changes: 9 additions & 6 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,8 @@ private ImmutableSet<LinkerInput> computeLtoIndexingObjectFileInputs() {
LinkerInputs.simpleLinkerInput(
this.ltoCompilationContext.getMinimizedBitcodeOrSelf(objectFile),
ArtifactCategory.OBJECT_FILE,
/* disableWholeArchive= */ false));
/* disableWholeArchive= */ false,
objectFile.getRootRelativePathString()));
}
return objectFileInputsBuilder.build();
}
Expand Down Expand Up @@ -843,7 +844,8 @@ public CppLinkAction build() throws InterruptedException, RuleErrorException {
thinltoParamFile,
allowLtoIndexing,
nonExpandedLinkerInputs,
needWholeArchive);
needWholeArchive,
ruleErrorConsumer);
CollectedLibrariesToLink collectedLibrariesToLink =
librariesToLinkCollector.collectLibrariesToLink();

Expand Down Expand Up @@ -1286,16 +1288,17 @@ private void addObjectFile(LinkerInput input) {
public CppLinkActionBuilder addObjectFile(Artifact input) {
addObjectFile(
LinkerInputs.simpleLinkerInput(
input, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive= */ false));
input,
ArtifactCategory.OBJECT_FILE,
/* disableWholeArchive= */ false,
input.getRootRelativePathString()));
return this;
}

/** Adds object files to the linker action. */
public CppLinkActionBuilder addObjectFiles(Iterable<Artifact> inputs) {
for (Artifact input : inputs) {
addObjectFile(
LinkerInputs.simpleLinkerInput(
input, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive= */ false));
addObjectFile(input);
}
return this;
}
Expand Down
49 changes: 36 additions & 13 deletions 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 @@ -21,6 +21,7 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.packages.RuleErrorConsumer;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
import com.google.devtools.build.lib.rules.cpp.CcToolchainVariables.LibraryToLinkValue;
import com.google.devtools.build.lib.rules.cpp.CcToolchainVariables.SequenceBuilder;
Expand Down Expand Up @@ -50,6 +51,7 @@ public class LibrariesToLinkCollector {
private final String rpathRoot;
private final boolean needToolchainLibrariesRpath;
private final Map<Artifact, Artifact> ltoMap;
private final RuleErrorConsumer ruleErrorConsumer;

public LibrariesToLinkCollector(
boolean isNativeDeps,
Expand All @@ -66,7 +68,8 @@ public LibrariesToLinkCollector(
Artifact thinltoParamFile,
boolean allowLtoIndexing,
Iterable<LinkerInput> linkerInputs,
boolean needWholeArchive) {
boolean needWholeArchive,
RuleErrorConsumer ruleErrorConsumer) {
this.isNativeDeps = isNativeDeps;
this.cppConfiguration = cppConfiguration;
this.ccToolchainProvider = toolchain;
Expand All @@ -80,6 +83,7 @@ public LibrariesToLinkCollector(
this.allowLtoIndexing = allowLtoIndexing;
this.linkerInputs = linkerInputs;
this.needWholeArchive = needWholeArchive;
this.ruleErrorConsumer = ruleErrorConsumer;

needToolchainLibrariesRpath =
toolchainLibrariesSolibDir != null
Expand Down Expand Up @@ -236,20 +240,30 @@ private Pair<Boolean, Boolean> addLinkerInputs(
NestedSetBuilder<LinkerInput> expandedLinkerInputsBuilder) {
boolean includeSolibDir = false;
boolean includeToolchainLibrariesSolibDir = false;
Map<String, PathFragment> linkedLibrariesPaths = new HashMap<>();

for (LinkerInput input : linkerInputs) {
if (input.getArtifactCategory() == ArtifactCategory.DYNAMIC_LIBRARY
|| input.getArtifactCategory() == ArtifactCategory.INTERFACE_LIBRARY) {
PathFragment libDir = input.getArtifact().getExecPath().getParentDirectory();
Preconditions.checkNotNull(libDir);
String libraryIdentifier = input.getLibraryIdentifier();
PathFragment previousLibDir = linkedLibrariesPaths.get(libraryIdentifier);

if (previousLibDir == null) {
linkedLibrariesPaths.put(libraryIdentifier, libDir);
} else if (!previousLibDir.equals(libDir)) {
ruleErrorConsumer.ruleError(
String.format(
"You are trying to link the same dynamic library %s built in a different"
+ " configuration. Previously registered instance had path %s, current one"
+ " has path %s",
libraryIdentifier, previousLibDir, libDir));
}

// When COPY_DYNAMIC_LIBRARIES_TO_BINARY is enabled, dynamic libraries are not symlinked
// under solibDir, so don't check it and don't include solibDir.
if (!featureConfiguration.isEnabled(CppRuleClasses.COPY_DYNAMIC_LIBRARIES_TO_BINARY)) {
Preconditions.checkState(
libDir.startsWith(solibDir) || libDir.startsWith(toolchainLibrariesSolibDir),
"Artifact '%s' is not under directory expected '%s',"
+ " neither it is in directory for toolchain libraries '%s'.",
input.getArtifact(),
solibDir,
toolchainLibrariesSolibDir);
if (libDir.equals(solibDir)) {
includeSolibDir = true;
}
Expand Down Expand Up @@ -392,7 +406,10 @@ private void addStaticInputLinkOptions(
// still an input to this action.
expandedLinkerInputsBuilder.add(
LinkerInputs.simpleLinkerInput(
a, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive= */ false));
a,
ArtifactCategory.OBJECT_FILE,
/* disableWholeArchive= */ false,
a.getRootRelativePathString()));
continue;
}
// No LTO indexing step, so use the LTO backend's generated artifact directly
Expand All @@ -402,7 +419,10 @@ private void addStaticInputLinkOptions(
nonLtoArchiveMembersBuilder.add(member);
expandedLinkerInputsBuilder.add(
LinkerInputs.simpleLinkerInput(
member, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive = */ false));
member,
ArtifactCategory.OBJECT_FILE,
/* disableWholeArchive = */ false,
member.getRootRelativePathString()));
}
ImmutableList<Artifact> nonLtoArchiveMembers = nonLtoArchiveMembersBuilder.build();
if (!nonLtoArchiveMembers.isEmpty()) {
Expand Down Expand Up @@ -443,7 +463,10 @@ private void addStaticInputLinkOptions(
// still an input to this action.
expandedLinkerInputsBuilder.add(
LinkerInputs.simpleLinkerInput(
a, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive= */ false));
a,
ArtifactCategory.OBJECT_FILE,
/* disableWholeArchive= */ false,
a.getRootRelativePathString()));
return;
}
// No LTO indexing step, so use the LTO backend's generated artifact directly
Expand Down Expand Up @@ -487,8 +510,8 @@ private static boolean handledByLtoIndexing(Artifact a, boolean allowLtoIndexing
// LTO indexing because we are linking a test, to improve scalability when linking many tests.
return allowLtoIndexing
&& !a.getRootRelativePath()
.startsWith(
PathFragment.create(CppLinkActionBuilder.SHARED_NONLTO_BACKEND_ROOT_PREFIX));
.startsWith(
PathFragment.create(CppLinkActionBuilder.SHARED_NONLTO_BACKEND_ROOT_PREFIX));
}

private Map<Artifact, Artifact> generateLtoMap() {
Expand Down
7 changes: 7 additions & 0 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/LinkerInput.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,11 @@ default boolean isLinkstamp() {

/** If true, Bazel will not wrap this input in whole-archive block. */
boolean disableWholeArchive();

/**
* Return the identifier for the library. This is used for de-duplication of linker inputs: two
* libraries should have the same identifier iff they are in fact the same library but linked in a
* different way (e.g. static/dynamic, PIC/no-PIC)
*/
String getLibraryIdentifier();
}
72 changes: 40 additions & 32 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/LinkerInputs.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;

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

@AutoCodec.Instantiator
public SimpleLinkerInput(
Artifact artifact, ArtifactCategory category, boolean disableWholeArchive) {
Artifact artifact,
ArtifactCategory category,
boolean disableWholeArchive,
String libraryIdentifier) {
Preconditions.checkNotNull(libraryIdentifier);
String basename = artifact.getFilename();
switch (category) {
case STATIC_LIBRARY:
Expand All @@ -65,6 +68,7 @@ public SimpleLinkerInput(
this.artifact = Preconditions.checkNotNull(artifact);
this.category = category;
this.disableWholeArchive = disableWholeArchive;
this.libraryIdentifier = libraryIdentifier;
}

@Override
Expand Down Expand Up @@ -125,12 +129,21 @@ public boolean isMustKeepDebug() {
public boolean disableWholeArchive() {
return disableWholeArchive;
}

@Override
public String getLibraryIdentifier() {
return libraryIdentifier;
}
}

@ThreadSafety.Immutable
private static class LinkstampLinkerInput extends SimpleLinkerInput {
private LinkstampLinkerInput(Artifact artifact) {
super(artifact, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive= */ false);
private LinkstampLinkerInput(Artifact artifact, String libraryIdentifier) {
super(
artifact,
ArtifactCategory.OBJECT_FILE,
/* disableWholeArchive= */ false,
libraryIdentifier);
Preconditions.checkState(Link.OBJECT_FILETYPES.matches(artifact.getFilename()));
}

Expand All @@ -155,13 +168,6 @@ public interface LibraryToLink extends LinkerInput {
* number of LTO backends that can be generated for a single blaze test invocation.
*/
ImmutableMap<Artifact, LtoBackendArtifacts> getSharedNonLtoBackends();

/**
* Return the identifier for the library. This is used for de-duplication of linker inputs: two
* libraries should have the same identifier iff they are in fact the same library but linked
* in a different way (e.g. static/dynamic, PIC/no-PIC)
*/
String getLibraryIdentifier();
}

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

@Override
public String toString() {
return String.format("SolibLibraryToLink(%s -> %s",
solibSymlinkArtifact.toString(), libraryArtifact.toString());
return String.format("SolibLibraryToLink(%s -> %s", solibSymlinkArtifact, libraryArtifact);
}

@Override
Expand Down Expand Up @@ -245,9 +250,8 @@ public boolean equals(Object that) {
}

SolibLibraryToLink thatSolib = (SolibLibraryToLink) that;
return
solibSymlinkArtifact.equals(thatSolib.solibSymlinkArtifact) &&
libraryArtifact.equals(thatSolib.libraryArtifact);
return solibSymlinkArtifact.equals(thatSolib.solibSymlinkArtifact)
&& libraryArtifact.equals(thatSolib.libraryArtifact);
}

@Override
Expand Down Expand Up @@ -426,20 +430,28 @@ public boolean disableWholeArchive() {
public static Iterable<LinkerInput> simpleLinkerInputs(
Iterable<Artifact> input, final ArtifactCategory category, boolean disableWholeArchive) {
return Iterables.transform(
input, artifact -> simpleLinkerInput(artifact, category, disableWholeArchive));
input,
artifact ->
simpleLinkerInput(
artifact, category, disableWholeArchive, artifact.getRootRelativePathString()));
}

public static Iterable<LinkerInput> linkstampLinkerInputs(Iterable<Artifact> input) {
return Iterables.transform(input, artifact -> new LinkstampLinkerInput(artifact));
return Iterables.transform(
input,
artifact -> new LinkstampLinkerInput(artifact, artifact.getRootRelativePathString()));
}

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

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

/**
* Creates a solib library symlink from the given artifact.
*/
/** Creates a solib library symlink from the given artifact. */
public static LibraryToLink solibLibraryToLink(
Artifact solibSymlink, Artifact original, String libraryIdentifier) {
return new SolibLibraryToLink(solibSymlink, original, libraryIdentifier);
}

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

public static LibraryToLink opaqueLibraryToLink(
Artifact artifact, ArtifactCategory category, String libraryIdentifier,
Artifact artifact,
ArtifactCategory category,
String libraryIdentifier,
CppConfiguration.StripMode stripMode) {
return new CompoundLibraryToLink(
artifact,
Expand Down Expand Up @@ -558,9 +568,7 @@ public static Iterable<Artifact> toNonSolibArtifacts(Iterable<LibraryToLink> lib
return Iterables.transform(libraries, LibraryToLink::getOriginalLibraryArtifact);
}

/**
* Returns the linker input artifacts from a collection of {@link LinkerInput} objects.
*/
/** Returns the linker input artifacts from a collection of {@link LinkerInput} objects. */
public static Iterable<Artifact> toLibraryArtifacts(Iterable<? extends LinkerInput> artifacts) {
return Iterables.transform(artifacts, LinkerInput::getArtifact);
}
Expand Down
Loading

1 comment on commit a90c408

@konste
Copy link

@konste konste commented on a90c408 Aug 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agluszak I see you removed the check and the error "Artifact '%s' is not under directory expected...". Interestingly we recently hit exactly this error from the different angle: this check (before you removed it) was not applied when COPY_DYNAMIC_LIBRARIES_TO_BINARY feature is in effect. When we turned COPY_DYNAMIC_LIBRARIES_TO_BINARY to OFF we started getting this error on Windows. It seems that before you removed the check COPY_DYNAMIC_LIBRARIES_TO_BINARY must be ON for Windows otherwise this error always triggers. I wonder if your commit inadvertently fixes our scenario as well - without the check we now should be able to turn COPY_DYNAMIC_LIBRARIES_TO_BINARY to OFF on Windows. What do you think?

Please sign in to comment.