Skip to content

Commit

Permalink
Don't require supports_fission to be set in the crosstool
Browse files Browse the repository at this point in the history
    Having a 'per_object_debug_data' feature enabled is giving the same signal as having supports_fission enabled. This is a safe change because all crosstools that have supports_fission: true have per_object_debug_info disabled, and with the legacy crosstool fields migration we will migrate this feature to be enabled by default for these crosstools.

    bazelbuild/bazel#6861
    bazelbuild/bazel#5883

    RELNOTES: None.
    PiperOrigin-RevId: 230701029
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent d8c2738 commit 6411de6
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 94 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
@AutoCodec
public final class CcToolchainProvider extends ToolchainInfo
implements CcToolchainProviderApi<FeatureConfiguration>, HasCcToolchainLabel {
public static final String SKYLARK_NAME = "CcToolchainInfo";

/** An empty toolchain to be returned in the error case (instead of null). */
public static final CcToolchainProvider EMPTY_TOOLCHAIN_IS_ERROR =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -310,17 +310,6 @@ public boolean apply(Artifact artifact) {
public static final String FDO_IMPLICIT_THINLTO_CONFIGURATION =
"" + "feature { name: 'fdo_implicit_thinlto'}";

public static final String ENABLE_XFDO_THINLTO_CONFIGURATION =
""
+ "feature {"
+ " name: 'enable_xbinaryfdo_thinlto'"
+ " requires { feature: 'xbinaryfdo_implicit_thinlto' }"
+ " implies: 'thin_lto'"
+ "}";

public static final String XFDO_IMPLICIT_THINLTO_CONFIGURATION =
"" + "feature { name: 'xbinaryfdo_implicit_thinlto'}";

public static final String AUTO_FDO_CONFIGURATION =
""
+ "feature {"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
import com.google.devtools.build.lib.analysis.util.AnalysisMock;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import com.google.devtools.build.lib.packages.util.MockCcSupport;
Expand Down Expand Up @@ -186,87 +185,6 @@ public void testPresenceOfPerObjectDebugFileBuildVariable() throws Exception {
.isNotNull();
}

@Test
public void testPresenceOfIsUsingFissionVariable() throws Exception {
AnalysisMock.get()
.ccSupport()
.setupCrosstool(mockToolsConfig, MockCcSupport.PER_OBJECT_DEBUG_INFO_CONFIGURATION);
useConfiguration("--fission=yes");

scratch.file("x/BUILD", "cc_binary(name = 'bin', srcs = ['bin.cc'])");
scratch.file("x/bin.cc");

CcToolchainVariables variables = getCompileBuildVariables("//x:bin", "bin");

assertThat(
variables.getStringVariable(CompileBuildVariables.IS_USING_FISSION.getVariableName()))
.isNotNull();
}

@Test
public void testPresenceOfIsUsingFissionAndPerDebugObjectFileVariablesWithThinlto()
throws Exception {
AnalysisMock.get()
.ccSupport()
.setupCrosstool(
mockToolsConfig,
"feature {",
" name: 'fission_flags_for_lto_backend'",
" enabled: true",
" flag_set {",
" action: 'lto-backend'",
" flag_group {",
" expand_if_all_available: 'is_using_fission'",
" flag: '-<IS_USING_FISSION>'",
" }",
" flag_group {",
" expand_if_all_available: 'per_object_debug_info_file'",
" flag: '-<PER_OBJECT_DEBUG_INFO_FILE>'",
" }",
" }",
"}",
MockCcSupport.PER_OBJECT_DEBUG_INFO_CONFIGURATION,
"supports_start_end_lib: true",
MockCcSupport.THIN_LTO_CONFIGURATION,
MockCcSupport.HOST_AND_NONHOST_CONFIGURATION);
useConfiguration("--fission=yes", "--features=thin_lto");

scratch.file("x/BUILD", "cc_binary(name = 'bin', srcs = ['bin.cc'])");
scratch.file("x/bin.cc");

RuleConfiguredTarget target = (RuleConfiguredTarget) getConfiguredTarget("//x:bin");
LtoBackendAction backendAction =
(LtoBackendAction)
target.getActions().stream()
.filter(a -> a.getMnemonic().equals("CcLtoBackendCompile"))
.findFirst()
.get();
CppCompileAction bitcodeAction =
(CppCompileAction)
target.getActions().stream()
.filter(a -> a.getMnemonic().equals("CppCompile"))
.findFirst()
.get();

// We don't pass per_object_debug_info_file to bitcode compiles
assertThat(
bitcodeAction
.getCompileCommandLine()
.getVariables()
.isAvailable(CompileBuildVariables.IS_USING_FISSION.getVariableName()))
.isTrue();
assertThat(
bitcodeAction
.getCompileCommandLine()
.getVariables()
.isAvailable(CompileBuildVariables.PER_OBJECT_DEBUG_INFO_FILE.getVariableName()))
.isFalse();

// We do pass per_object_debug_info_file to backend compiles
assertThat(backendAction.getArguments()).contains("-<PER_OBJECT_DEBUG_INFO_FILE>");
assertThat(backendAction.getArguments()).contains("-<IS_USING_FISSION>");
}

@Test
public void testPresenceOfPerObjectDebugFileBuildVariableUsingLegacyFields() throws Exception {
AnalysisMock.get().ccSupport().setupCrosstool(mockToolsConfig, "supports_fission: true");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -460,9 +460,33 @@ private Action getPredecessorByInputName(Action action, String str) {
return null;
}

@Test
public void testUserLinkFlags() throws Exception {
useConfiguration("--linkopt=-bar", "--noexperimental_linkopts_in_user_link_flags");

scratch.file("x/BUILD", "cc_binary(name = 'foo', srcs = ['a.cc'], linkopts = ['-foo'])");
scratch.file("x/a.cc");

ConfiguredTarget testTarget = getConfiguredTarget("//x:foo");
CcToolchainVariables testVariables =
getLinkBuildVariables(testTarget, LinkTargetType.EXECUTABLE);

ImmutableList<String> userLinkFlags =
CcToolchainVariables.toStringList(
testVariables, LinkBuildVariables.USER_LINK_FLAGS.getVariableName());
assertThat(userLinkFlags).contains("-foo");
assertThat(userLinkFlags).doesNotContain("-bar");

ImmutableList<String> legacyLinkFlags =
CcToolchainVariables.toStringList(
testVariables, LinkBuildVariables.LEGACY_LINK_FLAGS.getVariableName());
assertThat(legacyLinkFlags).doesNotContain("-foo");
assertThat(legacyLinkFlags).contains("-bar");
}

@Test
public void testUserLinkFlagsWithLinkoptOption() throws Exception {
useConfiguration("--linkopt=-bar");
useConfiguration("--linkopt=-bar", "--experimental_linkopts_in_user_link_flags");

scratch.file("x/BUILD", "cc_binary(name = 'foo', srcs = ['a.cc'], linkopts = ['-foo'])");
scratch.file("x/a.cc");
Expand Down

0 comments on commit 6411de6

Please sign in to comment.