From 8ca66458a42266f11aba77732b05ab06b96e95fb Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 26 Apr 2018 08:29:54 -0700 Subject: Consolidating start/end lib archive expansion Different places in the link logic were in charge of expanding start/end lib archives, causing discrepancies. This CL gets rid of the special iteration in Link.java and makes LibrariesToLinkCollecter solely responsible for mapping static libraries to their object files on the link command line. RELNOTES: Fixing start/end lib expansion for linking. There were many cases where archive files were still being used with toolchains that support start/end lib. This change consolidates the places that make that decision so they can be more consistent. PiperOrigin-RevId: 194400112 --- .../build/lib/rules/cpp/CppLinkActionBuilder.java | 43 ++++--- .../lib/rules/cpp/LibrariesToLinkCollector.java | 107 +++++++++++++---- .../google/devtools/build/lib/rules/cpp/Link.java | 126 --------------------- 3 files changed, 105 insertions(+), 171 deletions(-) (limited to 'src/main/java/com/google/devtools/build') diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java index 19bd3a7ddd..4739b7be53 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java @@ -838,7 +838,7 @@ public class CppLinkActionBuilder { } Map ltoMapping = new HashMap<>(); - ; + if (isFinalLinkOfLtoBuild()) { for (LtoBackendArtifacts a : allLtoArtifacts) { ltoMapping.put(a.getBitcodeFile(), a.getObjectFile()); @@ -846,15 +846,8 @@ public class CppLinkActionBuilder { } Iterable objectArtifacts = getArtifactsPossiblyLtoMapped(objectFileInputs, ltoMapping); - Iterable linkstampObjectArtifacts = + ImmutableSet linkstampObjectArtifacts = getArtifactsPossiblyLtoMapped(linkstampObjectFileInputs, ltoMapping); - Iterable expandedInputs = - getArtifactsPossiblyLtoMapped( - Link.mergeInputsDependencies( - uniqueLibraries, - needWholeArchive, - CppHelper.getArchiveType(cppConfiguration, toolchain)), - ltoMapping); ImmutableSet combinedObjectArtifacts = ImmutableSet.builder() @@ -928,16 +921,12 @@ public class CppLinkActionBuilder { symbolCounts); } - final Iterable linkerInputs = + // Linker inputs without any start/end lib expansions. + final Iterable nonExpandedLinkerInputs = IterablesChain.builder() .add(objectFileInputs) .add(linkstampObjectFileInputs) - .add( - ImmutableIterable.from( - Link.mergeInputsCmdLine( - uniqueLibraries, - needWholeArchive, - CppHelper.getArchiveType(cppConfiguration, toolchain)))) + .add(ImmutableIterable.from(uniqueLibraries)) .add( // Adding toolchain libraries without whole archive no-matter-what. People don't // want to include whole libstdc++ in their binary ever. @@ -982,10 +971,21 @@ public class CppLinkActionBuilder { featureConfiguration, thinltoParamFile, allowLtoIndexing, - linkerInputs, + nonExpandedLinkerInputs, needWholeArchive); CollectedLibrariesToLink collectedLibrariesToLink = librariesToLinkCollector.collectLibrariesToLink(); + + ImmutableSet expandedLinkerArtifacts = + getArtifactsPossiblyLtoMapped( + collectedLibrariesToLink.getExpandedLinkerInputs(), ltoMapping); + + // Remove the linkstamp objects from inputs so that createLinkstampCompileAction doesn't cause a + // circular dependency. + Set expandedLinkerArtifactsNoLinkstamps = + new LinkedHashSet(expandedLinkerArtifacts); + expandedLinkerArtifactsNoLinkstamps.removeAll(linkstampObjectArtifacts); + Variables variables = LinkBuildVariables.setupVariables( this, @@ -1041,7 +1041,7 @@ public class CppLinkActionBuilder { LinkCommandLine.Builder linkCommandLineBuilder = new LinkCommandLine.Builder(ruleContext) - .setLinkerInputs(linkerInputs) + .setLinkerInputs(collectedLibrariesToLink.getExpandedLinkerInputs()) .setLinkTargetType(linkType) .setLinkStaticness(linkStaticness) .setToolchainLibrariesSolibDir( @@ -1107,7 +1107,7 @@ public class CppLinkActionBuilder { .add(ImmutableList.copyOf(objectArtifacts)) .add(ImmutableList.copyOf(nonCodeInputs)) .add(dependencyInputsBuilder.build()) - .add(ImmutableIterable.from(expandedInputs)); + .add(ImmutableSet.copyOf(expandedLinkerArtifactsNoLinkstamps)); if (thinltoParamFile != null && !isLtoIndexing) { inputsBuilder.add(ImmutableList.of(thinltoParamFile)); @@ -1116,9 +1116,8 @@ public class CppLinkActionBuilder { inputsBuilder.add(ImmutableList.of(linkCommandLine.getParamFile())); // Pass along tree artifacts, so they can be properly expanded. ImmutableSet paramFileActionInputs = - ImmutableSet.copyOf(linkerInputs) + expandedLinkerArtifacts .stream() - .map(LinkerInput::getArtifact) .filter(a -> a.isTreeArtifact()) .collect(ImmutableSet.toImmutableSet()); @@ -1207,7 +1206,7 @@ public class CppLinkActionBuilder { return !isLtoIndexing && allLtoArtifacts != null; } - private Iterable getArtifactsPossiblyLtoMapped( + private ImmutableSet getArtifactsPossiblyLtoMapped( Iterable inputs, Map ltoMapping) { Preconditions.checkNotNull(ltoMapping); ImmutableSet.Builder result = ImmutableSet.builder(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java index dc7fb3bc6b..ec86c09f6c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java @@ -117,14 +117,17 @@ public class LibrariesToLinkCollector { */ public static class CollectedLibrariesToLink { private final SequenceBuilder librariesToLink; + private final ImmutableSet expandedLinkerInputs; private final ImmutableSet librarySearchDirectories; private final ImmutableSet runtimeLibrarySearchDirectories; public CollectedLibrariesToLink( SequenceBuilder librariesToLink, + ImmutableSet expandedLinkerInputs, ImmutableSet librarySearchDirectories, ImmutableSet runtimeLibrarySearchDirectories) { this.librariesToLink = librariesToLink; + this.expandedLinkerInputs = expandedLinkerInputs; this.librarySearchDirectories = librarySearchDirectories; this.runtimeLibrarySearchDirectories = runtimeLibrarySearchDirectories; } @@ -133,6 +136,11 @@ public class LibrariesToLinkCollector { return librariesToLink; } + // TODO(b/78347840): Figure out how to make these Artifacts. + public ImmutableSet getExpandedLinkerInputs() { + return expandedLinkerInputs; + } + public ImmutableSet getLibrarySearchDirectories() { return librarySearchDirectories; } @@ -155,6 +163,7 @@ public class LibrariesToLinkCollector { ImmutableSet.Builder librarySearchDirectories = ImmutableSet.builder(); ImmutableSet.Builder runtimeLibrarySearchDirectories = ImmutableSet.builder(); ImmutableSet.Builder rpathRootsForExplicitSoDeps = ImmutableSet.builder(); + ImmutableSet.Builder expandedLinkerInputsBuilder = ImmutableSet.builder(); // List of command line parameters that need to be placed *outside* of // --whole-archive ... --no-whole-archive. SequenceBuilder librariesToLink = new SequenceBuilder(); @@ -193,7 +202,11 @@ public class LibrariesToLinkCollector { } Pair includeSolibsPair = - addLinkerInputs(librarySearchDirectories, rpathRootsForExplicitSoDeps, librariesToLink); + addLinkerInputs( + librarySearchDirectories, + rpathRootsForExplicitSoDeps, + librariesToLink, + expandedLinkerInputsBuilder); boolean includeSolibDir = includeSolibsPair.first; boolean includeToolchainLibrariesSolibDir = includeSolibsPair.second; Preconditions.checkState( @@ -211,6 +224,7 @@ public class LibrariesToLinkCollector { return new CollectedLibrariesToLink( librariesToLink, + expandedLinkerInputsBuilder.build(), librarySearchDirectories.build(), allRuntimeLibrarySearchDirectories.build()); } @@ -218,7 +232,8 @@ public class LibrariesToLinkCollector { private Pair addLinkerInputs( Builder librarySearchDirectories, Builder rpathEntries, - SequenceBuilder librariesToLink) { + SequenceBuilder librariesToLink, + ImmutableSet.Builder expandedLinkerInputsBuilder) { boolean includeSolibDir = false; boolean includeToolchainLibrariesSolibDir = false; for (LinkerInput input : linkerInputs) { @@ -242,9 +257,14 @@ public class LibrariesToLinkCollector { includeToolchainLibrariesSolibDir = true; } } - addDynamicInputLinkOptions(input, librariesToLink, librarySearchDirectories, rpathEntries); + addDynamicInputLinkOptions( + input, + librariesToLink, + expandedLinkerInputsBuilder, + librarySearchDirectories, + rpathEntries); } else { - addStaticInputLinkOptions(input, librariesToLink); + addStaticInputLinkOptions(input, librariesToLink, expandedLinkerInputsBuilder); } } return Pair.of(includeSolibDir, includeToolchainLibrariesSolibDir); @@ -258,6 +278,7 @@ public class LibrariesToLinkCollector { private void addDynamicInputLinkOptions( LinkerInput input, SequenceBuilder librariesToLink, + ImmutableSet.Builder expandedLinkerInputsBuilder, ImmutableSet.Builder librarySearchDirectories, ImmutableSet.Builder rpathRootsForExplicitSoDeps) { Preconditions.checkState( @@ -267,6 +288,7 @@ public class LibrariesToLinkCollector { !Link.useStartEndLib( input, CppHelper.getArchiveType(cppConfiguration, ccToolchainProvider))); + expandedLinkerInputsBuilder.add(input); Artifact inputArtifact = input.getArtifact(); PathFragment libDir = inputArtifact.getExecPath().getParentDirectory(); if (!libDir.equals(solibDir) @@ -309,9 +331,24 @@ public class LibrariesToLinkCollector { * * @param librariesToLink - a collection that will be exposed as a build variable. */ - private void addStaticInputLinkOptions(LinkerInput input, SequenceBuilder librariesToLink) { + private void addStaticInputLinkOptions( + LinkerInput input, + SequenceBuilder librariesToLink, + ImmutableSet.Builder expandedLinkerInputsBuilder) { ArtifactCategory artifactCategory = input.getArtifactCategory(); - Preconditions.checkState(artifactCategory != ArtifactCategory.DYNAMIC_LIBRARY); + Preconditions.checkArgument( + artifactCategory.equals(ArtifactCategory.OBJECT_FILE) + || artifactCategory.equals(ArtifactCategory.STATIC_LIBRARY) + || artifactCategory.equals(ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY)); + boolean isAlwaysLinkStaticLibrary = + artifactCategory == ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY; + + // input.disableWholeArchive() should only be true for libstdc++/libc++ etc. + boolean inputIsWholeArchive = + !input.disableWholeArchive() && (isAlwaysLinkStaticLibrary || needWholeArchive); + + String pathPrefix = input.isFake() ? Link.FAKE_OBJECT_PREFIX : ""; + // If we had any LTO artifacts, ltoMap whould be non-null. In that case, // we should have created a thinltoParamFile which the LTO indexing // step will populate with the exec paths that correspond to the LTO @@ -335,6 +372,12 @@ public class LibrariesToLinkCollector { if (handledByLtoIndexing(a, allowLtoIndexing)) { // The LTO artifacts that should be included in the final link // are listed in the thinltoParamFile, generated by the LTO indexing. + + // Even if this object file is being skipped for exposure as a Build variable, it's + // still an input to this action. + expandedLinkerInputsBuilder.add( + LinkerInputs.simpleLinkerInput( + a, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive = */ false)); continue; } // No LTO indexing step, so use the LTO backend's generated artifact directly @@ -342,29 +385,50 @@ public class LibrariesToLinkCollector { member = a; } nonLtoArchiveMembersBuilder.add(member); + expandedLinkerInputsBuilder.add( + LinkerInputs.simpleLinkerInput( + member, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive = */ false)); } ImmutableList nonLtoArchiveMembers = nonLtoArchiveMembersBuilder.build(); if (!nonLtoArchiveMembers.isEmpty()) { - librariesToLink.addValue( - LibraryToLinkValue.forObjectFileGroup(nonLtoArchiveMembers, needWholeArchive)); + if (inputIsWholeArchive) { + for (Artifact member : nonLtoArchiveMembers) { + if (member.isTreeArtifact()) { + // TODO(b/78189629): This object filegroup is expanded at action time but wrapped + // with --start/--end-lib. There's currently no way to force these objects to be + // linked in. + librariesToLink.addValue( + LibraryToLinkValue.forObjectFileGroup( + ImmutableList.of(member), /* isWholeArchive= */ true)); + } else { + // TODO(b/78189629): These each need to be their own LibraryToLinkValue so they're + // not wrapped in --start/--end-lib (which lets the linker leave out objects with + // unreferenced code). + librariesToLink.addValue( + LibraryToLinkValue.forObjectFile( + pathPrefix + member.getExecPathString(), /* isWholeArchive= */ true)); + } + } + } else { + librariesToLink.addValue( + LibraryToLinkValue.forObjectFileGroup( + nonLtoArchiveMembers, /* isWholeArchive= */ false)); + } } } } else { - Preconditions.checkArgument( - artifactCategory.equals(ArtifactCategory.OBJECT_FILE) - || artifactCategory.equals(ArtifactCategory.STATIC_LIBRARY) - || artifactCategory.equals(ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY)); - boolean isAlwaysLinkStaticLibrary = - artifactCategory == ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY; - boolean inputIsWholeArchive = - !input.disableWholeArchive() && (isAlwaysLinkStaticLibrary || needWholeArchive); - Artifact inputArtifact = input.getArtifact(); Artifact a; if (ltoMap != null && (a = ltoMap.remove(inputArtifact)) != null) { if (handledByLtoIndexing(a, allowLtoIndexing)) { // The LTO artifacts that should be included in the final link // are listed in the thinltoParamFile, generated by the LTO indexing. + + // Even if this object file is being skipped for exposure as a Build variable, it's + // still an input to this action. + expandedLinkerInputsBuilder.add( + LinkerInputs.simpleLinkerInput( + a, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive = */ false)); return; } // No LTO indexing step, so use the LTO backend's generated artifact directly @@ -372,12 +436,7 @@ public class LibrariesToLinkCollector { inputArtifact = a; } - String name; - if (input.isFake()) { - name = Link.FAKE_OBJECT_PREFIX + inputArtifact.getExecPathString(); - } else { - name = inputArtifact.getExecPathString(); - } + String name = pathPrefix + inputArtifact.getExecPathString(); if (artifactCategory.equals(ArtifactCategory.OBJECT_FILE)) { if (inputArtifact.isTreeArtifact()) { @@ -387,8 +446,10 @@ public class LibrariesToLinkCollector { } else { librariesToLink.addValue(LibraryToLinkValue.forObjectFile(name, inputIsWholeArchive)); } + expandedLinkerInputsBuilder.add(input); } else { librariesToLink.addValue(LibraryToLinkValue.forStaticLibrary(name, inputIsWholeArchive)); + expandedLinkerInputsBuilder.add(input); } } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/Link.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/Link.java index 74142be962..30cdbbe7dd 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/Link.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/Link.java @@ -14,13 +14,7 @@ package com.google.devtools.build.lib.rules.cpp; -import com.google.common.collect.AbstractIterator; -import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.collect.CollectionUtils; -import com.google.devtools.build.lib.collect.nestedset.NestedSet; -import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink; import com.google.devtools.build.lib.util.FileTypeSet; -import java.util.Iterator; /** * Utility types and methods for generating command lines for the linker, given @@ -295,124 +289,4 @@ public abstract class Link { || linkerInput.getArtifactCategory() == ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY) && linkerInput.containsObjectFiles(); } - - /** - * Replace always used archives with its members. This is used to build the linker cmd line. - */ - public static Iterable mergeInputsCmdLine(NestedSet inputs, - boolean globalNeedWholeArchive, ArchiveType archiveType) { - return new FilterMembersForLinkIterable(inputs, globalNeedWholeArchive, archiveType, false); - } - - /** - * Add in any object files which are implicitly named as inputs by the linker. - */ - public static Iterable mergeInputsDependencies(NestedSet inputs, - boolean globalNeedWholeArchive, ArchiveType archiveType) { - return new FilterMembersForLinkIterable(inputs, globalNeedWholeArchive, archiveType, true); - } - - /** - * On the fly implementation to filter the members. - */ - private static final class FilterMembersForLinkIterable implements Iterable { - private final boolean globalNeedWholeArchive; - private final ArchiveType archiveType; - private final boolean deps; - - private final Iterable inputs; - - private FilterMembersForLinkIterable(Iterable inputs, - boolean globalNeedWholeArchive, ArchiveType archiveType, boolean deps) { - this.globalNeedWholeArchive = globalNeedWholeArchive; - this.archiveType = archiveType; - this.deps = deps; - this.inputs = CollectionUtils.makeImmutable(inputs); - } - - @Override - public Iterator iterator() { - return new FilterMembersForLinkIterator(inputs.iterator(), globalNeedWholeArchive, - archiveType, deps); - } - } - - /** - * On the fly implementation to filter the members. - */ - private static final class FilterMembersForLinkIterator extends AbstractIterator { - private final boolean globalNeedWholeArchive; - private final ArchiveType archiveType; - private final boolean deps; - - private final Iterator inputs; - private Iterator delayList = ImmutableList.of().iterator(); - - private FilterMembersForLinkIterator(Iterator inputs, - boolean globalNeedWholeArchive, ArchiveType archiveType, boolean deps) { - this.globalNeedWholeArchive = globalNeedWholeArchive; - this.archiveType = archiveType; - this.deps = deps; - this.inputs = inputs; - } - - @Override - protected LinkerInput computeNext() { - if (delayList.hasNext()) { - return delayList.next(); - } - - while (inputs.hasNext()) { - LibraryToLink inputLibrary = inputs.next(); - - // True if the linker might use the members of this file, i.e., if the file is a thin or - // start_end_lib archive (aka static library). Also check if the library contains object - // files - otherwise getObjectFiles returns null, which would lead to an NPE in - // simpleLinkerInputs. - boolean needMembersForLink = archiveType != ArchiveType.REGULAR - && (inputLibrary.getArtifactCategory() == ArtifactCategory.STATIC_LIBRARY - || inputLibrary.getArtifactCategory() == ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY) - && inputLibrary.containsObjectFiles(); - - // True if we will pass the members instead of the original archive. - boolean passMembersToLinkCmd = needMembersForLink && (globalNeedWholeArchive - || inputLibrary.getArtifactCategory() == ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY); - - // If deps is false (when computing the inputs to be passed on the command line), then it's - // an if-then-else, i.e., the passMembersToLinkCmd flag decides whether to pass the object - // files or the archive itself. This flag in turn is based on whether the archives are fat - // or not (thin archives or start_end_lib) - we never expand fat archives, but we do expand - // non-fat archives if we need whole-archives for the entire link, or for the specific - // library (i.e., if alwayslink=1). - // - // If deps is true (when computing the inputs to be passed to the action as inputs), then it - // becomes more complicated. We always need to pass the members for thin and start_end_lib - // archives (needMembersForLink). And we _also_ need to pass the archive file itself unless - // it's a start_end_lib archive (unless it's an alwayslink library). - - // A note about ordering: the order in which the object files and the library are returned - // does not currently matter - this code results in the library returned first, and the - // object files returned after, but only if both are returned, which can only happen if - // deps is true, in which case this code only computes the list of inputs for the link - // action (so the order isn't critical). - if (passMembersToLinkCmd || (deps && needMembersForLink)) { - delayList = - LinkerInputs.simpleLinkerInputs( - inputLibrary.getObjectFiles(), - ArtifactCategory.OBJECT_FILE, - /* disableWholeArchive= */ false) - .iterator(); - } - - if (!(passMembersToLinkCmd || (deps && useStartEndLib(inputLibrary, archiveType)))) { - return inputLibrary; - } - - if (delayList.hasNext()) { - return delayList.next(); - } - } - return endOfData(); - } - } } -- cgit v1.2.3