Skip to content

Commit

Permalink
Put C++ toolchain environment variables into CppLinkAction key
Browse files Browse the repository at this point in the history 
This cl fixes the correctness issue with CppLinkAction where Bazel was not detecting changes to the environment variables set from the cc_toolchain. CppCompileActions and LtoBackendActions are already doing this, it's only CppLinkAction that was affected.

Fixes #8787.

RELNOTES: None.
PiperOrigin-RevId: 257220291
  • Loading branch information
@hlopko @copybara-github
hlopko authored and copybara-github committed Jul 9, 2019
1 parent f372207 commit b120388
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 21 deletions.
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,7 @@ protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp)
fp.addString(fake ? FAKE_LINK_GUID : LINK_GUID);
fp.addString(ldExecutable.getPathString());
fp.addStrings(linkCommandLine.arguments());
fp.addStringMap(toolchainEnv);
fp.addStrings(getExecutionInfo().keySet());

// TODO(bazel-team): For correctness, we need to ensure the invariant that all values accessed
Expand Down
67 changes: 46 additions & 21 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 @@ -20,6 +20,7 @@
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.primitives.Ints;
Expand Down Expand Up @@ -59,6 +60,7 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CToolchain;
import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CToolchain.EnvEntry;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -108,19 +110,31 @@ public Artifact.DerivedArtifact getDerivedArtifact(
masterConfig);
}

private FeatureConfiguration getMockFeatureConfiguration() throws Exception {
private FeatureConfiguration getMockFeatureConfiguration(ImmutableMap<String, String> envVars) {
CToolchain.FlagGroup flagGroup =
CToolchain.FlagGroup.newBuilder().addFlag("-lcpp_standard_library").build();
CToolchain.FlagSet flagSet =
CToolchain.FlagSet.newBuilder()
.addAction("c++-link-executable")
.addFlagGroup(flagGroup)
.build();
CToolchain.EnvSet.Builder envSet =
CToolchain.EnvSet.newBuilder()
.addAction("c++-link-executable")
.addAction("c++-link-static-library")
.addAction("c++-link-dynamic-library")
.addAction("c++-link-nodeps-dynamic-library");
for (String envVar : envVars.keySet()) {
envSet.addEnvEntry(
EnvEntry.newBuilder().setKey(envVar).setValue(envVars.get(envVar)).build());
}

CToolchain.Feature linkCppStandardLibrary =
CToolchain.Feature.newBuilder()
.setName("link_cpp_standard_library")
.setEnabled(true)
.addFlagSet(flagSet)
.addEnvSet(envSet.build())
.build();
ImmutableList<CToolchain.Feature> features =
new ImmutableList.Builder<CToolchain.Feature>()
Expand All @@ -147,14 +161,18 @@ private FeatureConfiguration getMockFeatureConfiguration() throws Exception {
/* supportsInterfaceSharedLibraries= */ false,
/* existingActionConfigNames= */ ImmutableSet.of());

return CcToolchainFeaturesTest.buildFeatures(features, actionConfigs)
.getFeatureConfiguration(
ImmutableSet.of(
"link_cpp_standard_library",
Link.LinkTargetType.EXECUTABLE.getActionName(),
Link.LinkTargetType.NODEPS_DYNAMIC_LIBRARY.getActionName(),
Link.LinkTargetType.DYNAMIC_LIBRARY.getActionName(),
Link.LinkTargetType.STATIC_LIBRARY.getActionName()));
try {
return CcToolchainFeaturesTest.buildFeatures(features, actionConfigs)
.getFeatureConfiguration(
ImmutableSet.of(
"link_cpp_standard_library",
LinkTargetType.EXECUTABLE.getActionName(),
LinkTargetType.NODEPS_DYNAMIC_LIBRARY.getActionName(),
LinkTargetType.DYNAMIC_LIBRARY.getActionName(),
LinkTargetType.STATIC_LIBRARY.getActionName()));
} catch (Exception e) {
throw new RuntimeException(e);
}
}

@Test
Expand Down Expand Up @@ -445,7 +463,8 @@ private enum NonStaticAttributes {
NATIVE_DEPS,
USE_TEST_ONLY_FLAGS,
FAKE,
RUNTIME_SOLIB_DIR
RUNTIME_SOLIB_DIR,
ENVIRONMENT,
}

/**
Expand All @@ -459,7 +478,6 @@ public void testComputeKeyNonStatic() throws Exception {
final PathFragment dynamicOutputPath = PathFragment.create("dummyRuleContext/output/path.so");
final Artifact staticOutputFile = getBinArtifactWithNoOwner(exeOutputPath.getPathString());
final Artifact dynamicOutputFile = getBinArtifactWithNoOwner(dynamicOutputPath.getPathString());
final FeatureConfiguration featureConfiguration = getMockFeatureConfiguration();

ActionTester.runTest(
NonStaticAttributes.class,
Expand All @@ -481,7 +499,10 @@ public Action generate(ImmutableSet<NonStaticAttributes> attributesToFlip)
ruleContext.getConfiguration(),
toolchain,
toolchain.getFdoContext(),
featureConfiguration,
getMockFeatureConfiguration(
attributesToFlip.contains(NonStaticAttributes.ENVIRONMENT)
? ImmutableMap.of("var", "value")
: ImmutableMap.of()),
MockCppSemantics.INSTANCE);
if (attributesToFlip.contains(NonStaticAttributes.OUTPUT_FILE)) {
builder.setLinkType(LinkTargetType.NODEPS_DYNAMIC_LIBRARY);
Expand All @@ -507,6 +528,7 @@ public Action generate(ImmutableSet<NonStaticAttributes> attributesToFlip)

private enum StaticKeyAttributes {
OUTPUT_FILE,
ENVIRONMENT,
}

/**
Expand All @@ -520,7 +542,6 @@ public void testComputeKeyStatic() throws Exception {
final PathFragment dynamicOutputPath = PathFragment.create("dummyRuleContext/output/path.so");
final Artifact staticOutputFile = getBinArtifactWithNoOwner(staticOutputPath.getPathString());
final Artifact dynamicOutputFile = getBinArtifactWithNoOwner(dynamicOutputPath.getPathString());
final FeatureConfiguration featureConfiguration = getMockFeatureConfiguration();

ActionTester.runTest(
StaticKeyAttributes.class,
Expand All @@ -542,7 +563,10 @@ public Action generate(ImmutableSet<StaticKeyAttributes> attributes)
ruleContext.getConfiguration(),
toolchain,
toolchain.getFdoContext(),
featureConfiguration,
getMockFeatureConfiguration(
attributes.contains(StaticKeyAttributes.ENVIRONMENT)
? ImmutableMap.of("var", "value")
: ImmutableMap.of()),
MockCppSemantics.INSTANCE);
builder.setLinkType(
attributes.contains(StaticKeyAttributes.OUTPUT_FILE)
Expand Down Expand Up @@ -627,7 +651,7 @@ private void assertLinkSizeAccuracy(int inputs) throws Exception {
"dummyRuleContext/binary2",
objects.build(),
ImmutableList.<LibraryToLink>of(),
getMockFeatureConfiguration())
getMockFeatureConfiguration(/* envVars= */ ImmutableMap.of()))
.setFake(true)
.build();

Expand Down Expand Up @@ -694,7 +718,7 @@ private CppLinkActionBuilder createLinkBuilder(RuleContext ruleContext, Link.Lin
output.getPathString(),
ImmutableList.<Artifact>of(),
ImmutableList.<LibraryToLink>of(),
getMockFeatureConfiguration());
getMockFeatureConfiguration(/* envVars= */ ImmutableMap.of()));
}

public Artifact getOutputArtifact(String relpath) {
Expand Down Expand Up @@ -969,7 +993,7 @@ public void testStaticLinking() throws Exception {
output.getExecPathString(),
ImmutableList.<Artifact>of(),
ImmutableList.<LibraryToLink>of(),
getMockFeatureConfiguration())
getMockFeatureConfiguration(/* envVars= */ ImmutableMap.of()))
.setLibraryIdentifier("foo")
.addObjectFiles(ImmutableList.of(testTreeArtifact))
.addObjectFile(objectFile)
Expand Down Expand Up @@ -998,7 +1022,7 @@ public void testPieOptionDisabledForSharedLibraries() throws Exception {
"dummyRuleContext/out.so",
ImmutableList.of(),
ImmutableList.of(),
getMockFeatureConfiguration())
getMockFeatureConfiguration(/* envVars= */ ImmutableMap.of()))
.setLinkingMode(Link.LinkingMode.STATIC)
.addLinkopts(ImmutableList.of("-pie", "-other", "-pie"))
.setLibraryIdentifier("foo")
Expand All @@ -1021,7 +1045,7 @@ public void testPieOptionKeptForExecutables() throws Exception {
"dummyRuleContext/out",
ImmutableList.of(),
ImmutableList.of(),
getMockFeatureConfiguration())
getMockFeatureConfiguration(/* envVars= */ ImmutableMap.of()))
.setLinkingMode(Link.LinkingMode.STATIC)
.addLinkopts(ImmutableList.of("-pie", "-other", "-pie"))
.build();
Expand Down Expand Up @@ -1052,7 +1076,7 @@ public void testLinkoptsComeAfterLinkerInputs() throws Exception {
"dummyRuleContext/out",
ImmutableList.of(),
ImmutableList.copyOf(linkerInputs),
getMockFeatureConfiguration())
getMockFeatureConfiguration(/* envVars= */ ImmutableMap.of()))
.addLinkopts(ImmutableList.of("FakeLinkopt1", "FakeLinkopt2"))
.build();

Expand Down Expand Up @@ -1113,7 +1137,8 @@ public void testSplitExecutableLinkCommandDynamicWithNoSplitting() throws Except
CcToolchainConfig.builder().withFeatures(CppRuleClasses.DO_NOT_SPLIT_LINKING_CMDLINE));
RuleContext ruleContext = createDummyRuleContext();

FeatureConfiguration featureConfiguration = getMockFeatureConfiguration();
FeatureConfiguration featureConfiguration =
getMockFeatureConfiguration(/* envVars= */ ImmutableMap.of());

CppLinkAction linkAction =
createLinkBuilder(
Expand Down

0 comments on commit b120388

Please sign in to comment.