From 64a966dd8cd5dc564d179d2fe9ecb1c3c7b56b14 Mon Sep 17 00:00:00 2001 From: hlopko Date: Tue, 12 Feb 2019 22:22:08 -0800 Subject: [PATCH] Introduce --incompatible_remove_legacy_whole_archive This flag disables legacy whole archive behavior, as well as makes the --legacy_whole_archive option a noop. See details in https://github.com/bazelbuild/bazel/issues/7362. It also introduces a possibility to enable legacy behavior per target by `--features=legacy_whole_archive` option, or `features = [ "legacy_whole_archive" ]` attribute. RELNOTES: None. PiperOrigin-RevId: 233691404 --- .../build/lib/rules/cpp/CppConfiguration.java | 4 ++ .../lib/rules/cpp/CppLinkActionBuilder.java | 32 +++++++++++-- .../build/lib/rules/cpp/CppOptions.java | 37 ++++++++++---- .../build/lib/rules/cpp/CppRuleClasses.java | 3 ++ .../rules/nativedeps/NativeDepsHelper.java | 10 ++-- .../lib/rules/cpp/CppLinkActionTest.java | 48 +++++++++++++++++++ 6 files changed, 115 insertions(+), 19 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java index 87f5cfb4860b0d..27979403351ec4 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java @@ -396,6 +396,10 @@ public boolean legacyWholeArchive() { return cppOptions.legacyWholeArchive; } + public boolean removeLegacyWholeArchive() { + return cppOptions.removeLegacyWholeArchive; + } + public boolean getInmemoryDotdFiles() { return cppOptions.inmemoryDotdFiles; } 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 187e1c8884f352..1d9ec673c5b166 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 @@ -649,7 +649,7 @@ public CppLinkAction build() throws InterruptedException { boolean needWholeArchive = wholeArchive - || needWholeArchive(linkingMode, linkType, linkopts, isNativeDeps, cppConfiguration); + || needWholeArchive(featureConfiguration, linkType, linkopts, cppConfiguration); // Disallow LTO indexing for test targets that link statically, and optionally for any // linkstatic target (which can be used to disable LTO indexing for non-testonly cc_binary // built due to data dependences for a blaze test invocation). Otherwise this will provoke @@ -1120,15 +1120,37 @@ private boolean shouldUseLinkDynamicLibraryTool() { /** The default heuristic on whether we need to use whole-archive for the link. */ private static boolean needWholeArchive( - Link.LinkingMode staticness, + FeatureConfiguration featureConfiguration, LinkTargetType type, Collection linkopts, - boolean isNativeDeps, CppConfiguration cppConfig) { - boolean mostlyStatic = (staticness == Link.LinkingMode.STATIC); boolean sharedLinkopts = type.isDynamicLibrary() || linkopts.contains("-shared") || cppConfig.hasSharedLinkOption(); - return (isNativeDeps || cppConfig.legacyWholeArchive()) && mostlyStatic && sharedLinkopts; + // Fasten your seat belt, the logic below doesn't make perfect sense and it's full of obviously + // missed corner cases. The world still stands and depends on this behavior, so ¯\_(ツ)_/¯. + if (!sharedLinkopts) { + // We are not producing shared library, there is no reason to use --whole-archive with + // executable (if the executable doesn't use the symbols, nobody else will, so --whole-archive + // is not needed). + return false; + } + if (cppConfig.removeLegacyWholeArchive()) { + // --incompatible_remove_legacy_whole_archive has been flipped, no --whole-archive for the + // entire build. + return false; + } + if (featureConfiguration.getRequestedFeatures().contains(CppRuleClasses.LEGACY_WHOLE_ARCHIVE)) { + // --incompatible_remove_legacy_whole_archive has not been flipped, and this target requested + // --whole-archive using features. + return true; + } + if (cppConfig.legacyWholeArchive()) { + // --incompatible_remove_legacy_whole_archive has not been flipped, so whether to + // use --whole-archive depends on --legacy_whole_archive. + return true; + } + // Hopefully future default. + return false; } private static ImmutableSet constructOutputs( diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java index 769c250039fa6d..314919bc13ba0c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java @@ -360,16 +360,18 @@ public String getTypeDescription() { public Label customMalloc; @Option( - name = "legacy_whole_archive", - defaultValue = "true", - documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS, - effectTags = {OptionEffectTag.ACTION_COMMAND_LINES, OptionEffectTag.AFFECTS_OUTPUTS}, - help = - "When on, use --whole-archive for cc_binary rules that have " - + "linkshared=1 and either linkstatic=1 or '-static' in linkopts. " - + "This is for backwards compatibility only. " - + "A better alternative is to use alwayslink=1 where required." - ) + name = "legacy_whole_archive", + defaultValue = "true", + documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS, + effectTags = {OptionEffectTag.ACTION_COMMAND_LINES, OptionEffectTag.AFFECTS_OUTPUTS}, + metadataTags = {OptionMetadataTag.DEPRECATED}, + help = + "Deprecated, superseded by --incompatible_remove_legacy_whole_archive " + + "(see https://github.com/bazelbuild/bazel/issues/7362 for details). " + + "When on, use --whole-archive for cc_binary rules that have " + + "linkshared=1 and either linkstatic=1 or '-static' in linkopts. " + + "This is for backwards compatibility only. " + + "A better alternative is to use alwayslink=1 where required.") public boolean legacyWholeArchive; @Option( @@ -698,6 +700,20 @@ public Label getFdoPrefetchHintsLabel() { + "(see https://github.com/bazelbuild/bazel/issues/6861 for migration instructions).") public boolean disableLegacyCrosstoolFields; + @Option( + name = "incompatible_remove_legacy_whole_archive", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.TOOLCHAIN, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = + "If true, Bazel will not link library dependencies as whole archive by default " + + "(see https://github.com/bazelbuild/bazel/issues/7362 for migration instructions).") + public boolean removeLegacyWholeArchive; + @Option( name = "incompatible_remove_cpu_and_compiler_attributes_from_cc_toolchain", defaultValue = "false", @@ -869,6 +885,7 @@ public FragmentOptions getHost() { host.disableLegacyCrosstoolFields = disableLegacyCrosstoolFields; host.disableCrosstool = disableCrosstool; host.enableCcToolchainResolution = enableCcToolchainResolution; + host.removeLegacyWholeArchive = removeLegacyWholeArchive; return host; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java index ef836e76054b75..03999104b63c46 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java @@ -412,6 +412,9 @@ public static Label ccToolchainTypeAttribute(RuleDefinitionEnvironment env) { */ public static final String SUPPORTS_DYNAMIC_LINKER = "supports_dynamic_linker"; + /** A feature marking that the target needs to link its deps in --whole-archive block. */ + public static final String LEGACY_WHOLE_ARCHIVE = "legacy_whole_archive"; + /** Ancestor for all rules that do include scanning. */ public static final class CcIncludeScanningRule implements RuleDefinition { @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java b/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java index 797f6703e56849..56cff9b276b113 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java @@ -230,13 +230,15 @@ public static NativeDepsRunfiles createNativeDepsAction( sharedLibrary = nativeDeps; } FdoContext fdoContext = toolchain.getFdoContext(); + ImmutableSet.Builder requestedFeatures = + ImmutableSet.builder().addAll(ruleContext.getFeatures()).add(STATIC_LINKING_MODE); + if (!ruleContext.getDisabledFeatures().contains(CppRuleClasses.LEGACY_WHOLE_ARCHIVE)) { + requestedFeatures.add(CppRuleClasses.LEGACY_WHOLE_ARCHIVE); + } FeatureConfiguration featureConfiguration = CcCommon.configureFeaturesOrReportRuleError( ruleContext, - /* requestedFeatures= */ ImmutableSet.builder() - .addAll(ruleContext.getFeatures()) - .add(STATIC_LINKING_MODE) - .build(), + /* requestedFeatures= */ requestedFeatures.build(), /* unsupportedFeatures= */ ruleContext.getDisabledFeatures(), toolchain); CppLinkActionBuilder builder = 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 3392bfba4b2222..24dc83165bd5c2 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 @@ -41,6 +41,7 @@ import com.google.devtools.build.lib.analysis.util.ActionTester.ActionCombinationFactory; import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; @@ -211,6 +212,53 @@ public void testLibOptsAndLibSrcsAreInCorrectOrder() throws Exception { .matches(".* -Wl,-rpath[^ ]*some-dir(?= ).* -Wl,-rpath[^ ]*some-other-dir .*"); } + @Test + public void testLegacyWholeArchive() throws Exception { + scratch.file( + "x/BUILD", + "cc_binary(", + " name = 'libfoo.so',", + " srcs = ['foo.cc'],", + " linkshared = 1,", + ")"); + // --incompatible_remove_legacy_whole_archive not flipped, --legacy_whole_archive wins. + useConfiguration( + "--cpu=k8", "--legacy_whole_archive", "--noincompatible_remove_legacy_whole_archive"); + assertThat(getLibfooArguments()).contains("-Wl,-whole-archive"); + useConfiguration( + "--cpu=k8", "--nolegacy_whole_archive", "--noincompatible_remove_legacy_whole_archive"); + assertThat(getLibfooArguments()).doesNotContain("-Wl,-whole-archive"); + + // --incompatible_remove_legacy_whole_archive flipped, --legacy_whole_archive ignored. + useConfiguration( + "--cpu=k8", "--legacy_whole_archive", "--incompatible_remove_legacy_whole_archive"); + assertThat(getLibfooArguments()).doesNotContain("-Wl,-whole-archive"); + useConfiguration( + "--cpu=k8", "--nolegacy_whole_archive", "--incompatible_remove_legacy_whole_archive"); + assertThat(getLibfooArguments()).doesNotContain("-Wl,-whole-archive"); + + // Even when --nolegacy_whole_archive, features can still add the behavior back. + useConfiguration( + "--cpu=k8", + "--nolegacy_whole_archive", + "--noincompatible_remove_legacy_whole_archive", + "--features=legacy_whole_archive"); + assertThat(getLibfooArguments()).contains("-Wl,-whole-archive"); + // Even when --nolegacy_whole_archive, features can still add the behavior, but not when + // --incompatible_remove_legacy_whole_archive is flipped. + useConfiguration( + "--cpu=k8", + "--incompatible_remove_legacy_whole_archive", + "--features=legacy_whole_archive"); + assertThat(getLibfooArguments()).doesNotContain("-Wl,-whole-archive"); + } + + private List getLibfooArguments() throws LabelSyntaxException { + ConfiguredTarget configuredTarget = getConfiguredTarget("//x:libfoo.so"); + CppLinkAction linkAction = (CppLinkAction) getGeneratingAction(configuredTarget, "x/libfoo.so"); + return linkAction.getArguments(); + } + @Test public void testExposesRuntimeLibrarySearchDirectoriesVariable() throws Exception { scratch.file(