Move split/no-split command-line decision to CppLinkAction.

--
MOS_MIGRATED_REVID=96301836

2 files changed, 129 insertions, 94 deletions

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 4d6399457e..46988c91bb 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
@@ -54,7 +54,6 @@ import com.google.devtools.build.lib.rules.cpp.Link.LinkStaticness;
 import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
 import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink;
 import com.google.devtools.build.lib.util.Fingerprint;
-import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.ShellEscaper;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -68,6 +67,8 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
+import javax.annotation.Nullable;
+
 /**
  * Action that represents an ELF linking step.
  */
@@ -204,16 +205,7 @@ public final class CppLinkAction extends AbstractAction {
    */
   public final List<String> getCommandLine()
       throws ExecException {
-    List<String> commandlineArgs;
-    // Try to shorten the command line by use of a parameter file.
-    // This makes the output with --subcommands (et al) more readable.
-    if (linkCommandLine.canBeSplit()) {
-      Pair<List<String>, List<String>> split = linkCommandLine.splitCommandline();
-      commandlineArgs = split.first;
-    } else {
-      commandlineArgs = linkCommandLine.getRawLinkArgv();
-    }
-    return linkCommandLine.finalizeWithLinkstampCommands(commandlineArgs);
+    return linkCommandLine.getCommandLine();
   }
 
   @Override
@@ -456,7 +448,9 @@ public final class CppLinkAction extends AbstractAction {
     private final RuleContext ruleContext;
     private final AnalysisEnvironment analysisEnvironment;
     private final PathFragment outputPath;
-    private final CcToolchainProvider toolchain;
+
+    // can be null for CppLinkAction.createTestBuilder()
+    @Nullable private final CcToolchainProvider toolchain;
     private PathFragment interfaceOutputPath;
     private PathFragment runtimeSolibDir;
     protected final BuildConfiguration configuration;
@@ -479,7 +473,6 @@ public final class CppLinkAction extends AbstractAction {
     private boolean isNativeDeps;
     private boolean useTestOnlyFlags;
     private boolean wholeArchive;
-    private boolean supportsParamFiles = true;
 
     /**
      * Creates a builder that builds {@link CppLinkAction} instances.
@@ -524,14 +517,9 @@ public final class CppLinkAction extends AbstractAction {
       this.configuration = Preconditions.checkNotNull(configuration);
       this.cppConfiguration = configuration.getFragment(CppConfiguration.class);
       this.toolchain = toolchain;
-
-      // The toolchain != null is here for CppLinkAction.createTestBuilder(). Meh.
       if (cppConfiguration.supportsEmbeddedRuntimes() && toolchain != null) {
         runtimeSolibDir = toolchain.getDynamicRuntimeSolibDir();
       }
-      if (toolchain != null) {
-        supportsParamFiles = toolchain.supportsParamFiles();
-      }
     }
 
     /**
@@ -566,6 +554,29 @@ public final class CppLinkAction extends AbstractAction {
       this.useTestOnlyFlags = linkContext.useTestOnlyFlags;
     }
 
+    @VisibleForTesting
+    boolean canSplitCommandLine() {
+      if (toolchain == null || !toolchain.supportsParamFiles()) {
+        return false;
+      }
+
+      switch (linkType) {
+          // We currently can't split dynamic library links if they have interface outputs. That was
+          // probably an unintended side effect of the change that introduced interface outputs.
+        case DYNAMIC_LIBRARY:
+          return (interfaceOutputPath == null);
+        case EXECUTABLE:
+        case STATIC_LIBRARY:
+        case PIC_STATIC_LIBRARY:
+        case ALWAYS_LINK_STATIC_LIBRARY:
+        case ALWAYS_LINK_PIC_STATIC_LIBRARY:
+          return true;
+
+        default:
+          return false;
+      }
+    }
+
     /**
      * Builds the Action as configured and returns it.
      *
@@ -595,8 +606,8 @@ public final class CppLinkAction extends AbstractAction {
           linkStaticness, linkType, linkopts, isNativeDeps, cppConfiguration);
 
       NestedSet<LibraryToLink> uniqueLibraries = libraries.build();
-      final Iterable<Artifact> filteredNonLibraryArtifacts = filterLinkerInputArtifacts(
-          LinkerInputs.toLibraryArtifacts(nonLibraries));
+      final Iterable<Artifact> filteredNonLibraryArtifacts =
+          filterLinkerInputArtifacts(LinkerInputs.toLibraryArtifacts(nonLibraries));
       final Iterable<LinkerInput> linkerInputs = IterablesChain.<LinkerInput>builder()
           .add(ImmutableList.copyOf(filterLinkerInputs(nonLibraries)))
           .add(ImmutableIterable.from(Link.mergeInputsCmdLine(
@@ -616,37 +627,42 @@ public final class CppLinkAction extends AbstractAction {
       final ImmutableMap<Artifact, Artifact> linkstampMap =
           mapLinkstampsToOutputs(linkstamps, ruleContext, output);
 
-      final ImmutableList<Artifact> actionOutputs = constructOutputs(
-          outputLibrary.getArtifact(),
-          linkstampMap.values(),
-          interfaceOutputLibrary == null ? null : interfaceOutputLibrary.getArtifact(),
-          symbolCountOutput);
-
-      final PathFragment paramFileFragment =
-          supportsParamFiles
-              ? ParameterFile.derivePath(outputLibrary.getArtifact().getExecPath())
+      final ImmutableList<Artifact> actionOutputs =
+          constructOutputs(
+              outputLibrary.getArtifact(),
+              linkstampMap.values(),
+              interfaceOutputLibrary == null ? null : interfaceOutputLibrary.getArtifact(),
+              symbolCountOutput);
+
+      @Nullable
+      final Artifact paramFile =
+          canSplitCommandLine()
+              ? createArtifact(
+                  ParameterFile.derivePath(outputLibrary.getArtifact().getRootRelativePath()))
               : null;
 
-      LinkCommandLine linkCommandLine = new LinkCommandLine.Builder(configuration, getOwner())
-          .setOutput(outputLibrary.getArtifact())
-          .setInterfaceOutput(interfaceOutput)
-          .setSymbolCountsOutput(symbolCountOutput)
-          .setBuildInfoHeaderArtifacts(buildInfoHeaderArtifacts)
-          .setLinkerInputs(linkerInputs)
-          .setRuntimeInputs(ImmutableList.copyOf(LinkerInputs.simpleLinkerInputs(runtimeInputs)))
-          .setLinkTargetType(linkType)
-          .setLinkStaticness(linkStaticness)
-          .setLinkopts(ImmutableList.copyOf(linkopts))
-          .setFeatures(features)
-          .setLinkstamps(linkstampMap)
-          .addLinkstampCompileOptions(linkstampOptions)
-          .setRuntimeSolibDir(linkType.isStaticLibraryLink() ? null : runtimeSolibDir)
-          .setNativeDeps(isNativeDeps)
-          .setUseTestOnlyFlags(useTestOnlyFlags)
-          .setNeedWholeArchive(needWholeArchive)
-          .setInterfaceSoBuilder(getInterfaceSoBuilder())
-          .setParamFileFragment(paramFileFragment)
-          .build();
+      LinkCommandLine linkCommandLine =
+          new LinkCommandLine.Builder(configuration, getOwner())
+              .setOutput(outputLibrary.getArtifact())
+              .setInterfaceOutput(interfaceOutput)
+              .setSymbolCountsOutput(symbolCountOutput)
+              .setBuildInfoHeaderArtifacts(buildInfoHeaderArtifacts)
+              .setLinkerInputs(linkerInputs)
+              .setRuntimeInputs(
+                  ImmutableList.copyOf(LinkerInputs.simpleLinkerInputs(runtimeInputs)))
+              .setLinkTargetType(linkType)
+              .setLinkStaticness(linkStaticness)
+              .setLinkopts(ImmutableList.copyOf(linkopts))
+              .setFeatures(features)
+              .setLinkstamps(linkstampMap)
+              .addLinkstampCompileOptions(linkstampOptions)
+              .setRuntimeSolibDir(linkType.isStaticLibraryLink() ? null : runtimeSolibDir)
+              .setNativeDeps(isNativeDeps)
+              .setUseTestOnlyFlags(useTestOnlyFlags)
+              .setNeedWholeArchive(needWholeArchive)
+              .setInterfaceSoBuilder(getInterfaceSoBuilder())
+              .setParamFile(paramFile)
+              .build();
 
       // Compute the set of inputs - we only need stable order here.
       NestedSetBuilder<Artifact> dependencyInputsBuilder = NestedSetBuilder.stableOrder();
@@ -668,13 +684,15 @@ public final class CppLinkAction extends AbstractAction {
           .add(dependencyInputsBuilder.build())
           .add(ImmutableIterable.from(expandedInputs));
 
-      if (linkCommandLine.canBeSplit()) {
-        Artifact paramFile = createArtifact(
-            ParameterFile.derivePath(outputLibrary.getArtifact().getRootRelativePath()));
-        inputsBuilder.add(ImmutableList.of(paramFile));
-        Action parameterFileWriteAction = new ParameterFileWriteAction(
-            getOwner(), paramFile, linkCommandLine.paramCmdLine(),
-            ParameterFile.ParameterFileType.UNQUOTED, ISO_8859_1);
+      if (linkCommandLine.getParamFile() != null) {
+        inputsBuilder.add(ImmutableList.of(linkCommandLine.getParamFile()));
+        Action parameterFileWriteAction =
+            new ParameterFileWriteAction(
+                getOwner(),
+                paramFile,
+                linkCommandLine.paramCmdLine(),
+                ParameterFile.ParameterFileType.UNQUOTED,
+                ISO_8859_1);
         analysisEnvironment.registerAction(parameterFileWriteAction);
       }
 
@@ -748,8 +766,9 @@ public final class CppLinkAction extends AbstractAction {
       return ruleContext.getActionOwner();
     }
 
-    protected Artifact createArtifact(PathFragment path) {
-      return analysisEnvironment.getDerivedArtifact(path, configuration.getBinDirectory());
+    protected Artifact createArtifact(PathFragment rootRelativePath) {
+      return analysisEnvironment.getDerivedArtifact(
+          rootRelativePath, configuration.getBinDirectory());
     }
 
     protected Artifact getInterfaceSoBuilder() {
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 f8fc37e0a1..fd30c25d82 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
@@ -74,7 +74,7 @@ public final class LinkCommandLine extends CommandLine {
   private final boolean nativeDeps;
   private final boolean useTestOnlyFlags;
   private final boolean needWholeArchive;
-  private final PathFragment paramFileExecPath;
+  @Nullable private final Artifact paramFile;
   @Nullable private final Artifact interfaceSoBuilder;
 
   private LinkCommandLine(
@@ -96,7 +96,7 @@ public final class LinkCommandLine extends CommandLine {
       boolean nativeDeps,
       boolean useTestOnlyFlags,
       boolean needWholeArchive,
-      PathFragment paramFileFragment,
+      @Nullable Artifact paramFile,
       Artifact interfaceSoBuilder) {
     Preconditions.checkArgument(linkTargetType != LinkTargetType.INTERFACE_DYNAMIC_LIBRARY,
         "you can't link an interface dynamic library directly");
@@ -143,7 +143,8 @@ public final class LinkCommandLine extends CommandLine {
     this.nativeDeps = nativeDeps;
     this.useTestOnlyFlags = useTestOnlyFlags;
     this.needWholeArchive = needWholeArchive;
-    this.paramFileExecPath = paramFileFragment;
+    this.paramFile = paramFile;
+
     // For now, silently ignore interfaceSoBuilder if we don't build an interface dynamic library.
     this.interfaceSoBuilder =
         ((linkTargetType == LinkTargetType.DYNAMIC_LIBRARY) && (interfaceOutput != null))
@@ -157,7 +158,8 @@ public final class LinkCommandLine extends CommandLine {
    * non-null if {@link #getLinkTargetType} is {@code DYNAMIC_LIBRARY} and an interface shared
    * object was requested.
    */
-  @Nullable public Artifact getInterfaceOutput() {
+  @Nullable
+  public Artifact getInterfaceOutput() {
     return interfaceOutput;
   }
 
@@ -170,6 +172,11 @@ public final class LinkCommandLine extends CommandLine {
     return symbolCountsOutput;
   }
 
+  @Nullable
+  public Artifact getParamFile() {
+    return paramFile;
+  }
+
   /**
    * Returns the (ordered, immutable) list of header files that contain build info.
    */
@@ -250,14 +257,13 @@ public final class LinkCommandLine extends CommandLine {
 
   /**
    * Splits the link command-line into a part to be written to a parameter file, and the remaining
-   * actual command line to be executed (which references the parameter file). Call {@link
-   * #canBeSplit} first to check if the command-line can be split.
+   * actual command line to be executed (which references the parameter file). Should only be used
+   * if getParamFile() is not null.
    *
    * @throws IllegalStateException if the command-line cannot be split
    */
   @VisibleForTesting
   final Pair<List<String>, List<String>> splitCommandline() {
-    Preconditions.checkState(canBeSplit());
     List<String> args = getRawLinkArgv();
     if (linkTargetType.isStaticLibraryLink()) {
       // Ar link commands can also generate huge command lines.
@@ -265,7 +271,7 @@ public final class LinkCommandLine extends CommandLine {
       List<String> commandlineArgs = new ArrayList<>();
       commandlineArgs.add(args.get(0));
 
-      commandlineArgs.add("@" + paramFileExecPath.getPathString());
+      commandlineArgs.add("@" + paramFile.getExecPath().getPathString());
       return Pair.of(commandlineArgs, paramFileArgs);
     } else {
       // Gcc link commands tend to generate humongous commandlines for some targets, which may
@@ -275,7 +281,7 @@ public final class LinkCommandLine extends CommandLine {
       List<String> commandlineArgs = new ArrayList<>();
       extractArgumentsForParamFile(args, commandlineArgs, paramFileArgs);
 
-      commandlineArgs.add("-Wl,@" + paramFileExecPath.getPathString());
+      commandlineArgs.add("-Wl,@" + paramFile.getExecPath().getPathString());
       return Pair.of(commandlineArgs, paramFileArgs);
     }
   }
@@ -286,7 +292,7 @@ public final class LinkCommandLine extends CommandLine {
    * @throws IllegalStateException if the command-line cannot be split
    */
   CommandLine paramCmdLine() {
-    Preconditions.checkState(canBeSplit());
+    Preconditions.checkNotNull(paramFile);
     return new CommandLine() {
       @Override
       public Iterable<String> arguments() {
@@ -295,25 +301,6 @@ public final class LinkCommandLine extends CommandLine {
     };
   }
 
-  boolean canBeSplit() {
-    if (paramFileExecPath == null) {
-      return false;
-    }
-    switch (linkTargetType) {
-      // We currently can't split dynamic library links if they have interface outputs. That was
-      // probably an unintended side effect of the change that introduced interface outputs.
-      case DYNAMIC_LIBRARY:
-        return interfaceOutput == null;
-      case EXECUTABLE:
-      case STATIC_LIBRARY:
-      case PIC_STATIC_LIBRARY:
-      case ALWAYS_LINK_STATIC_LIBRARY:
-      case ALWAYS_LINK_PIC_STATIC_LIBRARY:
-        return true;
-      default:
-        return false;
-    }
-  }
 
   private static void extractArgumentsForParamFile(List<String> args, List<String> commandlineArgs,
       List<String> paramFileArgs) {
@@ -421,6 +408,19 @@ public final class LinkCommandLine extends CommandLine {
     return argv;
   }
 
+  List<String> getCommandLine() {
+    List<String> commandlineArgs;
+    // Try to shorten the command line by use of a parameter file.
+    // This makes the output with --subcommands (et al) more readable.
+    if (paramFile != null) {
+      Pair<List<String>, List<String>> split = splitCommandline();
+      commandlineArgs = split.first;
+    } else {
+      commandlineArgs = getRawLinkArgv();
+    }
+    return finalizeWithLinkstampCommands(commandlineArgs);
+  }
+
   @Override
   public List<String> arguments() {
     return finalizeWithLinkstampCommands(getRawLinkArgv());
@@ -946,7 +946,7 @@ public final class LinkCommandLine extends CommandLine {
     private boolean nativeDeps;
     private boolean useTestOnlyFlags;
     private boolean needWholeArchive;
-    private PathFragment paramFileFragment;
+    @Nullable private Artifact paramFile;
     @Nullable private Artifact interfaceSoBuilder;
 
     public Builder(BuildConfiguration configuration, ActionOwner owner) {
@@ -966,10 +966,26 @@ public final class LinkCommandLine extends CommandLine {
         actualLinkstampCompileOptions = ImmutableList.copyOf(
             Iterables.concat(DEFAULT_LINKSTAMP_OPTIONS, linkstampCompileOptions));
       }
-      return new LinkCommandLine(configuration, owner, output, interfaceOutput,
-          symbolCountsOutput, buildInfoHeaderArtifacts, linkerInputs, runtimeInputs, linkTargetType,
-          linkStaticness, linkopts, features, linkstamps, actualLinkstampCompileOptions,
-          runtimeSolibDir, nativeDeps, useTestOnlyFlags, needWholeArchive, paramFileFragment,
+      return new LinkCommandLine(
+          configuration,
+          owner,
+          output,
+          interfaceOutput,
+          symbolCountsOutput,
+          buildInfoHeaderArtifacts,
+          linkerInputs,
+          runtimeInputs,
+          linkTargetType,
+          linkStaticness,
+          linkopts,
+          features,
+          linkstamps,
+          actualLinkstampCompileOptions,
+          runtimeSolibDir,
+          nativeDeps,
+          useTestOnlyFlags,
+          needWholeArchive,
+          paramFile,
           interfaceSoBuilder);
     }
 
@@ -1131,8 +1147,8 @@ public final class LinkCommandLine extends CommandLine {
       return this;
     }
 
-    public Builder setParamFileFragment(PathFragment paramFragment) {
-      this.paramFileFragment = paramFragment;
+    public Builder setParamFile(Artifact paramFile) {
+      this.paramFile = paramFile;
       return this;
     }
   }