From 2101281d8bdad957ed1f624b2b10d8c7c6c36f65 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 5 Jan 2024 00:17:42 -0800 Subject: [PATCH] Refactor splitting of LinkerCommandLine 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: https://github.com/bazelbuild/bazel/issues/18074 PiperOrigin-RevId: 595912823 Change-Id: I54b36113d87f975af63341b2dec17b2f861c0ffa --- .../build/lib/rules/cpp/CppLinkAction.java | 6 +- .../build/lib/rules/cpp/LinkCommandLine.java | 215 +++++------------- .../lib/rules/cpp/LinkCommandLineTest.java | 32 ++- 3 files changed, 78 insertions(+), 175 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 c6ab67616171ff..25577e927b4c50 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 @@ -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; @@ -274,11 +273,8 @@ public Sequence 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); 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 00d8b9d8002d89..76c33697072c86 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 @@ -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; @@ -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; @@ -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()); + } } /** @@ -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> splitCommandline() throws CommandLineExpansionException { - return splitCommandline(paramFile, getRawLinkArgv(null)); - } - - @VisibleForTesting - final Pair, List> splitCommandline(@Nullable ArtifactExpander expander) - throws CommandLineExpansionException { - return splitCommandline(paramFile, getRawLinkArgv(expander)); - } - - private static Pair, List> splitCommandline( - Artifact paramFile, List 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 paramFileArgs = new ArrayList<>(); - List commandlineArgs = new ArrayList<>(); - extractArguments(args, commandlineArgs, paramFileArgs); - return Pair.of(commandlineArgs, paramFileArgs); - } - - public CommandLine getCommandLineForStarlark() { return new CommandLine() { @Override public Iterable arguments() throws CommandLineExpansionException { - return arguments(/* artifactExpander= */ null, PathMapper.NOOP); + return getParamCommandLine(null); } @Override - public ImmutableList arguments( - ArtifactExpander artifactExpander, PathMapper pathMapper) + public List arguments(ArtifactExpander expander, PathMapper pathMapper) throws CommandLineExpansionException { - if (paramFile == null) { - return ImmutableList.copyOf(getRawLinkArgv(artifactExpander)); - } else { - return ImmutableList.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 arguments() throws CommandLineExpansionException { - List argv = - getRawLinkArgv( - null, forcedToolPath, featureConfiguration, actionName, linkTargetType, variables); - return splitCommandline(paramsFile, argv).getSecond(); - } - - @Override - public Iterable arguments(ArtifactExpander expander, PathMapper pathMapper) - throws CommandLineExpansionException { - List 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 args, List commandlineArgs, List 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 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 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 getRawLinkArgv(@Nullable ArtifactExpander expander) - throws CommandLineExpansionException { - return getRawLinkArgv( - expander, forcedToolPath, featureConfiguration, actionName, linkTargetType, variables); - } - - private static List getRawLinkArgv( - @Nullable ArtifactExpander expander, - String forcedToolPath, - FeatureConfiguration featureConfiguration, - String actionName, - LinkTargetType linkTargetType, - CcToolchainVariables variables) + public List getParamCommandLine(@Nullable ArtifactExpander expander) throws CommandLineExpansionException { List 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 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> split = splitCommandline(expander); - return split.first; - } else { - return getRawLinkArgv(expander); - } - } - @Override public List arguments() throws CommandLineExpansionException { - return getRawLinkArgv(null); + return arguments(null, null); } @Override public List arguments(ArtifactExpander artifactExpander, PathMapper pathMapper) throws CommandLineExpansionException { - return getRawLinkArgv(artifactExpander); + return ImmutableList.builder() + .add(getLinkerPathString()) + .addAll(getParamCommandLine(artifactExpander)) + .build(); } /** A builder for a {@link LinkCommandLine}. */ diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLineTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLineTest.java index aac913a858ce5d..f79135693e5237 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLineTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLineTest.java @@ -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; @@ -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 @@ -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 basicArgv(LinkTargetType targetType) throws Exception { @@ -288,9 +287,12 @@ public void testSplitStaticLinkCommand() throws Exception { .forceToolPath("foo/bar/ar") .setParamFile(paramFile) .build(); - Pair, List> 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 @@ -311,9 +313,12 @@ public void testSplitDynamicLinkCommand() throws Exception { .forceToolPath("foo/bar/linker") .setParamFile(paramFile) .build(); - Pair, List> 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 @@ -354,10 +359,13 @@ public void testSplitAlwaysLinkLinkCommand() throws Exception { .forceToolPath("foo/bar/ar") .setParamFile(paramFile) .build(); - Pair, List> 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) {