Skip to content

Commit

Permalink
Make bazel option --collect_symbol_counts a noop
Browse files Browse the repository at this point in the history
Our reasoning:
* it only works with gold
* it was broken for the last 3 months (since 93b1912) (silently did
nothing and nobody complained)
* we see almost zero usage of this option

RELNOTES: None.
PiperOrigin-RevId: 228116158
  • Loading branch information
hlopko authored and Copybara-Service committed Jan 7, 2019
1 parent deb028e commit f55ae7a
Show file tree
Hide file tree
Showing 20 changed files with 11 additions and 121 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,15 @@
public class BazelRulesModule extends BlazeModule {
/** This is where deprecated options go to die. */
public static class GraveyardOptions extends OptionsBase {
@Option(
name = "output_symbol_counts",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.ACTION_COMMAND_LINES, OptionEffectTag.AFFECTS_OUTPUTS},
metadataTags = {OptionMetadataTag.DEPRECATED},
help = "Deprecated no-op.")
public boolean symbolCounts;

@Option(
name = "incompatible_disable_sysroot_from_configuration",
defaultValue = "true",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -657,14 +657,6 @@ private void createDynamicLibrary(
ccToolchain.getStaticRuntimeLinkInputs(ruleContext, featureConfiguration));
}

if (CppLinkAction.enableSymbolsCounts(
cppConfiguration, ccToolchain.supportsGoldLinker(), fake, staticLinkType)) {
dynamicLinkActionBuilder.setSymbolCountsOutput(
ruleContext.getBinArtifact(
CppLinkAction.symbolCountsFileName(
PathFragment.create(ruleContext.getTarget().getName()))));
}

// On Windows, we cannot build a shared library with symbols unresolved, so here we
// dynamically link to all its dependencies, even for LinkTargetType.NODEPS_DYNAMIC_LIBRARY.
boolean shouldLinkTransitively =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,6 @@ public CcToolchainVariables getLinkBuildVariables(
/* thinltoParamFile= */ null,
/* thinltoMergedObjectFile= */ null,
mustKeepDebug,
/* symbolCounts= */ null,
ccToolchainProvider,
featureConfiguration,
useTestOnlyFlags,
Expand Down Expand Up @@ -961,7 +960,6 @@ public CcToolchainConfigInfo ccToolchainConfigInfoFromSkylark(
compiler,
abiVersion,
abiLibcVersion,
/* supportsGoldLinker= */ false,
/* supportsStartEndLib= */ false,
/* supportsInterfaceSharedLibraries= */ false,
/* supportsEmbeddedRuntimes= */ false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ public class CcToolchainConfigInfo extends NativeInfo implements CcToolchainConf
private final String compiler;
private final String abiVersion;
private final String abiLibcVersion;
private final boolean supportsGoldLinker;
private final boolean supportsStartEndLib;
private final boolean supportsInterfaceSharedLibraries;
private final boolean supportsEmbeddedRuntimes;
Expand Down Expand Up @@ -109,7 +108,6 @@ public CcToolchainConfigInfo(
String compiler,
String abiVersion,
String abiLibcVersion,
boolean supportsGoldLinker,
boolean supportsStartEndLib,
boolean supportsInterfaceSharedLibraries,
boolean supportsEmbeddedRuntimes,
Expand Down Expand Up @@ -152,7 +150,6 @@ public CcToolchainConfigInfo(
this.compiler = compiler;
this.abiVersion = abiVersion;
this.abiLibcVersion = abiLibcVersion;
this.supportsGoldLinker = supportsGoldLinker;
this.supportsStartEndLib = supportsStartEndLib;
this.supportsInterfaceSharedLibraries = supportsInterfaceSharedLibraries;
this.supportsEmbeddedRuntimes = supportsEmbeddedRuntimes;
Expand Down Expand Up @@ -413,7 +410,6 @@ public static CcToolchainConfigInfo fromToolchain(RuleContext ruleContext, CTool
toolchain.getCompiler(),
toolchain.getAbiVersion(),
toolchain.getAbiLibcVersion(),
toolchain.getSupportsGoldLinker(),
toolchain.getSupportsStartEndLib(),
toolchain.getSupportsInterfaceSharedObjects(),
toolchain.getSupportsEmbeddedRuntimes(),
Expand Down Expand Up @@ -513,12 +509,6 @@ public String getAbiLibcVersion() {
return abiLibcVersion;
}

// TODO(b/65151735): Remove once this field is migrated to features.
@Deprecated
public boolean supportsGoldLinker() {
return supportsGoldLinker;
}

// TODO(b/65151735): Remove once this field is migrated to features.
@Deprecated
public boolean supportsStartEndLib() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -593,13 +593,6 @@ public CompilationMode getCompilationMode() {
return cppConfiguration == null ? null : cppConfiguration.getCompilationMode();
}

/**
* Returns whether the toolchain supports the gold linker.
*/
public boolean supportsGoldLinker() {
return toolchainInfo.supportsGoldLinker();
}

/** Returns whether the toolchain supports dynamic linking. */
public boolean supportsDynamicLinker(FeatureConfiguration featureConfiguration) {
return toolchainInfo.supportsDynamicLinker()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -396,10 +396,6 @@ public boolean legacyWholeArchive() {
return cppOptions.legacyWholeArchive;
}

public boolean getSymbolCounts() {
return cppOptions.symbolCounts;
}

public boolean getInmemoryDotdFiles() {
return cppOptions.inmemoryDotdFiles;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.collect.CollectionUtils;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
import com.google.devtools.build.lib.rules.cpp.Link.LinkingMode;
import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
Expand Down Expand Up @@ -505,20 +504,4 @@ public ResourceSet estimateResourceConsumptionLocal() {
public Iterable<Artifact> getMandatoryInputs() {
return mandatoryInputs;
}

/** Determines whether or not this link should output a symbol counts file. */
public static boolean enableSymbolsCounts(
CppConfiguration cppConfiguration,
boolean supportsGoldLinker,
boolean fake,
LinkTargetType linkType) {
return cppConfiguration.getSymbolCounts()
&& supportsGoldLinker
&& linkType == LinkTargetType.EXECUTABLE
&& !fake;
}

public static PathFragment symbolCountsFileName(PathFragment binaryName) {
return binaryName.replaceName(binaryName.getBaseName() + ".sc");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ public Artifact create(
@Nullable private final CcToolchainProvider toolchain;
private final FdoProvider fdoProvider;
private Artifact interfaceOutput;
private Artifact symbolCounts;
/** Directory where toolchain stores language-runtime libraries (libstdc++, libc++ ...) */
private PathFragment toolchainLibrariesSolibDir;

Expand Down Expand Up @@ -765,8 +764,7 @@ public CppLinkAction build() throws InterruptedException {
constructOutputs(
output,
linkActionOutputs.build(),
interfaceOutputLibrary == null ? null : interfaceOutputLibrary.getArtifact(),
symbolCounts);
interfaceOutputLibrary == null ? null : interfaceOutputLibrary.getArtifact());
}

// Linker inputs without any start/end lib expansions.
Expand Down Expand Up @@ -856,7 +854,6 @@ public CppLinkAction build() throws InterruptedException {
thinltoParamFile != null ? thinltoParamFile.getExecPathString() : null,
thinltoMergedObjectFile != null ? thinltoMergedObjectFile.getExecPathString() : null,
mustKeepDebug,
symbolCounts,
toolchain,
featureConfiguration,
useTestOnlyFlags,
Expand Down Expand Up @@ -898,8 +895,6 @@ public CppLinkAction build() throws InterruptedException {

Preconditions.checkArgument(
linkingMode == Link.LinkingMode.STATIC, "static library link must be static");
Preconditions.checkArgument(
symbolCounts == null, "the symbol counts output must be null for static links");
Preconditions.checkArgument(
!isNativeDeps, "the native deps flag must be false for static links");
Preconditions.checkArgument(
Expand Down Expand Up @@ -1215,11 +1210,6 @@ public CppLinkActionBuilder setInterfaceOutput(Artifact interfaceOutput) {
return this;
}

public CppLinkActionBuilder setSymbolCountsOutput(Artifact symbolCounts) {
this.symbolCounts = symbolCounts;
return this;
}

public CppLinkActionBuilder addLtoCompilationContext(
LtoCompilationContext ltoCompilationContext) {
Preconditions.checkState(this.ltoCompilationContext == null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -596,17 +596,6 @@ public Label getFdoPrefetchHintsLabel() {
)
public Label hostLibcTopLabel;

@Option(
name = "output_symbol_counts",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.OUTPUT_SELECTION,
effectTags = {OptionEffectTag.ACTION_COMMAND_LINES, OptionEffectTag.AFFECTS_OUTPUTS},
help =
"If enabled, for every C++ binary linked with gold, the number of defined symbols "
+ "and the number of used symbols per input file is stored in a .sc file."
)
public boolean symbolCounts;

@Option(
name = "experimental_inmemory_dotd_files",
defaultValue = "false",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ public final class CppToolchainInfo {
private final boolean supportsEmbeddedRuntimes;
private final boolean supportsDynamicLinker;
private final boolean supportsInterfaceSharedLibraries;
private final boolean supportsGoldLinker;
private final boolean toolchainNeedsPic;

/**
Expand Down Expand Up @@ -255,7 +254,6 @@ public static CppToolchainInfo create(
disableLegacyCrosstoolFields
? false
: ccToolchainConfigInfo.supportsInterfaceSharedLibraries(),
disableLegacyCrosstoolFields ? false : ccToolchainConfigInfo.supportsGoldLinker(),
disableLegacyCrosstoolFields ? false : ccToolchainConfigInfo.needsPic());
} catch (LabelSyntaxException e) {
// All of the above label.getRelativeWithRemapping() calls are valid labels, and the
Expand Down Expand Up @@ -303,7 +301,6 @@ public static CppToolchainInfo create(
boolean supportsEmbeddedRuntimes,
boolean supportsDynamicLinker,
boolean supportsInterfaceSharedLibraries,
boolean supportsGoldLinker,
boolean toolchainNeedsPic)
throws EvalException {
this.toolchainIdentifier = toolchainIdentifier;
Expand Down Expand Up @@ -344,7 +341,6 @@ public static CppToolchainInfo create(
this.supportsEmbeddedRuntimes = supportsEmbeddedRuntimes;
this.supportsDynamicLinker = supportsDynamicLinker;
this.supportsInterfaceSharedLibraries = supportsInterfaceSharedLibraries;
this.supportsGoldLinker = supportsGoldLinker;
this.toolchainNeedsPic = toolchainNeedsPic;
}

Expand Down Expand Up @@ -586,11 +582,6 @@ public CcToolchainFeatures getFeatures() {
return toolchainFeatures;
}

/** Returns whether the toolchain supports the gold linker. */
public boolean supportsGoldLinker() {
return supportsGoldLinker;
}

/** Returns whether the toolchain supports the --start-lib/--end-lib options. */
public boolean supportsStartEndLib() {
return supportsStartEndLib;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import com.google.common.base.Predicates;
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.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
import com.google.devtools.build.lib.rules.cpp.CcToolchainVariables.SequenceBuilder;
import com.google.devtools.build.lib.syntax.EvalException;
Expand Down Expand Up @@ -66,8 +65,6 @@ public enum LinkBuildVariables {
LEGACY_LINK_FLAGS("legacy_link_flags"),
/** Linker flags coming from the --linkopt or linkopts attribute. */
USER_LINK_FLAGS("user_link_flags"),
/** Path to which to write symbol counts. */
SYMBOL_COUNTS_OUTPUT("symbol_counts_output"),
/** A build variable giving linkstamp paths. */
LINKSTAMP_PATHS("linkstamp_paths"),
/** Presence of this variable indicates that PIC code should be generated. */
Expand Down Expand Up @@ -101,7 +98,6 @@ public static CcToolchainVariables setupVariables(
String thinltoParamFile,
String thinltoMergedObjectFile,
boolean mustKeepDebug,
Artifact symbolCounts,
CcToolchainProvider ccToolchainProvider,
FeatureConfiguration featureConfiguration,
boolean useTestOnlyFlags,
Expand All @@ -122,12 +118,6 @@ public static CcToolchainVariables setupVariables(
CcToolchainVariables.Builder buildVariables =
new CcToolchainVariables.Builder(ccToolchainProvider.getBuildVariables());

// symbol counting
if (symbolCounts != null) {
buildVariables.addStringVariable(
SYMBOL_COUNTS_OUTPUT.getVariableName(), symbolCounts.getExecPathString());
}

// pic
if (ccToolchainProvider.getForcePic()) {
buildVariables.addStringVariable(FORCE_PIC.getVariableName(), "");
Expand Down
1 change: 1 addition & 0 deletions src/main/protobuf/crosstool_config.proto
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,7 @@ message CToolchain {

// Feature flags.
// TODO(bazel-team): Sink those into 'Feature' instances.
// Legacy field, ignored by Bazel.
optional bool supports_gold_linker = 10 [default = false];
// Legacy field, ignored by Bazel.
optional bool supports_thin_archives = 11 [default = false];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ toolchain {
compiler: "compiler"
host_system_name: "local"
needsPic: true
supports_gold_linker: false
supports_incremental_linker: false
supports_fission: false
supports_interface_shared_objects: false
Expand Down Expand Up @@ -132,7 +131,6 @@ toolchain {
compiler: "compiler"
host_system_name: "local"
needsPic: true
supports_gold_linker: false
supports_incremental_linker: false
supports_fission: false
supports_interface_shared_objects: false
Expand Down Expand Up @@ -600,7 +598,6 @@ toolchain {
linking_mode_flags { mode: DYNAMIC }
host_system_name: "armeabi-v7a"
needsPic: true
supports_gold_linker: false
supports_incremental_linker: false
supports_fission: false
supports_interface_shared_objects: false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -729,19 +729,6 @@ public void testStaticLinkWithDynamicIsError() throws Exception {
assertError("static library link must be static", builder);
}

@Test
public void testStaticLinkWithSymbolsCountOutputIsError() throws Exception {
RuleContext ruleContext = createDummyRuleContext();

CppLinkActionBuilder builder =
createLinkBuilder(ruleContext, LinkTargetType.STATIC_LIBRARY)
.setLinkingMode(LinkingMode.STATIC)
.setLibraryIdentifier("foo")
.setSymbolCountsOutput(scratchArtifact("dummySymbolCounts"));

assertError("the symbol counts output must be null for static links", builder);
}

@Test
public void testStaticLinkWithNativeDepsIsError() throws Exception {
RuleContext ruleContext = createDummyRuleContext();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ private CppConfigurationLoader loaderWithOptionalTool(String optionalTool) throw
+ " tool_path { name: \"strip\" path: \"path-to-strip\" }"
+ " tool_path { name: \"dwp\" path: \"path-to-dwp\" }"
+ optionalTool
+ " supports_gold_linker: true"
+ " supports_normalizing_ar: true"
+ " supports_incremental_linker: true"
+ " supports_fission: true"
Expand Down Expand Up @@ -175,7 +174,6 @@ public void testSimpleCompleteConfiguration() throws Exception {
assertThat(ccProvider.getAbi()).isEqualTo("abi-version");
assertThat(ccProvider.getAbiGlibcVersion()).isEqualTo("abi-libc-version");

assertThat(ccProvider.supportsGoldLinker()).isTrue();
assertThat(ccProvider.supportsStartEndLib(FeatureConfiguration.EMPTY)).isFalse();
assertThat(ccProvider.supportsInterfaceSharedLibraries(FeatureConfiguration.EMPTY)).isFalse();
assertThat(ccProvider.supportsEmbeddedRuntimes()).isFalse();
Expand Down Expand Up @@ -251,7 +249,6 @@ public void testComprehensiveCompleteConfiguration() throws Exception {
+ " tool_path { name: \"objdump\" path: \"path/to/objdump-A\" }\n"
+ " tool_path { name: \"strip\" path: \"path/to/strip-A\" }\n"
+ " tool_path { name: \"dwp\" path: \"path/to/dwp\" }\n"
+ " supports_gold_linker: true\n"
+ " supports_start_end_lib: true\n"
+ " supports_normalizing_ar: true\n"
+ " supports_embedded_runtimes: true\n"
Expand Down Expand Up @@ -337,7 +334,6 @@ public void testComprehensiveCompleteConfiguration() throws Exception {
+ " tool_path { name: \"objdump\" path: \"path/to/objdump-B\" }\n"
+ " tool_path { name: \"strip\" path: \"path/to/strip-B\" }\n"
+ " tool_path { name: \"dwp\" path: \"path/to/dwp\" }\n"
+ " supports_gold_linker: true\n"
+ " supports_start_end_lib: true\n"
+ " supports_normalizing_ar: true\n"
+ " supports_embedded_runtimes: true\n"
Expand Down Expand Up @@ -458,7 +454,6 @@ public void testComprehensiveCompleteConfiguration() throws Exception {
.isEqualTo(getToolPath("path/to/objdump-A"));
assertThat(ccProviderA.getToolPathFragment(Tool.STRIP))
.isEqualTo(getToolPath("path/to/strip-A"));
assertThat(ccProviderA.supportsGoldLinker()).isTrue();
assertThat(ccProviderA.supportsStartEndLib(FeatureConfiguration.EMPTY)).isTrue();
assertThat(ccProviderA.supportsEmbeddedRuntimes()).isTrue();
assertThat(ccProviderA.toolchainNeedsPic()).isTrue();
Expand Down Expand Up @@ -559,7 +554,6 @@ public void testComprehensiveCompleteConfiguration() throws Exception {
assertThat(ccProviderC.getAbi()).isEqualTo("abi-version-C");
assertThat(ccProviderC.getAbiGlibcVersion()).isEqualTo("abi-libc-version-C");
// Don't bother with testing the list of tools again.
assertThat(ccProviderC.supportsGoldLinker()).isFalse();
assertThat(ccProviderC.supportsStartEndLib(FeatureConfiguration.EMPTY)).isFalse();
assertThat(ccProviderC.supportsInterfaceSharedLibraries(FeatureConfiguration.EMPTY)).isFalse();
assertThat(ccProviderC.supportsEmbeddedRuntimes()).isFalse();
Expand Down
Loading

0 comments on commit f55ae7a

Please sign in to comment.