Simplify propagation of linker params by replacing global_whole_archive with individual --whole_archive/--no_whole_archive blocks. -- PiperOrigin-RevId: 141555295 MOS_MIGRATED_REVID=141555295
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java index 8c15eb7..be01fd4 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
@@ -953,6 +953,85 @@ * significantly reduces memory overhead. */ @Immutable + public static final class LibraryToLinkValue implements VariableValue { + + public static final String NAMES_FIELD_NAME = "names"; + public static final String IS_WHOLE_ARCHIVE_FIELD_NAME = "is_whole_archive"; + public static final String IS_LIB_GROUP_FIELD_NAME = "is_lib_group"; + + private final ImmutableList<String> names; + private final boolean isWholeArchive; + private final boolean isLibGroup; + + public LibraryToLinkValue(String name, boolean isWholeArchive) { + this(ImmutableList.of(name), isWholeArchive, false); + } + + public LibraryToLinkValue( + ImmutableList<String> names, boolean isWholeArchive, boolean isLibGroup) { + this.names = names; + this.isWholeArchive = isWholeArchive; + this.isLibGroup = isLibGroup; + } + + @Override + public Iterable<? extends VariableValue> getSequenceValue(String variableName) { + throw new ExpansionException( + String.format( + "Invalid toolchain configuration: Cannot expand variable '%s': expected sequence, " + + "found structure (LibraryToLink)", + variableName)); + } + + @Override + public boolean isSequence() { + return false; + } + + @Override + public VariableValue getFieldValue(String variableName, String field) { + Preconditions.checkNotNull(field); + if (NAMES_FIELD_NAME.equals(field)) { + return new StringSequence(names); + } else if (IS_WHOLE_ARCHIVE_FIELD_NAME.equals(field)) { + return new IntegerValue(isWholeArchive ? 1 : 0); + } else if (IS_LIB_GROUP_FIELD_NAME.equals(field)) { + return new IntegerValue(isLibGroup ? 1 : 0); + } else if ("whole_archive_presence".equals(field)) { + // TODO(b/33403458): Cleanup this workaround once bazel >=0.4.3 is released. + return isWholeArchive ? new IntegerValue(0) : null; + } else if ("no_whole_archive_presence".equals(field)) { + // TODO(b/33403458): Cleanup this workaround once bazel >=0.4.3 is released. + return !isWholeArchive ? new IntegerValue(0) : null; + } else if ("lib_group_presence".equals(field)) { + // TODO(b/33403458): Cleanup this workaround once bazel >=0.4.3 is released. + return names.size() > 1 ? new IntegerValue(0) : null; + } else { + return null; + } + } + + @Override + public String getStringValue(String variableName) { + throw new ExpansionException( + String.format( + "Invalid toolchain configuration: Cannot expand variable '%s': expected string, " + + "found structure (LibraryToLink)", + variableName)); + } + + @Override + public boolean isTruthy() { + return true; + } + } + + /** + * A sequence of structure values. Exists as a memory optimization - a typical build can contain + * millions of feature values, so getting rid of the overhead of {@code StructureValue} objects + * significantly reduces memory overhead. + */ + @Immutable private static final class StructureSequence implements VariableValue { private final ImmutableList<ImmutableMap<String, VariableValue>> values;
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 4b32f11..35f16bf 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
@@ -39,6 +39,8 @@ import com.google.devtools.build.lib.packages.RuleErrorConsumer; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables; +import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables.LibraryToLinkValue; +import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables.SequenceBuilder; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables.VariablesExtension; import com.google.devtools.build.lib.rules.cpp.CppLinkAction.Context; import com.google.devtools.build.lib.rules.cpp.CppLinkAction.LinkArtifactFactory; @@ -73,21 +75,8 @@ */ public static final String LIBOPTS_VARIABLE = "libopts"; - /** - * A build variable for flags providing files to link as inputs in the linker invocation that - * should not go in a -whole_archive block. - */ - public static final String LINKER_INPUT_PARAMS_VARIABLE = "linker_input_params"; - - /** - * A build variable for flags providing files to link as inputs in the linker invocation that - * should go in a -whole_archive block. - */ - public static final String WHOLE_ARCHIVE_LINKER_INPUT_PARAMS_VARIABLE = - "whole_archive_linker_params"; - - /** A build variable whose presence indicates that whole archive flags should be applied. */ - public static final String GLOBAL_WHOLE_ARCHIVE_VARIABLE = "global_whole_archive"; + /** 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 the execpath of the output of the linker. */ public static final String OUTPUT_EXECPATH_VARIABLE = "output_execpath"; @@ -631,11 +620,6 @@ .setToolPath(getToolPath()) .setFeatureConfiguration(featureConfiguration); - // TODO(b/30228443): Refactor noWholeArchiveInputs into action_configs, and remove this. - if (needWholeArchive) { - linkCommandLineBuilder.setNoWholeArchiveFlags(variablesExtension.getNoWholeArchiveInputs()); - } - if (!isLTOIndexing) { linkCommandLineBuilder .setOutput(output) @@ -1184,9 +1168,7 @@ String rpathRoot; ImmutableList<String> rpathEntries; ImmutableSet<String> libopts; - ImmutableList<String> linkerInputParams; - ImmutableList<String> wholeArchiveLinkerInputParams; - ImmutableList<String> noWholeArchiveInputs; + SequenceBuilder librariesToLink; public void setRpathRoot(String rPathRoot) { this.rpathRoot = rPathRoot; @@ -1200,16 +1182,8 @@ this.libopts = libopts; } - public void setLinkerInputParams(ImmutableList<String> linkerInputParams) { - this.linkerInputParams = linkerInputParams; - } - - public void setWholeArchiveLinkerInputParams(ImmutableList<String> wholeArchiveInputParams) { - this.wholeArchiveLinkerInputParams = wholeArchiveInputParams; - } - - public void setNoWholeArchiveInputs(ImmutableList<String> noWholeArchiveInputs) { - this.noWholeArchiveInputs = noWholeArchiveInputs; + public void setLibrariesToLink(SequenceBuilder librariesToLink) { + this.librariesToLink = librariesToLink; } public String getRpathRoot() { @@ -1224,16 +1198,8 @@ return libopts; } - public ImmutableList<String> getLinkerInputParams() { - return linkerInputParams; - } - - public ImmutableList<String> getWholeArchiveLinkerInputParams() { - return wholeArchiveLinkerInputParams; - } - - public ImmutableList<String> getNoWholeArchiveInputs() { - return noWholeArchiveInputs; + public SequenceBuilder getLibrariesToLink() { + return librariesToLink; } } @@ -1277,16 +1243,6 @@ addInputFileLinkOptions(linkArgCollector); } - /** - * Returns linker parameters indicating libraries that should not be linked inside a - * --whole_archive block. - * - * <p>TODO(b/30228443): Refactor into action configs - */ - public List<String> getNoWholeArchiveInputs() { - return linkArgCollector.getNoWholeArchiveInputs(); - } - @Override public void addVariables(Variables.Builder buildVariables) { @@ -1323,16 +1279,9 @@ } buildVariables.addStringSequenceVariable(LIBOPTS_VARIABLE, linkArgCollector.getLibopts()); - buildVariables.addStringSequenceVariable( - LINKER_INPUT_PARAMS_VARIABLE, linkArgCollector.getLinkerInputParams()); - buildVariables.addStringSequenceVariable( - WHOLE_ARCHIVE_LINKER_INPUT_PARAMS_VARIABLE, - linkArgCollector.getWholeArchiveLinkerInputParams()); - // global archive - if (needWholeArchive) { - buildVariables.addStringVariable(GLOBAL_WHOLE_ARCHIVE_VARIABLE, ""); - } + buildVariables.addCustomBuiltVariable( + LIBRARIES_TO_LINK_VARIABLE, linkArgCollector.getLibrariesToLink()); // mostly static if (linkStaticness == LinkStaticness.MOSTLY_STATIC && cppConfiguration.skipStaticOutputs()) { @@ -1385,15 +1334,6 @@ return isNativeDeps && cppConfiguration.shareNativeDeps(); } - private boolean inputNeedsWholeArchive(LinkerInput input) { - if (Link.useStartEndLib(input, cppConfiguration.archiveType())) { - return false; - } - - return input.getArtifactCategory() == ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY - && !wholeArchive && !needWholeArchive; - } - /** * When linking a shared library fully or mostly static then we need to link in *all* dependent * files, not just what the shared library needs for its own code. This is done by wrapping all @@ -1410,7 +1350,7 @@ // List of command line parameters that need to be placed *outside* of // --whole-archive ... --no-whole-archive. - ImmutableList.Builder<String> noWholeArchiveInputs = ImmutableList.builder(); + SequenceBuilder librariesToLink = new SequenceBuilder(); PathFragment solibDir = configuration @@ -1506,9 +1446,6 @@ } } - ImmutableList.Builder<String> wholeArchiveInputParams = ImmutableList.builder(); - ImmutableList.Builder<String> standardArchiveInputParams = ImmutableList.builder(); - for (LinkerInput input : linkerInputs) { if (input.getArtifactCategory() == ArtifactCategory.DYNAMIC_LIBRARY) { PathFragment libDir = input.getArtifact().getExecPath().getParentDirectory(); @@ -1520,20 +1457,15 @@ if (libDir.equals(solibDir)) { includeSolibDir = true; } - addDynamicInputLinkOptions( - input, standardArchiveInputParams, libOpts, solibDir, rpathRoot); + addDynamicInputLinkOptions(input, librariesToLink, false, libOpts, solibDir, rpathRoot); } else { - addStaticInputLinkOptions( - input, wholeArchiveInputParams, standardArchiveInputParams, ltoMap); + addStaticInputLinkOptions(input, librariesToLink, false, ltoMap); } } boolean includeRuntimeSolibDir = false; for (LinkerInput input : runtimeLinkerInputs) { - ImmutableList.Builder<String> optionsList = - needWholeArchive ? noWholeArchiveInputs : standardArchiveInputParams; - if (input.getArtifactCategory() == ArtifactCategory.DYNAMIC_LIBRARY) { PathFragment libDir = input.getArtifact().getExecPath().getParentDirectory(); Preconditions.checkState( @@ -1542,16 +1474,15 @@ input.getArtifact(), solibDir); includeRuntimeSolibDir = true; - addDynamicInputLinkOptions(input, optionsList, libOpts, solibDir, rpathRoot); + addDynamicInputLinkOptions(input, librariesToLink, true, libOpts, solibDir, rpathRoot); } else { - addStaticInputLinkOptions(input, - needWholeArchive ? noWholeArchiveInputs : wholeArchiveInputParams, - needWholeArchive ? noWholeArchiveInputs : standardArchiveInputParams, - ltoMap); + addStaticInputLinkOptions(input, librariesToLink, true, ltoMap); } } if (linkerParamsFile != null && ltoOutputRootPrefix.equals(PathFragment.EMPTY_FRAGMENT)) { - standardArchiveInputParams.add("-Wl,@" + linkerParamsFile.getExecPathString()); + librariesToLink.addValue( + new LibraryToLinkValue( + "-Wl,@" + linkerParamsFile.getExecPathString(), needWholeArchive)); } // rpath ordering matters for performance; first add the one where most libraries are found. @@ -1564,19 +1495,22 @@ linkArgCollector.setLibopts(libOpts.build()); - linkArgCollector.setLinkerInputParams(standardArchiveInputParams.build()); - linkArgCollector.setWholeArchiveLinkerInputParams(wholeArchiveInputParams.build()); - linkArgCollector.setNoWholeArchiveInputs(noWholeArchiveInputs.build()); + linkArgCollector.setLibrariesToLink(librariesToLink); if (ltoMap != null) { Preconditions.checkState(ltoMap.isEmpty(), "Still have LTO objects left: %s", ltoMap); } } - /** Adds command-line options for a dynamic library input file into options and libOpts. */ + /** + * Adds command-line options for a dynamic library input file into options and libOpts. + * + * @param librariesToLink - a collection that will be exposed as a build variable. + */ private void addDynamicInputLinkOptions( LinkerInput input, - ImmutableList.Builder<String> options, + SequenceBuilder librariesToLink, + boolean isRuntimeLinkerInput, ImmutableSet.Builder<String> libOpts, PathFragment solibDir, String rpathRoot) { @@ -1601,28 +1535,30 @@ libOpts.add("-L" + inputArtifact.getExecPath().getParentDirectory().getPathString()); String name = inputArtifact.getFilename(); + boolean inputIsWholeArchive = !isRuntimeLinkerInput && needWholeArchive; if (CppFileTypes.SHARED_LIBRARY.matches(name)) { String libName = name.replaceAll("(^lib|\\.(so|dylib)$)", ""); - options.add("-l" + libName); + librariesToLink.addValue(new LibraryToLinkValue("-l" + libName, inputIsWholeArchive)); } else { // Interface shared objects have a non-standard extension // that the linker won't be able to find. So use the // filename directly rather than a -l option. Since the // library has an SONAME attribute, this will work fine. - options.add(inputArtifact.getExecPathString()); + librariesToLink.addValue( + new LibraryToLinkValue(inputArtifact.getExecPathString(), inputIsWholeArchive)); } } /** * Adds command-line options for a static library or non-library input into options. * + * @param librariesToLink - a collection that will be exposed as a build variable. * @param ltoMap is a mutable list of exec paths that should be on the command-line, which must - * be supplied for LTO final links. */ private void addStaticInputLinkOptions( LinkerInput input, - ImmutableList.Builder<String> wholeArchiveOptions, - ImmutableList.Builder<String> standardOptions, + SequenceBuilder librariesToLink, + boolean isRuntimeLinkerInput, @Nullable Map<Artifact, Artifact> ltoMap) { Preconditions.checkState(!(input.getArtifactCategory() == ArtifactCategory.DYNAMIC_LIBRARY)); // If we had any LTO artifacts, ltoMap whould be non-null. In that case, @@ -1638,7 +1574,7 @@ if (Link.useStartEndLib(input, cppConfiguration.archiveType())) { Iterable<Artifact> archiveMembers = input.getObjectFiles(); if (!Iterables.isEmpty(archiveMembers)) { - List<String> nonLTOArchiveMembers = new ArrayList<>(); + ImmutableList.Builder<String> nonLTOArchiveMembersBuilder = ImmutableList.builder(); for (Artifact member : archiveMembers) { if (ltoMap != null && ltoMap.remove(member) != null) { // The LTO artifacts that should be included in the final link @@ -1647,19 +1583,23 @@ // files explicitly, or generate .o files from assembler. continue; } - nonLTOArchiveMembers.add(member.getExecPathString()); + nonLTOArchiveMembersBuilder.add(member.getExecPathString()); } + ImmutableList<String> nonLTOArchiveMembers = nonLTOArchiveMembersBuilder.build(); if (!nonLTOArchiveMembers.isEmpty()) { - standardOptions.add("-Wl,--start-lib"); - standardOptions.addAll(nonLTOArchiveMembers); - standardOptions.add("-Wl,--end-lib"); + boolean inputIsWholeArchive = !isRuntimeLinkerInput && needWholeArchive; + librariesToLink.addValue( + new LibraryToLinkValue(nonLTOArchiveMembers, inputIsWholeArchive, true)); } } } else { - ImmutableList.Builder<String> options = - inputNeedsWholeArchive(input) ? wholeArchiveOptions : standardOptions; + boolean isAlwaysLinkStaticLibrary = + input.getArtifactCategory() == ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY; // For anything else, add the input directly. Artifact inputArtifact = input.getArtifact(); + boolean inputIsWholeArchive = + (!isRuntimeLinkerInput && (isAlwaysLinkStaticLibrary || needWholeArchive)) + || (isRuntimeLinkerInput && isAlwaysLinkStaticLibrary && !needWholeArchive); if (ltoMap != null && ltoMap.remove(inputArtifact) != null) { // The LTO artifacts that should be included in the final link @@ -1668,9 +1608,13 @@ } if (input.isFake()) { - options.add(Link.FAKE_OBJECT_PREFIX + inputArtifact.getExecPathString()); + librariesToLink.addValue( + new LibraryToLinkValue( + Link.FAKE_OBJECT_PREFIX + inputArtifact.getExecPathString(), + inputIsWholeArchive)); } else { - options.add(inputArtifact.getExecPathString()); + librariesToLink.addValue( + new LibraryToLinkValue(inputArtifact.getExecPathString(), inputIsWholeArchive)); } } }
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 dc7b250..6eba06d 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
@@ -62,10 +62,9 @@ " implies: 'symbol_counts'", " implies: 'linkstamps'", " implies: 'output_execpath_flags_executable'", - " implies: 'global_whole_archive_open'", " implies: 'runtime_root_flags'", " implies: 'input_param_flags'", - " implies: 'global_whole_archive_close'", + " implies: 'libraries_to_link'", " implies: 'force_pic_flags'", "}", "action_config {", @@ -80,10 +79,9 @@ " implies: 'shared_flag'", " implies: 'linkstamps'", " implies: 'output_execpath_flags'", - " implies: 'global_whole_archive_open'", " implies: 'runtime_root_flags'", " implies: 'input_param_flags'", - " implies: 'global_whole_archive_close'", + " implies: 'libraries_to_link'", "}", "action_config {", " config_name: 'c++-link-static-library'", @@ -91,10 +89,9 @@ " tool {", " tool_path: 'DUMMY_TOOL'", " }", - " implies: 'global_whole_archive_open'", " implies: 'runtime_root_flags'", " implies: 'input_param_flags'", - " implies: 'global_whole_archive_close'", + " implies: 'libraries_to_link'", "}", "action_config {", " config_name: 'c++-link-alwayslink-static-library'", @@ -102,10 +99,9 @@ " tool {", " tool_path: 'DUMMY_TOOL'", " }", - " implies: 'global_whole_archive_open'", " implies: 'runtime_root_flags'", " implies: 'input_param_flags'", - " implies: 'global_whole_archive_close'", + " implies: 'libraries_to_link'", "}", "action_config {", " config_name: 'c++-link-pic-static-library'", @@ -113,10 +109,9 @@ " tool {", " tool_path: 'DUMMY_TOOL'", " }", - " implies: 'global_whole_archive_open'", " implies: 'runtime_root_flags'", " implies: 'input_param_flags'", - " implies: 'global_whole_archive_close'", + " implies: 'libraries_to_link'", "}", "action_config {", " config_name: 'c++-link-alwayslink-pic-static-library'", @@ -124,10 +119,9 @@ " tool {", " tool_path: 'DUMMY_TOOL'", " }", - " implies: 'global_whole_archive_open'", " implies: 'runtime_root_flags'", " implies: 'input_param_flags'", - " implies: 'global_whole_archive_close'", + " implies: 'libraries_to_link'", "}", "feature {", " name: 'build_interface_libraries'", @@ -215,24 +209,6 @@ " }", "}", "feature {", - " name: 'global_whole_archive_open'", - " flag_set {", - " expand_if_all_available: 'global_whole_archive'", - " action: 'c++-link-executable'", - " action: 'c++-link-dynamic-library'", - " 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 {", - // TODO: Factor platform difference into respective linux and OSX crosstools. - String.format( - " flag:'%s'", - platform == CppLinkPlatform.LINUX ? "-Wl,-whole-archive" : "-Wl,-all_load"), - " }", - " }", - "}", - "feature {", " name: 'runtime_root_flags',", " flag_set {", " expand_if_all_available: 'runtime_root_flags'", @@ -273,39 +249,11 @@ " flag: '%{libopts}'", " }", " }", - " flag_set {", - " expand_if_all_available: 'whole_archive_linker_params'", - " action: 'c++-link-executable'", - " action: 'c++-link-dynamic-library'", - " 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 {", - platform == CppLinkPlatform.LINUX - ? " flag: '-Wl,-whole-archive'\n" - + " flag: '%{whole_archive_linker_params}'\n" - + " flag: '-Wl,-no-whole-archive'" - : " flag: '-Wl,-force_load,%{whole_archive_linker_params}'", - " }", - " }", - " flag_set {", - " expand_if_all_available: 'linker_input_params'", - " action: 'c++-link-executable'", - " action: 'c++-link-dynamic-library'", - " 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_input_params}'", - " }", - " }", "}", "feature {", - " name: 'global_whole_archive_close'", + " name: 'libraries_to_link'", " flag_set {", - " expand_if_all_available: 'global_whole_archive'", + " expand_if_all_available: 'libraries_to_link'", " action: 'c++-link-executable'", " action: 'c++-link-dynamic-library'", " action: 'c++-link-static-library'", @@ -313,10 +261,41 @@ " action: 'c++-link-pic-static-library'", " action: 'c++-link-alwayslink-pic-static-library'", " flag_group {", - // TODO: Factor platform difference into respective linux and OSX crosstools. - String.format( - " flag: '%s'", - platform == CppLinkPlatform.LINUX ? "-Wl,-no-whole-archive" : ""), + " iterate_over: 'libraries_to_link'", + " flag_group {", + " expand_if_true: 'libraries_to_link.is_lib_group'", + " flag: '-Wl,--start-lib'", + " }", + ifLinux( + platform, + " flag_group {", + " expand_if_true: 'libraries_to_link.is_whole_archive'", + " flag: '-Wl,-whole-archive'", + " }", + " flag_group {", + " iterate_over: 'libraries_to_link.names'", + " flag: '%{libraries_to_link.names}'", + " }", + " flag_group {", + " expand_if_true: 'libraries_to_link.is_whole_archive'", + " flag: '-Wl,-no-whole-archive'", + " }"), + ifMac( + platform, + " flag_group {", + " expand_if_true: 'libraries_to_link.is_whole_archive'", + " iterate_over: 'libraries_to_link.names'", + " flag: '-Wl,-force_load,%{libraries_to_link.names}'", + " }", + " flag_group {", + " expand_if_false: 'libraries_to_link.is_whole_archive'", + " iterate_over: 'libraries_to_link.names'", + " flag: '%{libraries_to_link.names}'", + " }"), + " flag_group {", + " expand_if_true: 'libraries_to_link.is_lib_group'", + " flag: '-Wl,--end-lib'", + " }", " }", " }", "}", @@ -331,4 +310,20 @@ " }", "}")); } + + private static String ifLinux(CppLinkPlatform platform, String... lines) { + if (platform == CppLinkPlatform.LINUX) { + return Joiner.on("\n").join(lines); + } else { + return ""; + } + } + + private static String ifMac(CppLinkPlatform platform, String... lines) { + if (platform == CppLinkPlatform.MAC) { + return Joiner.on("\n").join(lines); + } else { + return ""; + } + } }
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 0cb237e..145088e 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
@@ -73,7 +73,6 @@ @Nullable private final PathFragment runtimeSolibDir; private final boolean nativeDeps; private final boolean useTestOnlyFlags; - private final List<String> noWholeArchiveFlags; @Nullable private final Artifact paramFile; @@ -96,9 +95,7 @@ @Nullable PathFragment runtimeSolibDir, boolean nativeDeps, boolean useTestOnlyFlags, - boolean needWholeArchive, @Nullable Artifact paramFile, - List<String> noWholeArchiveFlags, CcToolchainFeatures.Variables variables, @Nullable FeatureConfiguration featureConfiguration) { @@ -126,7 +123,6 @@ this.nativeDeps = nativeDeps; this.useTestOnlyFlags = useTestOnlyFlags; this.paramFile = paramFile; - this.noWholeArchiveFlags = noWholeArchiveFlags; } @Nullable @@ -380,14 +376,12 @@ case EXECUTABLE: argv.add(cppConfiguration.getCppExecutable().getPathString()); argv.addAll(featureConfiguration.getCommandLine(actionName, variables)); - argv.addAll(noWholeArchiveFlags); addToolchainFlags(argv); break; case DYNAMIC_LIBRARY: argv.add(toolPath); argv.addAll(featureConfiguration.getCommandLine(actionName, variables)); - argv.addAll(noWholeArchiveFlags); addToolchainFlags(argv); break; @@ -401,7 +395,6 @@ argv.addAll(cppConfiguration.getArFlags()); argv.add(output.getExecPathString()); argv.addAll(featureConfiguration.getCommandLine(actionName, variables)); - argv.addAll(noWholeArchiveFlags); break; // Since the objc case is not hardcoded in CppConfiguration, we can use the actual tool. @@ -417,7 +410,6 @@ default: throw new IllegalArgumentException(); } - return argv; } @@ -641,12 +633,10 @@ @Nullable private PathFragment runtimeSolibDir; private boolean nativeDeps; private boolean useTestOnlyFlags; - private boolean needWholeArchive; @Nullable private Artifact paramFile; @Nullable private CcToolchainProvider toolchain; private Variables variables; private FeatureConfiguration featureConfiguration; - private List<String> noWholeArchiveFlags = ImmutableList.of(); // This interface is needed to support tests that don't create a // ruleContext, in which case the configuration and action owner @@ -719,9 +709,7 @@ runtimeSolibDir, nativeDeps, useTestOnlyFlags, - needWholeArchive, paramFile, - noWholeArchiveFlags, variables, featureConfiguration); } @@ -874,15 +862,5 @@ this.runtimeSolibDir = runtimeSolibDir; return this; } - - /** - * Set flags that should not be in a --whole_archive block. - * - * <p>TODO(b/30228443): Refactor into action_configs. - */ - public Builder setNoWholeArchiveFlags(List<String> noWholeArchiveFlags) { - this.noWholeArchiveFlags = noWholeArchiveFlags; - return this; - } } }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java index cd92b39..03750a4 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
@@ -195,7 +195,7 @@ final FeatureConfiguration featureConfiguration = getMockFeatureConfiguration(); ActionTester.runTest( - 128, + 64, new ActionCombinationFactory() { @Override @@ -219,9 +219,8 @@ builder.setLinkStaticness(LinkStaticness.DYNAMIC); builder.setNativeDeps((i & 4) == 0); builder.setUseTestOnlyFlags((i & 8) == 0); - builder.setWholeArchive((i & 16) == 0); - builder.setFake((i & 32) == 0); - builder.setRuntimeSolibDir((i & 64) == 0 ? null : new PathFragment("so1")); + builder.setFake((i & 16) == 0); + builder.setRuntimeSolibDir((i & 32) == 0 ? null : new PathFragment("so1")); builder.setFeatureConfiguration(featureConfiguration); return builder.build();
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java index c5a5629..9089051 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java
@@ -23,6 +23,8 @@ import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables; +import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables.LibraryToLinkValue; +import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables.VariableValue; import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; import java.util.List; import org.junit.Test; @@ -118,51 +120,27 @@ } @Test - public void testWholeArchiveBuildVariables() throws Exception { - scratch.file( - "x/BUILD", - "cc_binary(", - " name = 'bin.so',", - " srcs = ['a.cc'],", - " linkopts = ['-shared'],", - " linkstatic = 1", - ")"); + public void testLibrariesToLinkAreExported() throws Exception { + AnalysisMock.get().ccSupport().setupCrosstool(mockToolsConfig); + useConfiguration(); + + scratch.file("x/BUILD", "cc_library(", " name = 'foo',", " srcs = ['a.cc'],", ")"); scratch.file("x/a.cc"); - ConfiguredTarget target = getConfiguredTarget("//x:bin.so"); - Variables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE); - List<String> variableValue = - getVariableValue(variables, CppLinkActionBuilder.GLOBAL_WHOLE_ARCHIVE_VARIABLE); - assertThat(variableValue).contains(""); - } - - /** - * Tests that "--whole_archive" is not propagated twice through whole archive inputs and global - * whole archive. - */ - @Test - public void testGlobalWholeArchiveOrWholeArchiveBuildVariables() throws Exception { - scratch.file( - "x/BUILD", - "cc_binary(", - " name = 'bin',", - " srcs = ['a.cc', 'b.lo'],", - " linkopts = ['-shared'],", - " linkstatic = 1,", - ")"); - scratch.file("x/a.cc"); - scratch.file("x/b.lo"); - - ConfiguredTarget target = getConfiguredTarget("//x:bin"); - Variables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE); - List<String> globalWholeArchiveVariableValue = - getVariableValue(variables, CppLinkActionBuilder.GLOBAL_WHOLE_ARCHIVE_VARIABLE); - List<String> wholeArchiveInputVariableValue = - getVariableValue( - variables, CppLinkActionBuilder.WHOLE_ARCHIVE_LINKER_INPUT_PARAMS_VARIABLE); - assertThat(globalWholeArchiveVariableValue).contains(""); - assertThat(wholeArchiveInputVariableValue).isEmpty(); - ; + ConfiguredTarget target = getConfiguredTarget("//x:foo"); + Variables variables = getLinkBuildVariables(target, LinkTargetType.DYNAMIC_LIBRARY); + VariableValue librariesToLinkSequence = variables.getVariable("libraries_to_link"); + assertThat(librariesToLinkSequence).isNotNull(); + Iterable<? extends VariableValue> librariesToLink = + librariesToLinkSequence.getSequenceValue("libraries_to_link"); + assertThat(librariesToLink).hasSize(1); + VariableValue nameValue = librariesToLink.iterator().next().getFieldValue( + "librariesToLink", LibraryToLinkValue.NAMES_FIELD_NAME); + assertThat(nameValue).isNotNull(); + Iterable<? extends VariableValue> names = nameValue.getSequenceValue("names"); + assertThat(names).isNotNull(); + assertThat(names).hasSize(1); + assertThat(names.iterator().next().getStringValue("names")).matches(".*a\\..*o"); } @Test
diff --git a/tools/cpp/CROSSTOOL.tpl b/tools/cpp/CROSSTOOL.tpl index 2ede35d..3728f11 100644 --- a/tools/cpp/CROSSTOOL.tpl +++ b/tools/cpp/CROSSTOOL.tpl
@@ -302,7 +302,6 @@ implies: 'linkstamps' implies: 'output_execpath_flags' implies: 'input_param_flags' - implies: 'global_whole_archive' } action_config { @@ -315,7 +314,6 @@ implies: 'linkstamps' implies: 'output_execpath_flags' implies: 'input_param_flags' - implies: 'global_whole_archive' implies: 'has_configured_linker_path' } @@ -326,7 +324,6 @@ tool_path: 'wrapper/bin/msvc_link.bat' } implies: 'input_param_flags' - implies: 'global_whole_archive' } action_config { @@ -336,7 +333,6 @@ tool_path: 'wrapper/bin/msvc_link.bat' } implies: 'input_param_flags' - implies: 'global_whole_archive' } # TODO(pcloudy): The following action_config is listed in MANDATORY_LINK_TARGET_TYPES. @@ -348,7 +344,6 @@ tool_path: 'wrapper/bin/msvc_link.bat' } implies: 'input_param_flags' - implies: 'global_whole_archive' } action_config { @@ -358,7 +353,6 @@ tool_path: 'wrapper/bin/msvc_link.bat' } implies: 'input_param_flags' - implies: 'global_whole_archive' } action_config { @@ -408,61 +402,43 @@ } feature { - name: 'input_param_flags' - flag_set { - expand_if_all_available: 'libopts' - action: 'c++-link-executable' - action: 'c++-link-dynamic-library' - 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: '%{libopts}' - } - } - flag_set { - expand_if_all_available: 'whole_archive_linker_params' - action: 'c++-link-executable' - action: 'c++-link-dynamic-library' - 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: '/WHOLEARCHIVE:%{whole_archive_linker_params}' - } - } - flag_set { - expand_if_all_available: 'linker_input_params' - action: 'c++-link-executable' - action: 'c++-link-dynamic-library' - 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_input_params}' - } - } + name: 'input_param_flags' + flag_set { + expand_if_all_available: 'libopts' + action: 'c++-link-executable' + action: 'c++-link-dynamic-library' + 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: '%{libopts}' + } + } + flag_set { + expand_if_all_available: 'libraries_to_link' + action: 'c++-link-executable' + action: 'c++-link-dynamic-library' + 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 { + iterate_over: 'libraries_to_link' + flag_group { + expand_if_true: 'libraries_to_link.is_whole_archive' + iterate_over: 'libraries_to_link.names' + flag: '/WHOLEARCHIVE:%{libraries_to_link.names}' + } + flag_group { + expand_if_false: 'libraries_to_link.is_whole_archive' + iterate_over: 'libraries_to_link.names' + flag: '%{libraries_to_link.names}' + } + } + } } - feature { - name: 'global_whole_archive' - flag_set { - expand_if_all_available: 'global_whole_archive' - action: 'c++-link-executable' - action: 'c++-link-dynamic-library' - 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: '/WHOLEARCHIVE' - } - } -} - compilation_mode_flags { mode: DBG compiler_flag: "/DDEBUG=1"