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.

RELNOTES: None.
PiperOrigin-RevId: 196940832

7 files changed, 71 insertions, 50 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
index 21ccb4a116..d5f1fba1f8 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
@@ -116,9 +116,9 @@ public final class CcCommon {
 
   public static final ImmutableSet<String> ALL_LINK_ACTIONS =
       ImmutableSet.of(
+          LinkTargetType.EXECUTABLE.getActionName(),
           Link.LinkTargetType.DYNAMIC_LIBRARY.getActionName(),
-          Link.LinkTargetType.NODEPS_DYNAMIC_LIBRARY.getActionName(),
-          LinkTargetType.EXECUTABLE.getActionName());
+          Link.LinkTargetType.NODEPS_DYNAMIC_LIBRARY.getActionName());
 
   public static final ImmutableSet<String> ALL_ARCHIVE_ACTIONS =
       ImmutableSet.of(Link.LinkTargetType.STATIC_LIBRARY.getActionName());
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 d5e692f75f..b338d5b173 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
@@ -86,6 +86,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 9ec07a3177..69294f04ef 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
@@ -686,19 +686,6 @@ public class CppLinkActionBuilder {
 
     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(
@@ -734,11 +721,15 @@ public class CppLinkActionBuilder {
     // 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(result));
+  }
+
+  private Iterable<String> removePieIfCreatingSharedLibrary(List<String> flags) {
     if (linkType == LinkTargetType.DYNAMIC_LIBRARY) {
-      Iterables.removeIf(result, Predicates.equalTo("-pie"));
+      return Iterables.filter(flags, Predicates.not(Predicates.equalTo("-pie")));
+    } else {
+      return flags;
     }
-
-    return ImmutableList.copyOf(result);
   }
 
   /** Builds the Action as configured and returns it. */
@@ -1084,11 +1075,13 @@ public class CppLinkActionBuilder {
         linkType.linkerOrArchiver() == LinkerOrArchiver.ARCHIVER
             ? ImmutableList.of()
             : linkoptsForVariables;
-    linkCommandLineBuilder.setLinkopts(linkoptsForVariables);
 
     CcToolchainVariables patchedVariables =
         new CcToolchainVariables.Builder(buildVariables)
             .addStringSequenceVariable(
+                LinkBuildVariables.USER_LINK_FLAGS.getVariableName(),
+                removePieIfCreatingSharedLibrary(linkoptsForVariables))
+            .addStringSequenceVariable(
                 LinkBuildVariables.LEGACY_LINK_FLAGS.getVariableName(),
                 getToolchainFlags(linkoptsForVariables))
             .build();
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 76018356ea..ffbfd76b8d 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
 
   /**
@@ -98,7 +116,7 @@ public abstract class Link {
     STATIC_LIBRARY(
         ".a",
         LinkerOrArchiver.ARCHIVER,
-        "c++-link-static-library",
+        CPP_LINK_STATIC_LIBRARY_ACTION_NAME,
         Picness.NOPIC,
         ArtifactCategory.STATIC_LIBRARY,
         Executable.NOT_EXECUTABLE),
@@ -107,7 +125,7 @@ public abstract class Link {
     OBJC_ARCHIVE(
         ".a",
         LinkerOrArchiver.ARCHIVER,
-        "objc-archive",
+        OBJC_ARCHIVE_ACTION_NAME,
         Picness.NOPIC,
         ArtifactCategory.STATIC_LIBRARY,
         Executable.NOT_EXECUTABLE),
@@ -116,7 +134,7 @@ public abstract class Link {
     OBJC_FULLY_LINKED_ARCHIVE(
         ".a",
         LinkerOrArchiver.ARCHIVER,
-        "objc-fully-link",
+        OBJC_FULLY_LINK_ACTION_NAME,
         Picness.NOPIC,
         ArtifactCategory.STATIC_LIBRARY,
         Executable.NOT_EXECUTABLE),
@@ -125,7 +143,7 @@ public abstract class Link {
     OBJC_EXECUTABLE(
         "",
         LinkerOrArchiver.LINKER,
-        "objc-executable",
+        OBJC_EXECUTABLE_ACTION_NAME,
         Picness.NOPIC,
         ArtifactCategory.EXECUTABLE,
         Executable.EXECUTABLE),
@@ -134,7 +152,7 @@ public abstract class Link {
     OBJCPP_EXECUTABLE(
         "",
         LinkerOrArchiver.LINKER,
-        "objc++-executable",
+        OBJCPP_EXECUTABLE_ACTION_NAME,
         Picness.NOPIC,
         ArtifactCategory.EXECUTABLE,
         Executable.EXECUTABLE),
@@ -143,7 +161,7 @@ public abstract class Link {
     PIC_STATIC_LIBRARY(
         ".pic.a",
         LinkerOrArchiver.ARCHIVER,
-        "c++-link-static-library",
+        CPP_LINK_STATIC_LIBRARY_ACTION_NAME,
         Picness.PIC,
         ArtifactCategory.STATIC_LIBRARY,
         Executable.NOT_EXECUTABLE),
@@ -152,7 +170,7 @@ public abstract class Link {
     INTERFACE_DYNAMIC_LIBRARY(
         ".ifso",
         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),
@@ -161,7 +179,7 @@ public abstract class Link {
     NODEPS_DYNAMIC_LIBRARY(
         ".so",
         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),
@@ -169,7 +187,7 @@ public abstract class Link {
     DYNAMIC_LIBRARY(
         ".so",
         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),
@@ -178,7 +196,7 @@ public abstract class Link {
     ALWAYS_LINK_STATIC_LIBRARY(
         ".lo",
         LinkerOrArchiver.ARCHIVER,
-        "c++-link-static-library",
+        CPP_LINK_STATIC_LIBRARY_ACTION_NAME,
         Picness.NOPIC,
         ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY,
         Executable.NOT_EXECUTABLE),
@@ -187,7 +205,7 @@ public abstract class Link {
     ALWAYS_LINK_PIC_STATIC_LIBRARY(
         ".pic.lo",
         LinkerOrArchiver.ARCHIVER,
-        "c++-link-static-library",
+        CPP_LINK_STATIC_LIBRARY_ACTION_NAME,
         Picness.PIC,
         ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY,
         Executable.NOT_EXECUTABLE),
@@ -196,7 +214,7 @@ public abstract class Link {
     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 34d39c3690..248c1b4a7c 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
@@ -61,6 +61,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. */
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..19a646506d 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,10 +438,10 @@ public final class LinkCommandLine extends CommandLine {
       if (ruleContext != null) {
         Preconditions.checkNotNull(featureConfiguration);
       }
-      
+
       if (variables == null) {
         variables = CcToolchainVariables.EMPTY;
-      }
+       }
 
       String actionName = linkTargetType.getActionName();
 
@@ -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