Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[7.1.0] Cherry-pick: linker_param_file only added to command line if it starts with "@" #21235

Merged
merged 3 commits into from
Feb 8, 2024
Merged

[7.1.0] Cherry-pick: linker_param_file only added to command line if it starts with "@" #21235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d24eb8a
Merge extractArgumentsForStaticLinkParamFile and extractArgumentsForD…
comius Jan 4, 2024
7f62f0e
Replace calls to getRawLinkArgv with calls to arguments
comius Jan 4, 2024
ebf5a8f
Refactor splitting of LinkerCommandLine
comius Jan 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading