Skip to content

Commit

Permalink
Accept included headers which are provided by tree artifacts.
Browse files Browse the repository at this point in the history 
This allows a C++ file to include headers from a tree artifact, and pass header inclusion checks.

RELNOTES: None.
PiperOrigin-RevId: 193967617
  • Loading branch information
@cgrushko
cgrushko authored and Copybara-Service committed Apr 23, 2018
1 parent a7c6786 commit 060b162
Show file tree
Hide file tree
Showing 5 changed files with 205 additions and 43 deletions.
35 changes: 16 additions & 19 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
View file
Expand Up @@ -13,6 +13,8 @@
// limitations under the License.
package com.google.devtools.build.lib.rules.cpp;

import static java.util.Collections.unmodifiableSet;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableCollection;
Expand Down Expand Up @@ -1041,28 +1043,23 @@ private static CcToolchainFeatures.Variables getOverwrittenVariables(

@Override
public Iterable<Artifact> getAllowedDerivedInputs() {
return getAllowedDerivedInputsMap().values();
}

protected Map<PathFragment, Artifact> getAllowedDerivedInputsMap() {
Map<PathFragment, Artifact> allowedDerivedInputMap = new HashMap<>();
addToMap(allowedDerivedInputMap, mandatoryInputs);
addToMap(allowedDerivedInputMap, prunableInputs);
addToMap(allowedDerivedInputMap, getDeclaredIncludeSrcs());
addToMap(
allowedDerivedInputMap, ccCompilationContextInfo.getTransitiveCompilationPrerequisites());
addToMap(allowedDerivedInputMap, ccCompilationContextInfo.getTransitiveModules(usePic));
HashSet<Artifact> result = new HashSet<>();
addNonSources(result, mandatoryInputs);
addNonSources(result, prunableInputs);
addNonSources(result, getDeclaredIncludeSrcs());
addNonSources(result, ccCompilationContextInfo.getTransitiveCompilationPrerequisites());
addNonSources(result, ccCompilationContextInfo.getTransitiveModules(usePic));
Artifact artifact = getSourceFile();
if (!artifact.isSourceArtifact()) {
allowedDerivedInputMap.put(artifact.getExecPath(), artifact);
result.add(artifact);
}
return allowedDerivedInputMap;
return unmodifiableSet(result);
}

private void addToMap(Map<PathFragment, Artifact> map, Iterable<Artifact> artifacts) {
for (Artifact artifact : artifacts) {
if (!artifact.isSourceArtifact()) {
map.put(artifact.getExecPath(), artifact);
private static void addNonSources(HashSet<Artifact> result, Iterable<Artifact> artifacts) {
for (Artifact a : artifacts) {
if (!a.isSourceArtifact()) {
result.add(a);
}
}
}
Expand Down Expand Up @@ -1229,7 +1226,7 @@ public NestedSet<Artifact> discoverInputsFromShowIncludesFilters(
.setSourceFile(getSourceFile())
.setDependencies(dependencies.build())
.setPermittedSystemIncludePrefixes(getPermittedSystemIncludePrefixes(execRoot))
.setAllowedDerivedinputsMap(getAllowedDerivedInputsMap());
.setAllowedDerivedinputs(getAllowedDerivedInputs());

if (needsIncludeValidation) {
discoveryBuilder.shouldValidateInclusions();
Expand All @@ -1255,7 +1252,7 @@ public NestedSet<Artifact> discoverInputsFromDotdFiles(
.setDependencies(
processDepset(actionExecutionContext, execRoot, reply).getDependencies())
.setPermittedSystemIncludePrefixes(getPermittedSystemIncludePrefixes(execRoot))
.setAllowedDerivedinputsMap(getAllowedDerivedInputsMap());
.setAllowedDerivedinputs(getAllowedDerivedInputs());

if (needsIncludeValidation) {
discoveryBuilder.shouldValidateInclusions();
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
View file
Expand Up @@ -161,7 +161,7 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext)
.setDependencies(
processDepset(actionExecutionContext, execRoot, reply).getDependencies())
.setPermittedSystemIncludePrefixes(getPermittedSystemIncludePrefixes(execRoot))
.setAllowedDerivedinputsMap(getAllowedDerivedInputsMap());
.setAllowedDerivedinputs(getAllowedDerivedInputs());

if (needsIncludeValidation) {
discoveryBuilder.shouldValidateInclusions();
Expand Down
109 changes: 86 additions & 23 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
View file
Expand Up @@ -14,8 +14,9 @@

package com.google.devtools.build.lib.rules.cpp;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.ActionExecutionException;
import com.google.devtools.build.lib.actions.Artifact;
Expand All @@ -30,50 +31,86 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;

/**
* Manages the process of obtaining inputs used in a compilation from a dependency set parsed from
* either .d files or /showIncludes output.
* HeaderDiscovery checks whether all header files that a compile action uses are actually declared
* as inputs.
*
* <p>Tree artifacts: a tree artifact with path P causes any header file prefixed by P to be
* accepted. Testing whether a used header file is prefixed by any tree artifact is linear search,
* but the result is cached. If all files in a tree artifact are at the root of the artifact, the
* entire check is performed by hash lookups.
*/
public class HeaderDiscovery {

/** Indicates if a compile should perform dotd pruning. */
public static enum DotdPruningMode {
public enum DotdPruningMode {
USE,
DO_NOT_USE
}

private final Action action;
private final Artifact sourceFile;

private final boolean shouldValidateInclusions;

private final Collection<Path> dependencies;
private final List<Path> permittedSystemIncludePrefixes;
private final Map<PathFragment, Artifact> allowedDerivedInputsMap;


/**
* allowedDerivedInputsMap maps paths of derived artifacts to the artifacts. These only include
* file artifacts.
*/
private final ImmutableMap<PathFragment, Artifact> allowedDerivedInputsMap;

/**
* treeArtifactPaths contains the paths of tree artifacts given as input to the action.
*
* <p>Header files whose prefix is in this set are considered as included, and will not trigger a
* header inclusion error.
*/
private final ImmutableSet<PathFragment> treeArtifactPaths;

/**
* allowedDirs caches answers to "does a fragment have a prefix in treeArtifactPaths".
*
* <p>It is initialized to (p, true) for each p in treeArtifactPaths, in order to speed up the
* typical case of headers coming from a flat tree artifact.
*/
private final HashMap<PathFragment, Boolean> allowedDirs;

/**
* Creates a HeaderDiscover instance
*
* @param action the action instance requiring header discovery
* @param sourceFile the source file for the compile
* @param shouldValidateInclusions true if include validation should be performed
* @param allowedDerivedInputsMap see javadoc for field
* @param treeArtifactPaths see javadoc for field
*/
public HeaderDiscovery(
private HeaderDiscovery(
Action action,
Artifact sourceFile,
boolean shouldValidateInclusions,
Collection<Path> dependencies,
List<Path> permittedSystemIncludePrefixes,
Map<PathFragment, Artifact> allowedDerivedInputsMap) {
ImmutableMap<PathFragment, Artifact> allowedDerivedInputsMap,
ImmutableSet<PathFragment> treeArtifactPaths) {
this.action = Preconditions.checkNotNull(action);
this.sourceFile = Preconditions.checkNotNull(sourceFile);
this.shouldValidateInclusions = shouldValidateInclusions;
this.dependencies = dependencies;
this.permittedSystemIncludePrefixes = permittedSystemIncludePrefixes;
this.allowedDerivedInputsMap = allowedDerivedInputsMap;
this.treeArtifactPaths = treeArtifactPaths;

this.allowedDirs = new HashMap<>();
for (PathFragment p : treeArtifactPaths) {
allowedDirs.put(p, true);
}
}

/**
Expand All @@ -86,23 +123,21 @@ public HeaderDiscovery(
* @throws ActionExecutionException iff the .d is missing (when required), malformed, or has
* unresolvable included artifacts.
*/
@VisibleForTesting
@ThreadCompatible
public NestedSet<Artifact> discoverInputsFromDependencies(
NestedSet<Artifact> discoverInputsFromDependencies(
Path execRoot, ArtifactResolver artifactResolver) throws ActionExecutionException {
NestedSetBuilder<Artifact> inputs = NestedSetBuilder.stableOrder();
if (dependencies == null) {
return inputs.build();
}
List<Path> systemIncludePrefixes = permittedSystemIncludePrefixes;

// Check inclusions.
IncludeProblems problems = new IncludeProblems();
for (Path execPath : dependencies) {
PathFragment execPathFragment = execPath.asFragment();
if (execPathFragment.isAbsolute()) {
// Absolute includes from system paths are ignored.
if (FileSystemUtils.startsWithAny(execPath, systemIncludePrefixes)) {
if (FileSystemUtils.startsWithAny(execPath, permittedSystemIncludePrefixes)) {
continue;
}
// Since gcc is given only relative paths on the command line,
Expand All @@ -125,23 +160,41 @@ public NestedSet<Artifact> discoverInputsFromDependencies(
} catch (LabelSyntaxException e) {
throw new ActionExecutionException(
String.format("Could not find the external repository for %s", execPathFragment),
e, action, false);
e,
action,
false);
}
}
if (artifact != null) {
inputs.add(artifact);
} else {
// Abort if we see files that we can't resolve, likely caused by
// undeclared includes or illegal include constructs.
problems.add(execPathFragment.getPathString());
continue;
}

if (inTreeArtifact(execPathFragment)) {
continue;
}

// Abort if we see files that we can't resolve, likely caused by
// undeclared includes or illegal include constructs.
problems.add(execPathFragment.getPathString());
}
if (shouldValidateInclusions) {
problems.assertProblemFree(action, sourceFile);
}
return inputs.build();
}

private boolean inTreeArtifact(PathFragment execPathFragment) {
PathFragment dir = execPathFragment.getParentDirectory();
Boolean allowed = allowedDirs.get(dir);
if (allowed != null) {
return allowed;
}
allowed = treeArtifactPaths.stream().anyMatch(p -> dir.startsWith(p));
allowedDirs.put(execPathFragment, allowed);
return allowed;
}

/** A Builder for HeaderDiscovery */
public static class Builder {
private Action action;
Expand All @@ -150,7 +203,7 @@ public static class Builder {

private Collection<Path> dependencies;
private List<Path> permittedSystemIncludePrefixes;
private Map<PathFragment, Artifact> allowedDerivedInputsMap;
private Iterable<Artifact> allowedDerivedInputs;

/** Sets the action for which to discover inputs. */
public Builder setAction(Action action) {
Expand Down Expand Up @@ -183,20 +236,30 @@ public Builder setPermittedSystemIncludePrefixes(List<Path> systemIncludePrefixe
}

/** Sets permitted inputs to the build */
public Builder setAllowedDerivedinputsMap(Map<PathFragment, Artifact> allowedDerivedInputsMap) {
this.allowedDerivedInputsMap = allowedDerivedInputsMap;
public Builder setAllowedDerivedinputs(Iterable<Artifact> allowedDerivedInputs) {
this.allowedDerivedInputs = allowedDerivedInputs;
return this;
}

/** Creates a CppHeaderDiscovery instance. */
public HeaderDiscovery build() {
HashMap<PathFragment, Artifact> allowedDerivedInputsMap = new HashMap<>();
HashSet<PathFragment> treeArtifactPrefixes = new HashSet<>();
for (Artifact a : allowedDerivedInputs) {
if (a.isTreeArtifact()) {
treeArtifactPrefixes.add(a.getExecPath());
}
allowedDerivedInputsMap.put(a.getExecPath(), a);
}

return new HeaderDiscovery(
action,
sourceFile,
shouldValidateInclusions,
dependencies,
permittedSystemIncludePrefixes,
allowedDerivedInputsMap);
ImmutableMap.copyOf(allowedDerivedInputsMap),
ImmutableSet.copyOf(treeArtifactPrefixes));
}
}
}
1 change: 1 addition & 0 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD
View file
Expand Up @@ -53,6 +53,7 @@ java_test(
"//third_party:guava-testlib",
"//third_party:jsr305",
"//third_party:junit4",
"//third_party:mockito",
"//third_party:truth",
"//third_party/protobuf:protobuf_java",
],
Expand Down

0 comments on commit 060b162

Please sign in to comment.