Skip to content

Commit

Permalink
Add --incompatible_use_specific_tool_files.
Browse files Browse the repository at this point in the history
This option makes
1. C++ compilation actions include only the toolchain's compiler_files artifacts as inputs not all_files.
2. Non-preprocessed assembler actions include only the toolchain's as_files artifacts as inputs not all_files.
3. Archiver link actions include the toolchain's ar_files artifacts as inputs rather than linker_files.

See #8531.

Closes #7725.

PiperOrigin-RevId: 250880419
  • Loading branch information
benjaminp authored and copybara-github committed May 31, 2019
1 parent 3dc59e4 commit b691915
Show file tree
Hide file tree
Showing 6 changed files with 127 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.rules.cpp.AspectLegalCppSemantics;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
import com.google.devtools.build.lib.rules.cpp.CcToolchainProvider;
import com.google.devtools.build.lib.rules.cpp.CppActionNames;
import com.google.devtools.build.lib.rules.cpp.CppCompileActionBuilder;
import com.google.devtools.build.lib.rules.cpp.CppConfiguration;
import com.google.devtools.build.lib.rules.cpp.CppConfiguration.HeadersCheckingMode;
import com.google.devtools.build.lib.rules.cpp.IncludeProcessing;
import com.google.devtools.build.lib.rules.cpp.NoProcessing;
Expand All @@ -43,10 +46,14 @@ public void finalizeCompileActionBuilder(
BuildConfiguration configuration,
FeatureConfiguration featureConfiguration,
CppCompileActionBuilder actionBuilder) {
CcToolchainProvider toolchain = actionBuilder.getToolchain();
actionBuilder
// Because Bazel does not support include scanning, we need the entire crosstool filegroup,
// including header files, as opposed to just the "compile" filegroup.
.addTransitiveMandatoryInputs(actionBuilder.getToolchain().getAllFiles())
.addTransitiveMandatoryInputs(
configuration.getFragment(CppConfiguration.class).useSpecificToolFiles()
? (actionBuilder.getActionName().equals(CppActionNames.ASSEMBLE)
? toolchain.getAsFiles()
: toolchain.getCompilerFiles())
: toolchain.getAllFiles())
.setShouldScanIncludes(false);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -622,12 +622,11 @@ private CppLinkAction registerActionForStaticLibrary(
throws RuleErrorException, InterruptedException {
Artifact linkedArtifact = getLinkedArtifact(linkTargetTypeUsedForNaming);
CppLinkAction action =
newLinkActionBuilder(linkedArtifact)
newLinkActionBuilder(linkedArtifact, linkTargetTypeUsedForNaming)
.addObjectFiles(ccOutputs.getObjectFiles(usePic))
.addNonCodeInputs(nonCodeLinkerInputs)
.addLtoCompilationContext(ccOutputs.getLtoCompilationContext())
.setUsePicForLtoBackendActions(usePic)
.setLinkType(linkTargetTypeUsedForNaming)
.setLinkingMode(LinkingMode.STATIC)
.addActionInputs(linkActionInputs)
.setLibraryIdentifier(libraryIdentifier)
Expand Down Expand Up @@ -679,14 +678,13 @@ private boolean createDynamicLinkAction(
}

CppLinkActionBuilder dynamicLinkActionBuilder =
newLinkActionBuilder(linkerOutput)
newLinkActionBuilder(linkerOutput, dynamicLinkType)
.setWholeArchive(wholeArchive)
.setNativeDeps(nativeDeps)
.setAdditionalLinkstampDefines(additionalLinkstampDefines.build())
.setInterfaceOutput(soInterface)
.addNonCodeInputs(ccOutputs.getHeaderTokenFiles())
.addLtoCompilationContext(ccOutputs.getLtoCompilationContext())
.setLinkType(dynamicLinkType)
.setLinkingMode(linkingMode)
.setFake(fake)
.addActionInputs(linkActionInputs)
Expand Down Expand Up @@ -823,7 +821,8 @@ private boolean createDynamicLinkAction(
return hasBuiltDynamicLibrary;
}

private CppLinkActionBuilder newLinkActionBuilder(Artifact outputArtifact) {
private CppLinkActionBuilder newLinkActionBuilder(
Artifact outputArtifact, LinkTargetType linkType) {
return new CppLinkActionBuilder(
ruleErrorConsumer,
actionConstructionContext,
Expand All @@ -837,7 +836,12 @@ private CppLinkActionBuilder newLinkActionBuilder(Artifact outputArtifact) {
.setGrepIncludes(grepIncludes)
.setIsStampingEnabled(isStampingEnabled)
.setTestOrTestOnlyTarget(isTestOrTestOnlyTarget)
.setLinkerFiles(ccToolchain.getLinkerFiles())
.setLinkType(linkType)
.setLinkerFiles(
(cppConfiguration.useSpecificToolFiles()
&& linkType.linkerOrArchiver() == LinkerOrArchiver.ARCHIVER)
? ccToolchain.getArFiles()
: ccToolchain.getLinkerFiles())
.setLinkArtifactFactory(linkArtifactFactory)
.setUseTestOnlyFlags(useTestOnlyFlags);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ public NestedSet<Artifact> getMandatoryInputs() {
return mandatoryInputsBuilder.build();
}

private String getActionName() {
public String getActionName() {
if (actionName != null) {
return actionName;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -665,4 +665,8 @@ public boolean saveFeatureState() {
public boolean useStandaloneLtoIndexingCommandLines() {
return cppOptions.useStandaloneLtoIndexingCommandLines;
}

public boolean useSpecificToolFiles() {
return cppOptions.useSpecificToolFiles;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -864,6 +864,20 @@ public Label getFdoPrefetchHintsLabel() {
help = "Save the state of enabled and requested feautres as an output of compilation.")
public boolean saveFeatureState;

@Option(
name = "incompatible_use_specific_tool_files",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
metadataTags = {
OptionMetadataTag.INCOMPATIBLE_CHANGE,
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
},
help =
"Use cc toolchain's compiler_files, as_files, and ar_files as inputs to appropriate "
+ "actions. See https://github.com/bazelbuild/bazel/issues/8531")
public boolean useSpecificToolFiles;

@Override
public FragmentOptions getHost() {
CppOptions host = (CppOptions) getDefault();
Expand Down Expand Up @@ -918,6 +932,7 @@ public FragmentOptions getHost() {
host.dontEnableHostNonhost = dontEnableHostNonhost;
host.requireCtxInConfigureFeatures = requireCtxInConfigureFeatures;
host.useStandaloneLtoIndexingCommandLines = useStandaloneLtoIndexingCommandLines;
host.useSpecificToolFiles = useSpecificToolFiles;

// Save host options for further use.
host.hostCoptList = hostCoptList;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,15 @@
package com.google.devtools.build.lib.rules.cpp;

import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.MoreCollectors;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.RuleContext;
Expand All @@ -34,6 +39,7 @@
import com.google.devtools.build.lib.testutil.TestConstants;
import com.google.devtools.common.options.OptionsParsingException;
import java.io.IOException;
import java.util.Set;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand Down Expand Up @@ -643,4 +649,85 @@ public void testSysroot_fromCrosstool_unset() throws Exception {

assertThat(toolchainProvider.getSysroot()).isEqualTo("/usr/grte/v1");
}

@Test
public void correctToolFilesUsed() throws Exception {
scratch.file(
"a/BUILD",
"cc_toolchain_alias(name = 'a')",
"cc_library(name = 'l', srcs = ['l.c'])",
"cc_library(name = 'asm', srcs = ['a.s'])",
"cc_library(name = 'preprocessed-asm', srcs = ['a.S'])");
getAnalysisMock()
.ccSupport()
.setupCcToolchainConfig(
mockToolsConfig,
CcToolchainConfig.builder().withFeatures(CppRuleClasses.SUPPORTS_DYNAMIC_LINKER));
useConfiguration("--incompatible_use_specific_tool_files");
ConfiguredTarget target = getConfiguredTarget("//a:a");
CcToolchainProvider toolchainProvider =
(CcToolchainProvider) target.get(ToolchainInfo.PROVIDER);
// Check that the mock toolchain tool file sets are an antichain, so that our subset assertions
// below are meaningful.
ImmutableList<Set<Artifact>> fileGroups =
ImmutableList.of(
toolchainProvider.getArFiles().toSet(),
toolchainProvider.getLinkerFiles().toSet(),
toolchainProvider.getCompilerFiles().toSet(),
toolchainProvider.getAsFiles().toSet(),
toolchainProvider.getAllFiles().toSet());
for (int i = 0; i < fileGroups.size(); i++) {
assertThat(fileGroups.get(i)).isNotEmpty();
for (int j = 0; j < fileGroups.size(); j++) {
if (i == j) {
continue;
}
Set<Artifact> one = fileGroups.get(i);
Set<Artifact> two = fileGroups.get(j);
assertWithMessage(String.format("%s should not contain %s", one, two))
.that(one.containsAll(two))
.isFalse();
}
}
assertThat(
Sets.difference(
toolchainProvider.getArFiles().toSet(), toolchainProvider.getLinkerFiles().toSet()))
.isNotEmpty();
assertThat(
Sets.difference(
toolchainProvider.getLinkerFiles().toSet(), toolchainProvider.getArFiles().toSet()))
.isNotEmpty();

RuleConfiguredTarget libTarget = (RuleConfiguredTarget) getConfiguredTarget("//a:l");
Artifact staticLib =
getOutputGroup(libTarget, "archive").toList().stream()
.collect(MoreCollectors.onlyElement());
ActionAnalysisMetadata staticAction = getGeneratingAction(staticLib);
assertThat(staticAction.getInputs()).containsAtLeastElementsIn(toolchainProvider.getArFiles());
Artifact dynamicLib =
getOutputGroup(libTarget, "dynamic_library").toList().stream()
.collect(MoreCollectors.onlyElement());
ActionAnalysisMetadata dynamicAction = getGeneratingAction(dynamicLib);
assertThat(dynamicAction.getInputs())
.containsAtLeastElementsIn(toolchainProvider.getLinkerFiles());
ActionAnalysisMetadata cCompileAction =
libTarget.getActions().stream()
.filter((a) -> a.getMnemonic().equals("CppCompile"))
.collect(MoreCollectors.onlyElement());
assertThat(cCompileAction.getInputs())
.containsAtLeastElementsIn(toolchainProvider.getCompilerFiles());
ActionAnalysisMetadata asmAction =
((RuleConfiguredTarget) getConfiguredTarget("//a:asm"))
.getActions().stream()
.filter((a) -> a.getMnemonic().equals("CppCompile"))
.collect(MoreCollectors.onlyElement());
assertThat(asmAction.getInputs()).containsAtLeastElementsIn(toolchainProvider.getAsFiles());
ActionAnalysisMetadata preprocessedAsmAction =
((RuleConfiguredTarget) getConfiguredTarget("//a:preprocessed-asm"))
.getActions().stream()
.filter((a) -> a.getMnemonic().equals("CppCompile"))
.collect(MoreCollectors.onlyElement());
assertThat(preprocessedAsmAction.getInputs())
.containsAtLeastElementsIn(toolchainProvider.getCompilerFiles());
}
}

0 comments on commit b691915

Please sign in to comment.