From a51b436f4290a79f77157b9e1f2f2e7b26283a5c Mon Sep 17 00:00:00 2001 From: hlopko Date: Tue, 22 May 2018 09:27:41 -0700 Subject: Split user_link_flags from legacy_link_flags The difference between them is that user_link_flags will stay after we remove legacy fields from the crosstool. This is an encore of https://github.com/bazelbuild/bazel/commit/2661abb96b1fe51fb726a63eb08698564a82eb20 after mitigating the memory regression. Original cl regressed 12mb (on a big enough internal project :): objsize chg instances space KB class name ------------------------------------------------------ 24 +0 190265 4459 KB com.google.common.collect.ImmutableMapEntry 40 +0 95154 3716 KB com.google.common.collect.RegularImmutableMap 47 -2 95154 2230 KB [Ljava.util.Map$Entry; 44 -1 95154 2230 KB [Lcom.google.common.collect.ImmutableMapEntry; 24 +0 95149 2230 KB com.google.devtools.build.lib.rules.cpp.CcToolchainVariables$MapVariables 16 +0 95149 1486 KB com.google.devtools.build.lib.rules.cpp.CcToolchainVariables$StringSequence 89 +0 3188 176 KB [B 64 -8 0 -743 KB com.google.devtools.build.lib.rules.cpp.LinkCommandLine 76 +0 -1918 -1323 KB [Ljava.lang.Object; 24 +0 -95149 -2230 KB com.google.devtools.build.lib.rules.cpp.CcToolchainVariables$SingleVariables ------------------------------------------------------ The reason was that before legacy_link_flags were the only build variable patched because of thinlto in CppLinkActionBuilder, so it used optimized SingleVariables subclass. With the split, the patched variables instance had 2 variables therefore it received the full blown Variables instance. That accounted for the ~7mb. The fix was to move the patching logic to the initial link variables creation and not to create patched variables at all. Now the regression is ~4.8mb, which is perfectly expected since I introduce another variable and this is the overhead of additional hashmap entry: objsize chg instances space KB class name ------------------------------------------------------ 24 +0 190418 4462 KB com.google.common.collect.ImmutableMapEntry 44 +1 31 1487 KB [Lcom.google.common.collect.ImmutableMapEntry; 16 +0 95149 1486 KB com.google.devtools.build.lib.rules.cpp.CcToolchainVariables$StringSequence 47 +0 31 744 KB [Ljava.util.Map$Entry; 89 +0 5723 315 KB [B 24 +0 5721 134 KB java.lang.String 64 -8 0 -743 KB com.google.devtools.build.lib.rules.cpp.LinkCommandLine 76 +0 -2387 -840 KB [Ljava.lang.Object; 24 +0 -95149 -2230 KB com.google.devtools.build.lib.rules.cpp.CcToolchainVariables$SingleVariables ------------------------------------------------------ I'll pay this dept back once we get rid of legacy crosstool fields (= removal of legacy_link_flags variable). RELNOTES: None. PiperOrigin-RevId: 197574153 --- .../build/lib/rules/cpp/CppActionConfigs.java | 18 ++++ .../build/lib/rules/cpp/CppCompileAction.java | 1 + .../build/lib/rules/cpp/CppLinkActionBuilder.java | 103 ++---------------- .../google/devtools/build/lib/rules/cpp/Link.java | 42 +++++--- .../build/lib/rules/cpp/LinkBuildVariables.java | 116 ++++++++++++++++++++- .../build/lib/rules/cpp/LinkCommandLine.java | 25 ++--- 6 files changed, 178 insertions(+), 127 deletions(-) (limited to 'src/main/java/com/google') diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java index efd741a6c3..36d754e61e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java @@ -462,6 +462,7 @@ public class CppActionConfigs { " implies: 'library_search_directories'", " implies: 'libraries_to_link'", " implies: 'force_pic_flags'", + " implies: 'user_link_flags'", " implies: 'legacy_link_flags'", " implies: 'linker_param_file'", " implies: 'fission_support'", @@ -485,6 +486,7 @@ public class CppActionConfigs { " implies: 'runtime_library_search_directories'", " implies: 'library_search_directories'", " implies: 'libraries_to_link'", + " implies: 'user_link_flags'", " implies: 'legacy_link_flags'", " implies: 'linker_param_file'", " implies: 'fission_support'", @@ -508,6 +510,7 @@ public class CppActionConfigs { " implies: 'runtime_library_search_directories'", " implies: 'library_search_directories'", " implies: 'libraries_to_link'", + " implies: 'user_link_flags'", " implies: 'legacy_link_flags'", " implies: 'linker_param_file'", " implies: 'fission_support'", @@ -853,6 +856,21 @@ public class CppActionConfigs { " }", " }", "}"), + ifTrue( + !existingFeatureNames.contains("user_link_flags"), + "feature {", + " name: 'user_link_flags'", + " flag_set {", + " expand_if_all_available: 'user_link_flags'", + " action: 'c++-link-executable'", + " action: 'c++-link-dynamic-library'", + " action: 'c++-link-nodeps-dynamic-library'", + " flag_group {", + " iterate_over: 'user_link_flags'", + " flag: '%{user_link_flags}'", + " }", + " }", + "}"), ifTrue( !existingFeatureNames.contains("legacy_link_flags"), "feature {", diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index 8e3747ed5c..aa89b5ae4e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -87,6 +87,7 @@ import javax.annotation.Nullable; @ThreadCompatible public class CppCompileAction extends AbstractAction implements IncludeScannable, ExecutionInfoSpecifier, CommandAction { + private static final PathFragment BUILD_PATH_FRAGMENT = PathFragment.create("BUILD"); private static final int VALIDATION_DEBUG = 0; // 0==none, 1==warns/errors, 2==all 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 24167f01f2..1cf137f693 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 @@ -18,11 +18,9 @@ import static java.nio.charset.StandardCharsets.ISO_8859_1; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; -import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; import com.google.common.collect.Sets; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionOwner; @@ -51,6 +49,7 @@ import com.google.devtools.build.lib.rules.cpp.CppLinkAction.LinkArtifactFactory import com.google.devtools.build.lib.rules.cpp.LibrariesToLinkCollector.CollectedLibrariesToLink; import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; import com.google.devtools.build.lib.rules.cpp.Link.LinkerOrArchiver; +import com.google.devtools.build.lib.rules.cpp.Link.LinkingMode; import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -674,74 +673,6 @@ public class CppLinkActionBuilder { } } - private ImmutableList getToolchainFlags(List linkopts) { - if (LinkerOrArchiver.ARCHIVER.equals(linkType.linkerOrArchiver())) { - return ImmutableList.of(); - } - boolean fullyStatic = (linkingMode == Link.LinkingMode.LEGACY_FULLY_STATIC); - boolean mostlyStatic = (linkingMode == Link.LinkingMode.STATIC); - boolean sharedLinkopts = - linkType == LinkTargetType.DYNAMIC_LIBRARY - || linkopts.contains("-shared") - || cppConfiguration.hasSharedLinkOption(); - - List result = new ArrayList<>(); - - /* - * 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. - */ - result.addAll(linkopts); - // Extra toolchain link options based on the output's link staticness. - if (fullyStatic) { - result.addAll( - CppHelper.getFullyStaticLinkOptions(cppConfiguration, toolchain, sharedLinkopts)); - } else if (mostlyStatic) { - if (!featureConfiguration.isEnabled(CppRuleClasses.STATIC_LINKING_MODE)) { - result.addAll( - CppHelper.getMostlyStaticLinkOptions( - cppConfiguration, - toolchain, - sharedLinkopts, - featureConfiguration.isEnabled(CppRuleClasses.STATIC_LINK_CPP_RUNTIMES))); - } else { - result.addAll(toolchain.getLegacyLinkOptions()); - } - } else { - if (!featureConfiguration.isEnabled(CppRuleClasses.DYNAMIC_LINKING_MODE)) { - result.addAll(CppHelper.getDynamicLinkOptions(cppConfiguration, toolchain, sharedLinkopts)); - } else { - result.addAll(toolchain.getLegacyLinkOptions()); - } - } - - // Extra test-specific link options. - if (useTestOnlyFlags) { - result.addAll(toolchain.getTestOnlyLinkOptions()); - } - - result.addAll(toolchain.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 (linkType == LinkTargetType.DYNAMIC_LIBRARY) { - Iterables.removeIf(result, Predicates.equalTo("-pie")); - } - - return ImmutableList.copyOf(result); - } - /** Builds the Action as configured and returns it. */ public CppLinkAction build() throws InterruptedException { // Executable links do not have library identifiers. @@ -999,6 +930,7 @@ public class CppLinkActionBuilder { getLinkType().linkerOrArchiver().equals(LinkerOrArchiver.LINKER), configuration, output, + linkType.equals(LinkTargetType.DYNAMIC_LIBRARY), paramFile, thinltoParamFile, thinltoMergedObjectFile, @@ -1008,6 +940,7 @@ public class CppLinkActionBuilder { featureConfiguration, useTestOnlyFlags, isLtoIndexing, + ImmutableList.copyOf(linkopts), toolchain.getInterfaceSoBuilder(), interfaceOutput, ltoOutputRootPrefix, @@ -1015,7 +948,9 @@ public class CppLinkActionBuilder { fdoSupport, collectedLibrariesToLink.getRuntimeLibrarySearchDirectories(), collectedLibrariesToLink.getLibrariesToLink(), - collectedLibrariesToLink.getLibrarySearchDirectories()); + collectedLibrariesToLink.getLibrarySearchDirectories(), + linkingMode.equals(LinkingMode.LEGACY_FULLY_STATIC), + linkingMode.equals(LinkingMode.STATIC)); buildVariablesBuilder.addAllNonTransitive(variables); } catch (EvalException e) { ruleContext.ruleError(e.getLocalizedMessage()); @@ -1070,35 +1005,11 @@ public class CppLinkActionBuilder { toolchain.getLinkDynamicLibraryTool().getExecPathString()); } - ImmutableList linkoptsForVariables; if (!isLtoIndexing) { - linkoptsForVariables = ImmutableList.copyOf(linkopts); linkCommandLineBuilder.setBuildInfoHeaderArtifacts(buildInfoHeaderArtifacts); - - } else { - List opts = new ArrayList<>(linkopts); - opts.addAll( - featureConfiguration.getCommandLine( - "lto-indexing", buildVariables, /* expander= */ null)); - opts.addAll(cppConfiguration.getLtoIndexOptions()); - linkoptsForVariables = ImmutableList.copyOf(opts); } - // For now, silently ignore linkopts if this is a static library - linkoptsForVariables = - linkType.linkerOrArchiver() == LinkerOrArchiver.ARCHIVER - ? ImmutableList.of() - : linkoptsForVariables; - linkCommandLineBuilder.setLinkopts(linkoptsForVariables); - - CcToolchainVariables patchedVariables = - new CcToolchainVariables.Builder(buildVariables) - .addStringSequenceVariable( - LinkBuildVariables.LEGACY_LINK_FLAGS.getVariableName(), - getToolchainFlags(linkoptsForVariables)) - .build(); - - linkCommandLineBuilder.setBuildVariables(patchedVariables); + linkCommandLineBuilder.setBuildVariables(buildVariables); LinkCommandLine linkCommandLine = linkCommandLineBuilder.build(); // Compute the set of inputs - we only need stable order here. 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 42d13bbb66..c32a701207 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 @@ -28,6 +28,24 @@ import com.google.devtools.build.lib.util.FileTypeSet; */ public abstract class Link { + /** Name of the action producing static library. */ + public static final String CPP_LINK_STATIC_LIBRARY_ACTION_NAME = "c++-link-static-library"; + /** Name of the action producing dynamic library from cc_library. */ + public static final String CPP_LINK_NODEPS_DYNAMIC_LIBRARY_ACTION_NAME = + "c++-link-nodeps-dynamic-library"; + /** Name of the action producing dynamic library from cc_binary. */ + public static final String CPP_LINK_DYNAMIC_LIBRARY_ACTION_NAME = "c++-link-dynamic-library"; + /** Name of the action producing executable binary. */ + public static final String CPP_LINK_EXECUTABLE_ACTION_NAME = "c++-link-executable"; + /** Name of the objc action producing static library */ + public static final String OBJC_ARCHIVE_ACTION_NAME = "objc-archive"; + /** Name of the objc action producing dynamic library */ + public static final String OBJC_FULLY_LINK_ACTION_NAME = "objc-fully-link"; + /** Name of the objc action producing objc executable binary */ + public static final String OBJC_EXECUTABLE_ACTION_NAME = "objc-executable"; + /** Name of the objc action producing objc++ executable binary */ + public static final String OBJCPP_EXECUTABLE_ACTION_NAME = "objc++-executable"; + private Link() {} // uninstantiable /** @@ -97,7 +115,7 @@ public abstract class Link { /** A normal static archive. */ STATIC_LIBRARY( LinkerOrArchiver.ARCHIVER, - "c++-link-static-library", + CPP_LINK_STATIC_LIBRARY_ACTION_NAME, Picness.NOPIC, ArtifactCategory.STATIC_LIBRARY, Executable.NOT_EXECUTABLE), @@ -105,7 +123,7 @@ public abstract class Link { /** An objc static archive. */ OBJC_ARCHIVE( LinkerOrArchiver.ARCHIVER, - "objc-archive", + OBJC_ARCHIVE_ACTION_NAME, Picness.NOPIC, ArtifactCategory.STATIC_LIBRARY, Executable.NOT_EXECUTABLE), @@ -113,7 +131,7 @@ public abstract class Link { /** An objc fully linked static archive. */ OBJC_FULLY_LINKED_ARCHIVE( LinkerOrArchiver.ARCHIVER, - "objc-fully-link", + OBJC_FULLY_LINK_ACTION_NAME, Picness.NOPIC, ArtifactCategory.STATIC_LIBRARY, Executable.NOT_EXECUTABLE), @@ -121,7 +139,7 @@ public abstract class Link { /** An objc executable. */ OBJC_EXECUTABLE( LinkerOrArchiver.LINKER, - "objc-executable", + OBJC_EXECUTABLE_ACTION_NAME, Picness.NOPIC, ArtifactCategory.EXECUTABLE, Executable.EXECUTABLE), @@ -129,7 +147,7 @@ public abstract class Link { /** An objc executable that includes objc++/c++ source. */ OBJCPP_EXECUTABLE( LinkerOrArchiver.LINKER, - "objc++-executable", + OBJCPP_EXECUTABLE_ACTION_NAME, Picness.NOPIC, ArtifactCategory.EXECUTABLE, Executable.EXECUTABLE), @@ -137,7 +155,7 @@ public abstract class Link { /** A static archive with .pic.o object files (compiled with -fPIC). */ PIC_STATIC_LIBRARY( LinkerOrArchiver.ARCHIVER, - "c++-link-static-library", + CPP_LINK_STATIC_LIBRARY_ACTION_NAME, Picness.PIC, ArtifactCategory.STATIC_LIBRARY, Executable.NOT_EXECUTABLE), @@ -145,7 +163,7 @@ public abstract class Link { /** An interface dynamic library. */ INTERFACE_DYNAMIC_LIBRARY( LinkerOrArchiver.LINKER, - "c++-link-dynamic-library", + CPP_LINK_DYNAMIC_LIBRARY_ACTION_NAME, Picness.NOPIC, // Actually PIC but it's not indicated in the file name ArtifactCategory.INTERFACE_LIBRARY, Executable.NOT_EXECUTABLE), @@ -153,14 +171,14 @@ public abstract class Link { /** A dynamic library built from cc_library srcs. */ NODEPS_DYNAMIC_LIBRARY( LinkerOrArchiver.LINKER, - "c++-link-nodeps-dynamic-library", + CPP_LINK_NODEPS_DYNAMIC_LIBRARY_ACTION_NAME, Picness.NOPIC, // Actually PIC but it's not indicated in the file name ArtifactCategory.DYNAMIC_LIBRARY, Executable.NOT_EXECUTABLE), /** A transitive dynamic library used for distribution. */ DYNAMIC_LIBRARY( LinkerOrArchiver.LINKER, - "c++-link-dynamic-library", + CPP_LINK_DYNAMIC_LIBRARY_ACTION_NAME, Picness.NOPIC, // Actually PIC but it's not indicated in the file name ArtifactCategory.DYNAMIC_LIBRARY, Executable.NOT_EXECUTABLE), @@ -168,7 +186,7 @@ public abstract class Link { /** A static archive without removal of unused object files. */ ALWAYS_LINK_STATIC_LIBRARY( LinkerOrArchiver.ARCHIVER, - "c++-link-static-library", + CPP_LINK_STATIC_LIBRARY_ACTION_NAME, Picness.NOPIC, ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY, Executable.NOT_EXECUTABLE), @@ -176,7 +194,7 @@ public abstract class Link { /** A PIC static archive without removal of unused object files. */ ALWAYS_LINK_PIC_STATIC_LIBRARY( LinkerOrArchiver.ARCHIVER, - "c++-link-static-library", + CPP_LINK_STATIC_LIBRARY_ACTION_NAME, Picness.PIC, ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY, Executable.NOT_EXECUTABLE), @@ -184,7 +202,7 @@ public abstract class Link { /** An executable binary. */ EXECUTABLE( LinkerOrArchiver.LINKER, - "c++-link-executable", + CPP_LINK_EXECUTABLE_ACTION_NAME, Picness.NOPIC, // Picness is not indicate in the file name ArtifactCategory.EXECUTABLE, Executable.EXECUTABLE); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariables.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariables.java index de384b6bf9..f0e56358aa 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariables.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariables.java @@ -13,6 +13,8 @@ // limitations under the License. package com.google.devtools.build.lib.rules.cpp; +import com.google.common.base.Predicates; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.Artifact; @@ -22,6 +24,8 @@ import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfig import com.google.devtools.build.lib.rules.cpp.CcToolchainVariables.SequenceBuilder; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.ArrayList; +import java.util.List; /** Enum covering all build variables we create for all various {@link CppLinkAction}. */ public enum LinkBuildVariables { @@ -63,6 +67,8 @@ public enum LinkBuildVariables { INTERFACE_LIBRARY_OUTPUT("interface_library_output_path"), /** Linker flags coming from the legacy crosstool fields. */ LEGACY_LINK_FLAGS("legacy_link_flags"), + /** Linker flags coming from the --linkopt or linkopts attribute. */ + USER_LINK_FLAGS("user_link_flags"), /** Path to which to write symbol counts. */ SYMBOL_COUNTS_OUTPUT("symbol_counts_output"), /** A build variable giving linkstamp paths. */ @@ -97,6 +103,7 @@ public enum LinkBuildVariables { boolean isUsingLinkerNotArchiver, BuildConfiguration configuration, Artifact outputArtifact, + boolean isCreatingSharedLibrary, Artifact paramFile, Artifact thinltoParamFile, Artifact thinltoMergedObjectFile, @@ -106,6 +113,7 @@ public enum LinkBuildVariables { FeatureConfiguration featureConfiguration, boolean useTestOnlyFlags, boolean isLtoIndexing, + ImmutableList userLinkFlags, Artifact interfaceLibraryBuilder, Artifact interfaceLibraryOutput, PathFragment ltoOutputRootPrefix, @@ -113,7 +121,10 @@ public enum LinkBuildVariables { FdoSupportProvider fdoSupport, Iterable runtimeLibrarySearchDirectories, SequenceBuilder librariesToLink, - Iterable librarySearchDirectories) throws EvalException { + Iterable librarySearchDirectories, + boolean isLegacyFullyStaticLinkingMode, + boolean isStaticLinkingMode) + throws EvalException { CcToolchainVariables.Builder buildVariables = new CcToolchainVariables.Builder(); // symbol counting @@ -232,6 +243,109 @@ public enum LinkBuildVariables { } fdoSupport.getFdoSupport().getLinkOptions(featureConfiguration, buildVariables); + + ImmutableList userLinkFlagsWithLtoIndexingIfNeeded; + if (!isLtoIndexing) { + userLinkFlagsWithLtoIndexingIfNeeded = ImmutableList.copyOf(userLinkFlags); + + } else { + List opts = new ArrayList<>(userLinkFlags); + opts.addAll( + featureConfiguration.getCommandLine( + "lto-indexing", buildVariables.build(), /* expander= */ null)); + opts.addAll(ccToolchainProvider.getCppConfiguration().getLtoIndexOptions()); + userLinkFlagsWithLtoIndexingIfNeeded = ImmutableList.copyOf(opts); + } + + // For now, silently ignore linkopts if this is a static library + userLinkFlagsWithLtoIndexingIfNeeded = + isUsingLinkerNotArchiver ? userLinkFlagsWithLtoIndexingIfNeeded : ImmutableList.of(); + + buildVariables.addStringSequenceVariable( + LinkBuildVariables.USER_LINK_FLAGS.getVariableName(), + removePieIfCreatingSharedLibrary( + isCreatingSharedLibrary, userLinkFlagsWithLtoIndexingIfNeeded)); + buildVariables.addStringSequenceVariable( + LinkBuildVariables.LEGACY_LINK_FLAGS.getVariableName(), + getToolchainFlags( + isLegacyFullyStaticLinkingMode, + isStaticLinkingMode, + isUsingLinkerNotArchiver, + featureConfiguration, + ccToolchainProvider, + useTestOnlyFlags, + isCreatingSharedLibrary, + userLinkFlags)); + return buildVariables.build(); } + + private static ImmutableList getToolchainFlags( + boolean isLegacyFullyStaticLinkingMode, + boolean isStaticLinkingMode, + boolean isUsingLinkerNotArchiver, + FeatureConfiguration featureConfiguration, + CcToolchainProvider ccToolchainProvider, + boolean useTestOnlyFlags, + boolean isCreatingSharedLibrary, + List userLinkFlags) { + if (!isUsingLinkerNotArchiver) { + return ImmutableList.of(); + } + CppConfiguration cppConfiguration = ccToolchainProvider.getCppConfiguration(); + boolean sharedLinkopts = + isCreatingSharedLibrary + || userLinkFlags.contains("-shared") + || cppConfiguration.hasSharedLinkOption(); + + List result = new ArrayList<>(); + + // Extra toolchain link options based on the output's link staticness. + if (isLegacyFullyStaticLinkingMode) { + result.addAll( + CppHelper.getFullyStaticLinkOptions( + cppConfiguration, ccToolchainProvider, sharedLinkopts)); + } else if (isStaticLinkingMode) { + if (!featureConfiguration.isEnabled(CppRuleClasses.STATIC_LINKING_MODE)) { + result.addAll( + CppHelper.getMostlyStaticLinkOptions( + cppConfiguration, + ccToolchainProvider, + sharedLinkopts, + featureConfiguration.isEnabled(CppRuleClasses.STATIC_LINK_CPP_RUNTIMES))); + } else { + result.addAll(ccToolchainProvider.getLegacyLinkOptions()); + } + } else { + if (!featureConfiguration.isEnabled(CppRuleClasses.DYNAMIC_LINKING_MODE)) { + result.addAll( + CppHelper.getDynamicLinkOptions(cppConfiguration, ccToolchainProvider, sharedLinkopts)); + } else { + result.addAll(ccToolchainProvider.getLegacyLinkOptions()); + } + } + + // Extra test-specific link options. + if (useTestOnlyFlags) { + result.addAll(ccToolchainProvider.getTestOnlyLinkOptions()); + } + + result.addAll(ccToolchainProvider.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. + return ImmutableList.copyOf(removePieIfCreatingSharedLibrary(isCreatingSharedLibrary, result)); + } + + private static Iterable removePieIfCreatingSharedLibrary( + boolean isCreatingSharedLibrary, List flags) { + if (isCreatingSharedLibrary) { + return Iterables.filter(flags, Predicates.not(Predicates.equalTo("-pie"))); + } else { + return flags; + } + } } 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 a1c037f96e..92358daf14 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 @@ -50,7 +50,6 @@ public final class LinkCommandLine extends CommandLine { private final Iterable linkerInputArtifacts; private final LinkTargetType linkTargetType; private final Link.LinkingMode linkingMode; - private final ImmutableList linkopts; @Nullable private final PathFragment toolchainLibrariesSolibDir; private final boolean nativeDeps; private final boolean useTestOnlyFlags; @@ -65,7 +64,6 @@ public final class LinkCommandLine extends CommandLine { Iterable linkerInputArtifacts, LinkTargetType linkTargetType, Link.LinkingMode linkingMode, - ImmutableList linkopts, @Nullable PathFragment toolchainLibrariesSolibDir, boolean nativeDeps, boolean useTestOnlyFlags, @@ -81,7 +79,6 @@ public final class LinkCommandLine extends CommandLine { this.linkerInputArtifacts = Preconditions.checkNotNull(linkerInputArtifacts); this.linkTargetType = Preconditions.checkNotNull(linkTargetType); this.linkingMode = Preconditions.checkNotNull(linkingMode); - this.linkopts = linkopts; this.toolchainLibrariesSolibDir = toolchainLibrariesSolibDir; this.nativeDeps = nativeDeps; this.useTestOnlyFlags = useTestOnlyFlags; @@ -119,7 +116,12 @@ public final class LinkCommandLine extends CommandLine { * Returns the additional linker options for this link. */ public ImmutableList getLinkopts() { - return linkopts; + if (variables.isAvailable(LinkBuildVariables.USER_LINK_FLAGS.getVariableName())) { + return CcToolchainVariables.toStringList( + variables, LinkBuildVariables.USER_LINK_FLAGS.getVariableName()); + } else { + return ImmutableList.of(); + } } /** Returns the path to the linker. */ @@ -413,7 +415,6 @@ public final class LinkCommandLine extends CommandLine { private Iterable linkerInputArtifacts = ImmutableList.of(); @Nullable private LinkTargetType linkTargetType; private Link.LinkingMode linkingMode = Link.LinkingMode.LEGACY_FULLY_STATIC; - private ImmutableList linkopts = ImmutableList.of(); @Nullable private PathFragment toolchainLibrariesSolibDir; private boolean nativeDeps; private boolean useTestOnlyFlags; @@ -437,7 +438,7 @@ public final class LinkCommandLine extends CommandLine { if (ruleContext != null) { Preconditions.checkNotNull(featureConfiguration); } - + if (variables == null) { variables = CcToolchainVariables.EMPTY; } @@ -451,7 +452,6 @@ public final class LinkCommandLine extends CommandLine { linkerInputArtifacts, linkTargetType, linkingMode, - linkopts, toolchainLibrariesSolibDir, nativeDeps, useTestOnlyFlags, @@ -494,17 +494,6 @@ public final class LinkCommandLine extends CommandLine { 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 - * LinkTargetType#linkerOrArchiver()}). - */ - public Builder setLinkopts(ImmutableList linkopts) { - this.linkopts = linkopts; - return this; - } - /** * Sets how static the link is supposed to be. For static target types (see {@link * LinkTargetType#linkerOrArchiver()}}), the {@link #build} method throws an exception if this -- cgit v1.2.3