If an action_config is given for a particular type of link action, use that action_config to configure the build. Otherwise, revert to hard-coded behavior.

This change is designed to provide backwards-compatible support for crosstools that describe platform-specific linking semantics.

--
MOS_MIGRATED_REVID=123748494

6 files changed, 128 insertions, 63 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
index d4c4da31e6..aaf5e95410 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
@@ -92,7 +92,16 @@ public final class CcLibraryHelper {
             CppCompileAction.CPP_HEADER_PREPROCESSING,
             CppCompileAction.CPP_MODULE_COMPILE,
             CppCompileAction.ASSEMBLE,
-            CppCompileAction.PREPROCESS_ASSEMBLE)),
+            CppCompileAction.PREPROCESS_ASSEMBLE,
+            Link.LinkTargetType.STATIC_LIBRARY.getActionName(),
+            // We need to create pic-specific actions for link actions, as they will produce
+            // differently named outputs.
+            Link.LinkTargetType.PIC_STATIC_LIBRARY.getActionName(),
+            Link.LinkTargetType.INTERFACE_DYNAMIC_LIBRARY.getActionName(),
+            Link.LinkTargetType.DYNAMIC_LIBRARY.getActionName(),
+            Link.LinkTargetType.ALWAYS_LINK_STATIC_LIBRARY.getActionName(),
+            Link.LinkTargetType.ALWAYS_LINK_PIC_STATIC_LIBRARY.getActionName(),
+            Link.LinkTargetType.EXECUTABLE.getActionName())),
     CC_AND_OBJC(
         FileTypeSet.of(
             CppFileTypes.CPP_SOURCE,
@@ -111,8 +120,17 @@ public final class CcLibraryHelper {
             CppCompileAction.CPP_HEADER_PREPROCESSING,
             CppCompileAction.CPP_MODULE_COMPILE,
             CppCompileAction.ASSEMBLE,
-            CppCompileAction.PREPROCESS_ASSEMBLE));
-            
+            CppCompileAction.PREPROCESS_ASSEMBLE,
+            Link.LinkTargetType.STATIC_LIBRARY.getActionName(),
+            // We need to create pic-specific actions for link actions, as they will produce
+            // differently named outputs. 
+            Link.LinkTargetType.PIC_STATIC_LIBRARY.getActionName(),
+            Link.LinkTargetType.INTERFACE_DYNAMIC_LIBRARY.getActionName(),
+            Link.LinkTargetType.DYNAMIC_LIBRARY.getActionName(),
+            Link.LinkTargetType.ALWAYS_LINK_STATIC_LIBRARY.getActionName(),
+            Link.LinkTargetType.ALWAYS_LINK_PIC_STATIC_LIBRARY.getActionName(),
+            Link.LinkTargetType.EXECUTABLE.getActionName()));
+
 
     private final FileTypeSet sourceTypeSet;
     private final Set<String> actionConfigSet;
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 319610243b..07e87246ee 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
@@ -802,7 +802,13 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
                 + "  flag_set {"
                 + "    action: 'c-compile'"
                 + "    action: 'c++-compile'"
-                + "    action: 'c++-link'"
+                + "    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}'"
                 + "      flag: '-Xclang-only=-fprofile-instr-generate=%{fdo_instrument_path}'"
@@ -891,7 +897,13 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
                 + "    }"
                 + "  }"
                 + "  flag_set {"
-                + "    action: 'c++-link'"
+                + "    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'"
                 + "    }"
