Refactor CppLinkAction to construct its command line using the crosstool instead of a hardcoded switch. Add action configs to CppConfiguration for each link type. -- MOS_MIGRATED_REVID=128470872
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 f83158f..f202ce1 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
@@ -227,10 +227,6 @@ linkActionBuilder.setLinkStaticness(linkStaticness); linkActionBuilder.setFake(fake); linkActionBuilder.setFeatureConfiguration(featureConfiguration); - CcToolchainFeatures.Variables.Builder buildVariables = - new CcToolchainFeatures.Variables.Builder(); - buildVariables.addAllVariables(CppHelper.getToolchain(ruleContext).getBuildVariables()); - linkActionBuilder.setBuildVariables(buildVariables.build()); if (CppLinkAction.enableSymbolsCounts(cppConfiguration, fake, linkType)) { linkActionBuilder.setSymbolCountsOutput(ruleContext.getPackageRelativeArtifact(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java index b94a604..b48e03a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
@@ -30,8 +30,6 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CToolchain; -import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CToolchain.Tool; - import java.io.IOException; import java.io.ObjectInputStream; import java.io.Serializable; @@ -702,15 +700,17 @@ */ @Immutable public static class Variables { - + + /** An empty variables instance. */ + public static final Variables EMPTY = new Variables.Builder().build(); + /** - * Variables can be set as an arbitrarily deeply nested recursive sequence, which - * we represent as a tree of {@code Sequence} nodes. - * The nodes are {@code NestedSequence} objects, while the leafs are {@code ValueSequence} - * objects. We do not allow {@code Value} objects in the tree, as the object memory overhead - * is too large when we have millions of values. - * If we find single element {@code ValueSequence} in memory profiles in the future, we - * can introduce another special case type. + * Variables can be set as an arbitrarily deeply nested recursive sequence, which we represent + * as a tree of {@code Sequence} nodes. The nodes are {@code NestedSequence} objects, while the + * leafs are {@code ValueSequence} objects. We do not allow {@code Value} objects in the tree, + * as the object memory overhead is too large when we have millions of values. If we find single + * element {@code ValueSequence} in memory profiles in the future, we can introduce another + * special case type. */ interface Sequence { @@ -1035,10 +1035,8 @@ return new NestedView(viewMap, sequenceName, sequenceVariables.get(sequenceName)); } - /** - * Returns whether {@code variable} is set. - */ - private boolean isAvailable(String variable) { + /** Returns whether {@code variable} is set. */ + boolean isAvailable(String variable) { return variables.containsKey(variable) || sequenceVariables.containsKey(variable); } }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java index d2ddf4c..477d4bb 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -38,6 +38,7 @@ import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.rules.cpp.CppConfigurationLoader.CppConfigurationParameters; +import com.google.devtools.build.lib.rules.cpp.CppLinkActionConfigs.CppLinkPlatform; import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable; import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; @@ -51,7 +52,6 @@ import com.google.devtools.common.options.OptionsParsingException; import com.google.protobuf.TextFormat; import com.google.protobuf.TextFormat.ParseException; - import java.io.Serializable; import java.util.ArrayList; import java.util.EnumSet; @@ -681,6 +681,7 @@ return result.build(); } + // TODO(bazel-team): Remove this once bazel supports all crosstool flags through // feature configuration, and all crosstools have been converted. @@ -696,6 +697,15 @@ return toolchain; } try { + + if (getTargetLibc().equals("macosx")) { + TextFormat.merge( + CppLinkActionConfigs.getCppLinkActionConfigs(CppLinkPlatform.MAC), toolchainBuilder); + } else { + TextFormat.merge( + CppLinkActionConfigs.getCppLinkActionConfigs(CppLinkPlatform.LINUX), toolchainBuilder); + } + if (!features.contains("dependency_file")) { // Gcc options: // -MD turns on .d file output as a side-effect (doesn't imply -E) @@ -840,12 +850,8 @@ + " flag_set {" + " action: 'c-compile'" + " action: 'c++-compile'" - + " action: 'c++-link-static-library'" - + " action: 'c++-link-pic-static-library'" + " action: 'c++-link-interface-dynamic-library'" + " action: 'c++-link-dynamic-library'" - + " action: 'c++-link-alwayslink-static-library'" - + " action: 'c++-link-alwayslink-pic-static-library'" + " action: 'c++-link-executable'" + " flag_group {" + " flag: '-Xgcc-only=-fprofile-generate=%{fdo_instrument_path}'" @@ -935,12 +941,8 @@ + " }" + " }" + " flag_set {" - + " action: 'c++-link-static-library'" - + " action: 'c++-link-pic-static-library'" + " action: 'c++-link-interface-dynamic-library'" + " action: 'c++-link-dynamic-library'" - + " action: 'c++-link-always-link-static-library'" - + " action: 'c++-link-always-link-pic-static-library'" + " action: 'c++-link-executable'" + " flag_group {" + " flag: '-lgcov'"
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 3ec9843..cda2e26 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
@@ -17,6 +17,7 @@ import static java.nio.charset.StandardCharsets.ISO_8859_1; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -37,6 +38,8 @@ import com.google.devtools.build.lib.collect.nestedset.Order; 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.CcToolchainFeatures.Variables; +import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables.VariablesExtension; import com.google.devtools.build.lib.rules.cpp.CppLinkAction.Context; import com.google.devtools.build.lib.rules.cpp.CppLinkAction.LinkArtifactFactory; import com.google.devtools.build.lib.rules.cpp.Link.LinkStaticness; @@ -45,7 +48,6 @@ import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.PathFragment; - import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; @@ -53,11 +55,57 @@ import java.util.List; import java.util.Map; import java.util.Set; - import javax.annotation.Nullable; /** Builder class to construct {@link CppLinkAction}s. */ public class CppLinkActionBuilder { + + /** A build variable for clang flags that set the root of the linker search path. */ + public static final String RUNTIME_ROOT_FLAGS_VARIABLE = "runtime_root_flags"; + + /** A build variable for entries in the linker search path. */ + public static final String RUNTIME_ROOT_ENTRIES_VARIABLE = "runtime_root_entries"; + + /** + * A build variable for options applying to specific libraries in the linker invocation that + * either identify a library to be linked or add a directory to the runtime library search path. + */ + public static final String LIBOPTS_VARIABLE = "libopts"; + + /** + * A build variable for flags providing files to link as inputs in the linker invocation that + * should not go in a -whole_archive block. + */ + public static final String LINKER_INPUT_PARAMS_VARIABLE = "linker_input_params"; + + /** + * A build variable for flags providing files to link as inputs in the linker invocation that + * should go in a -whole_archive block. + */ + public static final String WHOLE_ARCHIVE_LINKER_INPUT_PARAMS_VARIABLE = + "whole_archive_linker_params"; + + /** A build variable whose presence indicates that whole archive flags should be applied. */ + public static final String GLOBAL_WHOLE_ARCHIVE_VARIABLE = "global_whole_archive"; + + /** A build variable for the execpath of the output of the linker. */ + public static final String OUTPUT_EXECPATH_VARIABLE = "output_execpath"; + + /** + * A build variable that is set to indicate a mostly static linking for which the linked binary + * should be piped to /dev/null. + */ + public static final String SKIP_MOSTLY_STATIC_VARIABLE = "skip_mostly_static"; + + /** A build variable giving a path to which to write symbol counts. */ + public static final String SYMBOL_COUNTS_OUTPUT_VARIABLE = "symbol_counts_output"; + + /** A build variable giving linkstamp paths. */ + public static final String LINKSTAMP_PATHS_VARIABLE = "linkstamp_paths"; + + /** A build variable whose presence indicates that PIC code should be generated. */ + public static final String FORCE_PIC_VARIABLE = "force_pic"; + // Builder-only // Null when invoked from tests (e.g. via createTestBuilder). @Nullable private final RuleContext ruleContext; @@ -72,8 +120,6 @@ protected final BuildConfiguration configuration; private final CppConfiguration cppConfiguration; private FeatureConfiguration featureConfiguration; - private CcToolchainFeatures.Variables buildVariables = - new CcToolchainFeatures.Variables.Builder().build(); // Morally equivalent with {@link Context}, except these are mutable. // Keep these in sync with {@link Context}. @@ -196,10 +242,8 @@ private String getActionName() { return linkType.getActionName(); } - - /** - * Returns linker inputs that are not libraries. - */ + + /** Returns linker inputs that are not libraries. */ public Set<LinkerInput> getNonLibraries() { return nonLibraries; } @@ -401,6 +445,17 @@ final ImmutableSet<String> features = (ruleContext == null) ? ImmutableSet.<String>of() : ruleContext.getFeatures(); + // For backwards compatibility, and for tests, we permit the link action to be + // instantiated without a feature configuration. + if (featureConfiguration == null) { + if (toolchain != null) { + featureConfiguration = + CcCommon.configureFeatures(ruleContext, toolchain, CcLibraryHelper.SourceCategory.CC); + } else { + featureConfiguration = CcCommon.configureFeatures(ruleContext); + } + } + final LibraryToLink outputLibrary = LinkerInputs.newInputLibrary(output, filteredNonLibraryArtifacts, this.ltoBitcodeFiles); final LibraryToLink interfaceOutputLibrary = @@ -436,6 +491,24 @@ symbolCounts); } + ImmutableList<LinkerInput> runtimeLinkerInputs = + ImmutableList.copyOf(LinkerInputs.simpleLinkerInputs(runtimeInputs)); + + // Add build variables necessary to template link args into the crosstool. + Variables.Builder buildVariablesBuilder = new Variables.Builder(); + CppLinkVariablesExtension variablesExtension = + isLTOIndexing + ? new CppLinkVariablesExtension( + linkstampMap, needWholeArchive, linkerInputs, runtimeLinkerInputs, null) + : new CppLinkVariablesExtension( + linkstampMap, + needWholeArchive, + linkerInputs, + runtimeLinkerInputs, + outputLibrary.getArtifact()); + variablesExtension.addVariables(buildVariablesBuilder); + Variables buildVariables = buildVariablesBuilder.build(); + PathFragment paramRootPath = ParameterFile.derivePath( outputLibrary.getArtifact().getRootRelativePath(), (isLTOIndexing) ? "lto" : "2"); @@ -446,28 +519,52 @@ ? linkArtifactFactory.create(ruleContext, configuration, paramRootPath) : null; + Preconditions.checkArgument( + linkType != LinkTargetType.INTERFACE_DYNAMIC_LIBRARY, + "you can't link an interface dynamic library directly"); + if (linkType != LinkTargetType.DYNAMIC_LIBRARY) { + Preconditions.checkArgument( + interfaceOutput == null, + "interface output may only be non-null for dynamic library links"); + } + if (linkType.isStaticLibraryLink()) { + // solib dir must be null for static links + runtimeSolibDir = null; + + Preconditions.checkArgument( + linkStaticness == LinkStaticness.FULLY_STATIC, "static library link must be static"); + Preconditions.checkArgument( + symbolCounts == null, "the symbol counts output must be null for static links"); + Preconditions.checkArgument( + !isNativeDeps, "the native deps flag must be false for static links"); + Preconditions.checkArgument( + !needWholeArchive, "the need whole archive flag must be false for static links"); + } + LinkCommandLine.Builder linkCommandLineBuilder = new LinkCommandLine.Builder(configuration, getOwner(), ruleContext) - .setActionName(getActionName()) .setLinkerInputs(linkerInputs) - .setRuntimeInputs(ImmutableList.copyOf(LinkerInputs.simpleLinkerInputs(runtimeInputs))) + .setRuntimeInputs(runtimeLinkerInputs) .setLinkTargetType(linkType) .setLinkStaticness(linkStaticness) .setFeatures(features) .setRuntimeSolibDir(linkType.isStaticLibraryLink() ? null : runtimeSolibDir) .setNativeDeps(isNativeDeps) .setUseTestOnlyFlags(useTestOnlyFlags) - .setNeedWholeArchive(needWholeArchive) .setParamFile(paramFile) - .setAllLTOArtifacts(isLTOIndexing ? null : allLTOArtifacts) .setToolchain(toolchain) + .setBuildVariables(buildVariables) .setFeatureConfiguration(featureConfiguration); + // TODO(b/30228443): Refactor noWholeArchiveInputs into action_configs, and remove this. + if (needWholeArchive) { + linkCommandLineBuilder.setNoWholeArchiveFlags(variablesExtension.getNoWholeArchiveInputs()); + } + if (!isLTOIndexing) { linkCommandLineBuilder .setOutput(outputLibrary.getArtifact()) .setInterfaceOutput(interfaceOutput) - .setSymbolCountsOutput(symbolCounts) .setBuildInfoHeaderArtifacts(buildInfoHeaderArtifacts) .setInterfaceSoBuilder(getInterfaceSoBuilder()) .setLinkstamps(linkstampMap) @@ -506,13 +603,6 @@ dependencyInputsBuilder.addTransitive(compilationInputs.build()); } - // For backwards compatibility, and for tests, we permit the link action to be - // instantiated without a feature configuration. In this case, an empty feature - // configuration is used. - if (featureConfiguration == null) { - this.featureConfiguration = new FeatureConfiguration(); - } - Iterable<Artifact> expandedInputs = LinkerInputs.toLibraryArtifacts( Link.mergeInputsDependencies( @@ -545,7 +635,7 @@ for (LTOBackendArtifacts a : allLTOArtifacts) { List<String> argv = new ArrayList<>(); argv.addAll(cppConfiguration.getLinkOptions()); - argv.addAll(featureConfiguration.getCommandLine(getActionName(), buildVariables)); + argv.addAll(featureConfiguration.getCommandLine(getActionName(), Variables.EMPTY)); argv.addAll(cppConfiguration.getCompilerOptions(features)); a.setCommandLine(argv); } @@ -684,12 +774,6 @@ return this; } - /** Sets the build variables that will be used to template the crosstool. */ - public CppLinkActionBuilder setBuildVariables(CcToolchainFeatures.Variables buildVariables) { - this.buildVariables = buildVariables; - return this; - } - /** * This is the LTO indexing step, rather than the real link. * @@ -936,4 +1020,439 @@ this.runtimeSolibDir = runtimeSolibDir; return this; } + + private static class LinkArgCollector { + String rpathRoot; + List<String> rpathEntries; + Set<String> libopts; + List<String> linkerInputParams; + List<String> wholeArchiveLinkerInputParams; + List<String> noWholeArchiveInputs; + + public void setRpathRoot(String rPathRoot) { + this.rpathRoot = rPathRoot; + } + + public void setRpathEntries(List<String> rpathEntries) { + this.rpathEntries = rpathEntries; + } + + public void setLibopts(Set<String> libopts) { + this.libopts = libopts; + } + + public void setLinkerInputParams(List<String> linkerInputParams) { + this.linkerInputParams = linkerInputParams; + } + + public void setWholeArchiveLinkerInputParams(List<String> wholeArchiveInputParams) { + this.wholeArchiveLinkerInputParams = wholeArchiveInputParams; + } + + public void setNoWholeArchiveInputs(List<String> noWholeArchiveInputs) { + this.noWholeArchiveInputs = noWholeArchiveInputs; + } + + public String getRpathRoot() { + return rpathRoot; + } + + public List<String> getRpathEntries() { + return rpathEntries; + } + + public Set<String> getLibopts() { + return libopts; + } + + public List<String> getLinkerInputParams() { + return linkerInputParams; + } + + public List<String> getWholeArchiveLinkerInputParams() { + return wholeArchiveLinkerInputParams; + } + + public List<String> getNoWholeArchiveInputs() { + return noWholeArchiveInputs; + } + } + + private class CppLinkVariablesExtension implements VariablesExtension { + + private final ImmutableMap<Artifact, Artifact> linkstampMap; + private final boolean needWholeArchive; + private final Iterable<LinkerInput> linkerInputs; + private final ImmutableList<LinkerInput> runtimeLinkerInputs; + private final Artifact outputArtifact; + + private final LinkArgCollector linkArgCollector = new LinkArgCollector(); + + public CppLinkVariablesExtension( + ImmutableMap<Artifact, Artifact> linkstampMap, + boolean needWholeArchive, + Iterable<LinkerInput> linkerInputs, + ImmutableList<LinkerInput> runtimeLinkerInputs, + Artifact output) { + this.linkstampMap = linkstampMap; + this.needWholeArchive = needWholeArchive; + this.linkerInputs = linkerInputs; + this.runtimeLinkerInputs = runtimeLinkerInputs; + this.outputArtifact = output; + + addInputFileLinkOptions(linkArgCollector); + } + + /** + * Returns linker parameters indicating libraries that should not be linked inside a + * --whole_archive block. + * + * <p>TODO(b/30228443): Refactor into action configs + */ + public List<String> getNoWholeArchiveInputs() { + return linkArgCollector.getNoWholeArchiveInputs(); + } + + @Override + public void addVariables(Variables.Builder buildVariables) { + + // symbol counting + if (symbolCounts != null) { + buildVariables.addVariable(SYMBOL_COUNTS_OUTPUT_VARIABLE, symbolCounts.getExecPathString()); + } + + // linkstamp + ImmutableSet.Builder<String> linkstampPaths = ImmutableSet.<String>builder(); + for (Artifact linkstampOutput : linkstampMap.values()) { + linkstampPaths.add(linkstampOutput.getExecPathString()); + } + + buildVariables.addSequenceVariable(LINKSTAMP_PATHS_VARIABLE, linkstampPaths.build()); + + // pic + boolean forcePic = cppConfiguration.forcePic(); + if (forcePic) { + buildVariables.addVariable(FORCE_PIC_VARIABLE, ""); + } + + // rpath + if (linkArgCollector.getRpathRoot() != null) { + buildVariables.addVariable(RUNTIME_ROOT_FLAGS_VARIABLE, linkArgCollector.getRpathRoot()); + } + + if (linkArgCollector.getRpathEntries() != null) { + buildVariables.addSequenceVariable( + RUNTIME_ROOT_ENTRIES_VARIABLE, linkArgCollector.getRpathEntries()); + } + + buildVariables.addSequenceVariable(LIBOPTS_VARIABLE, linkArgCollector.getLibopts()); + buildVariables.addSequenceVariable( + LINKER_INPUT_PARAMS_VARIABLE, linkArgCollector.getLinkerInputParams()); + buildVariables.addSequenceVariable( + WHOLE_ARCHIVE_LINKER_INPUT_PARAMS_VARIABLE, + linkArgCollector.getWholeArchiveLinkerInputParams()); + + // global archive + if (needWholeArchive) { + buildVariables.addVariable(GLOBAL_WHOLE_ARCHIVE_VARIABLE, ""); + } + + // mostly static + if (linkStaticness == LinkStaticness.MOSTLY_STATIC && cppConfiguration.skipStaticOutputs()) { + buildVariables.addVariable(SKIP_MOSTLY_STATIC_VARIABLE, ""); + } + + // output exec path + if (this.outputArtifact != null) { + buildVariables.addVariable( + OUTPUT_EXECPATH_VARIABLE, this.outputArtifact.getExecPathString()); + } + + // Variables arising from the toolchain + buildVariables + .addAllVariables(CppHelper.getToolchain(ruleContext).getBuildVariables()) + .build(); + CppHelper.getFdoSupport(ruleContext).getLinkOptions(featureConfiguration, buildVariables); + } + + private boolean isDynamicLibrary(LinkerInput linkInput) { + Artifact libraryArtifact = linkInput.getArtifact(); + String name = libraryArtifact.getFilename(); + return Link.SHARED_LIBRARY_FILETYPES.matches(name) && name.startsWith("lib"); + } + + private boolean isSharedNativeLibrary() { + return isNativeDeps && cppConfiguration.shareNativeDeps(); + } + + /** + * When linking a shared library fully or mostly static then we need to link in *all* dependent + * files, not just what the shared library needs for its own code. This is done by wrapping all + * objects/libraries with -Wl,-whole-archive and -Wl,-no-whole-archive. For this case the + * globalNeedWholeArchive parameter must be set to true. Otherwise only library objects (.lo) + * need to be wrapped with -Wl,-whole-archive and -Wl,-no-whole-archive. + * + * <p>TODO: Factor out of the bazel binary into build variables for crosstool action_configs. + */ + private void addInputFileLinkOptions(LinkArgCollector linkArgCollector) { + + // Used to collect -L and -Wl,-rpath options, ensuring that each used only once. + Set<String> libOpts = new LinkedHashSet<>(); + + // List of command line parameters to link input files (either directly or using -l). + List<String> linkerInputParameters = new ArrayList<>(); + + // List of command line parameters that need to be placed *outside* of + // --whole-archive ... --no-whole-archive. + List<String> noWholeArchiveInputs = new ArrayList<>(); + + PathFragment solibDir = + configuration + .getBinDirectory() + .getExecPath() + .getRelative(cppConfiguration.getSolibDirectory()); + String runtimeSolibName = runtimeSolibDir != null ? runtimeSolibDir.getBaseName() : null; + boolean runtimeRpath = + runtimeSolibDir != null + && (linkType == LinkTargetType.DYNAMIC_LIBRARY + || (linkType == LinkTargetType.EXECUTABLE + && linkStaticness == LinkStaticness.DYNAMIC)); + + String rpathRoot = null; + List<String> runtimeRpathEntries = new ArrayList<>(); + + if (output != null) { + String origin = + useTestOnlyFlags && cppConfiguration.supportsExecOrigin() + ? "$EXEC_ORIGIN/" + : "$ORIGIN/"; + if (runtimeRpath) { + runtimeRpathEntries.add("-Wl,-rpath," + origin + runtimeSolibName + "/"); + } + + // Calculate the correct relative value for the "-rpath" link option (which sets + // the search path for finding shared libraries). + if (isSharedNativeLibrary()) { + // For shared native libraries, special symlinking is applied to ensure C++ + // runtimes are available under $ORIGIN/_solib_[arch]. So we set the RPATH to find + // them. + // + // Note that we have to do this because $ORIGIN points to different paths for + // different targets. In other words, blaze-bin/d1/d2/d3/a_shareddeps.so and + // blaze-bin/d4/b_shareddeps.so have different path depths. The first could + // reference a standard blaze-bin/_solib_[arch] via $ORIGIN/../../../_solib[arch], + // and the second could use $ORIGIN/../_solib_[arch]. But since this is a shared + // artifact, both are symlinks to the same place, so + // there's no *one* RPATH setting that fits all targets involved in the sharing. + rpathRoot = + "-Wl,-rpath," + origin + ":" + origin + cppConfiguration.getSolibDirectory() + "/"; + if (runtimeRpath) { + runtimeRpathEntries.add("-Wl,-rpath," + origin + "../" + runtimeSolibName + "/"); + } + } else { + // For all other links, calculate the relative path from the output file to _solib_[arch] + // (the directory where all shared libraries are stored, which resides under the blaze-bin + // directory. In other words, given blaze-bin/my/package/binary, rpathRoot would be + // "../../_solib_[arch]". + if (runtimeRpath) { + runtimeRpathEntries.add( + "-Wl,-rpath," + + origin + + Strings.repeat("../", output.getRootRelativePath().segmentCount() - 1) + + runtimeSolibName + + "/"); + } + + rpathRoot = + "-Wl,-rpath," + + origin + + Strings.repeat("../", output.getRootRelativePath().segmentCount() - 1) + + cppConfiguration.getSolibDirectory() + + "/"; + + if (isNativeDeps) { + // We also retain the $ORIGIN/ path to solibs that are in _solib_<arch>, as opposed to + // the package directory) + if (runtimeRpath) { + runtimeRpathEntries.add("-Wl,-rpath," + origin + "../" + runtimeSolibName + "/"); + } + rpathRoot += ":" + origin; + } + } + } + + boolean includeSolibDir = false; + + Map<Artifact, Artifact> ltoMap = null; + if (!isLTOIndexing && (allLTOArtifacts != null)) { + // TODO(bazel-team): The LTO final link can only work if there are individual .o files on + // the command line. Rather than crashing, this should issue a nice error. We will get + // this by + // 1) moving supports_start_end_lib to a toolchain feature + // 2) having thin_lto require start_end_lib + // As a bonus, we can rephrase --nostart_end_lib as --features=-start_end_lib and get rid + // of a command line option. + + Preconditions.checkState(cppConfiguration.useStartEndLib()); + ltoMap = new HashMap<>(); + for (LTOBackendArtifacts l : allLTOArtifacts) { + ltoMap.put(l.getBitcodeFile(), l.getObjectFile()); + } + } + + for (LinkerInput input : linkerInputs) { + if (isDynamicLibrary(input)) { + PathFragment libDir = input.getArtifact().getExecPath().getParentDirectory(); + Preconditions.checkState( + libDir.startsWith(solibDir), + "Artifact '%s' is not under directory '%s'.", + input.getArtifact(), + solibDir); + if (libDir.equals(solibDir)) { + includeSolibDir = true; + } + addDynamicInputLinkOptions(input, linkerInputParameters, libOpts, solibDir, rpathRoot); + } else { + addStaticInputLinkOptions(input, linkerInputParameters, ltoMap); + } + } + + boolean includeRuntimeSolibDir = false; + + for (LinkerInput input : runtimeLinkerInputs) { + List<String> optionsList = needWholeArchive ? noWholeArchiveInputs : linkerInputParameters; + + if (isDynamicLibrary(input)) { + PathFragment libDir = input.getArtifact().getExecPath().getParentDirectory(); + Preconditions.checkState( + runtimeSolibDir != null && libDir.equals(runtimeSolibDir), + "Artifact '%s' is not under directory '%s'.", + input.getArtifact(), + solibDir); + includeRuntimeSolibDir = true; + addDynamicInputLinkOptions(input, optionsList, libOpts, solibDir, rpathRoot); + } else { + addStaticInputLinkOptions(input, optionsList, ltoMap); + } + } + + // rpath ordering matters for performance; first add the one where most libraries are found. + if (includeSolibDir && rpathRoot != null) { + linkArgCollector.setRpathRoot(rpathRoot); + } + if (includeRuntimeSolibDir) { + linkArgCollector.setRpathEntries(runtimeRpathEntries); + } + + linkArgCollector.setLibopts(libOpts); + + ImmutableList.Builder<String> wholeArchiveInputParams = ImmutableList.builder(); + ImmutableList.Builder<String> standardArchiveInputParams = ImmutableList.builder(); + for (String param : linkerInputParameters) { + if (!wholeArchive && Link.LINK_LIBRARY_FILETYPES.matches(param)) { + wholeArchiveInputParams.add(param); + } else { + standardArchiveInputParams.add(param); + } + } + + linkArgCollector.setLinkerInputParams(standardArchiveInputParams.build()); + linkArgCollector.setWholeArchiveLinkerInputParams(wholeArchiveInputParams.build()); + linkArgCollector.setNoWholeArchiveInputs(noWholeArchiveInputs); + + if (ltoMap != null) { + Preconditions.checkState(ltoMap.isEmpty(), "Still have LTO objects left: %s", ltoMap); + } + } + + /** Adds command-line options for a dynamic library input file into options and libOpts. */ + private void addDynamicInputLinkOptions( + LinkerInput input, + List<String> options, + Set<String> libOpts, + PathFragment solibDir, + String rpathRoot) { + Preconditions.checkState(isDynamicLibrary(input)); + Preconditions.checkState(!Link.useStartEndLib(input, cppConfiguration.archiveType())); + + Artifact inputArtifact = input.getArtifact(); + PathFragment libDir = inputArtifact.getExecPath().getParentDirectory(); + if (rpathRoot != null + && !libDir.equals(solibDir) + && (runtimeSolibDir == null || !runtimeSolibDir.equals(libDir))) { + String dotdots = ""; + PathFragment commonParent = solibDir; + while (!libDir.startsWith(commonParent)) { + dotdots += "../"; + commonParent = commonParent.getParentDirectory(); + } + + libOpts.add(rpathRoot + dotdots + libDir.relativeTo(commonParent).getPathString()); + } + + libOpts.add("-L" + inputArtifact.getExecPath().getParentDirectory().getPathString()); + + String name = inputArtifact.getFilename(); + if (CppFileTypes.SHARED_LIBRARY.matches(name)) { + String libName = name.replaceAll("(^lib|\\.(so|dylib)$)", ""); + options.add("-l" + libName); + } else { + // Interface shared objects have a non-standard extension + // that the linker won't be able to find. So use the + // filename directly rather than a -l option. Since the + // library has an SONAME attribute, this will work fine. + options.add(inputArtifact.getExecPathString()); + } + } + + /** + * Adds command-line options for a static library or non-library input into options. + * + * @param ltoMap is a mutable list of exec paths that should be on the command-line, which must + * be supplied for LTO final links. + */ + private void addStaticInputLinkOptions( + LinkerInput input, List<String> options, @Nullable Map<Artifact, Artifact> ltoMap) { + Preconditions.checkState(!isDynamicLibrary(input)); + + // start-lib/end-lib library: adds its input object files. + if (Link.useStartEndLib(input, cppConfiguration.archiveType())) { + Iterable<Artifact> archiveMembers = input.getObjectFiles(); + if (!Iterables.isEmpty(archiveMembers)) { + options.add("-Wl,--start-lib"); + for (Artifact member : archiveMembers) { + if (ltoMap != null) { + Artifact backend = ltoMap.remove(member); + + if (backend != null) { + // If the backend artifact is missing, we can't print a warning because this may + // happen normally, due libraries that list .o files explicitly, or generate .o + // files from assembler. + member = backend; + } + } + + options.add(member.getExecPathString()); + } + options.add("-Wl,--end-lib"); + } + } else { + // For anything else, add the input directly. + Artifact inputArtifact = input.getArtifact(); + + if (ltoMap != null) { + Artifact ltoArtifact = ltoMap.remove(inputArtifact); + if (ltoArtifact != null) { + inputArtifact = ltoArtifact; + } + } + + if (input.isFake()) { + options.add(Link.FAKE_OBJECT_PREFIX + inputArtifact.getExecPathString()); + } else { + options.add(inputArtifact.getExecPathString()); + } + } + } + } }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionConfigs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionConfigs.java new file mode 100644 index 0000000..a359350 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionConfigs.java
@@ -0,0 +1,297 @@ +// Copyright 2014 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.rules.cpp; + +import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableList; + +/** + * A helper class for creating action_configs for the c++ link action. + * + * <p>TODO(b/30109612): Replace this with action_configs in the crosstool instead of putting it in + * legacy features. + */ +public class CppLinkActionConfigs { + + /** A platform for linker invocations. */ + public static enum CppLinkPlatform { + LINUX, + MAC + } + + public static String getCppLinkActionConfigs(CppLinkPlatform platform) { + return Joiner.on("\n") + .join( + ImmutableList.of( + "action_config {", + " config_name: 'c++-link-executable'", + " action_name: 'c++-link-executable'", + " tool {", + " tool_path: 'DUMMY_TOOL'", + " }", + " implies: 'symbol_counts'", + " implies: 'linkstamps'", + " implies: 'output_execpath_flags_executable'", + " implies: 'global_whole_archive_open'", + " implies: 'runtime_root_flags'", + " implies: 'input_param_flags'", + " implies: 'global_whole_archive_close'", + " implies: 'force_pic_flags'", + "}", + "action_config {", + " config_name: 'c++-link-dynamic-library'", + " action_name: 'c++-link-dynamic-library'", + " tool {", + " tool_path: 'DUMMY_TOOL'", + " }", + " implies: 'symbol_counts'", + " implies: 'shared_flag'", + " implies: 'linkstamps'", + " implies: 'output_execpath_flags'", + " implies: 'global_whole_archive_open'", + " implies: 'runtime_root_flags'", + " implies: 'input_param_flags'", + " implies: 'global_whole_archive_close'", + "}", + "action_config {", + " config_name: 'c++-link-static-library'", + " action_name: 'c++-link-static-library'", + " tool {", + " tool_path: 'DUMMY_TOOL'", + " }", + " implies: 'global_whole_archive_open'", + " implies: 'runtime_root_flags'", + " implies: 'input_param_flags'", + " implies: 'global_whole_archive_close'", + "}", + "action_config {", + " config_name: 'c++-link-alwayslink-static-library'", + " action_name: 'c++-link-alwayslink-static-library'", + " tool {", + " tool_path: 'DUMMY_TOOL'", + " }", + " implies: 'global_whole_archive_open'", + " implies: 'runtime_root_flags'", + " implies: 'input_param_flags'", + " implies: 'global_whole_archive_close'", + "}", + "action_config {", + " config_name: 'c++-link-pic-static-library'", + " action_name: 'c++-link-pic-static-library'", + " tool {", + " tool_path: 'DUMMY_TOOL'", + " }", + " implies: 'global_whole_archive_open'", + " implies: 'runtime_root_flags'", + " implies: 'input_param_flags'", + " implies: 'global_whole_archive_close'", + "}", + "action_config {", + " config_name: 'c++-link-alwayslink-pic-static-library'", + " action_name: 'c++-link-alwayslink-pic-static-library'", + " tool {", + " tool_path: 'DUMMY_TOOL'", + " }", + " implies: 'global_whole_archive_open'", + " implies: 'runtime_root_flags'", + " implies: 'input_param_flags'", + " implies: 'global_whole_archive_close'", + "}", + "feature {", + " name: 'symbol_counts'", + " flag_set {", + " expand_if_all_available: 'symbol_counts_output'", + " action: 'c++-link-executable'", + " action: 'c++-link-dynamic-library'", + " flag_group {", + " flag: '-Wl,--print-symbol-counts=%{symbol_counts_output}'", + " }", + " }", + "}", + "feature {", + " name: 'shared_flag'", + " flag_set {", + " action: 'c++-link-dynamic-library'", + " flag_group {", + " flag: '-shared'", + " }", + " }", + "}", + "feature {", + " name: 'linkstamps'", + " flag_set {", + " action: 'c++-link-executable'", + " action: 'c++-link-dynamic-library'", + " expand_if_all_available: 'linkstamp_paths'", + " flag_group {", + " flag: '%{linkstamp_paths}'", + " }", + " }", + "}", + "feature {", + " name: 'output_execpath_flags'", + " flag_set {", + " expand_if_all_available: 'output_execpath'", + " action: 'c++-link-dynamic-library'", + " flag_group {", + " flag: '-o'", + " flag: '%{output_execpath}'", + " }", + " }", + "}", + "feature {", + " name: 'output_execpath_flags_executable'", + " flag_set {", + " expand_if_all_available: 'output_execpath'", + " action: 'c++-link-executable'", + " flag_group {", + " flag: '-o'", + " }", + " }", + " flag_set {", + " expand_if_all_available: 'skip_mostly_static'", + " expand_if_all_available: 'output_execpath'", + " action: 'c++-link-executable'", + " flag_group {", + " flag: '/dev/null'", + " flag: '-MMD'", + " flag: '-MF'", + " }", + " }", + " flag_set {", + " expand_if_all_available: 'output_execpath'", + " action: 'c++-link-executable'", + " flag_group {", + " flag: '%{output_execpath}'", + " }", + " }", + "}", + "feature {", + " name: 'global_whole_archive_open'", + " flag_set {", + " expand_if_all_available: 'global_whole_archive'", + " action: 'c++-link-executable'", + " action: 'c++-link-dynamic-library'", + " action: 'c++-link-static-library'", + " action: 'c++-link-alwayslink-static-library'", + " action: 'c++-link-pic-static-library'", + " action: 'c++-link-alwayslink-pic-static-library'", + " flag_group {", + // TODO: Factor platform difference into respective linux and OSX crosstools. + String.format( + " flag:'%s'", + platform == CppLinkPlatform.LINUX ? "-Wl,-whole-archive" : "-Wl,-all_load"), + " }", + " }", + "}", + "feature {", + " name: 'runtime_root_flags',", + " flag_set {", + " expand_if_all_available: 'runtime_root_flags'", + " action: 'c++-link-executable'", + " action: 'c++-link-dynamic-library'", + " action: 'c++-link-static-library'", + " action: 'c++-link-alwayslink-static-library'", + " action: 'c++-link-pic-static-library'", + " action: 'c++-link-alwayslink-pic-static-library'", + " flag_group {", + " flag: '%{runtime_root_flags}'", + " }", + " }", + " flag_set {", + " expand_if_all_available: 'runtime_root_entries'", + " action: 'c++-link-executable'", + " action: 'c++-link-dynamic-library'", + " action: 'c++-link-static-library'", + " action: 'c++-link-alwayslink-static-library'", + " action: 'c++-link-pic-static-library'", + " action: 'c++-link-alwayslink-pic-static-library'", + " flag_group {", + " flag: '%{runtime_root_entries}'", + " }", + " }", + "}", + "feature {", + " name: 'input_param_flags'", + " flag_set {", + " expand_if_all_available: 'libopts'", + " action: 'c++-link-executable'", + " action: 'c++-link-dynamic-library'", + " action: 'c++-link-static-library'", + " action: 'c++-link-alwayslink-static-library'", + " action: 'c++-link-pic-static-library'", + " action: 'c++-link-alwayslink-pic-static-library'", + " flag_group {", + " flag: '%{libopts}'", + " }", + " }", + " flag_set {", + " expand_if_all_available: 'whole_archive_linker_params'", + " action: 'c++-link-executable'", + " action: 'c++-link-dynamic-library'", + " action: 'c++-link-static-library'", + " action: 'c++-link-alwayslink-static-library'", + " action: 'c++-link-pic-static-library'", + " action: 'c++-link-alwayslink-pic-static-library'", + " flag_group {", + platform == CppLinkPlatform.LINUX + ? " flag: '-Wl,-whole-archive'\n" + + " flag: '%{whole_archive_linker_params}'\n" + + " flag: '-Wl,-no-whole-archive'" + : " flag: '-Wl,-force_load,%{whole_archive_linker_params}'", + " }", + " }", + " flag_set {", + " expand_if_all_available: 'linker_input_params'", + " action: 'c++-link-executable'", + " action: 'c++-link-dynamic-library'", + " action: 'c++-link-static-library'", + " action: 'c++-link-alwayslink-static-library'", + " action: 'c++-link-pic-static-library'", + " action: 'c++-link-alwayslink-pic-static-library'", + " flag_group {", + " flag: '%{linker_input_params}'", + " }", + " }", + "}", + "feature {", + " name: 'global_whole_archive_close'", + " flag_set {", + " expand_if_all_available: 'global_whole_archive'", + " action: 'c++-link-executable'", + " action: 'c++-link-dynamic-library'", + " action: 'c++-link-static-library'", + " action: 'c++-link-alwayslink-static-library'", + " action: 'c++-link-pic-static-library'", + " action: 'c++-link-alwayslink-pic-static-library'", + " flag_group {", + // TODO: Factor platform difference into respective linux and OSX crosstools. + String.format( + " flag: '%s'", + platform == CppLinkPlatform.LINUX ? "-Wl,-no-whole-archive" : ""), + " }", + " }", + "}", + "feature {", + " name: 'force_pic_flags'", + " flag_set {", + " expand_if_all_available: 'force_pic'", + " action: 'c++-link-executable'", + " flag_group {", + " flag: '-pie'", + " }", + " }", + "}")); + } +}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java index a70f9d9..38aa8b9 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
@@ -38,7 +38,6 @@ import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.PathFragment; - import java.util.ArrayList; import java.util.Collection; import java.util.LinkedHashSet; @@ -46,7 +45,6 @@ import java.util.Map; import java.util.Set; import java.util.regex.Pattern; - import javax.annotation.Nullable; /** @@ -359,11 +357,6 @@ return result; } - private CcToolchainFeatures.Variables linkBuildVariables() { - return new CcToolchainFeatures.Variables.Builder() - .addAllVariables(CppHelper.getToolchain(ruleContext).getBuildVariables()).build(); - } - private void setupCompileBuildVariables( CppCompileActionBuilder builder, boolean usePic, @@ -791,7 +784,6 @@ .setLinkType(linkType) .setLinkStaticness(LinkStaticness.FULLY_STATIC) .setFeatureConfiguration(featureConfiguration) - .setBuildVariables(linkBuildVariables()) .build(); env.registerAction(maybePicAction); result.addStaticLibrary(maybePicAction.getOutputLibrary()); @@ -813,7 +805,6 @@ .setLinkType(picLinkType) .setLinkStaticness(LinkStaticness.FULLY_STATIC) .setFeatureConfiguration(featureConfiguration) - .setBuildVariables(linkBuildVariables()) .build(); env.registerAction(picAction); result.addPicStaticLibrary(picAction.getOutputLibrary()); @@ -855,8 +846,7 @@ .setRuntimeInputs( CppHelper.getToolchain(ruleContext).getDynamicRuntimeLinkMiddleman(), CppHelper.getToolchain(ruleContext).getDynamicRuntimeLinkInputs()) - .setFeatureConfiguration(featureConfiguration) - .setBuildVariables(linkBuildVariables()); + .setFeatureConfiguration(featureConfiguration); if (!ccOutputs.getLtoBitcodeFiles().isEmpty() && featureConfiguration.isEnabled(CppRuleClasses.THIN_LTO)) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java index 96b5a3b..c5cb301 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java
@@ -17,7 +17,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; import com.google.common.base.Predicates; -import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -32,23 +31,18 @@ import com.google.devtools.build.lib.collect.CollectionUtils; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; +import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables; import com.google.devtools.build.lib.rules.cpp.Link.LinkStaticness; import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.util.ShellEscaper; import com.google.devtools.build.lib.vfs.PathFragment; - import java.util.ArrayList; -import java.util.Collections; -import java.util.HashMap; -import java.util.LinkedHashSet; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; - import javax.annotation.Nullable; /** @@ -66,7 +60,6 @@ @Nullable private final FeatureConfiguration featureConfiguration; @Nullable private final Artifact output; @Nullable private final Artifact interfaceOutput; - @Nullable private final Artifact symbolCountsOutput; private final ImmutableList<Artifact> buildInfoHeaderArtifacts; private final Iterable<? extends LinkerInput> linkerInputs; private final Iterable<? extends LinkerInput> runtimeInputs; @@ -80,9 +73,8 @@ @Nullable private final PathFragment runtimeSolibDir; private final boolean nativeDeps; private final boolean useTestOnlyFlags; - private final boolean needWholeArchive; + private final List<String> noWholeArchiveFlags; - @Nullable private final Iterable<LTOBackendArtifacts> allLTOArtifacts; @Nullable private final Artifact paramFile; @Nullable private final Artifact interfaceSoBuilder; @@ -92,7 +84,6 @@ ActionOwner owner, Artifact output, @Nullable Artifact interfaceOutput, - @Nullable Artifact symbolCountsOutput, ImmutableList<Artifact> buildInfoHeaderArtifacts, Iterable<? extends LinkerInput> linkerInputs, Iterable<? extends LinkerInput> runtimeInputs, @@ -107,33 +98,11 @@ boolean nativeDeps, boolean useTestOnlyFlags, boolean needWholeArchive, - @Nullable Iterable<LTOBackendArtifacts> allLTOArtifacts, @Nullable Artifact paramFile, Artifact interfaceSoBuilder, + List<String> noWholeArchiveFlags, CcToolchainFeatures.Variables variables, @Nullable FeatureConfiguration featureConfiguration) { - Preconditions.checkArgument(linkTargetType != LinkTargetType.INTERFACE_DYNAMIC_LIBRARY, - "you can't link an interface dynamic library directly"); - if (linkTargetType != LinkTargetType.DYNAMIC_LIBRARY) { - Preconditions.checkArgument(interfaceOutput == null, - "interface output may only be non-null for dynamic library links"); - } - if (linkTargetType.isStaticLibraryLink()) { - Preconditions.checkArgument(linkstamps.isEmpty(), - "linkstamps may only be present on dynamic library or executable links"); - Preconditions.checkArgument(linkStaticness == LinkStaticness.FULLY_STATIC, - "static library link must be static"); - Preconditions.checkArgument(buildInfoHeaderArtifacts.isEmpty(), - "build info headers may only be present on dynamic library or executable links"); - Preconditions.checkArgument(symbolCountsOutput == null, - "the symbol counts output must be null for static links"); - Preconditions.checkArgument(runtimeSolibDir == null, - "the runtime solib directory must be null for static links"); - Preconditions.checkArgument(!nativeDeps, - "the native deps flag must be false for static links"); - Preconditions.checkArgument(!needWholeArchive, - "the need whole archive flag must be false for static links"); - } this.actionName = actionName; this.configuration = Preconditions.checkNotNull(configuration); @@ -147,7 +116,6 @@ Preconditions.checkNotNull(this.output); } - this.symbolCountsOutput = symbolCountsOutput; this.buildInfoHeaderArtifacts = Preconditions.checkNotNull(buildInfoHeaderArtifacts); this.linkerInputs = Preconditions.checkNotNull(linkerInputs); this.runtimeInputs = Preconditions.checkNotNull(runtimeInputs); @@ -164,16 +132,15 @@ this.runtimeSolibDir = runtimeSolibDir; this.nativeDeps = nativeDeps; this.useTestOnlyFlags = useTestOnlyFlags; - this.needWholeArchive = needWholeArchive; - this.allLTOArtifacts = allLTOArtifacts; this.paramFile = paramFile; + this.noWholeArchiveFlags = noWholeArchiveFlags; // For now, silently ignore interfaceSoBuilder if we don't build an interface dynamic library. this.interfaceSoBuilder = ((linkTargetType == LinkTargetType.DYNAMIC_LIBRARY) && (interfaceOutput != null)) - ? Preconditions.checkNotNull(interfaceSoBuilder, - "cannot build interface dynamic library without builder") - : null; + ? Preconditions.checkNotNull( + interfaceSoBuilder, "cannot build interface dynamic library without builder") + : null; } /** @@ -186,15 +153,6 @@ return interfaceOutput; } - /** - * Returns an artifact containing the number of symbols used per object file passed to the linker. - * This is currently a gold only feature, and is only produced for executables. If another target - * is being linked, or if symbol counts output is disabled, this will be null. - */ - @Nullable public Artifact getSymbolCountsOutput() { - return symbolCountsOutput; - } - @Nullable public Artifact getParamFile() { return paramFile; @@ -278,6 +236,12 @@ return useTestOnlyFlags; } + /** Returns the build variables used to template the crosstool for this linker invocation. */ + @VisibleForTesting + public Variables getBuildVariables() { + return this.variables; + } + /** * Splits the link command-line into a part to be written to a parameter file, and the remaining * actual command line to be executed (which references the parameter file). Should only be used @@ -371,76 +335,110 @@ } } } + + private void addToolchainFlags(List<String> argv) { + boolean fullyStatic = (linkStaticness == LinkStaticness.FULLY_STATIC); + boolean mostlyStatic = (linkStaticness == LinkStaticness.MOSTLY_STATIC); + boolean sharedLinkopts = + linkTargetType == LinkTargetType.DYNAMIC_LIBRARY + || linkopts.contains("-shared") + || cppConfiguration.getLinkOptions().contains("-shared"); + + /* + * For backwards compatibility, linkopts come _after_ inputFiles. + * This is needed to allow linkopts to contain libraries and + * positional library-related options such as + * -Wl,--begin-group -lfoo -lbar -Wl,--end-group + * or + * -Wl,--as-needed -lfoo -Wl,--no-as-needed + * + * As for the relative order of the three different flavours of linkopts + * (global defaults, per-target linkopts, and command-line linkopts), + * we have no idea what the right order should be, or if anyone cares. + */ + argv.addAll(linkopts); + // Extra toolchain link options based on the output's link staticness. + if (fullyStatic) { + argv.addAll(cppConfiguration.getFullyStaticLinkOptions(features, sharedLinkopts)); + } else if (mostlyStatic) { + argv.addAll(cppConfiguration.getMostlyStaticLinkOptions(features, sharedLinkopts)); + } else { + argv.addAll(cppConfiguration.getDynamicLinkOptions(features, sharedLinkopts)); + } + + // Extra test-specific link options. + if (useTestOnlyFlags) { + argv.addAll(cppConfiguration.getTestOnlyLinkOptions()); + } + + argv.addAll(cppConfiguration.getLinkOptions()); + + // -pie is not compatible with shared and should be + // removed when the latter is part of the link command. Should we need to further + // distinguish between shared libraries and executables, we could add additional + // command line / CROSSTOOL flags that distinguish them. But as long as this is + // the only relevant use case we're just special-casing it here. + if (linkTargetType == LinkTargetType.DYNAMIC_LIBRARY) { + Iterables.removeIf(argv, Predicates.equalTo("-pie")); + } + + // Fission mode: debug info is in .dwo files instead of .o files. Inform the linker of this. + if (!linkTargetType.isStaticLibraryLink() && cppConfiguration.useFission()) { + argv.add("-Wl,--gdb-index"); + } + } /** - * Returns a raw link command for the given link invocation, including both command and - * arguments (argv). After any further usage-specific processing, this can be passed to - * {@link #finalizeWithLinkstampCommands} to give the final command line. + * Returns a raw link command for the given link invocation, including both command and arguments + * (argv). After any further usage-specific processing, this can be passed to {@link + * #finalizeWithLinkstampCommands} to give the final command line. * * @return raw link command line. */ public List<String> getRawLinkArgv() { List<String> argv = new ArrayList<>(); - // We create the command line from the feature configuration. If no configuration is present, - // we use hard-coded flags. - if (featureConfiguration.actionIsConfigured(actionName)) { - argv.add(featureConfiguration - .getToolForAction(actionName) - .getToolPath(cppConfiguration.getCrosstoolTopPathFragment()) - .getPathString()); - argv.addAll(featureConfiguration.getCommandLine(actionName, variables)); - } else { - // TODO(b/28928350): In order to simplify the extraction of this logic into the linux - // crosstool, this switch statement should be reframed as a group of action_configs and - // features. Until they can be migrated into the linux crosstool, those features should - // be added to CppConfiguration.addLegacyFeatures(). - switch (linkTargetType) { - case EXECUTABLE: - addCppArgv(argv); - break; + switch (linkTargetType) { + case EXECUTABLE: + argv.add(cppConfiguration.getCppExecutable().getPathString()); + argv.addAll(featureConfiguration.getCommandLine(actionName, variables)); + argv.addAll(noWholeArchiveFlags); + addToolchainFlags(argv); + break; - case DYNAMIC_LIBRARY: - if (interfaceOutput != null) { - argv.add(configuration.getShExecutable().getPathString()); - argv.add("-c"); - argv.add( - "build_iface_so=\"$0\"; impl=\"$1\"; iface=\"$2\"; cmd=\"$3\"; shift 3; " - + "\"$cmd\" \"$@\" && \"$build_iface_so\" \"$impl\" \"$iface\""); - argv.add(interfaceSoBuilder.getExecPathString()); - argv.add(output.getExecPathString()); - argv.add(interfaceOutput.getExecPathString()); - } - addCppArgv(argv); - // -pie is not compatible with -shared and should be - // removed when the latter is part of the link command. Should we need to further - // distinguish between shared libraries and executables, we could add additional - // command line / CROSSTOOL flags that distinguish them. But as long as this is - // the only relevant use case we're just special-casing it here. - Iterables.removeIf(argv, Predicates.equalTo("-pie")); - break; - - case STATIC_LIBRARY: - case PIC_STATIC_LIBRARY: - case ALWAYS_LINK_STATIC_LIBRARY: - case ALWAYS_LINK_PIC_STATIC_LIBRARY: - // The static library link command follows this template: - // ar <cmd> <output_archive> <input_files...> - argv.add(cppConfiguration.getArExecutable().getPathString()); - argv.addAll( - cppConfiguration.getArFlags(cppConfiguration.archiveType() == Link.ArchiveType.THIN)); + case DYNAMIC_LIBRARY: + if (interfaceOutput != null) { + argv.add(configuration.getShExecutable().getPathString()); + argv.add("-c"); + argv.add( + "build_iface_so=\"$0\"; impl=\"$1\"; iface=\"$2\"; cmd=\"$3\"; shift 3; " + + "\"$cmd\" \"$@\" && \"$build_iface_so\" \"$impl\" \"$iface\""); + argv.add(interfaceSoBuilder.getExecPathString()); argv.add(output.getExecPathString()); - addInputFileLinkOptions(argv, /*needWholeArchive=*/ false); - break; + argv.add(interfaceOutput.getExecPathString()); + } + argv.add(cppConfiguration.getCppExecutable().getPathString()); + argv.addAll(featureConfiguration.getCommandLine(actionName, variables)); + argv.addAll(noWholeArchiveFlags); + addToolchainFlags(argv); + break; - default: - throw new IllegalArgumentException(); - } - } + case STATIC_LIBRARY: + case PIC_STATIC_LIBRARY: + case ALWAYS_LINK_STATIC_LIBRARY: + case ALWAYS_LINK_PIC_STATIC_LIBRARY: + // The static library link command follows this template: + // ar <cmd> <output_archive> <input_files...> + argv.add(cppConfiguration.getArExecutable().getPathString()); + argv.addAll( + cppConfiguration.getArFlags(cppConfiguration.archiveType() == Link.ArchiveType.THIN)); + argv.add(output.getExecPathString()); + argv.addAll(featureConfiguration.getCommandLine(actionName, variables)); + argv.addAll(noWholeArchiveFlags); + break; - // Fission mode: debug info is in .dwo files instead of .o files. Inform the linker of this. - if (!linkTargetType.isStaticLibraryLink() && cppConfiguration.useFission()) { - argv.add("-Wl,--gdb-index"); + default: + throw new IllegalArgumentException(); } return argv; @@ -537,21 +535,23 @@ return ImmutableList.copyOf(batchCommand); } } + + private boolean isSharedNativeLibrary() { + return nativeDeps && cppConfiguration.shareNativeDeps(); + } /** - * Computes, for each C++ source file in - * {@link #getLinkstamps}, the command necessary to compile + * Computes, for each C++ source file in {@link #getLinkstamps}, the command necessary to compile * that file such that the output is correctly fed into the link command. * - * <p>As these options (as well as all others) are taken into account when - * computing the action key, they do not directly contain volatile build - * information to avoid unnecessary relinking. Instead this information is - * passed as an additional header generated by - * {@link com.google.devtools.build.lib.rules.cpp.WriteBuildInfoHeaderAction}. + * <p>As these options (as well as all others) are taken into account when computing the action + * key, they do not directly contain volatile build information to avoid unnecessary relinking. + * Instead this information is passed as an additional header generated by {@link + * com.google.devtools.build.lib.rules.cpp.WriteBuildInfoHeaderAction}. * * @param outputPrefix prefix to add before the linkstamp outputs' exec paths - * @return a list of shell-escaped compiler commmands, one for each entry - * in {@link #getLinkstamps} + * @return a list of shell-escaped compiler commmands, one for each entry in {@link + * #getLinkstamps} */ public List<String> getLinkstampCompileCommands(String outputPrefix) { if (linkstamps.isEmpty()) { @@ -626,369 +626,6 @@ } /** - * Determine the arguments to pass to the C++ compiler when linking. - * Add them to the {@code argv} parameter. - */ - private void addCppArgv(List<String> argv) { - argv.add(cppConfiguration.getCppExecutable().getPathString()); - - // When using gold to link an executable, output the number of used and unused symbols. - if (symbolCountsOutput != null) { - argv.add("-Wl,--print-symbol-counts=" + symbolCountsOutput.getExecPathString()); - } - - if (linkTargetType == LinkTargetType.DYNAMIC_LIBRARY) { - argv.add("-shared"); - } - - // Add the outputs of any associated linkstamp compilations. - for (Artifact linkstampOutput : linkstamps.values()) { - argv.add(linkstampOutput.getExecPathString()); - } - - boolean fullyStatic = (linkStaticness == LinkStaticness.FULLY_STATIC); - boolean mostlyStatic = (linkStaticness == LinkStaticness.MOSTLY_STATIC); - boolean sharedLinkopts = - linkTargetType == LinkTargetType.DYNAMIC_LIBRARY - || linkopts.contains("-shared") - || cppConfiguration.getLinkOptions().contains("-shared"); - - if (output != null) { - argv.add("-o"); - String execpath = output.getExecPathString(); - if (mostlyStatic - && linkTargetType == LinkTargetType.EXECUTABLE - && cppConfiguration.skipStaticOutputs()) { - // Linked binary goes to /dev/null; bogus dependency info in its place. - Collections.addAll(argv, "/dev/null", "-MMD", "-MF", execpath); - } else { - argv.add(execpath); - } - } - - addInputFileLinkOptions(argv, needWholeArchive); - - /* - * For backwards compatibility, linkopts come _after_ inputFiles. - * This is needed to allow linkopts to contain libraries and - * positional library-related options such as - * -Wl,--begin-group -lfoo -lbar -Wl,--end-group - * or - * -Wl,--as-needed -lfoo -Wl,--no-as-needed - * - * As for the relative order of the three different flavours of linkopts - * (global defaults, per-target linkopts, and command-line linkopts), - * we have no idea what the right order should be, or if anyone cares. - */ - argv.addAll(linkopts); - - // Extra toolchain link options based on the output's link staticness. - if (fullyStatic) { - argv.addAll(cppConfiguration.getFullyStaticLinkOptions(features, sharedLinkopts)); - } else if (mostlyStatic) { - argv.addAll(cppConfiguration.getMostlyStaticLinkOptions(features, sharedLinkopts)); - } else { - argv.addAll(cppConfiguration.getDynamicLinkOptions(features, sharedLinkopts)); - } - - // Extra test-specific link options. - if (useTestOnlyFlags) { - argv.addAll(cppConfiguration.getTestOnlyLinkOptions()); - } - - if (linkTargetType == LinkTargetType.EXECUTABLE && cppConfiguration.forcePic()) { - argv.add("-pie"); - } - - argv.addAll(cppConfiguration.getLinkOptions()); - // The feature config can be null for tests. - if (featureConfiguration != null) { - argv.addAll(featureConfiguration.getCommandLine(actionName, variables)); - } - } - - private static boolean isDynamicLibrary(LinkerInput linkInput) { - Artifact libraryArtifact = linkInput.getArtifact(); - String name = libraryArtifact.getFilename(); - return Link.SHARED_LIBRARY_FILETYPES.matches(name) && name.startsWith("lib"); - } - - private boolean isSharedNativeLibrary() { - return nativeDeps && cppConfiguration.shareNativeDeps(); - } - - /** - * When linking a shared library fully or mostly static then we need to link in - * *all* dependent files, not just what the shared library needs for its own - * code. This is done by wrapping all objects/libraries with - * -Wl,-whole-archive and -Wl,-no-whole-archive. For this case the - * globalNeedWholeArchive parameter must be set to true. Otherwise only - * library objects (.lo) need to be wrapped with -Wl,-whole-archive and - * -Wl,-no-whole-archive. - */ - private void addInputFileLinkOptions(List<String> argv, boolean globalNeedWholeArchive) { - // The Apple ld doesn't support -whole-archive/-no-whole-archive. It - // does have -all_load/-noall_load, but -all_load is a global setting - // that affects all subsequent files, and -noall_load is simply ignored. - // TODO(bazel-team): Not sure what the implications of this are, other than - // bloated binaries. - boolean macosx = cppConfiguration.getTargetLibc().equals("macosx"); - if (globalNeedWholeArchive) { - argv.add(macosx ? "-Wl,-all_load" : "-Wl,-whole-archive"); - } - - // Used to collect -L and -Wl,-rpath options, ensuring that each used only once. - Set<String> libOpts = new LinkedHashSet<>(); - - // List of command line parameters to link input files (either directly or using -l). - List<String> linkerInputs = new ArrayList<>(); - - // List of command line parameters that need to be placed *outside* of - // --whole-archive ... --no-whole-archive. - List<String> noWholeArchiveInputs = new ArrayList<>(); - - PathFragment solibDir = configuration.getBinDirectory().getExecPath() - .getRelative(cppConfiguration.getSolibDirectory()); - String runtimeSolibName = runtimeSolibDir != null ? runtimeSolibDir.getBaseName() : null; - boolean runtimeRpath = runtimeSolibDir != null - && (linkTargetType == LinkTargetType.DYNAMIC_LIBRARY - || (linkTargetType == LinkTargetType.EXECUTABLE - && linkStaticness == LinkStaticness.DYNAMIC)); - - String rpathRoot = null; - List<String> runtimeRpathEntries = new ArrayList<>(); - - if (output != null) { - String origin = - useTestOnlyFlags && cppConfiguration.supportsExecOrigin() ? "$EXEC_ORIGIN/" : "$ORIGIN/"; - if (runtimeRpath) { - runtimeRpathEntries.add("-Wl,-rpath," + origin + runtimeSolibName + "/"); - } - - // Calculate the correct relative value for the "-rpath" link option (which sets - // the search path for finding shared libraries). - if (isSharedNativeLibrary()) { - // For shared native libraries, special symlinking is applied to ensure C++ - // runtimes are available under $ORIGIN/_solib_[arch]. So we set the RPATH to find - // them. - // - // Note that we have to do this because $ORIGIN points to different paths for - // different targets. In other words, blaze-bin/d1/d2/d3/a_shareddeps.so and - // blaze-bin/d4/b_shareddeps.so have different path depths. The first could - // reference a standard blaze-bin/_solib_[arch] via $ORIGIN/../../../_solib[arch], - // and the second could use $ORIGIN/../_solib_[arch]. But since this is a shared - // artifact, both are symlinks to the same place, so - // there's no *one* RPATH setting that fits all targets involved in the sharing. - rpathRoot = "-Wl,-rpath," + origin + ":" - + origin + cppConfiguration.getSolibDirectory() + "/"; - if (runtimeRpath) { - runtimeRpathEntries.add("-Wl,-rpath," + origin + "../" + runtimeSolibName + "/"); - } - } else { - // For all other links, calculate the relative path from the output file to _solib_[arch] - // (the directory where all shared libraries are stored, which resides under the blaze-bin - // directory. In other words, given blaze-bin/my/package/binary, rpathRoot would be - // "../../_solib_[arch]". - if (runtimeRpath) { - runtimeRpathEntries.add("-Wl,-rpath," + origin - + Strings.repeat("../", output.getRootRelativePath().segmentCount() - 1) - + runtimeSolibName + "/"); - } - - rpathRoot = "-Wl,-rpath," - + origin + Strings.repeat("../", output.getRootRelativePath().segmentCount() - 1) - + cppConfiguration.getSolibDirectory() + "/"; - - if (nativeDeps) { - // We also retain the $ORIGIN/ path to solibs that are in _solib_<arch>, as opposed to - // the package directory) - if (runtimeRpath) { - runtimeRpathEntries.add("-Wl,-rpath," + origin + "../" + runtimeSolibName + "/"); - } - rpathRoot += ":" + origin; - } - } - } - - boolean includeSolibDir = false; - - - Map<Artifact, Artifact> ltoMap = null; - if (allLTOArtifacts != null) { - // TODO(bazel-team): The LTO final link can only work if there are individual .o files on the - // command line. Rather than crashing, this should issue a nice error. We will get this by - // 1) moving supports_start_end_lib to a toolchain feature - // 2) having thin_lto require start_end_lib - // As a bonus, we can rephrase --nostart_end_lib as --features=-start_end_lib and get rid - // of a command line option. - - Preconditions.checkState(cppConfiguration.useStartEndLib()); - ltoMap = new HashMap<>(); - for (LTOBackendArtifacts l : allLTOArtifacts) { - ltoMap.put(l.getBitcodeFile(), l.getObjectFile()); - } - } - - for (LinkerInput input : getLinkerInputs()) { - if (isDynamicLibrary(input)) { - PathFragment libDir = input.getArtifact().getExecPath().getParentDirectory(); - Preconditions.checkState( - libDir.startsWith(solibDir), - "Artifact '%s' is not under directory '%s'.", input.getArtifact(), solibDir); - if (libDir.equals(solibDir)) { - includeSolibDir = true; - } - addDynamicInputLinkOptions(input, linkerInputs, libOpts, solibDir, rpathRoot); - } else { - addStaticInputLinkOptions(input, linkerInputs, ltoMap); - } - } - - boolean includeRuntimeSolibDir = false; - - for (LinkerInput input : runtimeInputs) { - List<String> optionsList = globalNeedWholeArchive - ? noWholeArchiveInputs - : linkerInputs; - - if (isDynamicLibrary(input)) { - PathFragment libDir = input.getArtifact().getExecPath().getParentDirectory(); - Preconditions.checkState(runtimeSolibDir != null && libDir.equals(runtimeSolibDir), - "Artifact '%s' is not under directory '%s'.", input.getArtifact(), solibDir); - includeRuntimeSolibDir = true; - addDynamicInputLinkOptions(input, optionsList, libOpts, solibDir, rpathRoot); - } else { - addStaticInputLinkOptions(input, optionsList, ltoMap); - } - } - - // rpath ordering matters for performance; first add the one where most libraries are found. - if (includeSolibDir && rpathRoot != null) { - argv.add(rpathRoot); - } - if (includeRuntimeSolibDir) { - argv.addAll(runtimeRpathEntries); - } - argv.addAll(libOpts); - - // Need to wrap static libraries with whole-archive option - for (String option : linkerInputs) { - if (!globalNeedWholeArchive && Link.LINK_LIBRARY_FILETYPES.matches(option)) { - if (macosx) { - argv.add("-Wl,-force_load," + option); - } else { - argv.add("-Wl,-whole-archive"); - argv.add(option); - argv.add("-Wl,-no-whole-archive"); - } - } else { - argv.add(option); - } - } - - if (globalNeedWholeArchive) { - argv.add(macosx ? "-Wl,-noall_load" : "-Wl,-no-whole-archive"); - argv.addAll(noWholeArchiveInputs); - } - - if (ltoMap != null) { - Preconditions.checkState( - ltoMap.size() == 0, "Still have LTO objects left: %s, command-line: %s", ltoMap, argv); - } - } - - /** - * Adds command-line options for a dynamic library input file into - * options and libOpts. - */ - private void addDynamicInputLinkOptions(LinkerInput input, List<String> options, - Set<String> libOpts, PathFragment solibDir, String rpathRoot) { - Preconditions.checkState(isDynamicLibrary(input)); - Preconditions.checkState( - !Link.useStartEndLib(input, cppConfiguration.archiveType())); - - Artifact inputArtifact = input.getArtifact(); - PathFragment libDir = inputArtifact.getExecPath().getParentDirectory(); - if (rpathRoot != null - && !libDir.equals(solibDir) - && (runtimeSolibDir == null || !runtimeSolibDir.equals(libDir))) { - String dotdots = ""; - PathFragment commonParent = solibDir; - while (!libDir.startsWith(commonParent)) { - dotdots += "../"; - commonParent = commonParent.getParentDirectory(); - } - - libOpts.add(rpathRoot + dotdots + libDir.relativeTo(commonParent).getPathString()); - } - - libOpts.add("-L" + inputArtifact.getExecPath().getParentDirectory().getPathString()); - - String name = inputArtifact.getFilename(); - if (CppFileTypes.SHARED_LIBRARY.matches(name)) { - String libName = name.replaceAll("(^lib|\\.(so|dylib)$)", ""); - options.add("-l" + libName); - } else { - // Interface shared objects have a non-standard extension - // that the linker won't be able to find. So use the - // filename directly rather than a -l option. Since the - // library has an SONAME attribute, this will work fine. - options.add(inputArtifact.getExecPathString()); - } - } - - /** - * Adds command-line options for a static library or non-library input - * into options. - * - * @param ltoMap is a mutable list of exec paths that should be on the command-line, which - * must be supplied for LTO final links. - */ - private void addStaticInputLinkOptions( - LinkerInput input, List<String> options, @Nullable Map<Artifact, Artifact> ltoMap) { - Preconditions.checkState(!isDynamicLibrary(input)); - - // start-lib/end-lib library: adds its input object files. - if (Link.useStartEndLib(input, cppConfiguration.archiveType())) { - Iterable<Artifact> archiveMembers = input.getObjectFiles(); - if (!Iterables.isEmpty(archiveMembers)) { - options.add("-Wl,--start-lib"); - for (Artifact member : archiveMembers) { - if (ltoMap != null) { - Artifact backend = ltoMap.remove(member); - - if (backend != null) { - // If the backend artifact is missing, we can't print a warning because this may - // happen normally, due libraries that list .o files explicitly, or generate .o - // files from assembler. - member = backend; - } - } - - options.add(member.getExecPathString()); - } - options.add("-Wl,--end-lib"); - } - } else { - // For anything else, add the input directly. - Artifact inputArtifact = input.getArtifact(); - - if (ltoMap != null) { - Artifact ltoArtifact = ltoMap.remove(inputArtifact); - if (ltoArtifact != null) { - inputArtifact = ltoArtifact; - } - } - - if (input.isFake()) { - options.add(Link.FAKE_OBJECT_PREFIX + inputArtifact.getExecPathString()); - } else { - options.add(inputArtifact.getExecPathString()); - } - } - } - - /** * A builder for a {@link LinkCommandLine}. */ public static final class Builder { @@ -1013,10 +650,8 @@ private final ActionOwner owner; @Nullable private final RuleContext ruleContext; - private String actionName; @Nullable private Artifact output; @Nullable private Artifact interfaceOutput; - @Nullable private Artifact symbolCountsOutput; private ImmutableList<Artifact> buildInfoHeaderArtifacts = ImmutableList.of(); private Iterable<? extends LinkerInput> linkerInputs = ImmutableList.of(); private Iterable<? extends LinkerInput> runtimeInputs = ImmutableList.of(); @@ -1030,17 +665,18 @@ private boolean nativeDeps; private boolean useTestOnlyFlags; private boolean needWholeArchive; - @Nullable private Iterable<LTOBackendArtifacts> allLTOBackendArtifacts; @Nullable private Artifact paramFile; @Nullable private Artifact interfaceSoBuilder; @Nullable private CcToolchainProvider toolchain; + private Variables variables; private FeatureConfiguration featureConfiguration; + private List<String> noWholeArchiveFlags = ImmutableList.of(); // This interface is needed to support tests that don't create a // ruleContext, in which case the configuration and action owner // cannot be accessed off of the give ruleContext. - public Builder(BuildConfiguration configuration, ActionOwner owner, - @Nullable RuleContext ruleContext) { + public Builder( + BuildConfiguration configuration, ActionOwner owner, @Nullable RuleContext ruleContext) { this.configuration = configuration; this.owner = owner; this.ruleContext = ruleContext; @@ -1051,6 +687,16 @@ } public LinkCommandLine build() { + + if (linkTargetType.isStaticLibraryLink()) { + Preconditions.checkArgument( + linkstamps.isEmpty(), + "linkstamps may only be present on dynamic library or executable links"); + Preconditions.checkArgument( + buildInfoHeaderArtifacts.isEmpty(), + "build info headers may only be present on dynamic library or executable links"); + } + ImmutableList<String> actualLinkstampCompileOptions; if (linkstampCompileOptions.isEmpty()) { actualLinkstampCompileOptions = DEFAULT_LINKSTAMP_OPTIONS; @@ -1058,7 +704,7 @@ actualLinkstampCompileOptions = ImmutableList.copyOf( Iterables.concat(DEFAULT_LINKSTAMP_OPTIONS, linkstampCompileOptions)); } - CcToolchainFeatures.Variables variables = null; + // The ruleContext can be null for some tests. if (ruleContext != null) { if (featureConfiguration == null) { @@ -1070,18 +716,20 @@ featureConfiguration = CcCommon.configureFeatures(ruleContext); } } - CcToolchainFeatures.Variables.Builder buildVariables = - new CcToolchainFeatures.Variables.Builder(); - CppHelper.getFdoSupport(ruleContext).getLinkOptions(featureConfiguration, buildVariables); - variables = buildVariables.build(); } + + if (variables == null) { + variables = Variables.EMPTY; + } + + String actionName = linkTargetType.getActionName(); + return new LinkCommandLine( actionName, configuration, owner, output, interfaceOutput, - symbolCountsOutput, buildInfoHeaderArtifacts, linkerInputs, runtimeInputs, @@ -1096,22 +744,14 @@ nativeDeps, useTestOnlyFlags, needWholeArchive, - allLTOBackendArtifacts, paramFile, interfaceSoBuilder, + noWholeArchiveFlags, variables, featureConfiguration); } /** - * Sets the name of the action for the purposes of querying the crosstool. - */ - public Builder setActionName(String actionName) { - this.actionName = actionName; - return this; - } - - /** * Sets the toolchain to use for link flags. If this is not called, the toolchain * is retrieved from the rule. */ @@ -1174,19 +814,9 @@ } /** - * Sets an additional output artifact that contains symbol counts. The {@link #build} method - * throws an exception if this is non-null for a static link (see - * {@link LinkTargetType#isStaticLibraryLink}). - */ - public Builder setSymbolCountsOutput(Artifact symbolCountsOutput) { - this.symbolCountsOutput = symbolCountsOutput; - return this; - } - - /** * Sets the linker options. These are passed to the linker in addition to the other linker - * options like linker inputs, symbol count options, etc. The {@link #build} method - * throws an exception if the linker options are non-empty for a static link (see {@link + * options like linker inputs, symbol count options, etc. The {@link #build} method throws an + * exception if the linker options are non-empty for a static link (see {@link * LinkTargetType#isStaticLibraryLink}). */ public Builder setLinkopts(ImmutableList<String> linkopts) { @@ -1253,16 +883,6 @@ } /** - * Sets the directory of the dynamic runtime libraries, which is added to the rpath. The {@link - * #build} method throws an exception if the runtime dir is non-null for a static link (see - * {@link LinkTargetType#isStaticLibraryLink}). - */ - public Builder setRuntimeSolibDir(PathFragment runtimeSolibDir) { - this.runtimeSolibDir = runtimeSolibDir; - return this; - } - - /** * Whether the resulting library is intended to be used as a native library from another * programming language. This influences the rpath. The {@link #build} method throws an * exception if this is true for a static link (see {@link LinkTargetType#isStaticLibraryLink}). @@ -1281,18 +901,28 @@ return this; } - public Builder setNeedWholeArchive(boolean needWholeArchive) { - this.needWholeArchive = needWholeArchive; - return this; - } - public Builder setParamFile(Artifact paramFile) { this.paramFile = paramFile; return this; } + + public Builder setBuildVariables(Variables variables) { + this.variables = variables; + return this; + } - public Builder setAllLTOArtifacts(Iterable<LTOBackendArtifacts> allLTOArtifacts) { - this.allLTOBackendArtifacts = allLTOArtifacts; + public Builder setRuntimeSolibDir(PathFragment runtimeSolibDir) { + this.runtimeSolibDir = runtimeSolibDir; + return this; + } + + /** + * Set flags that should not be in a --whole_archive block. + * + * <p>TODO(b/30228443): Refactor into action_configs. + */ + public Builder setNoWholeArchiveFlags(List<String> noWholeArchiveFlags) { + this.noWholeArchiveFlags = noWholeArchiveFlags; return this; } }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java index c4ea2be..7531632 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
@@ -17,8 +17,8 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; -import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Action; @@ -34,18 +34,17 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; +import com.google.devtools.build.lib.rules.cpp.CppLinkActionConfigs.CppLinkPlatform; import com.google.devtools.build.lib.rules.cpp.Link.LinkStaticness; import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink; import com.google.devtools.build.lib.vfs.PathFragment; - +import java.io.IOException; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** - * Tests for {@link CppLinkAction}. - */ +/** Tests for {@link CppLinkAction}. */ @RunWith(JUnit4.class) public class CppLinkActionTest extends BuildViewTestCase { private RuleContext createDummyRuleContext() throws Exception { @@ -66,6 +65,18 @@ } }, masterConfig); } + + private final FeatureConfiguration getMockFeatureConfiguration() throws Exception { + return CcToolchainFeaturesTest.buildFeatures( + CppLinkActionConfigs.getCppLinkActionConfigs(CppLinkPlatform.LINUX)) + .getFeatureConfiguration( + Link.LinkTargetType.EXECUTABLE.getActionName(), + Link.LinkTargetType.DYNAMIC_LIBRARY.getActionName(), + Link.LinkTargetType.STATIC_LIBRARY.getActionName(), + Link.LinkTargetType.PIC_STATIC_LIBRARY.getActionName(), + Link.LinkTargetType.ALWAYS_LINK_STATIC_LIBRARY.getActionName(), + Link.LinkTargetType.ALWAYS_LINK_PIC_STATIC_LIBRARY.getActionName()); + } @Test public void testToolchainFeatureFlags() throws Exception { @@ -93,9 +104,9 @@ "out", ImmutableList.<Artifact>of(), ImmutableList.<LibraryToLink>of(), - featureConfiguration) + featureConfiguration, + false) .build(); - assertThat(Joiner.on(" ").join(linkAction.getArgv())).contains("mock_tool"); assertThat(linkAction.getArgv()).contains("some_flag"); } @@ -118,7 +129,8 @@ "out", ImmutableList.<Artifact>of(), ImmutableList.<LibraryToLink>of(), - featureConfiguration) + featureConfiguration, + false) .build(); assertThat(linkAction.getEnvironment()).containsEntry("foo", "bar"); } @@ -135,6 +147,8 @@ final Artifact oFile = getSourceArtifact("cc/a.o"); final Artifact oFile2 = getSourceArtifact("cc/a2.o"); final Artifact interfaceSoBuilder = getBinArtifactWithNoOwner("foo/build_interface_so"); + final FeatureConfiguration featureConfiguration = getMockFeatureConfiguration(); + ActionTester.runTest( 128, new ActionCombinationFactory() { @@ -158,7 +172,7 @@ builder.setWholeArchive((i & 16) == 0); builder.setFake((i & 32) == 0); builder.setRuntimeSolibDir((i & 64) == 0 ? null : new PathFragment("so1")); - builder.setFeatureConfiguration(new FeatureConfiguration()); + builder.setFeatureConfiguration(featureConfiguration); return builder.build(); } @@ -177,6 +191,8 @@ final Artifact oFile = getSourceArtifact("cc/a.o"); final Artifact oFile2 = getSourceArtifact("cc/a2.o"); final Artifact interfaceSoBuilder = getBinArtifactWithNoOwner("foo/build_interface_so"); + final FeatureConfiguration featureConfiguration = getMockFeatureConfiguration(); + ActionTester.runTest( 4, new ActionCombinationFactory() { @@ -194,6 +210,7 @@ (i & 1) == 0 ? ImmutableList.of(oFile) : ImmutableList.of(oFile2)); builder.setLinkType( (i & 2) == 0 ? LinkTargetType.STATIC_LIBRARY : LinkTargetType.DYNAMIC_LIBRARY); + builder.setFeatureConfiguration(featureConfiguration); return builder.build(); } }); @@ -254,7 +271,8 @@ "binary2", objects.build(), ImmutableList.<LibraryToLink>of(), - new FeatureConfiguration()) + new FeatureConfiguration(), + false) .setFake(true) .build(); @@ -285,7 +303,8 @@ String outputPath, Iterable<Artifact> nonLibraryInputs, ImmutableList<LibraryToLink> libraryInputs, - FeatureConfiguration featureConfiguration) + FeatureConfiguration featureConfiguration, + boolean shouldIncludeToolchain) throws Exception { RuleContext ruleContext = createDummyRuleContext(); CppLinkActionBuilder builder = @@ -294,7 +313,7 @@ new Artifact( new PathFragment(outputPath), getTargetConfiguration().getBinDirectory()), ruleContext.getConfiguration(), - null) + shouldIncludeToolchain ? CppHelper.getToolchain(ruleContext) : null) .addNonLibraryInputs(nonLibraryInputs) .addLibraries(NestedSetBuilder.wrap(Order.LINK_ORDER, libraryInputs)) .setLinkType(type) @@ -306,6 +325,17 @@ .setFeatureConfiguration(featureConfiguration); return builder; } + + private CppLinkActionBuilder createLinkBuilder(Link.LinkTargetType type) throws Exception { + PathFragment output = new PathFragment("dummyRuleContext/output/path.xyz"); + return createLinkBuilder( + type, + output.getPathString(), + ImmutableList.<Artifact>of(), + ImmutableList.<LibraryToLink>of(), + new FeatureConfiguration(), + true); + } public Artifact getOutputArtifact(String relpath) { return new Artifact( @@ -313,4 +343,70 @@ getTargetConfiguration().getBinDirectory(), getTargetConfiguration().getBinFragment().getRelative(relpath)); } + + private Artifact scratchArtifact(String s) { + try { + return new Artifact( + scratch.overwriteFile(outputBase.getRelative("WORKSPACE").getRelative(s).toString()), + Root.asDerivedRoot(scratch.dir(outputBase.getRelative("WORKSPACE").toString()))); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + private void assertError(String expectedSubstring, CppLinkActionBuilder builder) { + try { + builder.build(); + fail(); + } catch (RuntimeException e) { + assertThat(e.getMessage()).contains(expectedSubstring); + } + } + + @Test + public void testInterfaceOutputWithoutBuildingDynamicLibraryIsError() throws Exception { + CppLinkActionBuilder builder = + createLinkBuilder(LinkTargetType.EXECUTABLE) + .setInterfaceOutput(scratchArtifact("FakeInterfaceOutput")); + + assertError("Interface output can only be used with non-fake DYNAMIC_LIBRARY targets", builder); + } + + @Test + public void testStaticLinkWithDynamicIsError() throws Exception { + CppLinkActionBuilder builder = + createLinkBuilder(LinkTargetType.STATIC_LIBRARY).setLinkStaticness(LinkStaticness.DYNAMIC); + + assertError("static library link must be static", builder); + } + + @Test + public void testStaticLinkWithSymbolsCountOutputIsError() throws Exception { + CppLinkActionBuilder builder = + createLinkBuilder(LinkTargetType.STATIC_LIBRARY) + .setLinkStaticness(LinkStaticness.FULLY_STATIC) + .setSymbolCountsOutput(scratchArtifact("dummySymbolCounts")); + + assertError("the symbol counts output must be null for static links", builder); + } + + @Test + public void testStaticLinkWithNativeDepsIsError() throws Exception { + CppLinkActionBuilder builder = + createLinkBuilder(LinkTargetType.STATIC_LIBRARY) + .setLinkStaticness(LinkStaticness.FULLY_STATIC) + .setNativeDeps(true); + + assertError("the native deps flag must be false for static links", builder); + } + + @Test + public void testStaticLinkWithWholeArchiveIsError() throws Exception { + CppLinkActionBuilder builder = + createLinkBuilder(LinkTargetType.STATIC_LIBRARY) + .setLinkStaticness(LinkStaticness.FULLY_STATIC) + .setWholeArchive(true); + + assertError("the need whole archive flag must be false for static links", builder); + } }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java new file mode 100644 index 0000000..50fdeba --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java
@@ -0,0 +1,134 @@ +// Copyright 2016 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.rules.cpp; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.analysis.ConfiguredTarget; +import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; +import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables; +import java.util.List; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests that {@code CppLinkAction} is populated with the correct build variables. */ +@RunWith(JUnit4.class) +public class LinkBuildVariablesTest extends BuildViewTestCase { + + private CppLinkAction getCppLinkAction(ConfiguredTarget target, Link.LinkTargetType type) { + Artifact linkerOutput = null; + switch (type) { + case STATIC_LIBRARY: + case ALWAYS_LINK_STATIC_LIBRARY: + linkerOutput = getBinArtifact("lib" + target.getLabel().getName() + ".a", target); + break; + case PIC_STATIC_LIBRARY: + case ALWAYS_LINK_PIC_STATIC_LIBRARY: + linkerOutput = getBinArtifact("lib" + target.getLabel().getName() + "pic.a", target); + break; + case DYNAMIC_LIBRARY: + linkerOutput = getBinArtifact("lib" + target.getLabel().getName() + ".so", target); + break; + case EXECUTABLE: + linkerOutput = getExecutable(target); + break; + default: + throw new IllegalArgumentException( + String.format("Cannot get CppLinkAction for link type %s", type)); + } + return (CppLinkAction) getGeneratingAction(linkerOutput); + } + + private Variables getLinkBuildVariables(ConfiguredTarget target, Link.LinkTargetType type) { + return getCppLinkAction(target, type).getLinkCommandLine().getBuildVariables(); + } + + private List<String> getVariableValue(Variables variables, String variable) throws Exception { + FeatureConfiguration mockFeatureConfiguration = + CcToolchainFeaturesTest.buildFeatures( + "feature {", + " name: 'a'", + " flag_set {", + " action: 'foo'", + " flag_group {", + " flag: '%{" + variable + "}'", + " }", + " }", + "}") + .getFeatureConfiguration("a"); + return mockFeatureConfiguration.getCommandLine("foo", variables); + } + + @Test + public void testLinkstampBuildVariable() throws Exception { + scratch.file( + "x/BUILD", + "cc_binary(", + " name = 'bin',", + " srcs = ['a.cc'],", + " deps = [':lib'],", + ")", + "cc_library(", + " name = 'lib',", + " srcs = ['b.cc'],", + " linkstamp = 'c.cc',", + ")"); + scratch.file("x/a.cc"); + scratch.file("x/b.cc"); + scratch.file("x/c.cc"); + + ConfiguredTarget target = getConfiguredTarget("//x:bin"); + Variables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE); + List<String> variableValue = + getVariableValue(variables, CppLinkActionBuilder.LINKSTAMP_PATHS_VARIABLE); + assertThat(Iterables.getOnlyElement(variableValue)).contains("c.o"); + } + + @Test + public void testForcePicBuildVariable() throws Exception { + useConfiguration("--force_pic"); + scratch.file("x/BUILD", "cc_binary(", " name = 'bin',", " srcs = ['a.cc'],", ")"); + scratch.file("x/a.cc"); + + ConfiguredTarget target = getConfiguredTarget("//x:bin"); + Variables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE); + List<String> variableValue = + getVariableValue(variables, CppLinkActionBuilder.FORCE_PIC_VARIABLE); + assertThat(variableValue).contains(""); + } + + @Test + public void testWholeArchiveBuildVariables() throws Exception { + scratch.file( + "x/BUILD", + "cc_binary(", + " name = 'bin.so',", + " srcs = ['a.cc'],", + " linkopts = ['-shared'],", + " linkstatic = 1", + ")"); + scratch.file("x/a.cc"); + + ConfiguredTarget target = getConfiguredTarget("//x:bin.so"); + Variables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE); + List<String> variableValue = + getVariableValue(variables, CppLinkActionBuilder.GLOBAL_WHOLE_ARCHIVE_VARIABLE); + assertThat(variableValue).contains(""); + } +}