From 73138139b90e90996ad7764e5e46b1176ef05002 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 11 Jan 2017 13:30:06 +0000 Subject: Fix bug in --experimental_link_dynamic_binaries_separately. If linkstatic was explicitly set to 0 for a non-test target, we didn't set CcLibraryHelper to create CcLinkOutputs, but would still try to link those in instead of the compile output. Instead, pull out a variable that puts this logic into a single spot. Also rename the flag to --experimental_link_compile_output_separately, which IMO makes slightly more sense. Not too important as I don't think we should keep this flag long-term anyway. -- PiperOrigin-RevId: 144194903 MOS_MIGRATED_REVID=144194903 --- .../devtools/build/lib/rules/cpp/CcBinary.java | 55 ++++++++++++++-------- .../build/lib/rules/cpp/CppConfiguration.java | 4 +- .../devtools/build/lib/rules/cpp/CppOptions.java | 18 ++++--- 3 files changed, 50 insertions(+), 27 deletions(-) (limited to 'src/main/java/com') diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java index b68e2e3c5f..b21536e797 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java @@ -89,7 +89,8 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory { NestedSet filesToBuild, Iterable fakeLinkerInputs, boolean fake, - ImmutableSet cAndCppSources) { + ImmutableSet cAndCppSources, + boolean linkCompileOutputSeparately) { Runfiles.Builder builder = new Runfiles.Builder( context.getWorkspaceName(), context.getConfiguration().legacyExternalRunfiles()); Function runfilesMapping = @@ -104,12 +105,11 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory { // Add the C++ runtime libraries if linking them dynamically. if (linkStaticness == LinkStaticness.DYNAMIC) { builder.addTransitiveArtifacts(toolchain.getDynamicRuntimeLinkInputs()); - CppConfiguration cppConfiguration = context.getFragment(CppConfiguration.class); - if (cppConfiguration.getLinkDynamicBinariesSeparately()) { - builder.addArtifacts( - LinkerInputs.toLibraryArtifacts( - info.getCcLinkingOutputs().getExecutionDynamicLibraries())); - } + } + if (linkCompileOutputSeparately) { + builder.addArtifacts( + LinkerInputs.toLibraryArtifacts( + info.getCcLinkingOutputs().getExecutionDynamicLibraries())); } // For cc_binary and cc_test rules, there is an implicit dependency on // the malloc library package, which is specified by the "malloc" attribute. @@ -172,6 +172,15 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory { if (ruleContext.hasErrors()) { return null; } + + List linkopts = common.getLinkopts(); + LinkStaticness linkStaticness = getLinkStaticness(ruleContext, linkopts, cppConfiguration); + + // We currently only want link the dynamic library generated for test code separately. + boolean linkCompileOutputSeparately = + ruleContext.isTestTarget() + && cppConfiguration.getLinkCompileOutputSeparately() + && linkStaticness == LinkStaticness.DYNAMIC; CcLibraryHelper helper = new CcLibraryHelper(ruleContext, semantics, featureConfiguration) @@ -181,9 +190,19 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory { .setFake(fake) .addPrecompiledFiles(precompiledFiles) .enableInterfaceSharedObjects(); - // Always treat test targets as static libraries so that their code can be compiled into a - // dynamic library and profit from optimizations like interface so. - helper.setLinkType(ruleContext.isTestTarget() ? LinkTargetType.STATIC_LIBRARY : linkType); + // When linking the object files directly into the resulting binary, we do not need + // library-level link outputs; thus, we do not let CcLibraryHelper produce link outputs + // (either shared object files or archives) for a non-library link type [*], and add + // the object files explicitly in determineLinkerArguments. + // + // When linking the object files into their own library, we want CcLibraryHelper to + // take care of creating the library link outputs for us, so we need to set the link + // type to STATIC_LIBRARY. + // + // [*] The only library link type is STATIC_LIBRARY. EXECUTABLE specifies a normal + // cc_binary output, while DYNAMIC_LIBRARY is a cc_binary rules that produces an + // output matching a shared object, for example cc_binary(name="foo.so", ...) on linux. + helper.setLinkType(linkCompileOutputSeparately ? LinkTargetType.STATIC_LIBRARY : linkType); CcLibraryHelper.Info info = helper.build(); CppCompilationContext cppCompilationContext = info.getCppCompilationContext(); @@ -206,9 +225,6 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory { return null; } - List linkopts = common.getLinkopts(); - LinkStaticness linkStaticness = getLinkStaticness(ruleContext, linkopts, cppConfiguration); - CppLinkActionBuilder linkActionBuilder = determineLinkerArguments( ruleContext, @@ -219,7 +235,8 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory { fake, binary, linkStaticness, - linkopts); + linkopts, + linkCompileOutputSeparately); linkActionBuilder.setUseTestOnlyFlags(ruleContext.isTestTarget()); CcToolchainProvider ccToolchain = CppHelper.getToolchain(ruleContext); @@ -333,7 +350,8 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory { filesToBuild, fakeLinkerInputs, fake, - helper.getCompilationUnitSources()); + helper.getCompilationUnitSources(), + linkCompileOutputSeparately); RunfilesSupport runfilesSupport = RunfilesSupport.withExecutable( ruleContext, runfiles, executable, ruleContext.getConfiguration().buildRunfiles()); @@ -409,7 +427,8 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory { boolean fake, Artifact binary, LinkStaticness linkStaticness, - List linkopts) + List linkopts, + boolean linkCompileOutputSeparately) throws InterruptedException { CppLinkActionBuilder builder = new CppLinkActionBuilder(context, binary) @@ -418,9 +437,7 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory { // Either link in the .o files generated for the sources of this target or link in the // generated dynamic library they are compiled into. - CppConfiguration cppConfiguration = context.getFragment(CppConfiguration.class); - if (cppConfiguration.getLinkDynamicBinariesSeparately() - && linkStaticness == LinkStaticness.DYNAMIC) { + if (linkCompileOutputSeparately) { for (LibraryToLink library : info.getCcLinkingOutputs().getDynamicLibraries()) { builder.addLibrary(library); } 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 fd205d8951..b1228148c0 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 @@ -1598,8 +1598,8 @@ public class CppConfiguration extends BuildConfiguration.Fragment { return dynamicMode; } - public boolean getLinkDynamicBinariesSeparately() { - return cppOptions.linkDynamicBinariesSeparately; + public boolean getLinkCompileOutputSeparately() { + return cppOptions.linkCompileOutputSeparately; } /* 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 ecbe6bb70b..7382b00ce2 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 @@ -229,7 +229,7 @@ public class CppOptions extends FragmentOptions { public DynamicModeFlag dynamicMode; @Option( - name = "experimental_link_dynamic_binaries_separately", + name = "experimental_link_compile_output_separately", defaultValue = "false", category = "semantics", help = @@ -237,7 +237,7 @@ public class CppOptions extends FragmentOptions { + "If true, dynamically linked binary targets will build and link their own srcs as " + "a dynamic library instead of directly linking it in." ) - public boolean linkDynamicBinariesSeparately; + public boolean linkCompileOutputSeparately; @Option( name = "force_pic", @@ -550,9 +550,7 @@ public class CppOptions extends FragmentOptions { name = "experimental_skip_unused_modules", defaultValue = "false", category = "experimental", - help = - "If enabled, not all transitive modules automatically become an action's inputs. Instead," - + " input discovery adds just the required ones." + help = "Deprecated. No effect." ) public boolean skipUnusedModules; @@ -560,10 +558,18 @@ public class CppOptions extends FragmentOptions { name = "experimental_prune_more_modules", defaultValue = "false", category = "experimental", - help = "If enabled, modules pruning is used when building modules themselves." + help = "Deprecated. No effect." ) public boolean pruneMoreModules; + @Option( + name = "prune_cpp_modules", + defaultValue = "true", + category = "strategy", + help = "If enabled, use the results of input discovery to reduce the number of used modules." + ) + public boolean pruneCppModules; + @Option( name = "experimental_omitfp", defaultValue = "false", -- cgit v1.2.3