Skip to content

Commit

Permalink
Allow setting supports_dynamic_linker crosstool capability using feature
Browse files Browse the repository at this point in the history
This cl allows toolchain owners to express that toolchain supports creating dynamic libraries.

This is in theory a breaking change, for the crosstools that don't use `dynamic_library_linker_flag`, and don't specify `linking_mode_flags { mode: DYNAMIC }`, and they do specify feature { name: 'dynamic_linking_mode' }. This currently means the toolchain will generate shared libraries, but with this change it will not.

But since this only happens to toolchains that don't use legacy fields, and I don't think there are such toolchains in the wild, I'll go ahead and do this change without following the incompatible change process.

This cl is a step towards #5883. Also
see the rollout doc here:
https://docs.google.com/document/d/1uv4c1zag6KvdI31qdx8C6jiTognXPQrxgsUpVefm9fM/edit#.

Flag removing legacy behavior is #6861

RELNOTES: None.
PiperOrigin-RevId: 226631573
  • Loading branch information
hlopko authored and Copybara-Service committed Dec 22, 2018
1 parent d1a8a90 commit 8098e59
Show file tree
Hide file tree
Showing 8 changed files with 46 additions and 14 deletions.
7 changes: 7 additions & 0 deletions site/docs/crosstool-reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -1035,4 +1035,11 @@ conditions.
libraries. This makes incremental relinking faster.
</td>
</tr>
<tr>
<td><strong><code>supports_dynamic_linker</code></strong>
</td>
<td>If enabled, C++ rules will know the toolchain can produce shared
libraries.
</td>
</tr>
</table>
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ public static void init(
.addLinkstamps(ruleContext.getPrerequisites("linkstamp", Mode.TARGET));

Artifact soImplArtifact = null;
boolean supportsDynamicLinker = ccToolchain.supportsDynamicLinker();
boolean supportsDynamicLinker = ccToolchain.supportsDynamicLinker(featureConfiguration);
// TODO(djasper): This is hacky. We should actually try to figure out whether we generate
// ccOutputs.
boolean createDynamicLibrary =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -553,11 +553,10 @@ public boolean supportsGoldLinker() {
return toolchainInfo.supportsGoldLinker();
}

/**
* Returns whether the toolchain supports dynamic linking.
*/
public boolean supportsDynamicLinker() {
return toolchainInfo.supportsDynamicLinker();
/** Returns whether the toolchain supports dynamic linking. */
public boolean supportsDynamicLinker(FeatureConfiguration featureConfiguration) {
return toolchainInfo.supportsDynamicLinker()
|| featureConfiguration.isEnabled(CppRuleClasses.SUPPORTS_DYNAMIC_LINKER);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,11 @@ public static Label ccToolchainTypeAttribute(RuleDefinitionEnvironment env) {
/** A feature marking that the toolchain can use --start-lib/--end-lib flags */
public static final String SUPPORTS_START_END_LIB = "supports_start_end_lib";

/**
* A feature marking that the toolchain can produce binaries that load shared libraries at
* runtime.
*/
public static final String SUPPORTS_DYNAMIC_LINKER = "supports_dynamic_linker";
/** Ancestor for all rules that do include scanning. */
public static final class CcIncludeScanningRule implements RuleDefinition {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -332,11 +332,7 @@ public static CppToolchainInfo create(
this.supportsFission = supportsFission;
this.supportsStartEndLib = supportsStartEndLib;
this.supportsEmbeddedRuntimes = supportsEmbeddedRuntimes;
this.supportsDynamicLinker =
supportsDynamicLinker
|| toolchainFeatures
.getActivatableNames()
.contains(CppRuleClasses.DYNAMIC_LINKING_MODE);
this.supportsDynamicLinker = supportsDynamicLinker;
this.supportsInterfaceSharedLibraries = supportsInterfaceSharedLibraries;
this.supportsGoldLinker = supportsGoldLinker;
this.toolchainNeedsPic = toolchainNeedsPic;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ private CcLinkingHelper initializeLinkingHelper(
helper.addDeps(deps);
// TODO(dougk): Configure output artifact with action_config
// once proto compile action is configurable from the crosstool.
if (!toolchain.supportsDynamicLinker()) {
if (!toolchain.supportsDynamicLinker(featureConfiguration)) {
helper.setShouldCreateDynamicLibrary(false);
}
return helper;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ public boolean apply(Artifact artifact) {
public static final String DYNAMIC_LINKING_MODE_FEATURE =
"feature { name: '" + CppRuleClasses.DYNAMIC_LINKING_MODE + "'}";

public static final String SUPPORTS_DYNAMIC_LINKER_FEATURE =
"feature { name: '" + CppRuleClasses.SUPPORTS_DYNAMIC_LINKER + " enabled: true'}";

/** Feature expected by the C++ rules when pic build is requested */
public static final String PIC_FEATURE =
""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -774,7 +774,7 @@ private void writeStarlarkRule() throws IOException {
}

@Test
public void testSupportsDynamicLinkerCheckFeatures() throws Exception {
public void testSupportsDynamicLinkerIsFalseWhenFeatureNotSet() throws Exception {
scratch.file("a/BUILD", "cc_toolchain_alias(name = 'b')");

getAnalysisMock()
Expand All @@ -789,7 +789,29 @@ public void testSupportsDynamicLinkerCheckFeatures() throws Exception {
CcToolchainProvider toolchainProvider =
(CcToolchainProvider) target.get(ToolchainInfo.PROVIDER);

assertThat(toolchainProvider.supportsDynamicLinker()).isTrue();
assertThat(toolchainProvider.supportsDynamicLinker(FeatureConfiguration.EMPTY)).isFalse();
}

@Test
public void testSupportsDynamicLinkerIsTrueWhenFeatureSet() throws Exception {
scratch.file("a/BUILD", "cc_toolchain_alias(name = 'b')");

getAnalysisMock()
.ccSupport()
.setupCrosstool(
mockToolsConfig,
MockCcSupport.DYNAMIC_LINKING_MODE_FEATURE,
MockCcSupport.SUPPORTS_DYNAMIC_LINKER_FEATURE);

// To make sure the toolchain doesn't define linking_mode_flags { mode: DYNAMIC } as that would
// also result in supportsDynamicLinker returning true
useConfiguration("--compiler=compiler_no_dyn_linker");

ConfiguredTarget target = getConfiguredTarget("//a:b");
CcToolchainProvider toolchainProvider =
(CcToolchainProvider) target.get(ToolchainInfo.PROVIDER);

assertThat(toolchainProvider.supportsDynamicLinker(FeatureConfiguration.EMPTY)).isFalse();
}

// Tests CcCommon::enableStaticLinkCppRuntimesFeature when supports_embedded_runtimes is not
Expand Down

0 comments on commit 8098e59

Please sign in to comment.