From 4cb32a994308e1b4921471e32a02c49d8ceffb84 Mon Sep 17 00:00:00 2001 From: Marcel Hlopko Date: Tue, 17 Jan 2017 15:41:12 +0000 Subject: Expose thinlto_params_file and linker_param_file as build variables This cl exposes param files that were hard-coded before. This enables more precise placement on the link command line. This is a roll-forward of commit db7a9ea7f6b2af3c4c1f43ad4aa50cd4eca02921 -- PiperOrigin-RevId: 144708270 MOS_MIGRATED_REVID=144708270 --- .../build/lib/rules/cpp/CppLinkActionBuilder.java | 110 ++++++++++++++------- .../build/lib/rules/cpp/CppLinkActionConfigs.java | 31 ++++++ .../build/lib/rules/cpp/LinkCommandLine.java | 30 +++--- 3 files changed, 123 insertions(+), 48 deletions(-) (limited to 'src/main') diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java index f82c35b162..6bd3ca64c3 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java @@ -80,6 +80,20 @@ public class CppLinkActionBuilder { /** A build variable for flags providing files to link as inputs in the linker invocation */ public static final String LIBRARIES_TO_LINK_VARIABLE = "libraries_to_link"; + /** + * A build variable for thinlto param file produced by thinlto-indexing action and consumed by + * normal linking actions. + */ + public static final String THINLTO_PARAM_FILE_VARIABLE = "thinlto_param_file"; + + public static final String THINLTO_PREFIX_REPLACE_VARIABLE = "thinlto_prefix_replace"; + + /** + * A build variable for linker param file created by Bazel to overcome the command line length + * limit. + */ + public static final String LINKER_PARAM_FILE_VARIABLE = "linker_param_file"; + /** A build variable for the execpath of the output of the linker. */ public static final String OUTPUT_EXECPATH_VARIABLE = "output_execpath"; @@ -416,6 +430,10 @@ public class CppLinkActionBuilder { @VisibleForTesting boolean canSplitCommandLine() { + if (fake) { + return false; + } + if (toolchain == null || !toolchain.supportsParamFiles()) { return false; } @@ -510,8 +528,8 @@ public class CppLinkActionBuilder { allLTOArtifacts = createLTOArtifacts(ltoOutputRootPrefix, uniqueLibraries); } - PathFragment linkerParamsFileRootPath = null; - @Nullable Artifact linkerParamsFile = null; + PathFragment linkerParamFileRootPath = null; + @Nullable Artifact thinltoParamFile = null; if (allLTOArtifacts != null) { // Create artifact for the file that the LTO indexing step will emit // object file names into for any that were included in the link as @@ -520,10 +538,10 @@ public class CppLinkActionBuilder { // Note that the paths emitted into this file will have their prefixes // replaced with the final output directory, so they will be the paths // of the native object files not the input bitcode files. - linkerParamsFileRootPath = + linkerParamFileRootPath = ParameterFile.derivePath(output.getRootRelativePath(), "lto-final"); - linkerParamsFile = - linkArtifactFactory.create(ruleContext, configuration, linkerParamsFileRootPath); + thinltoParamFile = + linkArtifactFactory.create(ruleContext, configuration, linkerParamFileRootPath); } final ImmutableList actionOutputs; @@ -532,8 +550,8 @@ public class CppLinkActionBuilder { for (LTOBackendArtifacts ltoA : allLTOArtifacts) { ltoA.addIndexingOutputs(builder); } - if (linkerParamsFile != null) { - builder.add(linkerParamsFile); + if (thinltoParamFile != null) { + builder.add(thinltoParamFile); } actionOutputs = builder.build(); } else { @@ -548,6 +566,15 @@ public class CppLinkActionBuilder { ImmutableList runtimeLinkerInputs = ImmutableList.copyOf(LinkerInputs.simpleLinkerInputs(runtimeInputs, runtimeType)); + PathFragment paramRootPath = + ParameterFile.derivePath(output.getRootRelativePath(), (isLTOIndexing) ? "lto-index" : "2"); + + @Nullable + final Artifact paramFile = + canSplitCommandLine() + ? linkArtifactFactory.create(ruleContext, configuration, paramRootPath) + : null; + // Add build variables necessary to template link args into the crosstool. Variables.Builder buildVariablesBuilder = new Variables.Builder(); CppLinkVariablesExtension variablesExtension = @@ -559,7 +586,8 @@ public class CppLinkActionBuilder { linkerInputs, runtimeLinkerInputs, null, - linkerParamsFile, + paramFile, + thinltoParamFile, ltoOutputRootPrefix, null, null) @@ -570,7 +598,8 @@ public class CppLinkActionBuilder { linkerInputs, runtimeLinkerInputs, output, - linkerParamsFile, + paramFile, + thinltoParamFile, PathFragment.EMPTY_FRAGMENT, getInterfaceSoBuilder(), interfaceOutput); @@ -580,15 +609,6 @@ public class CppLinkActionBuilder { } Variables buildVariables = buildVariablesBuilder.build(); - PathFragment paramRootPath = - ParameterFile.derivePath(output.getRootRelativePath(), (isLTOIndexing) ? "lto-index" : "2"); - - @Nullable - final Artifact paramFile = - canSplitCommandLine() - ? linkArtifactFactory.create(ruleContext, configuration, paramRootPath) - : null; - Preconditions.checkArgument( linkType != LinkTargetType.INTERFACE_DYNAMIC_LIBRARY, "you can't link an interface dynamic library directly"); @@ -704,8 +724,8 @@ public class CppLinkActionBuilder { .add(dependencyInputsBuilder.build()) .add(ImmutableIterable.from(expandedInputs)); - if (linkerParamsFile != null && !isLTOIndexing) { - inputsBuilder.add(ImmutableList.of(linkerParamsFile)); + if (thinltoParamFile != null && !isLTOIndexing) { + inputsBuilder.add(ImmutableList.of(thinltoParamFile)); } if (linkCommandLine.getParamFile() != null) { inputsBuilder.add(ImmutableList.of(linkCommandLine.getParamFile())); @@ -1230,7 +1250,8 @@ public class CppLinkActionBuilder { private final Artifact outputArtifact; private final Artifact interfaceLibraryBuilder; private final Artifact interfaceLibraryOutput; - private final Artifact linkerParamsFile; + private final Artifact paramFile; + private final Artifact thinltoParamFile; private final PathFragment ltoOutputRootPrefix; private final LinkArgCollector linkArgCollector = new LinkArgCollector(); @@ -1242,7 +1263,8 @@ public class CppLinkActionBuilder { Iterable linkerInputs, ImmutableList runtimeLinkerInputs, Artifact output, - Artifact linkerParamsFile, + Artifact paramFile, + Artifact thinltoParamFile, PathFragment ltoOutputRootPrefix, Artifact interfaceLibraryBuilder, Artifact interfaceLibraryOutput) { @@ -1254,7 +1276,8 @@ public class CppLinkActionBuilder { this.outputArtifact = output; this.interfaceLibraryBuilder = interfaceLibraryBuilder; this.interfaceLibraryOutput = interfaceLibraryOutput; - this.linkerParamsFile = linkerParamsFile; + this.paramFile = paramFile; + this.thinltoParamFile = thinltoParamFile; this.ltoOutputRootPrefix = ltoOutputRootPrefix; addInputFileLinkOptions(linkArgCollector); @@ -1307,6 +1330,10 @@ public class CppLinkActionBuilder { buildVariables.addStringSequenceVariable( LIBRARY_SEARCH_DIRECTORIES_VARIABLE, linkArgCollector.getLibrarySearchDirectories()); + if (paramFile != null) { + buildVariables.addStringVariable(LINKER_PARAM_FILE_VARIABLE, paramFile.getExecPathString()); + } + // mostly static if (linkStaticness == LinkStaticness.MOSTLY_STATIC && cppConfiguration.skipStaticOutputs()) { buildVariables.addStringVariable(SKIP_MOSTLY_STATIC_VARIABLE, ""); @@ -1318,18 +1345,30 @@ public class CppLinkActionBuilder { OUTPUT_EXECPATH_VARIABLE, outputArtifact.getExecPathString()); } - if (!ltoOutputRootPrefix.equals(PathFragment.EMPTY_FRAGMENT)) { - if (linkerParamsFile != null) { + if (isLTOIndexing()) { + if (thinltoParamFile != null) { + // This is a lto-indexing action and we want it to populate param file. + buildVariables.addStringVariable( + THINLTO_PARAM_FILE_VARIABLE, thinltoParamFile.getExecPathString()); + // TODO(b/33846234): Remove once all the relevant crosstools don't depend on the variable. buildVariables.addStringVariable( - "thinlto_optional_params_file", "=" + linkerParamsFile.getExecPathString()); + "thinlto_optional_params_file", "=" + thinltoParamFile.getExecPathString()); } else { + buildVariables.addStringVariable(THINLTO_PARAM_FILE_VARIABLE, ""); + // TODO(b/33846234): Remove once all the relevant crosstools don't depend on the variable. buildVariables.addStringVariable("thinlto_optional_params_file", ""); } buildVariables.addStringVariable( - "thinlto_prefix_replace", + THINLTO_PREFIX_REPLACE_VARIABLE, configuration.getBinDirectory().getExecPathString() + ";" + configuration.getBinDirectory().getExecPath().getRelative(ltoOutputRootPrefix)); + } else { + if (thinltoParamFile != null) { + // This is a normal link action and we need to use param file created by lto-indexing. + buildVariables.addStringVariable( + THINLTO_PARAM_FILE_VARIABLE, thinltoParamFile.getExecPathString()); + } } boolean shouldGenerateInterfaceLibrary = outputArtifact != null @@ -1354,6 +1393,10 @@ public class CppLinkActionBuilder { CppHelper.getFdoSupport(ruleContext).getLinkOptions(featureConfiguration, buildVariables); } + private boolean isLTOIndexing() { + return !ltoOutputRootPrefix.equals(PathFragment.EMPTY_FRAGMENT); + } + private boolean isSharedNativeLibrary() { return isNativeDeps && cppConfiguration.shareNativeDeps(); } @@ -1512,11 +1555,6 @@ public class CppLinkActionBuilder { addStaticInputLinkOptions(input, librariesToLink, true, ltoMap); } } - if (linkerParamsFile != null && ltoOutputRootPrefix.equals(PathFragment.EMPTY_FRAGMENT)) { - librariesToLink.addValue( - new LibraryToLinkValue( - "-Wl,@" + linkerParamsFile.getExecPathString(), needWholeArchive)); - } // rpath ordering matters for performance; first add the one where most libraries are found. if (includeSolibDir && rpathRoot != null) { @@ -1604,13 +1642,13 @@ public class CppLinkActionBuilder { @Nullable Map ltoMap) { Preconditions.checkState(!(input.getArtifactCategory() == ArtifactCategory.DYNAMIC_LIBRARY)); // If we had any LTO artifacts, ltoMap whould be non-null. In that case, - // we should have created a linkerParamsFile which the LTO indexing + // we should have created a thinltoParamFile which the LTO indexing // step will populate with the exec paths that correspond to the LTO // artifacts that the linker decided to include based on symbol resolution. // Those files will be included directly in the link (and not wrapped // in --start-lib/--end-lib) to ensure consistency between the two link // steps. - Preconditions.checkState(ltoMap == null || linkerParamsFile != null); + Preconditions.checkState(ltoMap == null || thinltoParamFile != null); // start-lib/end-lib library: adds its input object files. if (Link.useStartEndLib(input, cppConfiguration.archiveType())) { @@ -1620,7 +1658,7 @@ public class CppLinkActionBuilder { for (Artifact member : archiveMembers) { if (ltoMap != null && ltoMap.remove(member) != null) { // The LTO artifacts that should be included in the final link - // are listed in the linkerParamsFile. When ltoMap is non-null + // are listed in the thinltoParamFile. When ltoMap is non-null // the backend artifact may be missing due to libraries that list .o // files explicitly, or generate .o files from assembler. continue; @@ -1645,7 +1683,7 @@ public class CppLinkActionBuilder { if (ltoMap != null && ltoMap.remove(inputArtifact) != null) { // The LTO artifacts that should be included in the final link - // are listed in the linkerParamsFile. + // are listed in the thinltoParamFile. return; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionConfigs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionConfigs.java index 8b8ae161b9..f0bd59a746 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionConfigs.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionConfigs.java @@ -68,6 +68,7 @@ public class CppLinkActionConfigs { " implies: 'libraries_to_link'", " implies: 'force_pic_flags'", " implies: 'legacy_link_flags'", + " implies: 'linker_param_file'", "}", "action_config {", " config_name: 'c++-link-dynamic-library'", @@ -86,6 +87,7 @@ public class CppLinkActionConfigs { " implies: 'input_param_flags'", " implies: 'libraries_to_link'", " implies: 'legacy_link_flags'", + " implies: 'linker_param_file'", "}", "action_config {", " config_name: 'c++-link-static-library'", @@ -97,6 +99,7 @@ public class CppLinkActionConfigs { " implies: 'library_search_directories'", " implies: 'input_param_flags'", " implies: 'libraries_to_link'", + " implies: 'linker_param_file'", "}", "action_config {", " config_name: 'c++-link-alwayslink-static-library'", @@ -108,6 +111,7 @@ public class CppLinkActionConfigs { " implies: 'library_search_directories'", " implies: 'input_param_flags'", " implies: 'libraries_to_link'", + " implies: 'linker_param_file'", "}", "action_config {", " config_name: 'c++-link-pic-static-library'", @@ -119,6 +123,7 @@ public class CppLinkActionConfigs { " implies: 'library_search_directories'", " implies: 'input_param_flags'", " implies: 'libraries_to_link'", + " implies: 'linker_param_file'", "}", "action_config {", " config_name: 'c++-link-alwayslink-pic-static-library'", @@ -130,6 +135,7 @@ public class CppLinkActionConfigs { " implies: 'library_search_directories'", " implies: 'input_param_flags'", " implies: 'libraries_to_link'", + " implies: 'linker_param_file'", "}", "feature {", " name: 'build_interface_libraries'", @@ -322,6 +328,10 @@ public class CppLinkActionConfigs { " flag: '-Wl,--end-lib'", " }", " }", + " flag_group {", + " expand_if_true: 'thinlto_param_file'", + " flag: '-Wl,@%{thinlto_param_file}'", + " }", " }", "}", "feature {", @@ -345,6 +355,27 @@ public class CppLinkActionConfigs { " flag: '%{legacy_link_flags}'", " }", " }", + "}", + "feature {", + " name: 'linker_param_file'", + " flag_set {", + " expand_if_all_available: 'linker_param_file'", + " action: 'c++-link-executable'", + " action: 'c++-link-dynamic-library'", + " flag_group {", + " flag: '-Wl,@%{linker_param_file}'", + " }", + " }", + " flag_set {", + " expand_if_all_available: 'linker_param_file'", + " action: 'c++-link-static-library'", + " action: 'c++-link-alwayslink-static-library'", + " action: 'c++-link-pic-static-library'", + " action: 'c++-link-alwayslink-pic-static-library'", + " flag_group {", + " flag: '@%{linker_param_file}'", + " }", + " }", "}")); } 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 b253826683..67cb8a3e8c 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 @@ -218,19 +218,15 @@ 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). Should only be used * if getParamFile() is not null. - * - * @throws IllegalStateException if the command-line cannot be split */ @VisibleForTesting final Pair, List> splitCommandline() { List args = getRawLinkArgv(); if (linkTargetType.staticness() == Staticness.STATIC) { // Ar link commands can also generate huge command lines. - List paramFileArgs = args.subList(1, args.size()); + List paramFileArgs = new ArrayList<>(); List commandlineArgs = new ArrayList<>(); - commandlineArgs.add(args.get(0)); - - commandlineArgs.add("@" + paramFile.getExecPath().getPathString()); + extractArgumentsForStaticLinkParamFile(args, commandlineArgs, paramFileArgs); return Pair.of(commandlineArgs, paramFileArgs); } else { // Gcc link commands tend to generate humongous commandlines for some targets, which may @@ -238,17 +234,14 @@ public final class LinkCommandLine extends CommandLine { // a parameter file and pass any linker options through it. List paramFileArgs = new ArrayList<>(); List commandlineArgs = new ArrayList<>(); - extractArgumentsForParamFile(args, commandlineArgs, paramFileArgs); + extractArgumentsForDynamicLinkParamFile(args, commandlineArgs, paramFileArgs); - commandlineArgs.add("-Wl,@" + paramFile.getExecPath().getPathString()); return Pair.of(commandlineArgs, paramFileArgs); } } /** * Returns just the .params file portion of the command-line as a {@link CommandLine}. - * - * @throws IllegalStateException if the command-line cannot be split */ CommandLine paramCmdLine() { Preconditions.checkNotNull(paramFile); @@ -260,9 +253,22 @@ public final class LinkCommandLine extends CommandLine { }; } + public static void extractArgumentsForStaticLinkParamFile( + 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 (arg.startsWith("@")) { + commandlineArgs.add(arg); // params file, keep it in the command line + } else { + paramFileArgs.add(arg); // the rest goes to the params file + } + } + } - public static void extractArgumentsForParamFile(List args, List commandlineArgs, - List paramFileArgs) { + public static void extractArgumentsForDynamicLinkParamFile( + List args, List commandlineArgs, List 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! -- cgit v1.2.3