Skip to content

Commit

Permalink
Automated rollback of commit 770c50b.
Browse files Browse the repository at this point in the history
*** Reason for rollback ***

This CL raises "Exec format error" in some targets.

*** Original change description ***

Move grep-includes to the cc_toolchain

Motivation for this change was an error with the wrong exec platform for grep-includes when Automatic Exec Groups are enabled.

To solve this, grep_includes is moved inside the cc_toolchain, which automatically chooses macos as an exec platform when needed.

PiperOrigin-RevId: 526631776
Change-Id: I48d2d06d41f3d44fd24ef0bc548da90fc79f9361
  • Loading branch information
kotlaja authored and copybara-github committed Apr 24, 2023
1 parent 25ae149 commit 13ea972
Show file tree
Hide file tree
Showing 17 changed files with 67 additions and 64 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -867,6 +867,9 @@ Collection<Inclusion> extractInclusions(
boolean isOutputFile)
throws IOException, ExecException, InterruptedException {
Collection<Inclusion> inclusions;

// TODO(ulfjack): grepIncludes may be null if the corresponding attribute on the rule is missing
// (see CppHelper.getGrepIncludes) or misspelled. It would be better to disallow this case.
if (remoteIncludeScanner != null
&& grepIncludes != null
&& remoteIncludeScanner.shouldParseRemotely(file)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -300,12 +300,14 @@ public CcCompilationContext getCcCompilationContext() {
private final ActionRegistry actionRegistry;
private final ActionConstructionContext actionConstructionContext;
private final Label label;
@Nullable private final Artifact grepIncludes;

/** Creates a CcCompilationHelper that outputs artifacts in a given configuration. */
public CcCompilationHelper(
ActionRegistry actionRegistry,
ActionConstructionContext actionConstructionContext,
Label label,
@Nullable Artifact grepIncludes,
CppSemantics semantics,
FeatureConfiguration featureConfiguration,
SourceCategory sourceCategory,
Expand All @@ -331,6 +333,7 @@ public CcCompilationHelper(
this.ruleErrorConsumer = actionConstructionContext.getRuleErrorConsumer();
this.actionRegistry = Preconditions.checkNotNull(actionRegistry);
this.label = Preconditions.checkNotNull(label);
this.grepIncludes = grepIncludes;
this.executionInfo = Preconditions.checkNotNull(executionInfo);
this.shouldProcessHeaders = shouldProcessHeaders;
}
Expand All @@ -340,6 +343,7 @@ public CcCompilationHelper(
ActionRegistry actionRegistry,
ActionConstructionContext actionConstructionContext,
Label label,
@Nullable Artifact grepIncludes,
CppSemantics semantics,
FeatureConfiguration featureConfiguration,
CcToolchainProvider ccToolchain,
Expand All @@ -350,6 +354,7 @@ public CcCompilationHelper(
actionRegistry,
actionConstructionContext,
label,
grepIncludes,
semantics,
featureConfiguration,
SourceCategory.CC,
Expand Down Expand Up @@ -1669,7 +1674,7 @@ private static String toPathString(Artifact a) {
*/
private CppCompileActionBuilder initializeCompileAction(Artifact sourceArtifact) {
return new CppCompileActionBuilder(
actionConstructionContext, ccToolchain, configuration, semantics)
actionConstructionContext, grepIncludes, ccToolchain, configuration, semantics)
.setSourceFile(sourceArtifact)
.setCcCompilationContext(ccCompilationContext)
.setCoptsFilter(coptsFilter)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ public CcLinkingOutputs getCcLinkingOutputs() {
private final SymbolGenerator<?> symbolGenerator;
private final ImmutableMap<String, String> executionInfo;

private Artifact grepIncludes;
private boolean isStampingEnabled;
private boolean isTestOrTestOnlyTarget;

Expand Down Expand Up @@ -507,6 +508,15 @@ public CcLinkingHelper setTestOrTestOnlyTarget(boolean isTestOrTestOnlyTarget) {
return this;
}

/**
* Used to test the grep-includes tool. This is needing during linking because of linkstamping.
*/
@CanIgnoreReturnValue
public CcLinkingHelper setGrepIncludes(Artifact grepIncludes) {
this.grepIncludes = grepIncludes;
return this;
}

/** Whether linkstamping is enabled. */
@CanIgnoreReturnValue
public CcLinkingHelper setIsStampingEnabled(boolean isStampingEnabled) {
Expand Down Expand Up @@ -866,6 +876,7 @@ private CppLinkActionBuilder newLinkActionBuilder(
fdoContext,
featureConfiguration,
semantics)
.setGrepIncludes(grepIncludes)
.setMnemonic(mnemonic)
.setIsStampingEnabled(isStampingEnabled)
.setTestOrTestOnlyTarget(isTestOrTestOnlyTarget)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1911,6 +1911,7 @@ public Tuple createLinkingContextFromCompilationOutputs(
TargetUtils.getExecutionInfo(
actions.getRuleContext().getRule(),
actions.getRuleContext().isAllowTagsPropagation()))
.setGrepIncludes(convertFromNoneable(grepIncludes, /* defaultValue= */ null))
.addNonCodeLinkerInputs(
Sequence.cast(additionalInputs, Artifact.class, "additional_inputs"))
.setShouldCreateStaticLibraries(!disallowStaticLibraries)
Expand Down Expand Up @@ -2422,6 +2423,7 @@ public Tuple compile(
StarlarkThread thread)
throws EvalException, InterruptedException {
isCalledFromStarlarkCcCommon(thread);
Artifact grepIncludes = convertFromNoneable(grepIncludesObject, /* defaultValue= */ null);
getSemantics()
.validateStarlarkCompileApiCall(
starlarkActionFactoryApi,
Expand Down Expand Up @@ -2507,6 +2509,7 @@ public Tuple compile(
actions.asActionRegistry(actions),
actions.getActionConstructionContext(),
label,
grepIncludes,
getSemantics(language),
featureConfiguration.getFeatureConfiguration(),
sourceCategory,
Expand Down Expand Up @@ -2751,6 +2754,7 @@ public CcLinkingOutputs link(
TargetUtils.getExecutionInfo(
actions.getRuleContext().getRule(),
actions.getRuleContext().isAllowTagsPropagation()))
.setGrepIncludes(convertFromNoneable(grepIncludes, /* defaultValue= */ null))
.setLinkingMode(linkDepsStatically ? LinkingMode.STATIC : LinkingMode.DYNAMIC)
.setIsStampingEnabled(isStampingEnabled)
.addTransitiveAdditionalLinkerInputs(additionalInputsSet)
Expand Down Expand Up @@ -2909,6 +2913,7 @@ public void registerLinkstampCompileAction(
CppLinkstampCompileHelper.createLinkstampCompileAction(
ruleContext,
ruleContext,
grepIncludes,
ruleContext.getConfiguration(),
sourceFile,
outputFile,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,6 @@ public CcToolchainProvider getCcToolchainProvider(
targetBuiltinIncludeFiles, Artifact.class, "target_builtin_include_files")
.getImmutableList(),
/* linkDynamicLibraryTool= */ attributes.getLinkDynamicLibraryTool(),
/* grepIncludes= */ attributes.getGrepIncludes(),
/* builtInIncludeDirectories= */ builtInIncludeDirectories,
/* sysroot= */ sysroot,
/* targetSysroot= */ targetSysroot,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ public class CcToolchainAttributesProvider extends NativeInfo implements HasCcTo
private final String cpu;
private final Artifact ifsoBuilder;
private final Artifact linkDynamicLibraryTool;
@Nullable private final Artifact grepIncludes;
private final TransitiveInfoCollection fdoOptimize;
private final ImmutableList<Artifact> fdoOptimizeArtifacts;
private final FdoPrefetchHintsProvider fdoPrefetch;
Expand Down Expand Up @@ -119,11 +118,6 @@ public CcToolchainAttributesProvider(
+ "https://github.com/bazelbuild/bazel/issues/7075 for details.");
}

// grep_includes is not supported by Bazel.
String toolsRepository = ruleContext.getRuleClassProvider().getToolsRepository().getName();
this.grepIncludes =
toolsRepository.isEmpty() ? ruleContext.getPrerequisiteArtifact("$grep_includes") : null;

this.cpu = ruleContext.attributes().get("cpu", Type.STRING);
this.compiler = ruleContext.attributes().get("compiler", Type.STRING);
this.supportsParamFiles = ruleContext.attributes().get("supports_param_files", BOOLEAN);
Expand Down Expand Up @@ -350,11 +344,6 @@ public Artifact getLinkDynamicLibraryTool() {
return linkDynamicLibraryTool;
}

@Nullable
public Artifact getGrepIncludes() {
return grepIncludes;
}

@StarlarkMethod(
name = "module_map",
documented = false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ public final class CcToolchainProvider extends NativeInfo
private final ImmutableList<Artifact> builtinIncludeFiles;
private final ImmutableList<Artifact> targetBuiltinIncludeFiles;
@Nullable private final Artifact linkDynamicLibraryTool;
@Nullable private final Artifact grepIncludes;
private final ImmutableList<PathFragment> builtInIncludeDirectories;
@Nullable private final PathFragment sysroot;
private final PathFragment targetSysroot;
Expand Down Expand Up @@ -164,7 +163,6 @@ public CcToolchainProvider(
ImmutableList<Artifact> builtinIncludeFiles,
ImmutableList<Artifact> targetBuiltinIncludeFiles,
Artifact linkDynamicLibraryTool,
@Nullable Artifact grepIncludes,
ImmutableList<PathFragment> builtInIncludeDirectories,
@Nullable PathFragment sysroot,
@Nullable PathFragment targetSysroot,
Expand Down Expand Up @@ -262,7 +260,6 @@ public CcToolchainProvider(
this.stripExecutable = stripExecutable;
this.ldExecutable = ldExecutable;
this.gcovExecutable = gcovExecutable;
this.grepIncludes = grepIncludes;
}

@Override
Expand Down Expand Up @@ -794,12 +791,6 @@ public Artifact getLinkDynamicLibraryTool() {
return linkDynamicLibraryTool;
}

/** Returns the grep-includes tool which is needing during linking because of linkstamping. */
@Nullable
public Artifact getGrepIncludes() {
return grepIncludes;
}

/** Returns the tool that builds interface libraries from dynamic libraries. */
public Artifact getInterfaceSoBuilder() {
return interfaceSoBuilder;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,6 @@ public static CcToolchainProvider getCcToolchainProvider(
getBuiltinIncludes(attributes.getLibc()),
getBuiltinIncludes(attributes.getTargetLibc()),
attributes.getLinkDynamicLibraryTool(),
attributes.getGrepIncludes(),
builtInIncludeDirectories,
sysroot,
targetSysroot,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -304,11 +304,6 @@ instead of having no transition (i.e. target platform by default).
.cfg(ExecutionTransitionFactory.createFactory())
.singleArtifact()
.value(env.getToolsLabel("//tools/cpp:link_dynamic_library")))
.add(
attr("$grep_includes", LABEL)
.exec()
.cfg(ExecutionTransitionFactory.createFactory())
.value(env.getToolsLabel("//tools/cpp:grep-includes")))
.add(
attr(CcToolchain.CC_TOOLCHAIN_TYPE_ATTRIBUTE_NAME, NODEP_LABEL)
.value(CppRuleClasses.ccToolchainTypeAttribute(env)))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ public class CppCompileActionBuilder {
private Map<String, String> executionInfo = new LinkedHashMap<>();
private CppSemantics cppSemantics;
private final CcToolchainProvider ccToolchain;
@Nullable private final Artifact grepIncludes;
private ActionEnvironment env;
private final boolean codeCoverageEnabled;
@Nullable private String actionName;
Expand All @@ -87,6 +88,7 @@ public class CppCompileActionBuilder {
/** Creates a builder from a rule and configuration. */
public CppCompileActionBuilder(
ActionConstructionContext actionConstructionContext,
@Nullable Artifact grepIncludes,
CcToolchainProvider ccToolchain,
BuildConfigurationValue configuration,
CppSemantics cppSemantics) {
Expand All @@ -107,6 +109,7 @@ public CppCompileActionBuilder(
this.codeCoverageEnabled = configuration.isCodeCoverageEnabled();
this.ccToolchain = ccToolchain;
this.builtinIncludeDirectories = ccToolchain.getBuiltInIncludeDirectories();
this.grepIncludes = grepIncludes;
this.cppSemantics = cppSemantics;
}

Expand Down Expand Up @@ -143,6 +146,7 @@ public CppCompileActionBuilder(CppCompileActionBuilder other) {
this.cppSemantics = other.cppSemantics;
this.ccToolchain = other.ccToolchain;
this.actionName = other.actionName;
this.grepIncludes = other.grepIncludes;
this.builtinIncludeDirectories = other.builtinIncludeDirectories;
this.additionalOutputs = ImmutableList.copyOf(other.additionalOutputs);
}
Expand Down Expand Up @@ -328,7 +332,7 @@ public CppCompileAction buildAndVerify() throws UnconfiguredActionConfigExceptio
actionName,
cppSemantics,
builtinIncludeDirectories,
ccToolchain.getGrepIncludes(),
grepIncludes,
additionalOutputs);
return action;
}
Expand Down Expand Up @@ -358,8 +362,8 @@ NestedSet<Artifact> buildMandatoryInputs() {
}
ccCompilationContext.addAdditionalInputs(realMandatoryInputsBuilder);
realMandatoryInputsBuilder.add(Preconditions.checkNotNull(sourceFile));
if (ccToolchain.getGrepIncludes() != null) {
realMandatoryInputsBuilder.add(ccToolchain.getGrepIncludes());
if (grepIncludes != null) {
realMandatoryInputsBuilder.add(grepIncludes);
}
if (!shouldScanIncludes && dotdFile == null && !shouldParseShowIncludes()) {
realMandatoryInputsBuilder.addTransitive(ccCompilationContext.getDeclaredIncludeSrcs());
Expand Down Expand Up @@ -663,4 +667,9 @@ public boolean shouldCompileHeaders() {
Preconditions.checkNotNull(featureConfiguration);
return ccToolchain.shouldProcessHeaders(featureConfiguration, cppConfiguration);
}

@Nullable
public Artifact getGrepIncludes() {
return grepIncludes;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -660,4 +660,11 @@ static boolean useToolchainResolution(RuleContext ruleContext) {

return cppOptions.enableCcToolchainResolution;
}

@Nullable
public static Artifact getGrepIncludes(RuleContext ruleContext) {
return ruleContext.attributes().has("$grep_includes")
? ruleContext.getPrerequisiteArtifact("$grep_includes")
: null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ public Artifact create(
private final RuleErrorConsumer ruleErrorConsumer;
private final RepositoryName repositoryName;

private Artifact grepIncludes;
// TODO(plf): This is not exactly the same as `useTestOnlyFlags` but perhaps we can remove one
// of them.
private boolean isTestOrTestOnlyTarget;
Expand Down Expand Up @@ -987,6 +988,7 @@ public CppLinkAction build() throws InterruptedException, RuleErrorException {
CppLinkstampCompileHelper.createLinkstampCompileAction(
ruleErrorConsumer,
actionConstructionContext,
grepIncludes,
configuration,
source,
linkstampEntry.getValue(),
Expand Down Expand Up @@ -1490,6 +1492,15 @@ public CppLinkActionBuilder setTestOrTestOnlyTarget(boolean isTestOrTestOnlyTarg
return this;
}

/**
* Used to test the grep-includes tool. This is needing during linking because of linkstamping.
*/
@CanIgnoreReturnValue
public CppLinkActionBuilder setGrepIncludes(Artifact grepIncludes) {
this.grepIncludes = grepIncludes;
return this;
}

/** Whether linkstamping is enabled. */
@CanIgnoreReturnValue
public CppLinkActionBuilder setIsStampingEnabled(boolean isStampingEnabled) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.regex.Pattern;
import javax.annotation.Nullable;

/** Handles creation of CppCompileAction used to compile linkstamp sources. */
public class CppLinkstampCompileHelper {
Expand All @@ -41,6 +42,7 @@ public class CppLinkstampCompileHelper {
public static CppCompileAction createLinkstampCompileAction(
RuleErrorConsumer ruleErrorConsumer,
ActionConstructionContext actionConstructionContext,
@Nullable Artifact grepIncludes,
BuildConfigurationValue configuration,
Artifact sourceFile,
Artifact outputFile,
Expand All @@ -60,7 +62,11 @@ public static CppCompileAction createLinkstampCompileAction(
CppSemantics semantics) {
CppCompileActionBuilder builder =
new CppCompileActionBuilder(
actionConstructionContext, ccToolchainProvider, configuration, semantics)
actionConstructionContext,
grepIncludes,
ccToolchainProvider,
configuration,
semantics)
.addMandatoryInputs(compilationInputs)
.setVariables(
getVariables(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ public static NativeDepsRunfiles createNativeDepsAction(
ruleContext.getSymbolGenerator(),
TargetUtils.getExecutionInfo(
ruleContext.getRule(), ruleContext.isAllowTagsPropagation()))
.setGrepIncludes(CppHelper.getGrepIncludes(ruleContext))
.setIsStampingEnabled(AnalysisUtils.isStampingEnabled(ruleContext))
.setTestOrTestOnlyTarget(ruleContext.isTestTarget() || ruleContext.isTestOnlyTarget())
.setLinkerOutputArtifact(sharedLibrary)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,7 @@ public CompilationSupport registerLinkActions(
ruleContext.getSymbolGenerator(),
TargetUtils.getExecutionInfo(
ruleContext.getRule(), ruleContext.isAllowTagsPropagation()))
.setGrepIncludes(CppHelper.getGrepIncludes(ruleContext))
.setIsStampingEnabled(isStampingEnabled)
.setTestOrTestOnlyTarget(ruleContext.isTestOnlyTarget() || ruleContext.isTestTarget())
.addNonCodeLinkerInputs(asNeededLibraryList)
Expand Down
Loading

0 comments on commit 13ea972

Please sign in to comment.