Skip to content

Commit

Permalink
[7.1.0] Cherry-pick: linker_param_file only added to command line if …
Browse files Browse the repository at this point in the history 
…it starts with "@" (#21235)

Cherry-picks of:
c92ecbf Replace calls to getRawLinkArgv with calls to arguments
3490df2 Merge extractArgumentsForStaticLinkParamFile and
extractArgumentsForDynamicLinkParamFile
b5cfea5 Refactor splitting of LinkerCommandLine

Fixes: #20761
  • Loading branch information
@comius
comius authored Feb 8, 2024
1 parent ffa8004 commit 90d9909
Show file tree
Hide file tree
Showing 6 changed files with 111 additions and 241 deletions.
8 changes: 2 additions & 6 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 @@ -287,11 +286,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 Expand Up @@ -399,7 +395,7 @@ public ExtraActionInfo.Builder getExtraActionInfo(ActionKeyContext actionKeyCont
info.setLinkStaticness(linkCommandLine.getLinkingMode().name());
info.addAllLinkStamp(Artifact.toExecPaths(getLinkstampObjects()));
info.addAllBuildInfoHeaderArtifact(Artifact.toExecPaths(getBuildInfoHeaderArtifacts()));
info.addAllLinkOpt(linkCommandLine.getRawLinkArgv(null));
info.addAllLinkOpt(linkCommandLine.arguments());

try {
return super.getExtraActionInfo(actionKeyContext)
Expand Down
248 changes: 57 additions & 191 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 Down Expand Up @@ -124,7 +127,15 @@ public Link.LinkingMode getLinkingMode() {

/** 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 @@ -157,229 +168,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), linkTargetType);
}

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

private static Pair<List<String>, List<String>> splitCommandline(
Artifact paramFile, List<String> args, LinkTargetType linkTargetType) {
/** Returns just the .params file portion of the command-line as a {@link CommandLine}. */
CommandLine paramCmdLine() {
Preconditions.checkNotNull(paramFile);
if (linkTargetType.linkerOrArchiver() == LinkerOrArchiver.ARCHIVER) {
// Ar link commands can also generate huge command lines.
List<String> paramFileArgs = new ArrayList<>();
List<String> commandlineArgs = new ArrayList<>();
extractArgumentsForStaticLinkParamFile(args, commandlineArgs, paramFileArgs);
return Pair.of(commandlineArgs, paramFileArgs);
} else {
// Gcc 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<>();
extractArgumentsForDynamicLinkParamFile(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, linkTargetType).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, linkTargetType).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 extractArgumentsForStaticLinkParamFile(
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
} else {
paramFileArgs.add(arg); // the rest goes to the params file
}
}
}

private static void extractArgumentsForDynamicLinkParamFile(
List<String> args, List<String> commandlineArgs, List<String> paramFileArgs) {
// Note, that it is not important that all linker arguments are extracted so that
// they can be moved into a parameter file, but the vast majority should.
commandlineArgs.add(args.get(0)); // gcc 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). The version that uses the expander is preferred, but that one can't be used during
* analysis.
*
* @return raw link command line.
*/
public List<String> getRawLinkArgv() throws CommandLineExpansionException {
return getRawLinkArgv(null);
}

/**
* 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.
*/
public 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
20 changes: 10 additions & 10 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinaryThinLtoObjDirTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,9 +278,9 @@ public void testActionGraph() throws Exception {
assertThat(ActionsTestUtil.getFirstArtifactEndingWith(linkAction.getInputs(), "linkstamp.o"))
.isNotNull();

List<String> commandLine = linkAction.getLinkCommandLineForTesting().getRawLinkArgv();
String prefix = getTargetConfiguration().getOutputDirectory(RepositoryName.MAIN)
.getExecPathString();
List<String> commandLine = linkAction.getLinkCommandLineForTesting().arguments();
String prefix =
getTargetConfiguration().getOutputDirectory(RepositoryName.MAIN).getExecPathString();
assertThat(commandLine)
.containsAtLeast(
prefix + "/bin/pkg/bin.lto.merged.o",
Expand Down Expand Up @@ -421,9 +421,9 @@ public void testNoLinkstatic() throws Exception {
CppLinkAction linkAction = getLinkAction();
String rootExecPath = getRootExecPath();

List<String> commandLine = linkAction.getLinkCommandLineForTesting().getRawLinkArgv();
String prefix = getTargetConfiguration().getOutputDirectory(RepositoryName.MAIN)
.getExecPathString();
List<String> commandLine = linkAction.getLinkCommandLineForTesting().arguments();
String prefix =
getTargetConfiguration().getOutputDirectory(RepositoryName.MAIN).getExecPathString();

assertThat(commandLine).contains("-Wl,@" + prefix + "/bin/pkg/bin-lto-final.params");

Expand Down Expand Up @@ -1818,7 +1818,7 @@ public void testPropellerOptimizeAbsoluteOptions() throws Exception {
assertThat(ActionsTestUtil.baseArtifactNames(linkAction.getInputs()))
.contains("ld_profile.txt");

List<String> commandLine = linkAction.getLinkCommandLineForTesting().getRawLinkArgv();
List<String> commandLine = linkAction.getLinkCommandLineForTesting().arguments();
assertThat(commandLine.toString())
.containsMatch("-Wl,--symbol-ordering-file=.*/ld_profile.txt");

Expand Down Expand Up @@ -1965,7 +1965,7 @@ public void testPropellerHostBuilds() throws Exception {
(CppLinkAction) getPredecessorByInputName(genruleAction, "pkg/gen_lib");
assertThat(ActionsTestUtil.baseArtifactNames(hostLinkAction.getInputs()))
.doesNotContain("ld_profile.txt");
assertThat(hostLinkAction.getLinkCommandLineForTesting().getRawLinkArgv().toString())
assertThat(hostLinkAction.getLinkCommandLineForTesting().arguments().toString())
.doesNotContainMatch("-Wl,--symbol-ordering-file=.*/ld_profile.txt");

// The hostLinkAction inputs has a different root from the backendAction.
Expand All @@ -1985,7 +1985,7 @@ public void testPropellerHostBuilds() throws Exception {
assertThat(hostIndexAction).isNotNull();
assertThat(ActionsTestUtil.baseArtifactNames(hostIndexAction.getInputs()))
.doesNotContain("ld_profile.txt");
assertThat(hostIndexAction.getLinkCommandLineForTesting().getRawLinkArgv().toString())
assertThat(hostIndexAction.getLinkCommandLineForTesting().arguments().toString())
.doesNotContainMatch("-Wl,--symbol-ordering-file=.*/ld_profile.txt");

CppCompileAction hostBitcodeAction =
Expand Down Expand Up @@ -2023,7 +2023,7 @@ private void testPropellerOptimizeOption(boolean label) throws Exception {
CppLinkAction linkAction = (CppLinkAction) getGeneratingAction(binArtifact);
assertThat(linkAction.getOutputs()).containsExactly(binArtifact);

List<String> commandLine = linkAction.getLinkCommandLineForTesting().getRawLinkArgv();
List<String> commandLine = linkAction.getLinkCommandLineForTesting().arguments();
assertThat(commandLine.toString())
.containsMatch("-Wl,--symbol-ordering-file=.*/ld_profile.txt");

Expand Down
Loading

0 comments on commit 90d9909

Please sign in to comment.