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

6 files changed, 178 insertions, 127 deletions

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'",
@@ -854,6 +857,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 {",
                     "  name: 'legacy_link_flags'",
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<String> getToolchainFlags(List<String> 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<String> 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<String> linkoptsForVariables;
     if (!isLtoIndexing) {
-      linkoptsForVariables = ImmutableList.copyOf(linkopts);
       linkCommandLineBuilder.setBuildInfoHeaderArtifacts(buildInfoHeaderArtifacts);
-
-    } else {
-      List<String> 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<String> userLinkFlags,
       Artifact interfaceLibraryBuilder,
       Artifact interfaceLibraryOutput,
       PathFragment ltoOutputRootPrefix,
@@ -113,7 +121,10 @@ public enum LinkBuildVariables {
       FdoSupportProvider fdoSupport,
       Iterable<String> runtimeLibrarySearchDirectories,
       SequenceBuilder librariesToLink,
-      Iterable<String> librarySearchDirectories) throws EvalException {
+      Iterable<String> 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<String> userLinkFlagsWithLtoIndexingIfNeeded;
+    if (!isLtoIndexing) {
+      userLinkFlagsWithLtoIndexingIfNeeded = ImmutableList.copyOf(userLinkFlags);
+
+    } else {
+      List<String> 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<String> getToolchainFlags(
+      boolean isLegacyFullyStaticLinkingMode,
+      boolean isStaticLinkingMode,
+      boolean isUsingLinkerNotArchiver,
+      FeatureConfiguration featureConfiguration,
+      CcToolchainProvider ccToolchainProvider,
+      boolean useTestOnlyFlags,
+      boolean isCreatingSharedLibrary,
+      List<String> userLinkFlags) {
+    if (!isUsingLinkerNotArchiver) {
+      return ImmutableList.of();
+    }
+    CppConfiguration cppConfiguration = ccToolchainProvider.getCppConfiguration();
+    boolean sharedLinkopts =
+        isCreatingSharedLibrary
+            || userLinkFlags.contains("-shared")
+            || cppConfiguration.hasSharedLinkOption();
+
+    List<String> 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<String> removePieIfCreatingSharedLibrary(
+      boolean isCreatingSharedLibrary, List<String> 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<Artifact> linkerInputArtifacts;
   private final LinkTargetType linkTargetType;
   private final Link.LinkingMode linkingMode;
-  private final ImmutableList<String> 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<Artifact> linkerInputArtifacts,
       LinkTargetType linkTargetType,
       Link.LinkingMode linkingMode,
-      ImmutableList<String> 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<String> 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<Artifact> linkerInputArtifacts = ImmutableList.of();
     @Nullable private LinkTargetType linkTargetType;
     private Link.LinkingMode linkingMode = Link.LinkingMode.LEGACY_FULLY_STATIC;
-    private ImmutableList<String> 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,
@@ -495,17 +495,6 @@ public final class LinkCommandLine extends CommandLine {
     }
 
     /**
-     * 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<String> 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
      * is not {@link Link.LinkingMode#LEGACY_FULLY_STATIC}. The default setting is {@link