Skip to content

Commit

Permalink
WIP
Browse files Browse the repository at this point in the history
  • Loading branch information
@fmeum
fmeum committed Jul 18, 2024
1 parent 489142b commit e63f761
Show file tree
Hide file tree
Showing 9 changed files with 156 additions and 44 deletions.
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,7 @@ void addAdditionalInputs(NestedSetBuilder<Artifact> builder) {
}

/** @return modules maps from direct dependencies. */
public Iterable<Artifact> getDirectModuleMaps() {
public ImmutableList<Artifact> getDirectModuleMaps() {
return directModuleMaps;
}

Expand Down
5 changes: 4 additions & 1 deletion src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.google.devtools.build.lib.analysis.LicensesProviderImpl;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
import com.google.devtools.build.lib.analysis.actions.PathMappers;
import com.google.devtools.build.lib.analysis.actions.SymlinkAction;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
import com.google.devtools.build.lib.analysis.starlark.Args;
Expand Down Expand Up @@ -440,7 +441,9 @@ public void createModuleMapAction(
compiledModule,
moduleMapHomeIsCwd,
generateSubmodules,
withoutExternDependencies));
withoutExternDependencies,
PathMappers.getOutputPathsMode(ruleContext.getConfiguration()),
ruleContext.getConfiguration()::modifiedExecutionInfo));
}

@SerializationConstant @VisibleForSerialization
Expand Down
61 changes: 61 additions & 0 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1185,6 +1185,54 @@ public int hashCode() {
}
}

@Immutable
private static final class ArtifactSetSequence extends VariableValueAdapter {
private final NestedSet<Artifact> values;

private ArtifactSetSequence(NestedSet<Artifact> values) {
Preconditions.checkNotNull(values);
this.values = values;
}

@Override
public ImmutableList<VariableValue> getSequenceValue(
String variableName, PathMapper pathMapper) {
ImmutableList<Artifact> valuesList = values.toList();
ImmutableList.Builder<VariableValue> sequences =
ImmutableList.builderWithExpectedSize(valuesList.size());
for (Artifact value : valuesList) {
sequences.add(new StringValue(pathMapper.getMappedExecPathString(value)));
}
return sequences.build();
}

@Override
public String getVariableTypeName() {
return Sequence.SEQUENCE_VARIABLE_TYPE_NAME;
}

@Override
public boolean isTruthy() {
return !values.isEmpty();
}

@Override
public boolean equals(Object other) {
if (this == other) {
return true;
}
if (!(other instanceof ArtifactSetSequence otherArtifacts)) {
return false;
}
return values.shallowEquals(otherArtifacts.values);
}

@Override
public int hashCode() {
return values.shallowHashCode();
}
}

/**
* Single structure value. Be careful not to create sequences of single structures, as the memory
* overhead is prohibitively big.
Expand Down Expand Up @@ -1489,6 +1537,19 @@ public Builder addPathFragmentSequenceVariable(String name, NestedSet<PathFragme
return this;
}

/**
* Add a sequence variable that expands {@code name} to {@link Artifact} {@code values}.
*
* <p>Accepts values as NestedSet. Nested set is stored directly, not cloned, not flattened.
*/
@CanIgnoreReturnValue
public Builder addArtifactSequenceVariable(String name, NestedSet<Artifact> values) {
checkVariableNotPresentAlready(name);
Preconditions.checkNotNull(values, "Cannot set null as a value for variable '%s'", name);
variablesMap.put(name, new ArtifactSetSequence(values));
return this;
}

