Skip to content

Commit

Permalink
Refactor splitting of LinkerCommandLine
Browse files Browse the repository at this point in the history 
Before `getRawLinkArgv` generated a joint/mixed command line, which was composed of 'linker executable' (first argument) and regular arguments. Reference to 'param file' was mixed into the regular arguments.

Linker command line is long, too long. To fix this, most of linkers are called like `linker @param.file`. The long list of arguments is hidden in `param.file`.

`splitCommandline` used `isLikelyParamFile` to filter out what that `param file` reference was. The result were 2 command lines. First one with 'linker executable' and reference to 'param file' and second command line contained regular arguments, that are written to param file.

Generate those 2 command lines directly. First one is returned by `getCommandLine` and second one (the regular arguments) by `getParamCommandLine`

Param file reference is formatted using `linker_param_file` feature, using `%{linker_param_file}` substitution. In most cases this is just `@%{linker_param_file}`, but there are some linkers that need different formatting.

This change is also needed for Starlarkification of CppLinkAction, because the Starlark interface requires to set how param file is formatted, directly. See https://bazel.build/rules/lib/builtins/Args#use_param_file

Fixes: #18074
PiperOrigin-RevId: 595912823
Change-Id: I54b36113d87f975af63341b2dec17b2f861c0ffa
  • Loading branch information
