diff options
author | cpeyser <cpeyser@google.com> | 2017-12-08 09:16:28 -0800 |
---|---|---|
committer | Copybara-Service <copybara-piper@google.com> | 2017-12-08 09:18:34 -0800 |
commit | 367f704e71f352b404df38161f4c367b9ff506c9 (patch) | |
tree | a9319876a96fcca04af89561263b085cc3db6ae0 /src/main/java/com | |
parent | baa99fb3d21a9563c9da5c20b975b41111c5519f (diff) |
Rollback of 178106899.
PiperOrigin-RevId: 178384991
Diffstat (limited to 'src/main/java/com')
10 files changed, 458 insertions, 324 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java index 3f82509a59..cdd388342a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java @@ -57,7 +57,7 @@ public final class CompileCommandLine { this.cppConfiguration = Preconditions.checkNotNull(cppConfiguration); this.variables = variables; this.actionName = actionName; - this.dotdFile = isGenerateDotdFile(sourceFile) ? dotdFile : null; + this.dotdFile = isGenerateDotdFile(sourceFile) ? Preconditions.checkNotNull(dotdFile) : null; } /** Returns true if Dotd file should be generated. */ diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index 4a97d11181..667ef4d10f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -266,7 +266,6 @@ public class CppCompileAction extends AbstractAction boolean usePic, boolean useHeaderModules, NestedSet<Artifact> mandatoryInputs, - ImmutableList<Artifact> builtinIncludeFiles, NestedSet<Artifact> prunableInputs, Artifact outputFile, DotdFile dotdFile, @@ -335,7 +334,7 @@ public class CppCompileAction extends AbstractAction // artifact and will definitely exist prior to this action execution. this.mandatoryInputs = mandatoryInputs; this.prunableInputs = prunableInputs; - this.builtinIncludeFiles = builtinIncludeFiles; + this.builtinIncludeFiles = ImmutableList.copyOf(cppProvider.getBuiltinIncludeFiles()); this.cppSemantics = cppSemantics; this.additionalIncludeScannables = ImmutableList.copyOf(additionalIncludeScannables); this.builtInIncludeDirectories = diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java index a079119ab5..c5fe8c2466 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java @@ -42,7 +42,6 @@ import java.util.List; import java.util.Map; import java.util.UUID; import java.util.function.Consumer; -import javax.annotation.Nullable; /** * Builder class to construct C++ compile actions. @@ -82,8 +81,6 @@ public class CppCompileActionBuilder { private CcToolchainProvider ccToolchain; private final ImmutableMap<String, String> localShellEnvironment; private final boolean codeCoverageEnabled; - @Nullable private String actionName; - private ImmutableList<Artifact> builtinIncludeFiles; // New fields need to be added to the copy constructor. /** @@ -176,7 +173,6 @@ public class CppCompileActionBuilder { this.codeCoverageEnabled = other.codeCoverageEnabled; this.cppSemantics = other.cppSemantics; this.ccToolchain = other.ccToolchain; - this.actionName = other.actionName; } public PathFragment getTempOutputFile() { @@ -219,9 +215,6 @@ public class CppCompileActionBuilder { } private String getActionName() { - if (actionName != null) { - return actionName; - } PathFragment sourcePath = sourceFile.getExecPath(); if (CppFileTypes.CPP_MODULE_MAP.matches(sourcePath)) { return CppCompileAction.CPP_MODULE_COMPILE; @@ -363,7 +356,6 @@ public class CppCompileActionBuilder { usePic, useHeaderModules, realMandatoryInputs, - getBuiltinIncludeFiles(), prunableInputs, outputFile, tempOutputFile, @@ -390,7 +382,6 @@ public class CppCompileActionBuilder { usePic, useHeaderModules, realMandatoryInputs, - getBuiltinIncludeFiles(), prunableInputs, outputFile, dotdFile, @@ -419,22 +410,13 @@ public class CppCompileActionBuilder { return action; } - private ImmutableList<Artifact> getBuiltinIncludeFiles() { - ImmutableList.Builder<Artifact> result = ImmutableList.builder(); - result.addAll(ccToolchain.getBuiltinIncludeFiles()); - if (builtinIncludeFiles != null) { - result.addAll(builtinIncludeFiles); - } - return result.build(); - } - /** * Returns the list of mandatory inputs for the {@link CppCompileAction} as configured. */ NestedSet<Artifact> buildMandatoryInputs() { NestedSetBuilder<Artifact> realMandatoryInputsBuilder = NestedSetBuilder.compileOrder(); realMandatoryInputsBuilder.addTransitive(mandatoryInputsBuilder.build()); - realMandatoryInputsBuilder.addAll(getBuiltinIncludeFiles()); + realMandatoryInputsBuilder.addAll(ccToolchain.getBuiltinIncludeFiles()); realMandatoryInputsBuilder.addAll(context.getTransitiveCompilationPrerequisites()); if (useHeaderModules() && !shouldPruneModules()) { realMandatoryInputsBuilder.addTransitive(context.getTransitiveModules(usePic)); @@ -494,15 +476,6 @@ public class CppCompileActionBuilder { } /** - * Set action name that is used to pick the right action_config and features from {@link - * FeatureConfiguration}. By default the action name is decided from the source filetype. - */ - public CppCompileActionBuilder setActionName(String actionName) { - this.actionName = actionName; - return this; - } - - /** * Sets the feature configuration to be used for the action. */ public CppCompileActionBuilder setFeatureConfiguration( @@ -691,10 +664,4 @@ public class CppCompileActionBuilder { this.coptsFilter = Preconditions.checkNotNull(coptsFilter); return this; } - - public CppCompileActionBuilder setBuiltinIncludeFiles( - ImmutableList<Artifact> builtinIncludeFiles) { - this.builtinIncludeFiles = builtinIncludeFiles; - return this; - } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java index 9392ffafe6..7fca555b30 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java @@ -590,10 +590,13 @@ public class CppHelper { } /** - * Emits a warning on the rule if there are identical linkstamp artifacts with different + * Resolves the linkstamp collection from the {@code CcLinkParams} into a map. + * + * <p>Emits a warning on the rule if there are identical linkstamp artifacts with different * compilation contexts. */ - public static void checkLinkstampsUnique(RuleErrorConsumer listener, CcLinkParams linkParams) { + public static Map<Artifact, NestedSet<Artifact>> resolveLinkstamps( + RuleErrorConsumer listener, CcLinkParams linkParams) { Map<Artifact, NestedSet<Artifact>> result = new LinkedHashMap<>(); for (Linkstamp pair : linkParams.getLinkstamps()) { Artifact artifact = pair.getArtifact(); @@ -603,6 +606,7 @@ public class CppHelper { } result.put(artifact, pair.getDeclaredIncludeSrcs()); } + return result; } public static void addTransitiveLipoInfoForCommonAttributes( diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java index 3cc3791e02..9d2076eabf 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java @@ -47,7 +47,6 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; -import com.google.devtools.build.lib.rules.cpp.CcLinkParams.Linkstamp; import com.google.devtools.build.lib.rules.cpp.CppConfiguration.Tool; import com.google.devtools.build.lib.rules.cpp.Link.LinkStaticness; import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; @@ -58,6 +57,7 @@ import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; +import java.util.Collection; import java.util.LinkedHashMap; import java.util.List; import javax.annotation.Nullable; @@ -110,7 +110,6 @@ public final class CppLinkAction extends AbstractAction private final ImmutableMap<String, String> actionEnv; private final ImmutableMap<String, String> toolchainEnv; private final ImmutableSet<String> executionRequirements; - private final ImmutableList<Artifact> linkstampObjects; private final LinkCommandLine linkCommandLine; @@ -159,7 +158,6 @@ public final class CppLinkAction extends AbstractAction LibraryToLink interfaceOutputLibrary, boolean fake, boolean isLtoIndexing, - ImmutableList<Artifact> linkstampObjects, LinkCommandLine linkCommandLine, ImmutableSet<String> clientEnvironmentVariables, ImmutableMap<String, String> actionEnv, @@ -179,7 +177,6 @@ public final class CppLinkAction extends AbstractAction this.interfaceOutputLibrary = interfaceOutputLibrary; this.fake = fake; this.isLtoIndexing = isLtoIndexing; - this.linkstampObjects = linkstampObjects; this.linkCommandLine = linkCommandLine; this.clientEnvironmentVariables = clientEnvironmentVariables; this.actionEnv = actionEnv; @@ -291,13 +288,14 @@ public final class CppLinkAction extends AbstractAction } /** - * Returns a (possibly empty) list of linkstamp object files. + * Returns a (possibly empty) mapping of (C++ source file, .o output file) pairs for source files + * that need to be compiled at link time. * * <p>This is used to embed various values from the build system into binaries to identify their * provenance. */ - public ImmutableList<Artifact> getLinkstampObjects() { - return linkstampObjects; + public ImmutableMap<Artifact, Artifact> getLinkstamps() { + return linkCommandLine.getLinkstamps(); } @Override @@ -334,6 +332,14 @@ public final class CppLinkAction extends AbstractAction @ThreadCompatible private void executeFake() throws ActionExecutionException { + // The uses of getLinkConfiguration in this method may not be consistent with the computed key. + // I.e., this may be incrementally incorrect. + final Collection<Artifact> linkstampOutputs = getLinkstamps().values(); + + // Prefix all fake output files in the command line with $TEST_TMPDIR/. + final String outputPrefix = "$TEST_TMPDIR/"; + List<String> escapedLinkArgv = + escapeLinkArgv(linkCommandLine.getRawLinkArgv(), linkstampOutputs, outputPrefix); // Write the commands needed to build the real target to the fake target // file. StringBuilder s = new StringBuilder(); @@ -359,13 +365,25 @@ public final class CppLinkAction extends AbstractAction } s.append(getOutputFile().getBaseName()).append(": "); - Joiner.on(' ').appendTo(s, ShellEscaper.escapeAll(linkCommandLine.getRawLinkArgv())); + for (Artifact linkstamp : linkstampOutputs) { + s.append( + "mkdir -p " + outputPrefix + linkstamp.getExecPath().getParentDirectory() + " && "); + } + Joiner.on(' ') + .appendTo( + s, + ShellEscaper.escapeAll( + linkCommandLine.finalizeAlreadyEscapedWithLinkstampCommands( + escapedLinkArgv, outputPrefix))); s.append('\n'); if (getOutputFile().exists()) { getOutputFile().setWritable(true); // (IOException) } FileSystemUtils.writeContent(getOutputFile(), ISO_8859_1, s.toString()); getOutputFile().setExecutable(true); // (IOException) + for (Artifact linkstamp : linkstampOutputs) { + FileSystemUtils.touchFile(linkstamp.getPath()); + } } catch (IOException e) { throw new ActionExecutionException("failed to create fake link command for rule '" + getOwner().getLabel() + ": " + e.getMessage(), @@ -373,6 +391,37 @@ public final class CppLinkAction extends AbstractAction } } + /** + * Shell-escapes the raw link command line. + * + * @param rawLinkArgv raw link command line + * @param linkstampOutputs linkstamp artifacts + * @param outputPrefix to be prepended to any outputs + * @return escaped link command line + */ + private List<String> escapeLinkArgv( + List<String> rawLinkArgv, + final Collection<Artifact> linkstampOutputs, + final String outputPrefix) { + final List<String> linkstampExecPaths = Artifact.asExecPaths(linkstampOutputs); + ImmutableList.Builder<String> escapedArgs = ImmutableList.builder(); + for (String rawArg : rawLinkArgv) { + String escapedArg; + if (rawArg.equals(getPrimaryOutput().getExecPathString()) + || linkstampExecPaths.contains(rawArg)) { + escapedArg = outputPrefix + ShellEscaper.escapeString(rawArg); + } else if (rawArg.startsWith(Link.FAKE_OBJECT_PREFIX)) { + escapedArg = + outputPrefix + + ShellEscaper.escapeString(rawArg.substring(Link.FAKE_OBJECT_PREFIX.length())); + } else { + escapedArg = ShellEscaper.escapeString(rawArg); + } + escapedArgs.add(escapedArg); + } + return escapedArgs.build(); + } + @Override public ExtraActionInfo.Builder getExtraActionInfo(ActionKeyContext actionKeyContext) { // The uses of getLinkConfiguration in this method may not be consistent with the computed key. @@ -388,7 +437,7 @@ public final class CppLinkAction extends AbstractAction } info.setLinkTargetType(getLinkCommandLine().getLinkTargetType().name()); info.setLinkStaticness(getLinkCommandLine().getLinkStaticness().name()); - info.addAllLinkStamp(Artifact.toExecPaths(getLinkstampObjects())); + info.addAllLinkStamp(Artifact.toExecPaths(getLinkstamps().values())); info.addAllBuildInfoHeaderArtifact(Artifact.toExecPaths(getBuildInfoHeaderArtifacts())); info.addAllLinkOpt(getLinkCommandLine().getRawLinkArgv()); @@ -520,7 +569,8 @@ public final class CppLinkAction extends AbstractAction final Artifact runtimeMiddleman; final NestedSet<Artifact> runtimeInputs; final ArtifactCategory runtimeType; - final ImmutableSet<Linkstamp> linkstamps; + final NestedSet<Artifact> compilationInputs; + final ImmutableSet<Artifact> linkstamps; final ImmutableList<String> linkopts; final LinkTargetType linkType; final LinkStaticness linkStaticness; @@ -546,7 +596,11 @@ public final class CppLinkAction extends AbstractAction this.runtimeInputs = NestedSetBuilder.<Artifact>stableOrder().addTransitive(builder.getRuntimeInputs()).build(); this.runtimeType = builder.getRuntimeType(); - this.linkstamps = builder.getLinkstamps(); + this.compilationInputs = + NestedSetBuilder.<Artifact>stableOrder() + .addTransitive(builder.getCompilationInputs().build()) + .build(); + this.linkstamps = ImmutableSet.copyOf(builder.getLinkstamps()); this.linkopts = ImmutableList.copyOf(builder.getLinkopts()); this.linkType = builder.getLinkType(); this.linkStaticness = builder.getLinkStaticness(); @@ -590,11 +644,16 @@ public final class CppLinkAction extends AbstractAction return this.runtimeInputs; } + /** Returns compilation inputs for compilations arising from the linking of this target. */ + public NestedSet<Artifact> getCompilationInputs() { + return this.compilationInputs; + } + /** Returns linkstamp artifacts. */ - public ImmutableSet<Linkstamp> getLinkstamps() { + public ImmutableSet<Artifact> getLinkstamps() { return this.linkstamps; } - + /** * Returns linkopts for the linking of this target. */ diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java index 1777257632..d7f1b31b86 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java @@ -32,7 +32,6 @@ import com.google.devtools.build.lib.analysis.AnalysisEnvironment; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.actions.ParameterFileWriteAction; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; -import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.CollectionUtils; import com.google.devtools.build.lib.collect.ImmutableIterable; import com.google.devtools.build.lib.collect.IterablesChain; @@ -40,7 +39,6 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.packages.RuleErrorConsumer; -import com.google.devtools.build.lib.rules.cpp.CcLinkParams.Linkstamp; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables.LibraryToLinkValue; @@ -60,9 +58,7 @@ import java.util.HashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; -import java.util.Map.Entry; import java.util.Set; -import java.util.regex.Matcher; import javax.annotation.Nullable; /** Builder class to construct {@link CppLinkAction}s. */ @@ -164,7 +160,7 @@ public class CppLinkActionBuilder { @Nullable private final RuleContext ruleContext; private final AnalysisEnvironment analysisEnvironment; private final Artifact output; - private final CppSemantics cppSemantics; + private final CppSemantics unusedCppSemantics; @Nullable private String mnemonic; // can be null for CppLinkAction.createTestBuilder() @@ -186,7 +182,8 @@ public class CppLinkActionBuilder { private Artifact runtimeMiddleman; private ArtifactCategory runtimeType = null; private NestedSet<Artifact> runtimeInputs = NestedSetBuilder.emptySet(Order.STABLE_ORDER); - private final ImmutableSet.Builder<Linkstamp> linkstampsBuilder = ImmutableSet.builder(); + private final NestedSetBuilder<Artifact> compilationInputs = NestedSetBuilder.stableOrder(); + private final Set<Artifact> linkstamps = new LinkedHashSet<>(); private ImmutableList<String> additionalLinkstampDefines = ImmutableList.of(); private final List<String> linkopts = new ArrayList<>(); private LinkTargetType linkType = LinkTargetType.STATIC_LIBRARY; @@ -296,7 +293,7 @@ public class CppLinkActionBuilder { runtimeSolibDir = toolchain.getDynamicRuntimeSolibDir(); } this.featureConfiguration = featureConfiguration; - this.cppSemantics = Preconditions.checkNotNull(cppSemantics); + this.unusedCppSemantics = Preconditions.checkNotNull(cppSemantics); } /** @@ -342,7 +339,8 @@ public class CppLinkActionBuilder { this.runtimeMiddleman = linkContext.runtimeMiddleman; this.runtimeInputs = linkContext.runtimeInputs; this.runtimeType = linkContext.runtimeType; - this.linkstampsBuilder.addAll(linkContext.linkstamps); + this.compilationInputs.addTransitive(linkContext.compilationInputs); + this.linkstamps.addAll(linkContext.linkstamps); this.linkopts.addAll(linkContext.linkopts); this.linkType = linkContext.linkType; this.linkStaticness = linkContext.linkStaticness; @@ -397,9 +395,14 @@ public class CppLinkActionBuilder { return runtimeType; } + /** Returns compilation inputs for this link action. */ + public final NestedSetBuilder<Artifact> getCompilationInputs() { + return this.compilationInputs; + } + /** Returns linkstamps for this link action. */ - public final ImmutableSet<Linkstamp> getLinkstamps() { - return linkstampsBuilder.build(); + public final Set<Artifact> getLinkstamps() { + return this.linkstamps; } /** @@ -726,9 +729,6 @@ public class CppLinkActionBuilder { boolean isExecutable = linkType.isExecutable(); Preconditions.checkState(hasIdentifier != isExecutable); Preconditions.checkNotNull(featureConfiguration); - ImmutableSet<Linkstamp> linkstamps = linkstampsBuilder.build(); - final ImmutableMap<Linkstamp, Artifact> linkstampMap = - mapLinkstampsToOutputs(linkstamps, ruleContext, configuration, output, linkArtifactFactory); if (interfaceOutput != null && (fake || linkType != LinkTargetType.DYNAMIC_LIBRARY)) { throw new RuntimeException( @@ -796,12 +796,7 @@ public class CppLinkActionBuilder { objectFileInputs = computeLtoIndexingObjectFileInputs(); uniqueLibraries = computeLtoIndexingUniqueLibraries(originalUniqueLibraries); } else { - ImmutableSet.Builder<LinkerInput> builder = - ImmutableSet.<LinkerInput>builder().addAll(objectFiles); - builder.addAll( - LinkerInputs.simpleLinkerInputs(linkstampMap.values(), ArtifactCategory.OBJECT_FILE)); - - objectFileInputs = builder.build(); + objectFileInputs = ImmutableSet.copyOf(objectFiles); uniqueLibraries = originalUniqueLibraries; } final Iterable<Artifact> objectArtifacts = LinkerInputs.toLibraryArtifacts(objectFileInputs); @@ -838,6 +833,9 @@ public class CppLinkActionBuilder { ltoBitcodeFiles, /* sharedNonLtoBackends= */ null); + final ImmutableMap<Artifact, Artifact> linkstampMap = + mapLinkstampsToOutputs(linkstamps, ruleContext, configuration, output, linkArtifactFactory); + @Nullable Artifact thinltoParamFile = null; if (allowLtoIndexing && allLtoArtifacts != null) { // Create artifact for the file that the LTO indexing step will emit @@ -867,7 +865,7 @@ public class CppLinkActionBuilder { actionOutputs = constructOutputs( output, - linkActionOutputs.build(), + Iterables.concat(linkstampMap.values(), linkActionOutputs.build()), interfaceOutputLibrary == null ? null : interfaceOutputLibrary.getArtifact(), symbolCounts); } @@ -891,6 +889,7 @@ public class CppLinkActionBuilder { isLtoIndexing ? new CppLinkVariablesExtension( configuration, + /* linkstampMap= */ ImmutableMap.of(), needWholeArchive, linkerInputs, runtimeLinkerInputs, @@ -904,6 +903,7 @@ public class CppLinkActionBuilder { /* interfaceLibraryOutput= */ null) : new CppLinkVariablesExtension( configuration, + linkstampMap, needWholeArchive, linkerInputs, runtimeLinkerInputs, @@ -943,7 +943,7 @@ public class CppLinkActionBuilder { } LinkCommandLine.Builder linkCommandLineBuilder = - new LinkCommandLine.Builder(configuration, ruleContext) + new LinkCommandLine.Builder(configuration, getOwner(), ruleContext) .setLinkerInputs(linkerInputs) .setRuntimeInputs(runtimeLinkerInputs) .setLinkTargetType(linkType) @@ -954,6 +954,7 @@ public class CppLinkActionBuilder { .setUseTestOnlyFlags(useTestOnlyFlags) .setParamFile(paramFile) .setToolchain(toolchain) + .setFdoSupport(fdoSupport.getFdoSupport()) .setBuildVariables(buildVariables) .setFeatureConfiguration(featureConfiguration); @@ -965,8 +966,11 @@ public class CppLinkActionBuilder { if (!isLtoIndexing) { linkCommandLineBuilder + .setOutput(output) .setBuildInfoHeaderArtifacts(buildInfoHeaderArtifacts) - .setLinkopts(ImmutableList.copyOf(linkopts)); + .setLinkstamps(linkstampMap) + .setLinkopts(ImmutableList.copyOf(linkopts)) + .setAdditionalLinkstampDefines(additionalLinkstampDefines); } else { List<String> opts = new ArrayList<>(linkopts); opts.addAll(featureConfiguration.getCommandLine("lto-indexing", buildVariables)); @@ -976,30 +980,6 @@ public class CppLinkActionBuilder { LinkCommandLine linkCommandLine = linkCommandLineBuilder.build(); - for (Entry<Linkstamp, Artifact> linkstampEntry : linkstampMap.entrySet()) { - analysisEnvironment.registerAction( - CppLinkstampCompileHelper.createLinkstampCompileAction( - ruleContext, - linkstampEntry.getKey().getArtifact(), - linkstampEntry.getValue(), - linkstampEntry.getKey().getDeclaredIncludeSrcs(), - ImmutableSet.copyOf(nonCodeInputs), - buildInfoHeaderArtifacts, - additionalLinkstampDefines, - toolchain, - configuration.isCodeCoverageEnabled(), - cppConfiguration, - CppHelper.getFdoBuildStamp(ruleContext, fdoSupport.getFdoSupport()), - featureConfiguration, - linkType == LinkTargetType.DYNAMIC_LIBRARY && toolchain.toolchainNeedsPic(), - Matcher.quoteReplacement( - isNativeDeps && cppConfiguration.shareNativeDeps() - ? output.getExecPathString() - : Label.print(getOwner().getLabel())), - Matcher.quoteReplacement(output.getExecPathString()), - cppSemantics)); - } - // Compute the set of inputs - we only need stable order here. NestedSetBuilder<Artifact> dependencyInputsBuilder = NestedSetBuilder.stableOrder(); dependencyInputsBuilder.addTransitive(crosstoolInputs); @@ -1012,7 +992,9 @@ public class CppLinkActionBuilder { dependencyInputsBuilder.add(runtimeMiddleman); } if (!isLtoIndexing) { - dependencyInputsBuilder.addAll(linkstampMap.values()); + dependencyInputsBuilder.addAll(buildInfoHeaderArtifacts); + dependencyInputsBuilder.addAll(linkstamps); + dependencyInputsBuilder.addTransitive(compilationInputs.build()); } if (defFile != null) { dependencyInputsBuilder.add(defFile); @@ -1098,11 +1080,6 @@ public class CppLinkActionBuilder { interfaceOutputLibrary, fake, isLtoIndexing, - linkstampMap - .keySet() - .stream() - .map(Linkstamp::getArtifact) - .collect(ImmutableList.toImmutableList()), linkCommandLine, configuration.getVariableShellEnvironment(), configuration.getLocalShellEnvironment(), @@ -1145,23 +1122,23 @@ public class CppLinkActionBuilder { } /** - * Translates a collection of {@link Linkstamp} instances to an immutable mapping from linkstamp - * to object files. In other words, given a set of source files, this method determines the output + * Translates a collection of linkstamp source files to an immutable mapping from source files to + * object files. In other words, given a set of source files, this method determines the output * path to which each file should be compiled. * - * @param linkstamps set of {@link Linkstamp}s + * @param linkstamps collection of linkstamp source files * @param ruleContext the rule for which this link is being performed * @param outputBinary the binary output path for this link * @return an immutable map that pairs each source file with the corresponding object file that * should be fed into the link */ - public static ImmutableMap<Linkstamp, Artifact> mapLinkstampsToOutputs( - ImmutableSet<Linkstamp> linkstamps, + public static ImmutableMap<Artifact, Artifact> mapLinkstampsToOutputs( + Collection<Artifact> linkstamps, RuleContext ruleContext, BuildConfiguration configuration, Artifact outputBinary, LinkArtifactFactory linkArtifactFactory) { - ImmutableMap.Builder<Linkstamp, Artifact> mapBuilder = ImmutableMap.builder(); + ImmutableMap.Builder<Artifact, Artifact> mapBuilder = ImmutableMap.builder(); PathFragment outputBinaryPath = outputBinary.getRootRelativePath(); PathFragment stampOutputDirectory = @@ -1170,11 +1147,10 @@ public class CppLinkActionBuilder { .getRelative(CppHelper.OBJS) .getRelative(outputBinaryPath.getBaseName()); - for (Linkstamp linkstamp : linkstamps) { + for (Artifact linkstamp : linkstamps) { PathFragment stampOutputPath = stampOutputDirectory.getRelative( - FileSystemUtils.replaceExtension( - linkstamp.getArtifact().getRootRelativePath(), ".o")); + FileSystemUtils.replaceExtension(linkstamp.getRootRelativePath(), ".o")); mapBuilder.put( linkstamp, // Note that link stamp actions can be shared between link actions that output shared @@ -1260,6 +1236,20 @@ public class CppLinkActionBuilder { return this; } + /** + * Add additional inputs needed for the linkstamp compilation that is being done as part of the + * link. + */ + public CppLinkActionBuilder addCompilationInputs(Iterable<Artifact> inputs) { + this.compilationInputs.addAll(inputs); + return this; + } + + public CppLinkActionBuilder addTransitiveCompilationInputs(NestedSet<Artifact> inputs) { + this.compilationInputs.addTransitive(inputs); + return this; + } + private void addObjectFile(LinkerInput input) { // We skip file extension checks for TreeArtifacts because they represent directory artifacts // without a file extension. @@ -1391,15 +1381,19 @@ public class CppLinkActionBuilder { } /** - * Adds {@link Linkstamp}s. - * - * <p>This is used to embed various values from the build system into binaries to identify their - * provenance. + * Adds a C++ source file which will be compiled at link time. This is used to embed various + * values from the build system into binaries to identify their provenance. * - * <p>Linkstamp object files are also automatically added to the inputs of the link action. + * <p>Link stamps are also automatically added to the inputs. */ - public CppLinkActionBuilder addLinkstamps(Iterable<Linkstamp> linkstamps) { - this.linkstampsBuilder.addAll(linkstamps); + public CppLinkActionBuilder addLinkstamps(Map<Artifact, NestedSet<Artifact>> linkstamps) { + this.linkstamps.addAll(linkstamps.keySet()); + // Add inputs for linkstamping. + if (!linkstamps.isEmpty()) { + for (Map.Entry<Artifact, NestedSet<Artifact>> entry : linkstamps.entrySet()) { + addCompilationInputs(entry.getValue()); + } + } return this; } @@ -1439,8 +1433,7 @@ public class CppLinkActionBuilder { addLibraries(extraLibrary.buildLibraries(ruleContext)); } } - CppHelper.checkLinkstampsUnique(errorListener, linkParams); - addLinkstamps(linkParams.getLinkstamps()); + addLinkstamps(CppHelper.resolveLinkstamps(errorListener, linkParams)); return this; } @@ -1556,6 +1549,7 @@ public class CppLinkActionBuilder { private class CppLinkVariablesExtension implements VariablesExtension { private final BuildConfiguration configuration; + private final ImmutableMap<Artifact, Artifact> linkstampMap; private final boolean needWholeArchive; private final Iterable<LinkerInput> linkerInputs; private final ImmutableList<LinkerInput> runtimeLinkerInputs; @@ -1571,6 +1565,7 @@ public class CppLinkActionBuilder { public CppLinkVariablesExtension( BuildConfiguration configuration, + ImmutableMap<Artifact, Artifact> linkstampMap, boolean needWholeArchive, Iterable<LinkerInput> linkerInputs, ImmutableList<LinkerInput> runtimeLinkerInputs, @@ -1582,6 +1577,7 @@ public class CppLinkActionBuilder { Artifact interfaceLibraryBuilder, Artifact interfaceLibraryOutput) { this.configuration = configuration; + this.linkstampMap = linkstampMap; this.needWholeArchive = needWholeArchive; this.linkerInputs = linkerInputs; this.runtimeLinkerInputs = runtimeLinkerInputs; @@ -1605,6 +1601,14 @@ public class CppLinkActionBuilder { SYMBOL_COUNTS_OUTPUT_VARIABLE, symbolCounts.getExecPathString()); } + // linkstamp + ImmutableSet.Builder<String> linkstampPaths = ImmutableSet.builder(); + for (Artifact linkstampOutput : linkstampMap.values()) { + linkstampPaths.add(linkstampOutput.getExecPathString()); + } + + buildVariables.addStringSequenceVariable(LINKSTAMP_PATHS_VARIABLE, linkstampPaths.build()); + // pic if (cppConfiguration.forcePic()) { buildVariables.addStringVariable(FORCE_PIC_VARIABLE, ""); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkstampCompileHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkstampCompileHelper.java deleted file mode 100644 index dac1517c4b..0000000000 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkstampCompileHelper.java +++ /dev/null @@ -1,186 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.rules.cpp; - -import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableList.Builder; -import com.google.common.collect.ImmutableSet; -import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.analysis.RuleContext; -import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; -import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables; -import java.util.regex.Pattern; - -/** Handles creation of CppCompileAction used to compile linkstamp sources. */ -public class CppLinkstampCompileHelper { - - /** Creates {@link CppCompileAction} to compile linkstamp source */ - public static CppCompileAction createLinkstampCompileAction( - RuleContext ruleContext, - Artifact sourceFile, - Artifact outputFile, - Iterable<Artifact> compilationInputs, - ImmutableSet<Artifact> nonCodeInputs, - ImmutableList<Artifact> buildInfoHeaderArtifacts, - Iterable<String> additionalLinkstampDefines, - CcToolchainProvider ccToolchainProvider, - boolean codeCoverageEnabled, - CppConfiguration cppConfiguration, - String fdoBuildStamp, - FeatureConfiguration featureConfiguration, - boolean needsPic, - String labelReplacement, - String outputReplacement, - CppSemantics semantics) { - CppCompileActionBuilder builder = - new CppCompileActionBuilder(ruleContext, ccToolchainProvider) - .addMandatoryInputs(compilationInputs) - .setVariables( - getVariables( - sourceFile, - outputFile, - labelReplacement, - outputReplacement, - additionalLinkstampDefines, - buildInfoHeaderArtifacts, - featureConfiguration, - cppConfiguration, - ccToolchainProvider, - needsPic, - fdoBuildStamp, - codeCoverageEnabled)) - .setFeatureConfiguration(featureConfiguration) - .setSourceFile(sourceFile) - .setSemantics(semantics) - .setOutputs(outputFile, null) - .setBuiltinIncludeFiles(buildInfoHeaderArtifacts) - .addMandatoryInputs(nonCodeInputs) - .setCppConfiguration(cppConfiguration) - .setActionName(CppCompileAction.LINKSTAMP_COMPILE); - semantics.finalizeCompileActionBuilder(ruleContext, builder); - return builder.buildOrThrowIllegalStateException(); - } - - private static ImmutableList<String> computeAllLinkstampDefines( - String labelReplacement, - String outputReplacement, - Iterable<String> additionalLinkstampDefines, - CppConfiguration cppConfiguration, - String fdoBuildStamp, - boolean codeCoverageEnabled) { - String labelPattern = Pattern.quote("${LABEL}"); - String outputPathPattern = Pattern.quote("${OUTPUT_PATH}"); - Builder<String> defines = - ImmutableList.<String>builder() - .add("GPLATFORM=\"" + cppConfiguration + "\"") - .add("BUILD_COVERAGE_ENABLED=" + (codeCoverageEnabled ? "1" : "0")) - .add(CppConfiguration.FDO_STAMP_MACRO + "=\"" + fdoBuildStamp + "\"") - // G3_VERSION_INFO and G3_TARGET_NAME are C string literals that normally - // contain the label of the target being linked. However, they are set - // differently when using shared native deps. In that case, a single .so file - // is shared by multiple targets, and its contents cannot depend on which - // target(s) were specified on the command line. So in that case we have - // to use the (obscure) name of the .so file instead, or more precisely - // the path of the .so file relative to the workspace root. - .add("G3_VERSION_INFO=\"${LABEL}\"") - .add("G3_TARGET_NAME=\"${LABEL}\"") - // G3_BUILD_TARGET is a C string literal containing the output of this - // link. (An undocumented and untested invariant is that G3_BUILD_TARGET is the - // location of the executable, either absolutely, or relative to the directory part of - // BUILD_INFO.) - .add("G3_BUILD_TARGET=\"${OUTPUT_PATH}\"") - .addAll(additionalLinkstampDefines); - - if (fdoBuildStamp != null) { - defines.add(CppConfiguration.FDO_STAMP_MACRO + "=\"" + fdoBuildStamp + "\""); - } - - return defines - .build() - .stream() - .map( - define -> - define - .replaceAll(labelPattern, labelReplacement) - .replaceAll(outputPathPattern, outputReplacement)) - .collect(ImmutableList.toImmutableList()); - } - - private static Variables getVariables( - Artifact sourceFile, - Artifact outputFile, - String labelReplacement, - String outputReplacement, - Iterable<String> additionalLinkstampDefines, - ImmutableList<Artifact> buildInfoHeaderArtifacts, - FeatureConfiguration featureConfiguration, - CppConfiguration cppConfiguration, - CcToolchainProvider ccToolchainProvider, - boolean needsPic, - String fdoBuildStamp, - boolean codeCoverageEnabled) { - // TODO(b/34761650): Remove all this hardcoding by separating a full blown compile action. - Preconditions.checkArgument( - featureConfiguration.actionIsConfigured(CppCompileAction.LINKSTAMP_COMPILE)); - - Variables.Builder variables = new Variables.Builder(ccToolchainProvider.getBuildVariables()); - // We need to force inclusion of build_info headers - variables.addStringSequenceVariable( - CppModel.INCLUDES_VARIABLE_NAME, - buildInfoHeaderArtifacts - .stream() - .map(Artifact::getExecPathString) - .collect(ImmutableList.toImmutableList())); - // Input/Output files. - variables.addStringVariable(CppModel.SOURCE_FILE_VARIABLE_NAME, sourceFile.getExecPathString()); - variables.addStringVariable(CppModel.OUTPUT_FILE_VARIABLE_NAME, outputFile.getExecPathString()); - variables.addStringVariable( - CppModel.OUTPUT_OBJECT_FILE_VARIABLE_NAME, outputFile.getExecPathString()); - // Include directories for (normal includes with ".", empty quote- and system- includes). - variables.addStringSequenceVariable( - CppModel.INCLUDE_PATHS_VARIABLE_NAME, ImmutableList.of(".")); - variables.addStringSequenceVariable( - CppModel.QUOTE_INCLUDE_PATHS_VARIABLE_NAME, ImmutableList.of()); - variables.addStringSequenceVariable( - CppModel.SYSTEM_INCLUDE_PATHS_VARIABLE_NAME, ImmutableList.of()); - // Legacy flags coming from fields such as compiler_flag - variables.addLazyStringSequenceVariable( - CppModel.LEGACY_COMPILE_FLAGS_VARIABLE_NAME, - CppModel.getLegacyCompileFlagsSupplier( - cppConfiguration, sourceFile.getExecPathString(), ImmutableSet.of())); - // Unfilterable flags coming from unfiltered_cxx_flag fields - variables.addLazyStringSequenceVariable( - CppModel.UNFILTERED_COMPILE_FLAGS_VARIABLE_NAME, - CppModel.getUnfilteredCompileFlagsSupplier(ccToolchainProvider, ImmutableSet.of())); - // Collect all preprocessor defines, and in each replace ${LABEL} by labelReplacements, and - // ${OUTPUT_PATH} with outputPathReplacement. - variables.addStringSequenceVariable( - CppModel.PREPROCESSOR_DEFINES_VARIABLE_NAME, - computeAllLinkstampDefines( - labelReplacement, - outputReplacement, - additionalLinkstampDefines, - cppConfiguration, - fdoBuildStamp, - codeCoverageEnabled)); - // For dynamic libraries, produce position independent code. - if (needsPic) { - variables.addStringVariable(CppModel.PIC_VARIABLE_NAME, ""); - } - - return variables.build(); - } -} diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java index 46ef0c6c4a..3fa6091dc2 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java @@ -66,7 +66,6 @@ public class FakeCppCompileAction extends CppCompileAction { boolean usePic, boolean useHeaderModules, NestedSet<Artifact> mandatoryInputs, - ImmutableList<Artifact> builtinIncludeFiles, NestedSet<Artifact> prunableInputs, Artifact outputFile, PathFragment tempOutputFile, @@ -91,7 +90,6 @@ public class FakeCppCompileAction extends CppCompileAction { usePic, useHeaderModules, mandatoryInputs, - builtinIncludeFiles, prunableInputs, outputFile, dotdFile, diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java index e1cde74d3d..3be6802314 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java @@ -15,15 +15,20 @@ package com.google.devtools.build.lib.rules.cpp; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Joiner; import com.google.common.base.Preconditions; import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.common.collect.Lists; +import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.actions.CommandLine; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.CollectionUtils; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; @@ -32,9 +37,13 @@ import com.google.devtools.build.lib.rules.cpp.Link.LinkStaticness; import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; import com.google.devtools.build.lib.rules.cpp.Link.Staticness; import com.google.devtools.build.lib.util.Pair; +import com.google.devtools.build.lib.util.ShellEscaper; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; import java.util.List; +import java.util.Map; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import javax.annotation.Nullable; /** @@ -45,10 +54,13 @@ import javax.annotation.Nullable; public final class LinkCommandLine extends CommandLine { private final String actionName; private final String forcedToolPath; + private final boolean codeCoverageEnabled; private final CppConfiguration cppConfiguration; + private final ActionOwner owner; private final CcToolchainFeatures.Variables variables; // The feature config can be null for tests. @Nullable private final FeatureConfiguration featureConfiguration; + @Nullable private final Artifact output; private final ImmutableList<Artifact> buildInfoHeaderArtifacts; private final Iterable<? extends LinkerInput> linkerInputs; private final Iterable<? extends LinkerInput> runtimeInputs; @@ -56,6 +68,9 @@ public final class LinkCommandLine extends CommandLine { private final LinkStaticness linkStaticness; private final ImmutableList<String> linkopts; private final ImmutableSet<String> features; + private final ImmutableMap<Artifact, Artifact> linkstamps; + private final ImmutableList<String> additionalLinkstampDefines; + @Nullable private final String fdoBuildStamp; @Nullable private final PathFragment runtimeSolibDir; private final boolean nativeDeps; private final boolean useTestOnlyFlags; @@ -67,6 +82,8 @@ public final class LinkCommandLine extends CommandLine { String actionName, String forcedToolPath, BuildConfiguration configuration, + ActionOwner owner, + Artifact output, ImmutableList<Artifact> buildInfoHeaderArtifacts, Iterable<? extends LinkerInput> linkerInputs, Iterable<? extends LinkerInput> runtimeInputs, @@ -74,6 +91,9 @@ public final class LinkCommandLine extends CommandLine { LinkStaticness linkStaticness, ImmutableList<String> linkopts, ImmutableSet<String> features, + ImmutableMap<Artifact, Artifact> linkstamps, + ImmutableList<String> additionalLinkstampDefines, + @Nullable String fdoBuildStamp, @Nullable PathFragment runtimeSolibDir, boolean nativeDeps, boolean useTestOnlyFlags, @@ -84,9 +104,12 @@ public final class LinkCommandLine extends CommandLine { this.actionName = actionName; this.forcedToolPath = forcedToolPath; + this.codeCoverageEnabled = configuration.isCodeCoverageEnabled(); this.cppConfiguration = configuration.getFragment(CppConfiguration.class); this.variables = variables; this.featureConfiguration = featureConfiguration; + this.owner = Preconditions.checkNotNull(owner); + this.output = output; this.buildInfoHeaderArtifacts = Preconditions.checkNotNull(buildInfoHeaderArtifacts); this.linkerInputs = Preconditions.checkNotNull(linkerInputs); this.runtimeInputs = Preconditions.checkNotNull(runtimeInputs); @@ -98,6 +121,9 @@ public final class LinkCommandLine extends CommandLine { ? ImmutableList.of() : Preconditions.checkNotNull(linkopts); this.features = Preconditions.checkNotNull(features); + this.linkstamps = Preconditions.checkNotNull(linkstamps); + this.additionalLinkstampDefines = additionalLinkstampDefines; + this.fdoBuildStamp = fdoBuildStamp; this.runtimeSolibDir = runtimeSolibDir; this.nativeDeps = nativeDeps; this.useTestOnlyFlags = useTestOnlyFlags; @@ -150,6 +176,11 @@ public final class LinkCommandLine extends CommandLine { return linkopts; } + /** See {@link CppLinkAction#getLinkstamps()} */ + protected ImmutableMap<Artifact, Artifact> getLinkstamps() { + return linkstamps; + } + /** * Returns the location of the C++ runtime solib symlinks. If null, the C++ dynamic runtime * libraries either do not exist (because they do not come from the depot) or they are in the @@ -342,7 +373,8 @@ public final class LinkCommandLine extends CommandLine { /** * Returns a raw link command for the given link invocation, including both command and arguments - * (argv). + * (argv). After any further usage-specific processing, this can be passed to {@link + * #finalizeWithLinkstampCommands} to give the final command line. * * @return raw link command line. */ @@ -371,27 +403,244 @@ public final class LinkCommandLine extends CommandLine { } List<String> getCommandLine() { + List<String> commandlineArgs; // Try to shorten the command line by use of a parameter file. // This makes the output with --subcommands (et al) more readable. if (paramFile != null) { Pair<List<String>, List<String>> split = splitCommandline(); - return split.first; + commandlineArgs = split.first; } else { - return getRawLinkArgv(); + commandlineArgs = getRawLinkArgv(); } + return finalizeWithLinkstampCommands(commandlineArgs); } @Override public List<String> arguments() { - return (getRawLinkArgv()); + return finalizeWithLinkstampCommands(getRawLinkArgv()); + } + + /** + * Takes a raw link command line and gives the final link command that will also first compile any + * linkstamps necessary. Elements of rawLinkArgv are shell-escaped. + * + * @param rawLinkArgv raw link command line + * @return final link command line suitable for execution + */ + public List<String> finalizeWithLinkstampCommands(List<String> rawLinkArgv) { + return addLinkstampingToCommand(getLinkstampCompileCommands(""), rawLinkArgv, true); + } + + /** + * Takes a raw link command line and gives the final link command that will also first compile any + * linkstamps necessary. Elements of rawLinkArgv are not shell-escaped. + * + * @param rawLinkArgv raw link command line + * @param outputPrefix prefix to add before the linkstamp outputs' exec paths + * @return final link command line suitable for execution + */ + public List<String> finalizeAlreadyEscapedWithLinkstampCommands( + List<String> rawLinkArgv, String outputPrefix) { + return addLinkstampingToCommand(getLinkstampCompileCommands(outputPrefix), rawLinkArgv, false); + } + + /** + * Adds linkstamp compilation to the (otherwise) fully specified link command if {@link + * #getLinkstamps} is non-empty. + * + * <p>Linkstamps were historically compiled implicitly as part of the link command, but implicit + * compilation doesn't guarantee consistent outputs. For example, the command "gcc input.o input.o + * foo/linkstamp.cc -o myapp" causes gcc to implicitly run "gcc foo/linkstamp.cc -o + * /tmp/ccEtJHDB.o", for some internally decided output path /tmp/ccEtJHDB.o, then add that path + * to the linker's command line options. The name of this path can change even between + * equivalently specified gcc invocations. + * + * <p>So now we explicitly compile these files in their own command invocations before running the + * link command, thus giving us direct control over the naming of their outputs. This method adds + * those extra steps as necessary. + * + * @param linkstampCommands individual linkstamp compilation commands + * @param linkCommand the complete list of link command arguments (after .params file compacting) + * for an invocation + * @param escapeArgs if true, linkCommand arguments are shell escaped. if false, arguments are + * returned as-is + * @return The original argument list if no linkstamps compilation commands are given, otherwise + * an expanded list that adds the linkstamp compilation commands and funnels their outputs + * into the link step. Note that these outputs only need to persist for the duration of the + * link step. + */ + private static List<String> addLinkstampingToCommand( + List<String> linkstampCommands, List<String> linkCommand, boolean escapeArgs) { + if (linkstampCommands.isEmpty()) { + return linkCommand; + } else { + List<String> batchCommand = Lists.newArrayListWithCapacity(3); + batchCommand.add("/bin/bash"); + batchCommand.add("-c"); + batchCommand.add( + Joiner.on(" && ").join(linkstampCommands) + + " && " + + (escapeArgs + ? ShellEscaper.escapeJoinAll(linkCommand) + : Joiner.on(" ").join(linkCommand))); + return ImmutableList.copyOf(batchCommand); + } + } + + private boolean isSharedNativeLibrary() { + return nativeDeps && cppConfiguration.shareNativeDeps(); + } + + /** + * Computes, for each C++ source file in {@link #getLinkstamps}, the command necessary to compile + * that file such that the output is correctly fed into the link command. + * + * <p>As these options (as well as all others) are taken into account when computing the action + * key, they do not directly contain volatile build information to avoid unnecessary relinking. + * Instead this information is passed as an additional header generated by {@link + * com.google.devtools.build.lib.rules.cpp.WriteBuildInfoHeaderAction}. + * + * @param outputPrefix prefix to add before the linkstamp outputs' exec paths + * @return a list of shell-escaped compiler commmands, one for each entry in {@link + * #getLinkstamps} + */ + public List<String> getLinkstampCompileCommands(String outputPrefix) { + if (linkstamps.isEmpty()) { + return ImmutableList.of(); + } + + List<String> commands = Lists.newArrayListWithCapacity(linkstamps.size()); + + for (Map.Entry<Artifact, Artifact> linkstamp : linkstamps.entrySet()) { + Artifact sourceFile = linkstamp.getKey(); + Artifact outputFile = linkstamp.getValue(); + Variables linkstampVariables = collectLinkstampVariables(sourceFile, outputFile); + + ImmutableList.Builder<String> linkstampCompileCommandLine = ImmutableList.builder(); + linkstampCompileCommandLine.add( + featureConfiguration + .getToolForAction(CppCompileAction.LINKSTAMP_COMPILE) + .getToolPath(cppConfiguration.getCrosstoolTopPathFragment()) + .getPathString()); + linkstampCompileCommandLine.addAll( + featureConfiguration.getCommandLine( + CppCompileAction.LINKSTAMP_COMPILE, linkstampVariables)); + // TODO(b/28946988): Remove -c/-o hardcoded flags from bazel + linkstampCompileCommandLine.add("-c"); + linkstampCompileCommandLine.add(sourceFile.getExecPathString()); + linkstampCompileCommandLine.add("-o"); + // outputPrefix is there only for cc_fake_binary, and it can contain env var expansions + // (such as $TEST_TMPDIR) and cannot be escaped. When we move linkstamping to a separate + // action, there will no longer be bash around the invocation and therefore no need to do + // shell escaping. + String escapedCommandWithoutOutput = + ShellEscaper.escapeJoinAll(linkstampCompileCommandLine.build()); + commands.add( + escapedCommandWithoutOutput + + " " + + outputPrefix + + ShellEscaper.escapeString(outputFile.getExecPathString())); + } + + return commands; + } + + private Variables collectLinkstampVariables(Artifact sourceFile, Artifact outputFile) { + // TODO(b/34761650): Remove all this hardcoding by separating a full blown compile action. + Preconditions.checkArgument( + featureConfiguration.actionIsConfigured(CppCompileAction.LINKSTAMP_COMPILE)); + + Variables.Builder linkstampVariables = new Variables.Builder(ccProvider.getBuildVariables()); + // We need to force inclusion of build_info headers + linkstampVariables.addStringSequenceVariable( + CppModel.INCLUDES_VARIABLE_NAME, + buildInfoHeaderArtifacts + .stream() + .map(Artifact::getExecPathString) + .collect(ImmutableList.toImmutableList())); + // Input/Output files. + linkstampVariables.addStringVariable( + CppModel.SOURCE_FILE_VARIABLE_NAME, sourceFile.getExecPathString()); + linkstampVariables.addStringVariable( + CppModel.OUTPUT_OBJECT_FILE_VARIABLE_NAME, outputFile.getExecPathString()); + // Include directories for (normal includes with ".", empty quote- and system- includes). + linkstampVariables.addStringSequenceVariable( + CppModel.INCLUDE_PATHS_VARIABLE_NAME, ImmutableList.of(".")); + linkstampVariables.addStringSequenceVariable( + CppModel.QUOTE_INCLUDE_PATHS_VARIABLE_NAME, ImmutableList.of()); + linkstampVariables.addStringSequenceVariable( + CppModel.SYSTEM_INCLUDE_PATHS_VARIABLE_NAME, ImmutableList.of()); + // Legacy flags coming from fields such as compiler_flag + linkstampVariables.addLazyStringSequenceVariable( + CppModel.LEGACY_COMPILE_FLAGS_VARIABLE_NAME, + CppModel.getLegacyCompileFlagsSupplier( + cppConfiguration, sourceFile.getExecPathString(), features)); + // Unfilterable flags coming from unfiltered_cxx_flag fields + linkstampVariables.addLazyStringSequenceVariable( + CppModel.UNFILTERED_COMPILE_FLAGS_VARIABLE_NAME, + CppModel.getUnfilteredCompileFlagsSupplier(ccProvider, features)); + // Collect all preprocessor defines, and in each replace ${LABEL} by labelReplacements, and + // ${OUTPUT_PATH} with outputPathReplacement. + linkstampVariables.addStringSequenceVariable( + CppModel.PREPROCESSOR_DEFINES_VARIABLE_NAME, computeAllLinkstampDefines()); + // For dynamic libraries, produce position independent code. + if (cppConfiguration.forcePic() + || (linkTargetType == LinkTargetType.DYNAMIC_LIBRARY && ccProvider.toolchainNeedsPic())) { + linkstampVariables.addStringVariable(CppModel.PIC_VARIABLE_NAME, ""); + } + return linkstampVariables.build(); + } + + private ImmutableList<String> computeAllLinkstampDefines() { + String labelReplacement = + Matcher.quoteReplacement( + isSharedNativeLibrary() ? output.getExecPathString() : Label.print(owner.getLabel())); + String outputPathReplacement = Matcher.quoteReplacement(output.getExecPathString()); + String labelPattern = Pattern.quote("${LABEL}"); + String outputPathPattern = Pattern.quote("${OUTPUT_PATH}"); + ImmutableList.Builder<String> defines = ImmutableList.builder(); + defines + .add("GPLATFORM=\"" + cppConfiguration + "\"") + .add("BUILD_COVERAGE_ENABLED=" + (codeCoverageEnabled ? "1" : "0")) + // G3_VERSION_INFO and G3_TARGET_NAME are C string literals that normally + // contain the label of the target being linked. However, they are set + // differently when using shared native deps. In that case, a single .so file + // is shared by multiple targets, and its contents cannot depend on which + // target(s) were specified on the command line. So in that case we have + // to use the (obscure) name of the .so file instead, or more precisely + // the path of the .so file relative to the workspace root. + .add("G3_VERSION_INFO=\"${LABEL}\"") + .add("G3_TARGET_NAME=\"${LABEL}\"") + // G3_BUILD_TARGET is a C string literal containing the output of this + // link. (An undocumented and untested invariant is that G3_BUILD_TARGET is the location of + // the executable, either absolutely, or relative to the directory part of BUILD_INFO.) + .add("G3_BUILD_TARGET=\"${OUTPUT_PATH}\"") + .addAll(additionalLinkstampDefines); + + if (fdoBuildStamp != null) { + defines.add(CppConfiguration.FDO_STAMP_MACRO + "=\"" + fdoBuildStamp + "\""); + } + + return defines + .build() + .stream() + .map( + define -> + define + .replaceAll(labelPattern, labelReplacement) + .replaceAll(outputPathPattern, outputPathReplacement)) + .collect(ImmutableList.toImmutableList()); } /** A builder for a {@link LinkCommandLine}. */ public static final class Builder { private final BuildConfiguration configuration; + private final ActionOwner owner; private final RuleContext ruleContext; + private String forcedToolPath; + @Nullable private Artifact output; private ImmutableList<Artifact> buildInfoHeaderArtifacts = ImmutableList.of(); private Iterable<? extends LinkerInput> linkerInputs = ImmutableList.of(); private Iterable<? extends LinkerInput> runtimeInputs = ImmutableList.of(); @@ -399,30 +648,37 @@ public final class LinkCommandLine extends CommandLine { private LinkStaticness linkStaticness = LinkStaticness.FULLY_STATIC; private ImmutableList<String> linkopts = ImmutableList.of(); private ImmutableSet<String> features = ImmutableSet.of(); + private ImmutableMap<Artifact, Artifact> linkstamps = ImmutableMap.of(); + private ImmutableList<String> additionalLinkstampDefines = ImmutableList.of(); @Nullable private PathFragment runtimeSolibDir; private boolean nativeDeps; private boolean useTestOnlyFlags; @Nullable private Artifact paramFile; private CcToolchainProvider toolchain; + private FdoSupport fdoSupport; private Variables variables; private FeatureConfiguration featureConfiguration; // This interface is needed to support tests that don't create a // ruleContext, in which case the configuration and action owner // cannot be accessed off of the give ruleContext. - public Builder(BuildConfiguration configuration, RuleContext ruleContext) { + public Builder(BuildConfiguration configuration, ActionOwner owner, RuleContext ruleContext) { this.configuration = configuration; + this.owner = owner; this.ruleContext = ruleContext; } public Builder(RuleContext ruleContext) { - this(ruleContext.getConfiguration(), ruleContext); + this(ruleContext.getConfiguration(), ruleContext.getActionOwner(), ruleContext); } public LinkCommandLine build() { if (linkTargetType.staticness() == Staticness.STATIC) { Preconditions.checkArgument( + linkstamps.isEmpty(), + "linkstamps may only be present on dynamic library or executable links"); + Preconditions.checkArgument( buildInfoHeaderArtifacts.isEmpty(), "build info headers may only be present on dynamic library or executable links"); } @@ -436,6 +692,11 @@ public final class LinkCommandLine extends CommandLine { // The ruleContext can be null for some tests. if (ruleContext != null) { Preconditions.checkNotNull(featureConfiguration); + + if (fdoSupport == null) { + fdoSupport = + CppHelper.getFdoSupportUsingDefaultCcToolchainAttribute(ruleContext).getFdoSupport(); + } } if (variables == null) { @@ -448,6 +709,8 @@ public final class LinkCommandLine extends CommandLine { actionName, forcedToolPath, configuration, + owner, + output, buildInfoHeaderArtifacts, linkerInputs, runtimeInputs, @@ -455,6 +718,9 @@ public final class LinkCommandLine extends CommandLine { linkStaticness, linkopts, features, + linkstamps, + additionalLinkstampDefines, + CppHelper.getFdoBuildStamp(ruleContext, fdoSupport), runtimeSolibDir, nativeDeps, useTestOnlyFlags, @@ -473,6 +739,11 @@ public final class LinkCommandLine extends CommandLine { return this; } + public Builder setFdoSupport(FdoSupport fdoSupport) { + this.fdoSupport = fdoSupport; + return this; + } + /** Use given tool path instead of the one from feature configuration */ public Builder forceToolPath(String forcedToolPath) { this.forcedToolPath = forcedToolPath; @@ -497,6 +768,12 @@ public final class LinkCommandLine extends CommandLine { return this; } + /** Sets the primary output artifact. This must be called before calling {@link #build}. */ + public Builder setOutput(Artifact output) { + this.output = output; + return this; + } + /** * Sets a list of linker inputs. These get turned into linker options depending on the * staticness and the target type. This call makes an immutable copy of the inputs, if the @@ -535,6 +812,22 @@ public final class LinkCommandLine extends CommandLine { } /** + * Sets the linkstamps. Linkstamps are additional C++ source files that are compiled as part of + * the link command. The {@link #build} method throws an exception if the linkstamps are + * non-empty for a static link (see {@link LinkTargetType#staticness()}}). + */ + public Builder setLinkstamps(ImmutableMap<Artifact, Artifact> linkstamps) { + this.linkstamps = linkstamps; + return this; + } + + /** Adds the given list of preprocessor defines to the linkstamp compilation. */ + public Builder setAdditionalLinkstampDefines(ImmutableList<String> additionalLinkstampDefines) { + this.additionalLinkstampDefines = Preconditions.checkNotNull(additionalLinkstampDefines); + return this; + } + + /** * The build info header artifacts are generated header files that are used for link stamping. * The {@link #build} method throws an exception if the build info header artifacts are * non-empty for a static link (see {@link LinkTargetType#staticness()}}). diff --git a/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java b/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java index 652f993625..a969aecad1 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java @@ -17,7 +17,6 @@ package com.google.devtools.build.lib.rules.nativedeps; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Root; import com.google.devtools.build.lib.analysis.RuleContext; @@ -27,7 +26,6 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.rules.cpp.ArtifactCategory; import com.google.devtools.build.lib.rules.cpp.CcCommon; import com.google.devtools.build.lib.rules.cpp.CcLinkParams; -import com.google.devtools.build.lib.rules.cpp.CcLinkParams.Linkstamp; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; import com.google.devtools.build.lib.rules.cpp.CcToolchainProvider; import com.google.devtools.build.lib.rules.cpp.CppBuildInfo; @@ -49,6 +47,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.LinkedList; import java.util.List; +import java.util.Map; /** * Helper class to create a dynamic library for rules which support integration with native code. @@ -186,8 +185,8 @@ public abstract class NativeDepsHelper { List<String> linkopts = new ArrayList<>(extraLinkOpts); linkopts.addAll(linkParams.flattenedLinkopts()); - CppHelper.checkLinkstampsUnique(ruleContext, linkParams); - ImmutableSet<Linkstamp> linkstamps = ImmutableSet.copyOf(linkParams.getLinkstamps()); + Map<Artifact, NestedSet<Artifact>> linkstamps = + CppHelper.resolveLinkstamps(ruleContext, linkParams); List<Artifact> buildInfoArtifacts = linkstamps.isEmpty() ? ImmutableList.<Artifact>of() : ruleContext.getAnalysisEnvironment().getBuildInfo( @@ -201,10 +200,7 @@ public abstract class NativeDepsHelper { getSharedNativeDepsPath( LinkerInputs.toLibraryArtifacts(linkerInputs), linkopts, - linkstamps - .stream() - .map(Linkstamp::getArtifact) - .collect(ImmutableList.toImmutableList()), + linkstamps.keySet(), buildInfoArtifacts, ruleContext.getFeatures()); libraryIdentifier = sharedPath.getPathString(); |