Skip to content

Commit

Permalink
Add feature to turn off param file for archives
Browse files Browse the repository at this point in the history 
This gets costly for Apple builds which (unfortunately) generates an
archive for each library.  Unlike link actions, library archiving
action commandline don't grow with the size of builds, so control them
with a feature and make it opt-in instead.

PiperOrigin-RevId: 471137173
Change-Id: I580071798edefa010c15fbe81cb112df11f2feb0
  • Loading branch information
@googlewalt @copybara-github
googlewalt authored and copybara-github committed Aug 31, 2022
1 parent 8c79b19 commit bff9730
Show file tree
Hide file tree
Showing 8 changed files with 249 additions and 14 deletions.
8 changes: 4 additions & 4 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -530,16 +530,16 @@ boolean canSplitCommandLine() {
return (interfaceOutput == null
|| featureConfiguration.isEnabled(CppRuleClasses.TARGETS_WINDOWS));
case EXECUTABLE:
case OBJC_EXECUTABLE:
case OBJCPP_EXECUTABLE:
return true;
case STATIC_LIBRARY:
case PIC_STATIC_LIBRARY:
case ALWAYS_LINK_STATIC_LIBRARY:
case ALWAYS_LINK_PIC_STATIC_LIBRARY:
case OBJC_ARCHIVE:
case OBJC_FULLY_LINKED_ARCHIVE:
case OBJC_EXECUTABLE:
case OBJCPP_EXECUTABLE:
return true;

return featureConfiguration.isEnabled(CppRuleClasses.ARCHIVE_PARAM_FILE);
default:
return false;
}
Expand Down
6 changes: 6 additions & 0 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,12 @@ public static ToolchainTypeRequirement ccToolchainTypeRequirement(RuleDefinition

public static final String COMPILER_PARAM_FILE = "compiler_param_file";

/**
* A feature to control whether to use param files for archiving commands. This can be applied to
* individual targets.
*/
public static final String ARCHIVE_PARAM_FILE = "archive_param_file";

/** A feature to use gcc quoting for linking param files. */
public static final String GCC_QUOTING_FOR_PARAM_FILES = "gcc_quoting_for_param_files";

Expand Down
6 changes: 6 additions & 0 deletions src/test/java/com/google/devtools/build/lib/analysis/mock/cc_toolchain_config.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ _FEATURE_NAMES = struct(
link_env = "link_env",
dynamic_linking_mode = "dynamic_linking_mode",
static_linking_mode = "static_linking_mode",
archive_param_file = "archive_param_file",
compiler_param_file = "compiler_param_file",
gcc_quoting_for_param_files = "gcc_quoting_for_param_files",
objcopy_embed_flags = "objcopy_embed_flags",
Expand Down Expand Up @@ -821,6 +822,10 @@ _targets_windows_feature = feature(
implies = ["copy_dynamic_libraries_to_binary"],
)

_archive_param_file_feature = feature(
name = _FEATURE_NAMES.archive_param_file,
)

_compiler_param_file_feature = feature(
name = _FEATURE_NAMES.compiler_param_file,
enabled = True,
Expand Down Expand Up @@ -1338,6 +1343,7 @@ _feature_name_to_feature = {
_FEATURE_NAMES.supports_start_end_lib: _supports_start_end_lib_feature,
_FEATURE_NAMES.supports_pic: _supports_pic_feature,
_FEATURE_NAMES.targets_windows: _targets_windows_feature,
_FEATURE_NAMES.archive_param_file: _archive_param_file_feature,
_FEATURE_NAMES.compiler_param_file: _compiler_param_file_feature,
_FEATURE_NAMES.gcc_quoting_for_param_files: _gcc_quoting_for_param_files_feature,
_FEATURE_NAMES.module_maps: _module_maps_feature,
Expand Down
5 changes: 5 additions & 0 deletions src/test/java/com/google/devtools/build/lib/packages/util/mock/osx_cc_toolchain_config.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ _OBJC_LINK_ACTIONS = [

_ALL_LINK_ACTIONS = _NON_OBJC_LINK_ACTIONS + _OBJC_LINK_ACTIONS

_archive_param_file_feature = feature(
name = "archive_param_file",
)

_default_feature = feature(
name = "default",
enabled = True,
Expand Down Expand Up @@ -82,6 +86,7 @@ _parse_headers_feature = feature(
)

_feature_name_to_feature = {
"archive_param_file": _archive_param_file_feature,
"default_feature": _default_feature,
"gcc_quoting_for_param_files": _gcc_quoting_for_param_files_feature,
"static_link_cpp_runtimes": _static_link_cpp_runtimes_feature,
Expand Down
217 changes: 207 additions & 10 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,11 @@ private FeatureConfiguration getMockFeatureConfiguration(ImmutableMap<String, St
.addFlagSet(flagSet)
.addEnvSet(envSet.build())
.build();
CToolchain.Feature archiveParamFile =
CToolchain.Feature.newBuilder()
.setName(CppRuleClasses.ARCHIVE_PARAM_FILE)
.setEnabled(true)
.build();
ImmutableList<CToolchain.Feature> features =
new ImmutableList.Builder<CToolchain.Feature>()
.addAll(
Expand All @@ -146,6 +151,7 @@ private FeatureConfiguration getMockFeatureConfiguration(ImmutableMap<String, St
/* supportsInterfaceSharedLibraries= */ false))
.addAll(CppActionConfigs.getFeaturesToAppearLastInFeaturesList(ImmutableSet.of()))
.add(linkCppStandardLibrary)
.add(archiveParamFile)
.build();

ImmutableList<CToolchain.ActionConfig> actionConfigs =
Expand All @@ -162,6 +168,7 @@ private FeatureConfiguration getMockFeatureConfiguration(ImmutableMap<String, St
.getFeatureConfiguration(
ImmutableSet.of(
"link_cpp_standard_library",
CppRuleClasses.ARCHIVE_PARAM_FILE,
LinkTargetType.EXECUTABLE.getActionName(),
LinkTargetType.NODEPS_DYNAMIC_LIBRARY.getActionName(),
LinkTargetType.DYNAMIC_LIBRARY.getActionName(),
Expand Down Expand Up @@ -535,20 +542,60 @@ public Action generate(ImmutableSet<StaticKeyAttributes> attributes)
}

@Test
public void testCommandLineSplitting() throws Exception {
public void testCommandLineSplittingWithoutArchiveParamFileFeature_shouldBeOnForLinking()
throws Exception {
RuleContext ruleContext = createDummyRuleContext();
Artifact output =
getDerivedArtifact(
PathFragment.create("output/path.xyz"),
getTargetConfiguration().getBinDirectory(RepositoryName.MAIN),
ruleContext.getBinDirectory(),
ActionsTestUtil.NULL_ARTIFACT_OWNER);
CcToolchainProvider toolchain =
CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext);
FeatureConfiguration featureConfiguration =
CcToolchainTestHelper.buildFeatures("feature {name: 'archive_param_file'}")
.getFeatureConfiguration(ImmutableSet.of());
CppLinkActionBuilder builder =
new CppLinkActionBuilder(
ruleContext,
ruleContext,
ruleContext.getLabel(),
output,
ruleContext.getConfiguration(),
toolchain,
toolchain.getFdoContext(),
featureConfiguration,
MockCppSemantics.INSTANCE);

builder.setLinkType(LinkTargetType.OBJC_EXECUTABLE);
assertThat(builder.canSplitCommandLine()).isTrue();

builder.setLinkType(LinkTargetType.OBJCPP_EXECUTABLE);
assertThat(builder.canSplitCommandLine()).isTrue();

builder.setLinkType(LinkTargetType.NODEPS_DYNAMIC_LIBRARY);
assertThat(builder.canSplitCommandLine()).isTrue();
}

@Test
public void testCommandLineSplittingWithoutArchiveParamFileFeature_shouldBeOffForIfSo()
throws Exception {
RuleContext ruleContext = createDummyRuleContext();
Artifact output =
getDerivedArtifact(
PathFragment.create("output/path.xyz"),
ruleContext.getBinDirectory(),
ActionsTestUtil.NULL_ARTIFACT_OWNER);
final Artifact outputIfso =
getDerivedArtifact(
PathFragment.create("output/path.ifso"),
getTargetConfiguration().getBinDirectory(RepositoryName.MAIN),
ruleContext.getBinDirectory(),
ActionsTestUtil.NULL_ARTIFACT_OWNER);
CcToolchainProvider toolchain =
CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext);
FeatureConfiguration featureConfiguration =
CcToolchainTestHelper.buildFeatures("feature {name: 'archive_param_file'}")
.getFeatureConfiguration(ImmutableSet.of());
CppLinkActionBuilder builder =
new CppLinkActionBuilder(
ruleContext,
Expand All @@ -558,26 +605,131 @@ public void testCommandLineSplitting() throws Exception {
ruleContext.getConfiguration(),
toolchain,
toolchain.getFdoContext(),
FeatureConfiguration.EMPTY,
featureConfiguration,
MockCppSemantics.INSTANCE);

builder.setLinkType(LinkTargetType.NODEPS_DYNAMIC_LIBRARY);
builder.setInterfaceOutput(outputIfso);
assertThat(builder.canSplitCommandLine()).isFalse();

builder.setInterfaceOutput(null);
builder.setLinkType(LinkTargetType.INTERFACE_DYNAMIC_LIBRARY);
assertThat(builder.canSplitCommandLine()).isFalse();
}

@Test
public void testCommandLineSplittingWithoutArchiveParamFileFeature_shouldBeOffForArchiving()
throws Exception {
RuleContext ruleContext = createDummyRuleContext();
Artifact output =
getDerivedArtifact(
PathFragment.create("output/path.xyz"),
ruleContext.getBinDirectory(),
ActionsTestUtil.NULL_ARTIFACT_OWNER);
CcToolchainProvider toolchain =
CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext);
FeatureConfiguration featureConfiguration =
CcToolchainTestHelper.buildFeatures("feature {name: 'archive_param_file'}")
.getFeatureConfiguration(ImmutableSet.of());
CppLinkActionBuilder builder =
new CppLinkActionBuilder(
ruleContext,
ruleContext,
ruleContext.getLabel(),
output,
ruleContext.getConfiguration(),
toolchain,
toolchain.getFdoContext(),
featureConfiguration,
MockCppSemantics.INSTANCE);

builder.setLinkType(LinkTargetType.STATIC_LIBRARY);
assertThat(builder.canSplitCommandLine()).isTrue();
assertThat(builder.canSplitCommandLine()).isFalse();

builder.setLinkType(LinkTargetType.PIC_STATIC_LIBRARY);
assertThat(builder.canSplitCommandLine()).isFalse();

builder.setLinkType(LinkTargetType.ALWAYS_LINK_STATIC_LIBRARY);
assertThat(builder.canSplitCommandLine()).isFalse();

builder.setLinkType(LinkTargetType.ALWAYS_LINK_PIC_STATIC_LIBRARY);
assertThat(builder.canSplitCommandLine()).isFalse();

builder.setLinkType(LinkTargetType.OBJC_ARCHIVE);
assertThat(builder.canSplitCommandLine()).isTrue();
assertThat(builder.canSplitCommandLine()).isFalse();

builder.setLinkType(LinkTargetType.OBJC_FULLY_LINKED_ARCHIVE);
assertThat(builder.canSplitCommandLine()).isFalse();
}

@Test
public void testCommandLineSplittingWithArchiveParamFileFeature_shouldBeOnForLinking()
throws Exception {
RuleContext ruleContext = createDummyRuleContext();
Artifact output =
getDerivedArtifact(
PathFragment.create("output/path.xyz"),
ruleContext.getBinDirectory(),
ActionsTestUtil.NULL_ARTIFACT_OWNER);
CcToolchainProvider toolchain =
CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext);
FeatureConfiguration featureConfiguration =
CcToolchainTestHelper.buildFeatures("feature {name: 'archive_param_file'}")
.getFeatureConfiguration(ImmutableSet.of("archive_param_file"));
CppLinkActionBuilder builder =
new CppLinkActionBuilder(
ruleContext,
ruleContext,
ruleContext.getLabel(),
output,
ruleContext.getConfiguration(),
toolchain,
toolchain.getFdoContext(),
featureConfiguration,
MockCppSemantics.INSTANCE);

builder.setLinkType(LinkTargetType.OBJC_EXECUTABLE);
assertThat(builder.canSplitCommandLine()).isTrue();

builder.setLinkType(LinkTargetType.OBJCPP_EXECUTABLE);
assertThat(builder.canSplitCommandLine()).isTrue();

builder.setLinkType(LinkTargetType.OBJC_FULLY_LINKED_ARCHIVE);
assertThat(builder.canSplitCommandLine()).isTrue();

builder.setLinkType(LinkTargetType.NODEPS_DYNAMIC_LIBRARY);
assertThat(builder.canSplitCommandLine()).isTrue();
}

@Test
public void testCommandLineSplittingWithArchiveParamFileFeature_shouldBeOffForIfSo()
throws Exception {
RuleContext ruleContext = createDummyRuleContext();
Artifact output =
getDerivedArtifact(
PathFragment.create("output/path.xyz"),
ruleContext.getBinDirectory(),
ActionsTestUtil.NULL_ARTIFACT_OWNER);
final Artifact outputIfso =
getDerivedArtifact(
PathFragment.create("output/path.ifso"),
ruleContext.getBinDirectory(),
ActionsTestUtil.NULL_ARTIFACT_OWNER);
CcToolchainProvider toolchain =
CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext);
FeatureConfiguration featureConfiguration =
CcToolchainTestHelper.buildFeatures("feature {name: 'archive_param_file'}")
.getFeatureConfiguration(ImmutableSet.of("archive_param_file"));
CppLinkActionBuilder builder =
new CppLinkActionBuilder(
ruleContext,
ruleContext,
ruleContext.getLabel(),
output,
ruleContext.getConfiguration(),
toolchain,
toolchain.getFdoContext(),
featureConfiguration,
MockCppSemantics.INSTANCE);

builder.setLinkType(LinkTargetType.NODEPS_DYNAMIC_LIBRARY);
builder.setInterfaceOutput(outputIfso);
assertThat(builder.canSplitCommandLine()).isFalse();

Expand All @@ -586,6 +738,51 @@ public void testCommandLineSplitting() throws Exception {
assertThat(builder.canSplitCommandLine()).isFalse();
}

@Test
public void testCommandLineSplittingWithArchiveParamFileFeature_shouldBeOnForArchiving()
throws Exception {
RuleContext ruleContext = createDummyRuleContext();
Artifact output =
getDerivedArtifact(
PathFragment.create("output/path.xyz"),
ruleContext.getBinDirectory(),
ActionsTestUtil.NULL_ARTIFACT_OWNER);
CcToolchainProvider toolchain =
CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext);
FeatureConfiguration featureConfiguration =
CcToolchainTestHelper.buildFeatures("feature {name: 'archive_param_file'}")
.getFeatureConfiguration(ImmutableSet.of("archive_param_file"));
CppLinkActionBuilder builder =
new CppLinkActionBuilder(
ruleContext,
ruleContext,
ruleContext.getLabel(),
output,
ruleContext.getConfiguration(),
toolchain,
toolchain.getFdoContext(),
featureConfiguration,
MockCppSemantics.INSTANCE);

builder.setLinkType(LinkTargetType.STATIC_LIBRARY);
assertThat(builder.canSplitCommandLine()).isTrue();

builder.setLinkType(LinkTargetType.PIC_STATIC_LIBRARY);
assertThat(builder.canSplitCommandLine()).isTrue();

builder.setLinkType(LinkTargetType.ALWAYS_LINK_STATIC_LIBRARY);
assertThat(builder.canSplitCommandLine()).isTrue();

builder.setLinkType(LinkTargetType.ALWAYS_LINK_PIC_STATIC_LIBRARY);
assertThat(builder.canSplitCommandLine()).isTrue();

builder.setLinkType(LinkTargetType.OBJC_ARCHIVE);
assertThat(builder.canSplitCommandLine()).isTrue();

builder.setLinkType(LinkTargetType.OBJC_FULLY_LINKED_ARCHIVE);
assertThat(builder.canSplitCommandLine()).isTrue();
}

@Test
public void tesLocalLinkResourceEstimate() throws Exception {
CppLinkAction linkAction =
Expand Down Expand Up @@ -1067,7 +1264,7 @@ public void testExposesLinkstampObjects() throws Exception {
}

@Test
public void testGccQuotingForParamFilesFeature_EnablesGccQuoting() throws Exception {
public void testGccQuotingForParamFilesFeature_enablesGccQuoting() throws Exception {
getAnalysisMock()
.ccSupport()
.setupCcToolchainConfig(
Expand Down
7 changes: 7 additions & 0 deletions tools/cpp/unix_cc_toolchain_config.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1210,6 +1210,11 @@ def _impl(ctx):
],
)

archive_param_file_feature = feature(
name = "archive_param_file",
enabled = True,
)

is_linux = ctx.attr.target_libc != "macosx"
libtool_feature = feature(
name = "libtool",
Expand Down Expand Up @@ -1270,6 +1275,7 @@ def _impl(ctx):
sysroot_feature,
unfiltered_compile_flags_feature,
treat_warnings_as_errors_feature,
archive_param_file_feature,
] + layering_check_features(ctx.attr.compiler)
else:
# macOS artifact name patterns differ from the defaults only for dynamic
Expand Down Expand Up @@ -1303,6 +1309,7 @@ def _impl(ctx):
sysroot_feature,
unfiltered_compile_flags_feature,
treat_warnings_as_errors_feature,
archive_param_file_feature,
] + layering_check_features(ctx.attr.compiler)

return cc_common.create_cc_toolchain_config_info(
Expand Down
Loading

0 comments on commit bff9730

Please sign in to comment.