@comius @copybara-github
comius authored and copybara-github committed Jan 5, 2024
1 parent debec5c commit b5cfea5
Show file tree
Hide file tree
Showing 3 changed files with 78 additions and 175 deletions.
6 changes: 1 addition & 5 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
import com.google.devtools.build.lib.actions.CommandAction;
import com.google.devtools.build.lib.actions.CommandLine;
import com.google.devtools.build.lib.actions.CommandLineExpansionException;
import com.google.devtools.build.lib.actions.CommandLines.CommandLineAndParamFileInfo;
import com.google.devtools.build.lib.actions.ExecException;
Expand Down Expand Up @@ -274,11 +273,8 @@ public Sequence<CommandLineArgsApi> getStarlarkArgs() {
getInputs().toList().stream()
.filter(Artifact::isDirectory)
.collect(ImmutableSet.toImmutableSet());

CommandLine commandLine = linkCommandLine.getCommandLineForStarlark();

CommandLineAndParamFileInfo commandLineAndParamFileInfo =
new CommandLineAndParamFileInfo(commandLine, /* paramFileInfo= */ null);
new CommandLineAndParamFileInfo(linkCommandLine, /* paramFileInfo= */ null);

Args args = Args.forRegisteredAction(commandLineAndParamFileInfo, directoryInputs);

Expand Down
215 changes: 57 additions & 158 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@

package com.google.devtools.build.lib.rules.cpp;

import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.devtools.build.lib.rules.cpp.LinkBuildVariables.LINKER_PARAM_FILE;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
Expand All @@ -26,7 +29,6 @@
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.ExpansionException;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.util.ArrayList;
Expand Down Expand Up @@ -84,7 +86,15 @@ public String getActionName() {

/** Returns the path to the linker. */
public String getLinkerPathString() {
return featureConfiguration.getToolPathForAction(linkTargetType.getActionName());
if (forcedToolPath != null) {
return forcedToolPath;
} else {
Preconditions.checkArgument(
featureConfiguration.actionIsConfigured(actionName),
"Expected action_config for '%s' to be configured",
actionName);
return featureConfiguration.getToolPathForAction(linkTargetType.getActionName());
}
}

/**
Expand Down Expand Up @@ -117,195 +127,84 @@ public CcToolchainVariables getBuildVariables() {
return this.variables;
}

/**
* 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). Should only be used
* if getParamFile() is not null.
*/
@VisibleForTesting
final Pair<List<String>, List<String>> splitCommandline() throws CommandLineExpansionException {
return splitCommandline(paramFile, getRawLinkArgv(null));
}

@VisibleForTesting
final Pair<List<String>, List<String>> splitCommandline(@Nullable ArtifactExpander expander)
throws CommandLineExpansionException {
return splitCommandline(paramFile, getRawLinkArgv(expander));
}

private static Pair<List<String>, List<String>> splitCommandline(
Artifact paramFile, List<String> args) {
/** Returns just the .params file portion of the command-line as a {@link CommandLine}. */
CommandLine paramCmdLine() {
Preconditions.checkNotNull(paramFile);

// Gcc and Ar link commands tend to generate humongous commandlines for some targets, which may
// not fit on some remote execution machines. To work around this we will employ the help of
// a parameter file and pass any linker options through it.
List<String> paramFileArgs = new ArrayList<>();
List<String> commandlineArgs = new ArrayList<>();
extractArguments(args, commandlineArgs, paramFileArgs);
return Pair.of(commandlineArgs, paramFileArgs);
}

public CommandLine getCommandLineForStarlark() {
return new CommandLine() {
@Override
public Iterable<String> arguments() throws CommandLineExpansionException {
return arguments(/* artifactExpander= */ null, PathMapper.NOOP);
return getParamCommandLine(null);
}

@Override
public ImmutableList<String> arguments(
ArtifactExpander artifactExpander, PathMapper pathMapper)
public List<String> arguments(ArtifactExpander expander, PathMapper pathMapper)
throws CommandLineExpansionException {
if (paramFile == null) {
return ImmutableList.copyOf(getRawLinkArgv(artifactExpander));
} else {
return ImmutableList.<String>builder()
.add(getLinkerPathString())
.addAll(splitCommandline(artifactExpander).getSecond())
.build();
}
return getParamCommandLine(expander);
}
};
}

/**
* A {@link CommandLine} implementation that returns the command line args pertaining to the
* .params file.
*/
private static class ParamFileCommandLine extends CommandLine {
private final Artifact paramsFile;
private final LinkTargetType linkTargetType;
private final String forcedToolPath;
private final FeatureConfiguration featureConfiguration;
private final String actionName;
private final CcToolchainVariables variables;

ParamFileCommandLine(
Artifact paramsFile,
LinkTargetType linkTargetType,
String forcedToolPath,
FeatureConfiguration featureConfiguration,
String actionName,
CcToolchainVariables variables) {
this.paramsFile = paramsFile;
this.linkTargetType = linkTargetType;
this.forcedToolPath = forcedToolPath;
this.featureConfiguration = featureConfiguration;
this.actionName = actionName;
this.variables = variables;
}

@Override
public Iterable<String> arguments() throws CommandLineExpansionException {
List<String> argv =
getRawLinkArgv(
null, forcedToolPath, featureConfiguration, actionName, linkTargetType, variables);
return splitCommandline(paramsFile, argv).getSecond();
}

@Override
public Iterable<String> arguments(ArtifactExpander expander, PathMapper pathMapper)
throws CommandLineExpansionException {
List<String> argv =
getRawLinkArgv(
expander,
forcedToolPath,
featureConfiguration,
actionName,
linkTargetType,
variables);
return splitCommandline(paramsFile, argv).getSecond();
}
}

/** Returns just the .params file portion of the command-line as a {@link CommandLine}. */
CommandLine paramCmdLine() {
Preconditions.checkNotNull(paramFile);
return new ParamFileCommandLine(
paramFile, linkTargetType, forcedToolPath, featureConfiguration, actionName, variables);
}

private static void extractArguments(
List<String> args, List<String> commandlineArgs, List<String> paramFileArgs) {
commandlineArgs.add(args.get(0)); // ar command, must not be moved!
int argsSize = args.size();
for (int i = 1; i < argsSize; i++) {
String arg = args.get(i);
if (isLikelyParamFile(arg)) {
commandlineArgs.add(arg); // params file, keep it in the command line
public List<String> getCommandLine(@Nullable ArtifactExpander expander)
throws CommandLineExpansionException {
// Try to shorten the command line by use of a parameter file.
// This makes the output with --subcommands (et al) more readable.
List<String> argv = new ArrayList<>();
argv.add(getLinkerPathString());
try {
if (paramFile != null) {
// Retrieve only reference to linker_param_file from the command line.
String linkerParamFile =
variables
.getVariable(LINKER_PARAM_FILE.getVariableName())
.getStringValue(LINKER_PARAM_FILE.getVariableName());
argv.addAll(
featureConfiguration.getCommandLine(actionName, variables, expander).stream()
.filter(s -> s.contains(linkerParamFile))
.collect(toImmutableList()));
} else {
paramFileArgs.add(arg); // the rest goes to the params file
argv.addAll(featureConfiguration.getCommandLine(actionName, variables, expander));
}
} catch (ExpansionException e) {
throw new CommandLineExpansionException(e.getMessage());
}
return argv;
}

private static boolean isLikelyParamFile(String arg) {
return arg.startsWith("@")
&& !arg.startsWith("@rpath")
&& !arg.startsWith("@loader_path")
&& !arg.startsWith("@executable_path");
}

/**
* Returns a raw link command for the given link invocation, including both command and arguments
* (argv).
*
* @param expander ArtifactExpander for expanding TreeArtifacts.
* @return raw link command line.
*/
private List<String> getRawLinkArgv(@Nullable ArtifactExpander expander)
throws CommandLineExpansionException {
return getRawLinkArgv(
expander, forcedToolPath, featureConfiguration, actionName, linkTargetType, variables);
}

private static List<String> getRawLinkArgv(
@Nullable ArtifactExpander expander,
String forcedToolPath,
FeatureConfiguration featureConfiguration,
String actionName,
LinkTargetType linkTargetType,
CcToolchainVariables variables)
public List<String> getParamCommandLine(@Nullable ArtifactExpander expander)
throws CommandLineExpansionException {
List<String> argv = new ArrayList<>();
if (forcedToolPath != null) {
argv.add(forcedToolPath);
} else {
Preconditions.checkArgument(
featureConfiguration.actionIsConfigured(actionName),
String.format("Expected action_config for '%s' to be configured", actionName));
argv.add(featureConfiguration.getToolPathForAction(linkTargetType.getActionName()));
}
try {
argv.addAll(featureConfiguration.getCommandLine(actionName, variables, expander));
if (variables.isAvailable(LINKER_PARAM_FILE.getVariableName())) {
// Filter out linker_param_file
String linkerParamFile =
variables
.getVariable(LINKER_PARAM_FILE.getVariableName())
.getStringValue(LINKER_PARAM_FILE.getVariableName());
argv.addAll(
featureConfiguration.getCommandLine(actionName, variables, expander).stream()
.filter(s -> !s.contains(linkerParamFile))
.collect(toImmutableList()));
} else {
argv.addAll(featureConfiguration.getCommandLine(actionName, variables, expander));
}
} catch (ExpansionException e) {
throw new CommandLineExpansionException(e.getMessage());
}
return argv;
}

List<String> getCommandLine(@Nullable ArtifactExpander expander)
throws CommandLineExpansionException {
// 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(expander);
return split.first;
} else {
return getRawLinkArgv(expander);
}
}

@Override
public List<String> arguments() throws CommandLineExpansionException {
return getRawLinkArgv(null);
return arguments(null, null);
}

@Override
public List<String> arguments(ArtifactExpander artifactExpander, PathMapper pathMapper)
throws CommandLineExpansionException {
return getRawLinkArgv(artifactExpander);
return ImmutableList.<String>builder()
.add(getLinkerPathString())
.addAll(getParamCommandLine(artifactExpander))
.build();
}

/** A builder for a {@link LinkCommandLine}. */
Expand Down
32 changes: 20 additions & 12 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLineTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
import com.google.devtools.build.lib.rules.cpp.CppActionConfigs.CppPlatform;
import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
import com.google.devtools.build.lib.testutil.TestUtils;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand Down Expand Up @@ -211,7 +210,7 @@ public void testLinkerParamFileForStaticLibrary() throws Exception {
.setActionName(LinkTargetType.STATIC_LIBRARY.getActionName())
.setLinkTargetType(LinkTargetType.STATIC_LIBRARY)
.build();
assertThat(linkConfig.arguments()).contains("@foo/bar.param");
assertThat(linkConfig.getCommandLine(null)).contains("@foo/bar.param");
}

@Test
Expand All @@ -226,7 +225,7 @@ public void testLinkerParamFileForDynamicLibrary() throws Exception {
.setActionName(LinkTargetType.NODEPS_DYNAMIC_LIBRARY.getActionName())
.setLinkTargetType(LinkTargetType.NODEPS_DYNAMIC_LIBRARY)
.build();
assertThat(linkConfig.arguments()).contains("@foo/bar.param");
assertThat(linkConfig.getCommandLine(null)).contains("@foo/bar.param");
}

private List<String> basicArgv(LinkTargetType targetType) throws Exception {
Expand Down Expand Up @@ -288,9 +287,12 @@ public void testSplitStaticLinkCommand() throws Exception {
.forceToolPath("foo/bar/ar")
.setParamFile(paramFile)
.build();
Pair<List<String>, List<String>> result = linkConfig.splitCommandline();
assertThat(result.first).isEqualTo(Arrays.asList("foo/bar/ar", "@some/file.params"));
assertThat(result.second).isEqualTo(Arrays.asList("rcsD", "a/FakeOutput"));
assertThat(linkConfig.getCommandLine(null))
.containsExactly("foo/bar/ar", "@some/file.params")
.inOrder();
assertThat(linkConfig.getParamCommandLine(null))
.containsExactly("rcsD", "a/FakeOutput")
.inOrder();
}

@Test
Expand All @@ -311,9 +313,12 @@ public void testSplitDynamicLinkCommand() throws Exception {
.forceToolPath("foo/bar/linker")
.setParamFile(paramFile)
.build();
Pair<List<String>, List<String>> result = linkConfig.splitCommandline();
assertThat(result.first).containsExactly("foo/bar/linker", "@some/file.params").inOrder();
assertThat(result.second).containsExactly("-shared", "-o", "a/FakeOutput", "").inOrder();
assertThat(linkConfig.getCommandLine(null))
.containsExactly("foo/bar/linker", "@some/file.params")
.inOrder();
assertThat(linkConfig.getParamCommandLine(null))
.containsExactly("-shared", "-o", "a/FakeOutput", "")
.inOrder();
}

@Test
Expand Down Expand Up @@ -354,10 +359,13 @@ public void testSplitAlwaysLinkLinkCommand() throws Exception {
.forceToolPath("foo/bar/ar")
.setParamFile(paramFile)
.build();
Pair<List<String>, List<String>> result = linkConfig.splitCommandline();

assertThat(result.first).isEqualTo(Arrays.asList("foo/bar/ar", "@some/file.params"));
assertThat(result.second).isEqualTo(Arrays.asList("rcsD", "a/FakeOutput", "foo.o", "bar.o"));
assertThat(linkConfig.getCommandLine(null))
.containsExactly("foo/bar/ar", "@some/file.params")
.inOrder();
assertThat(linkConfig.getParamCommandLine(null))
.containsExactly("rcsD", "a/FakeOutput", "foo.o", "bar.o")
.inOrder();
}

private SpecialArtifact createTreeArtifact(String name) {
Expand Down

0 comments on commit b5cfea5

Please sign in to comment.