@@ -1026,6 +1038,13 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
   }
 
   /**
+   * Returns the path of the crosstool.
+   */
+  public PathFragment getCrosstoolTopPathFragment() {
+    return crosstoolTopPathFragment;
+  }
+
+  /**
    * Returns the system name which is required by the toolchain to run.
    */
   public String getHostSystemName() {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
index 3c49d0a2ac..c774f1d187 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
@@ -108,11 +108,6 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo
 
   private static final String LINK_GUID = "58ec78bd-1176-4e36-8143-439f656b181d";
   private static final String FAKE_LINK_GUID = "da36f819-5a15-43a9-8a45-e01b60e10c8b";
-
-  /**
-   * The name of this action for the purpose of crosstool features/action_configs
-   */
-  private static final String ACTION_NAME = "c++-link";
   
   private final CppConfiguration cppConfiguration;
   private final LibraryToLink outputLibrary;
@@ -655,9 +650,8 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo
     /**
      * Returns the action name for purposes of querying the crosstool.
      */
-    // TODO(b/28791924): Expand action types to values in Link.LinkTargetType.
     private String getActionName() {
-      return ACTION_NAME;
+      return linkType.getActionName();
     }
 
     public CppLinkAction.Builder setLinkArtifactFactory(LinkArtifactFactory linkArtifactFactory) {
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 c51a06e533..38ab7e8ccf 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
@@ -94,32 +94,34 @@ public abstract class Link {
    */
   public enum LinkTargetType {
     /** A normal static archive. */
-    STATIC_LIBRARY(".a", true),
+    STATIC_LIBRARY(".a", true, "c++-link-static-library"),
 
     /** A static archive with .pic.o object files (compiled with -fPIC). */
-    PIC_STATIC_LIBRARY(".pic.a", true),
+    PIC_STATIC_LIBRARY(".pic.a", true, "c++-link-pic-static-library"),
 
     /** An interface dynamic library. */
-    INTERFACE_DYNAMIC_LIBRARY(".ifso", false),
+    INTERFACE_DYNAMIC_LIBRARY(".ifso", false, "c++-link-interface-dynamic-library"),
 
     /** A dynamic library. */
-    DYNAMIC_LIBRARY(".so", false),
+    DYNAMIC_LIBRARY(".so", false, "c++-link-dynamic-library"),
 
     /** A static archive without removal of unused object files. */
-    ALWAYS_LINK_STATIC_LIBRARY(".lo", true),
+    ALWAYS_LINK_STATIC_LIBRARY(".lo", true, "c++-link-alwayslink-static-library"),
 
     /** A PIC static archive without removal of unused object files. */
-    ALWAYS_LINK_PIC_STATIC_LIBRARY(".pic.lo", true),
+    ALWAYS_LINK_PIC_STATIC_LIBRARY(".pic.lo", true, "c++-link-alwayslink-pic-static-library"),
 
     /** An executable binary. */
-    EXECUTABLE("", false);
+    EXECUTABLE("", false, "c++-link-executable");
 
     private final String extension;
     private final boolean staticLibraryLink;
+    private final String actionName;
 
-    private LinkTargetType(String extension, boolean staticLibraryLink) {
+    private LinkTargetType(String extension, boolean staticLibraryLink, String actionName) {
       this.extension = extension;
       this.staticLibraryLink = staticLibraryLink;
+      this.actionName = actionName;
     }
 
     public String getExtension() {
@@ -129,6 +131,14 @@ public abstract class Link {
     public boolean isStaticLibraryLink() {
       return staticLibraryLink;
     }
+    
+    /**
+     * The name of a link action with this LinkTargetType, for the purpose of crosstool feature
+     * selection.
+     */
+    public String getActionName() {
+      return actionName;
+    }
   }
 
   /**
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 2a6d88d664..96b5a3bb3c 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
@@ -381,45 +381,61 @@ public final class LinkCommandLine extends CommandLine {
    */
   public List<String> getRawLinkArgv() {
     List<String> argv = new ArrayList<>();
-    switch (linkTargetType) {
-      case EXECUTABLE:
-        addCppArgv(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());
+
+    // 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;
+
+        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));
           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));
-        argv.add(output.getExecPathString());
-        addInputFileLinkOptions(argv, /*needWholeArchive=*/false);
-        break;
-
-      default:
-        throw new IllegalArgumentException();
+          addInputFileLinkOptions(argv, /*needWholeArchive=*/ false);
+          break;
+
+        default:
+          throw new IllegalArgumentException();
+      }
     }
 
     // Fission mode: debug info is in .dwo files instead of .o files. Inform the linker of 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 74ba8c565a..d88b9325f3 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
@@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
+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;
@@ -74,11 +75,18 @@ public class CppLinkActionTest extends BuildViewTestCase {
                 "feature {",
                 "   name: 'a'",
                 "   flag_set {",
-                "      action: 'c++-link'",
+                "      action: '" + Link.LinkTargetType.EXECUTABLE.getActionName() + "'",
                 "      flag_group { flag: 'some_flag' }",
                 "   }",
+                "}",
+                "action_config {",
+                "   config_name: '" + Link.LinkTargetType.EXECUTABLE.getActionName() + "'",
+                "   action_name: '" + Link.LinkTargetType.EXECUTABLE.getActionName() + "'",
+                "   tool {",
+                "      tool_path: 'toolchain/mock_tool'",
+                "   }",
                 "}")
-            .getFeatureConfiguration("a");
+            .getFeatureConfiguration("a", Link.LinkTargetType.EXECUTABLE.getActionName());
 
     CppLinkAction linkAction =
         createLinkBuilder(
@@ -88,17 +96,18 @@ public class CppLinkActionTest extends BuildViewTestCase {
                 ImmutableList.<LibraryToLink>of(),
                 featureConfiguration)
             .build();
+    assertThat(Joiner.on(" ").join(linkAction.getArgv())).contains("mock_tool");
     assertThat(linkAction.getArgv()).contains("some_flag");
   }
 
   @Test
   public void testToolchainFeatureEnv() throws Exception {
-     FeatureConfiguration featureConfiguration =
+    FeatureConfiguration featureConfiguration =
         CcToolchainFeaturesTest.buildFeatures(
                 "feature {",
                 "   name: 'a'",
                 "   env_set {",
-                "      action: 'c++-link'",
+                "      action: '" + Link.LinkTargetType.EXECUTABLE.getActionName() + "'",
                 "      env_entry { key: 'foo', value: 'bar' }",
                 "   }",
                 "}")
@@ -186,7 +195,6 @@ public class CppLinkActionTest extends BuildViewTestCase {
                 (i & 1) == 0 ? ImmutableList.of(oFile) : ImmutableList.of(oFile2));
             builder.setLinkType(
                 (i & 2) == 0 ? LinkTargetType.STATIC_LIBRARY : LinkTargetType.DYNAMIC_LIBRARY);
-            builder.setFeatureConfiguration(new FeatureConfiguration());
             return builder.build();
           }
         });