/**
* Add a variable built using {@code VariableValueBuilder} api that expands {@code name} to the
* value returned by the {@code builder}.
Expand Down
14 changes: 7 additions & 7 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariables.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ public static CcToolchainVariables setupVariablesOrReportRuleError(
Artifact diagnosticsFile,
ImmutableList<VariablesExtension> variablesExtensions,
ImmutableMap<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
ImmutableList<Artifact> directModuleMaps,
NestedSet<PathFragment> includeDirs,
NestedSet<PathFragment> quoteIncludeDirs,
NestedSet<PathFragment> systemIncludeDirs,
Expand Down Expand Up @@ -225,7 +225,7 @@ public static CcToolchainVariables setupVariablesOrThrowEvalException(
Artifact diagnosticsFile,
ImmutableList<VariablesExtension> variablesExtensions,
ImmutableMap<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
ImmutableList<Artifact> directModuleMaps,
NestedSet<String> includeDirs,
NestedSet<String> quoteIncludeDirs,
NestedSet<String> systemIncludeDirs,
Expand Down Expand Up @@ -293,7 +293,7 @@ private static CcToolchainVariables setupVariables(
Artifact diagnosticsFile,
ImmutableList<VariablesExtension> variablesExtensions,
ImmutableMap<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
ImmutableList<Artifact> directModuleMaps,
NestedSet<PathFragment> includeDirs,
NestedSet<PathFragment> quoteIncludeDirs,
NestedSet<PathFragment> systemIncludeDirs,
Expand Down Expand Up @@ -467,7 +467,7 @@ public static void setupCommonVariables(
boolean isUsingMemProf,
List<VariablesExtension> variablesExtensions,
Map<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
ImmutableList<Artifact> directModuleMaps,
ImmutableList<PathFragment> includeDirs,
ImmutableList<PathFragment> quoteIncludeDirs,
ImmutableList<PathFragment> systemIncludeDirs,
Expand Down Expand Up @@ -503,7 +503,7 @@ private static void setupCommonVariablesInternal(
boolean isUsingMemProf,
List<VariablesExtension> variablesExtensions,
Map<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
ImmutableList<Artifact> directModuleMaps,
NestedSet<PathFragment> includeDirs,
NestedSet<PathFragment> quoteIncludeDirs,
NestedSet<PathFragment> systemIncludeDirs,
Expand All @@ -522,9 +522,9 @@ private static void setupCommonVariablesInternal(
buildVariables.addStringVariable(MODULE_NAME.getVariableName(), cppModuleMap.getName());
buildVariables.addArtifactVariable(
MODULE_MAP_FILE.getVariableName(), cppModuleMap.getArtifact());
buildVariables.addStringSequenceVariable(
buildVariables.addArtifactSequenceVariable(
DEPENDENT_MODULE_MAP_FILES.getVariableName(),
Iterables.transform(directModuleMaps, Artifact::getExecPathString));
NestedSetBuilder.wrap(Order.STABLE_ORDER, directModuleMaps));
}
if (featureConfiguration.isEnabled(CppRuleClasses.USE_HEADER_MODULES)) {
// Module inputs will be set later when the action is executed.
Expand Down
78 changes: 65 additions & 13 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,19 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Optional;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionKeyContext;
import com.google.devtools.build.lib.actions.ActionOwner;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactExpander;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.PathMapper;
import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction;
import com.google.devtools.build.lib.analysis.actions.DeterministicWriter;
import com.google.devtools.build.lib.analysis.actions.PathMappers;
import com.google.devtools.build.lib.analysis.config.CoreOptions.OutputPathsMode;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.util.Fingerprint;
Expand All @@ -35,6 +40,7 @@
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.function.BiFunction;
import javax.annotation.Nullable;

/**
Expand All @@ -45,6 +51,7 @@
public final class CppModuleMapAction extends AbstractFileWriteAction {

private static final String GUID = "4f407081-1951-40c1-befc-d6b4daff5de3";
private static final String MNEMONIC = "CppModuleMap";

// C++ module map of the current target
private final CppModuleMap cppModuleMap;
Expand All @@ -65,6 +72,11 @@ public final class CppModuleMapAction extends AbstractFileWriteAction {
private final boolean compiledModule;
private final boolean generateSubmodules;
private final boolean externDependencies;
// Save memory by storing only the boolean value of whether path stripping is effectively enabled
// for this action, which it is if and only if the execution info and the output path mode have
// the required values. This avoids storing both, which would require two reference fields.
// See getExecutionInfo() and getOutputPathsMode().
private final boolean pathStrippingRequestedAndEnabled;

public CppModuleMapAction(
ActionOwner owner,
Expand All @@ -77,7 +89,10 @@ public CppModuleMapAction(
boolean compiledModule,
boolean moduleMapHomeIsCwd,
boolean generateSubmodules,
boolean externDependencies) {
boolean externDependencies,
OutputPathsMode outputPathsMode,
BiFunction<ImmutableMap<String, String>, String, ImmutableMap<String, String>>
modifyExecutionInfo) {
super(
owner,
NestedSetBuilder.<Artifact>stableOrder()
Expand All @@ -95,6 +110,11 @@ public CppModuleMapAction(
this.compiledModule = compiledModule;
this.generateSubmodules = generateSubmodules;
this.externDependencies = externDependencies;
this.pathStrippingRequestedAndEnabled =
modifyExecutionInfo
.apply(ImmutableMap.of(), MNEMONIC)
.containsKey(ExecutionRequirements.SUPPORTS_PATH_MAPPING)
&& outputPathsMode == OutputPathsMode.STRIP;
}

@Override
Expand All @@ -110,9 +130,15 @@ public boolean makeExecutable() {
@Override
public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) {
final ArtifactExpander artifactExpander = ctx.getArtifactExpander();
// TODO: It is possible that compile actions consuming the module map have path mapping disabled
// due to inputs conflicting across configurations. Since these inputs aren't inputs of the
// module map action, the generated map still contains mapped paths, which then results in
// compilation failures. This should be very rare as #include doesn't allow to disambiguate
// between headers from different configurations but with identical root-relative paths.
final PathMapper pathMapper = PathMappers.create(this, getOutputPathsMode());
return out -> {
OutputStreamWriter content = new OutputStreamWriter(out, StandardCharsets.ISO_8859_1);
PathFragment fragment = cppModuleMap.getArtifact().getExecPath();
PathFragment fragment = pathMapper.map(cppModuleMap.getArtifact().getExecPath());
int segmentsToExecPath = fragment.segmentCount() - 1;
Optional<Artifact> umbrellaHeader = cppModuleMap.getUmbrellaHeader();
String leadingPeriods = moduleMapHomeIsCwd ? "" : "../".repeat(segmentsToExecPath);
Expand All @@ -134,7 +160,8 @@ public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) {
leadingPeriods,
/* canCompile= */ false,
deduper,
/*isUmbrellaHeader*/ true);
/*isUmbrellaHeader*/ true,
pathMapper);
} else {
for (Artifact artifact : expandedHeaders(artifactExpander, publicHeaders)) {
appendHeader(
Expand All @@ -144,7 +171,8 @@ public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) {
leadingPeriods,
/* canCompile= */ true,
deduper,
/*isUmbrellaHeader*/ false);
/*isUmbrellaHeader*/ false,
pathMapper);
}
for (Artifact artifact : expandedHeaders(artifactExpander, privateHeaders)) {
appendHeader(
Expand All @@ -154,7 +182,8 @@ public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) {
leadingPeriods,
/* canCompile= */ true,
deduper,
/*isUmbrellaHeader*/ false);
/*isUmbrellaHeader*/ false,
pathMapper);
}
for (Artifact artifact : separateModuleHdrs) {
appendHeader(
Expand All @@ -164,7 +193,8 @@ public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) {
leadingPeriods,
/* canCompile= */ false,
deduper,
/*isUmbrellaHeader*/ false);
/*isUmbrellaHeader*/ false,
pathMapper);
}
for (PathFragment additionalExportedHeader : additionalExportedHeaders) {
appendHeader(
Expand All @@ -174,7 +204,8 @@ public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) {
leadingPeriods,
/*canCompile*/ false,
deduper,
/*isUmbrellaHeader*/ false);
/*isUmbrellaHeader*/ false,
pathMapper);
}
}
for (CppModuleMap dep : dependencies) {
Expand All @@ -196,7 +227,8 @@ public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) {
leadingPeriods,
/* canCompile= */ true,
deduper,
/*isUmbrellaHeader*/ false);
/*isUmbrellaHeader*/ false,
pathMapper);
}
for (CppModuleMap dep : dependencies) {
content.append(" use \"").append(dep.getName()).append("\"\n");
Expand All @@ -211,7 +243,7 @@ public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) {
.append(dep.getName())
.append("\" \"")
.append(leadingPeriods)
.append(dep.getArtifact().getExecPathString())
.append(pathMapper.getMappedExecPathString(dep.getArtifact()))
.append("\"");
}
}
Expand All @@ -233,9 +265,17 @@ private static Iterable<Artifact> expandedHeaders(ArtifactExpander artifactExpan
return ImmutableList.copyOf(expandedHeaders);
}

