Skip to content

Commit

Permalink
[7.2.0] Add NestedSet<PathFragment>- and Artifact-typed compile b…
Browse files Browse the repository at this point in the history 
…uild var… (#22557)

…iables

This change prepares for C++ path mapping, which also needs to map
include directories of generated headers. It may, as a side effect,
reduce memory usage slightly by possibly reusing nested set instances
retained elsewhere.

Work towards #6526

Closes #22463.

PiperOrigin-RevId: 636886343
Change-Id: Ic93d6439a51f37f44bb2ac67d0813c48c0c4a8bd

Fixes #22533

Co-authored-by: Googler <[email protected]>
  • Loading branch information
@fmeum @comius
fmeum and comius authored May 28, 2024
1 parent c6cabd8 commit 84f4b55
Show file tree
Hide file tree
Showing 4 changed files with 291 additions and 130 deletions.
30 changes: 7 additions & 23 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1271,14 +1271,6 @@ private CcToolchainVariables setupCompileBuildVariables(
ImmutableMap<String, String> additionalBuildVariables)
throws RuleErrorException, InterruptedException {
Artifact sourceFile = builder.getSourceFile();
String dotdFileExecPath = null;
if (builder.getDotdFile() != null) {
dotdFileExecPath = builder.getDotdFile().getExecPathString();
}
String diagnosticsFileExecPath = null;
if (builder.getDiagnosticsFile() != null) {
diagnosticsFileExecPath = builder.getDiagnosticsFile().getExecPathString();
}
if (needsFdoBuildVariables && fdoContext.hasArtifacts(cppConfiguration)) {
// This modifies the passed-in builder, which is a surprising side-effect, and makes it unsafe
// to call this method multiple times for the same builder.
Expand Down Expand Up @@ -1348,30 +1340,22 @@ private CcToolchainVariables setupCompileBuildVariables(

CompileBuildVariables.setupSpecificVariables(
buildVariables,
toPathString(sourceFile),
toPathString(builder.getOutputFile()),
sourceFile,
builder.getOutputFile(),
enableCoverage,
toPathString(gcnoFile),
toPathString(dwoFile),
gcnoFile,
dwoFile,
isUsingFission,
toPathString(ltoIndexingFile),
/* thinLtoIndex= */ null,
/* thinLtoInputBitcodeFile= */ null,
/* thinLtoOutputObjectFile= */ null,
ltoIndexingFile,
getCopts(builder.getSourceFile(), sourceLabel),
dotdFileExecPath,
diagnosticsFileExecPath,
builder.getDotdFile(),
builder.getDiagnosticsFile(),
usePic,
ccCompilationContext.getExternalIncludeDirs(),
additionalBuildVariables);
return buildVariables.build();
}

@Nullable
private static String toPathString(Artifact a) {
return a == null ? null : a.getExecPathString();
}

/**
* Returns a {@code CppCompileActionBuilder} with the common fields for a C++ compile action being
* initialized.
Expand Down
170 changes: 157 additions & 13 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 @@ -30,6 +30,7 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.ExpansionException;
import com.google.devtools.build.lib.starlarkbuildapi.cpp.CcToolchainVariablesApi;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.util.ArrayList;
import java.util.LinkedHashMap;
Expand Down Expand Up @@ -1000,7 +1001,8 @@ private StringSequence(Iterable<String> values) {

@Override
public ImmutableList<VariableValue> getSequenceValue(String variableName) {
ImmutableList.Builder<VariableValue> sequences = ImmutableList.builder();
ImmutableList.Builder<VariableValue> sequences =
ImmutableList.builderWithExpectedSize(values.size());
for (String value : values) {
sequences.add(new StringValue(value));
}
Expand Down Expand Up @@ -1059,8 +1061,10 @@ private StringSetSequence(NestedSet<String> values) {

@Override
public ImmutableList<VariableValue> getSequenceValue(String variableName) {
ImmutableList.Builder<VariableValue> sequences = ImmutableList.builder();
for (String value : values.toList()) {
ImmutableList<String> valuesList = values.toList();
ImmutableList.Builder<VariableValue> sequences =
ImmutableList.builderWithExpectedSize(valuesList.size());
for (String value : valuesList) {
sequences.add(new StringValue(value));
}
return sequences.build();
Expand Down Expand Up @@ -1093,6 +1097,53 @@ public int hashCode() {
}
}

@Immutable
private static final class PathFragmentSetSequence extends VariableValueAdapter {
private final NestedSet<PathFragment> values;

private PathFragmentSetSequence(NestedSet<PathFragment> values) {
Preconditions.checkNotNull(values);
this.values = values;
}

@Override
public ImmutableList<VariableValue> getSequenceValue(String variableName) {
ImmutableList<PathFragment> valuesList = values.toList();
ImmutableList.Builder<VariableValue> sequences =
ImmutableList.builderWithExpectedSize(valuesList.size());
for (PathFragment value : valuesList) {
sequences.add(new StringValue(value.getSafePathString()));
}
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 PathFragmentSetSequence otherPathFragments)) {
return false;
}
return values.shallowEquals(otherPathFragments.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 @@ -1145,7 +1196,7 @@ public int hashCode() {
}

/**
* The leaves in the variable sequence node tree are simple string values. Note that this should
* Most leaves in the variable sequence node tree are simple string values. Note that this should
* never live outside of {@code expand}, as the object overhead is prohibitively expensive.
*/
@Immutable
Expand Down Expand Up @@ -1221,6 +1272,53 @@ public boolean isTruthy() {
}
}

/**
* Represents leaves in the variable sequence node tree that are paths of artifacts. Note that
* this should never live outside of {@code expand}, as the object overhead is prohibitively
* expensive.
*/
@Immutable
private static final class ArtifactValue extends VariableValueAdapter {
private static final String ARTIFACT_VARIABLE_TYPE_NAME = "artifact";

private final Artifact value;

ArtifactValue(Artifact value) {
this.value = value;
}

@Override
public String getStringValue(String variableName) {
return value.getExecPathString();
}

@Override
public String getVariableTypeName() {
return ARTIFACT_VARIABLE_TYPE_NAME;
}

@Override
public boolean isTruthy() {
return true;
}

@Override
public boolean equals(Object other) {
if (this == other) {
return true;
}
if (!(other instanceof ArtifactValue otherValue)) {
return false;
}
return value.equals(otherValue.value);
}

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

public static Builder builder() {
return new Builder(null);
}
Expand Down Expand Up @@ -1256,6 +1354,34 @@ public Builder addStringVariable(String name, String value) {
return this;
}

/** Add an artifact variable that expands {@code name} to {@code value}. */
@CanIgnoreReturnValue
public Builder addArtifactVariable(String name, Artifact value) {
checkVariableNotPresentAlready(name);
Preconditions.checkNotNull(value, "Cannot set null as a value for variable '%s'", name);
variablesMap.put(name, value);
return this;
}

/**
* Add an artifact or string variable that expands {@code name} to {@code value}.
*
* <p>Prefer {@link #addArtifactVariable} and {@link #addStringVariable}. This method is only
* meant to support string-based Starlark API.
*/
@CanIgnoreReturnValue
public Builder addArtifactOrStringVariable(String name, Object value) {
return switch (value) {
case String s -> addStringVariable(name, s);
case Artifact artifact -> addArtifactVariable(name, artifact);
case null ->
throw new IllegalArgumentException(
"Cannot set null as a value for variable '" + name + "'");
default ->
throw new IllegalArgumentException("Unsupported value type: " + value.getClass());
};
}

/** Overrides a variable to expands {@code name} to {@code value} instead. */
@CanIgnoreReturnValue
public Builder overrideStringVariable(String name, String value) {
Expand Down Expand Up @@ -1309,6 +1435,19 @@ public Builder addStringSequenceVariable(String name, Iterable<String> values) {
return this;
}

/**
* Add a sequence variable that expands {@code name} to {@link PathFragment} {@code values}.
*
* <p>Accepts values as NestedSet. Nested set is stored directly, not cloned, not flattened.
*/
@CanIgnoreReturnValue
public Builder addPathFragmentSequenceVariable(String name, NestedSet<PathFragment> values) {
checkVariableNotPresentAlready(name);
Preconditions.checkNotNull(values, "Cannot set null as a value for variable '%s'", name);
variablesMap.put(name, new PathFragmentSetSequence(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 Expand Up @@ -1356,15 +1495,24 @@ public Builder addAllNonTransitive(CcToolchainVariables variables) {
/** @return a new {@link CcToolchainVariables} object. */
public CcToolchainVariables build() {
if (variablesMap.size() == 1) {
Object o = variablesMap.values().iterator().next();
VariableValue variableValue =
o instanceof String ? new StringValue((String) o) : (VariableValue) o;
return new SingleVariables(parent, variablesMap.keySet().iterator().next(), variableValue);
return new SingleVariables(
parent,
variablesMap.keySet().iterator().next(),
asVariableValue(variablesMap.values().iterator().next()));
}
return new MapVariables(parent, variablesMap);
}
}

/** Wraps a raw variablesMap value into an appropriate VariableValue if necessary. */
private static VariableValue asVariableValue(Object o) {
return switch (o) {
case String s -> new StringValue(s);
case Artifact artifact -> new ArtifactValue(artifact);
default -> (VariableValue) o;
};
}

/**
* A group of extra {@code Variable} instances, packaged as logic for adding to a {@code Builder}
*/
Expand Down Expand Up @@ -1424,11 +1572,7 @@ void addVariablesToMap(Map<String, Object> variablesMap) {
@Override
VariableValue getNonStructuredVariable(String name) {
if (keyToIndex.containsKey(name)) {
Object o = values.get(keyToIndex.get(name));
if (o instanceof String) {
return new StringValue((String) o);
}
return (VariableValue) o;
return CcToolchainVariables.asVariableValue(values.get(keyToIndex.get(name)));
}

if (parent != null) {
Expand Down
Loading

0 comments on commit 84f4b55

Please sign in to comment.