private void appendHeader(Appendable content, String visibilitySpecifier,
PathFragment path, String leadingPeriods, boolean canCompile, HashSet<PathFragment> deduper,
boolean isUmbrellaHeader) throws IOException {
private void appendHeader(
Appendable content,
String visibilitySpecifier,
PathFragment unmappedPath,
String leadingPeriods,
boolean canCompile,
HashSet<PathFragment> deduper,
boolean isUmbrellaHeader,
PathMapper pathMapper)
throws IOException {
PathFragment path = pathMapper.map(unmappedPath);
if (deduper.contains(path)) {
return;
}
Expand Down Expand Up @@ -268,7 +308,7 @@ private boolean shouldCompileHeader(PathFragment path) {

@Override
public String getMnemonic() {
return "CppModuleMap";
return MNEMONIC;
}

@Override
Expand Down Expand Up @@ -308,6 +348,18 @@ protected void computeKey(
fp.addBoolean(compiledModule);
fp.addBoolean(generateSubmodules);
fp.addBoolean(externDependencies);
PathMappers.addToFingerprint(getMnemonic(), getExecutionInfo(), getOutputPathsMode(), fp);
}

@Override
public ImmutableMap<String, String> getExecutionInfo() {
return pathStrippingRequestedAndEnabled
? ImmutableMap.of(ExecutionRequirements.SUPPORTS_PATH_MAPPING, "")
: ImmutableMap.of();
}

private OutputPathsMode getOutputPathsMode() {
return pathStrippingRequestedAndEnabled ? OutputPathsMode.STRIP : OutputPathsMode.OFF;
}

@VisibleForTesting
Expand Down
1 change: 1 addition & 0 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,7 @@ java_test(
deps = [
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/rules/cpp",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/test/java/com/google/devtools/build/lib/actions/util",
Expand Down
Loading

0 comments on commit e63f761

Please sign in to comment.