From daf78cc149c135514e557485007fffb058bd94f2 Mon Sep 17 00:00:00 2001 From: hlopko Date: Thu, 22 Feb 2018 03:55:11 -0800 Subject: Recompile linkstamps whenever any input of the owning action changes. Linkstamps were not re-built when only volatile data changed, i.e. when we modified cc_binary source, linkstamp was not recompiled so we got old timestamps. The proper behavior is to recompile linkstamp whenever any input to cc_binary linking action changes. And the implementation in this cl solves this by adding all linking inputs as inputs to linkstamp compile action. RELNOTES: None. PiperOrigin-RevId: 186595143 --- .../build/lib/rules/cpp/CppLinkActionBuilder.java | 122 +++++++++++++-------- .../lib/rules/cpp/CppLinkstampCompileHelper.java | 2 +- 2 files changed, 77 insertions(+), 47 deletions(-) (limited to 'src/main/java/com/google') 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 0fce270143..c95cd590c6 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 @@ -801,20 +801,28 @@ public class CppLinkActionBuilder { // a native link. NestedSet uniqueLibraries; ImmutableSet objectFileInputs; + ImmutableSet linkstampObjectFileInputs; if (isLtoIndexing) { objectFileInputs = computeLtoIndexingObjectFileInputs(); uniqueLibraries = computeLtoIndexingUniqueLibraries(originalUniqueLibraries); + linkstampObjectFileInputs = ImmutableSet.of(); } else { - ImmutableSet.Builder builder = - ImmutableSet.builder().addAll(objectFiles); - builder.addAll( - LinkerInputs.simpleLinkerInputs(linkstampMap.values(), ArtifactCategory.OBJECT_FILE)); - - objectFileInputs = builder.build(); + objectFileInputs = ImmutableSet.copyOf(objectFiles); + linkstampObjectFileInputs = + ImmutableSet.copyOf( + LinkerInputs.simpleLinkerInputs(linkstampMap.values(), ArtifactCategory.OBJECT_FILE)); uniqueLibraries = originalUniqueLibraries; } - final Iterable objectArtifacts = LinkerInputs.toLibraryArtifacts(objectFileInputs); - + final ImmutableSet objectArtifacts = + ImmutableSet.copyOf(LinkerInputs.toLibraryArtifacts(objectFileInputs)); + final ImmutableSet linkstampObjectArtifacts = + ImmutableSet.copyOf(LinkerInputs.toLibraryArtifacts(linkstampObjectFileInputs)); + + ImmutableSet combinedObjectArtifacts = + ImmutableSet.builder() + .addAll(objectArtifacts) + .addAll(linkstampObjectArtifacts) + .build(); final LibraryToLink outputLibrary = linkType.isExecutable() ? null @@ -822,7 +830,7 @@ public class CppLinkActionBuilder { output, linkType.getLinkerOutput(), libraryIdentifier, - objectArtifacts, + combinedObjectArtifacts, ltoBitcodeFiles, createSharedNonLtoArtifacts(features, isLtoIndexing)); final LibraryToLink interfaceOutputLibrary = @@ -832,7 +840,7 @@ public class CppLinkActionBuilder { interfaceOutput, ArtifactCategory.DYNAMIC_LIBRARY, libraryIdentifier, - objectArtifacts, + combinedObjectArtifacts, ltoBitcodeFiles, /* sharedNonLtoBackends= */ null); @@ -885,6 +893,7 @@ public class CppLinkActionBuilder { final Iterable linkerInputs = IterablesChain.builder() .add(objectFileInputs) + .add(linkstampObjectFileInputs) .add( ImmutableIterable.from( Link.mergeInputsCmdLine( @@ -1004,34 +1013,6 @@ public class CppLinkActionBuilder { LinkCommandLine linkCommandLine = linkCommandLineBuilder.build(); - if (!isLtoIndexing) { - for (Entry 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, - cppConfiguration.forcePic() - || (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 dependencyInputsBuilder = NestedSetBuilder.stableOrder(); dependencyInputsBuilder.addTransitive(crosstoolInputs); @@ -1043,9 +1024,6 @@ public class CppLinkActionBuilder { if (runtimeMiddleman != null) { dependencyInputsBuilder.add(runtimeMiddleman); } - if (!isLtoIndexing) { - dependencyInputsBuilder.addAll(linkstampMap.values()); - } if (defFile != null) { dependencyInputsBuilder.add(defFile); } @@ -1056,7 +1034,8 @@ public class CppLinkActionBuilder { uniqueLibraries, needWholeArchive, CppHelper.getArchiveType(cppConfiguration, toolchain))); - Iterable expandedNonLibraryInputs = LinkerInputs.toLibraryArtifacts(objectFileInputs); + ImmutableSet expandedNonLibraryInputs = objectArtifacts; + ImmutableSet expandedNonLibraryLinkstampInputs = linkstampObjectArtifacts; if (!isLtoIndexing && allLtoArtifacts != null) { // We are doing LTO, and this is the real link, so substitute @@ -1075,12 +1054,19 @@ public class CppLinkActionBuilder { expandedInputs = renamedInputs; // Handle non-libraries. - List renamedNonLibraryInputs = new ArrayList<>(); + ImmutableSet.Builder renamedNonLibraryInputs = ImmutableSet.builder(); for (Artifact a : expandedNonLibraryInputs) { Artifact renamed = ltoMapping.get(a); renamedNonLibraryInputs.add(renamed == null ? a : renamed); } - expandedNonLibraryInputs = renamedNonLibraryInputs; + expandedNonLibraryInputs = renamedNonLibraryInputs.build(); + + ImmutableSet.Builder renamedNonLibraryLinkstampInputs = ImmutableSet.builder(); + for (Artifact a : expandedNonLibraryLinkstampInputs) { + Artifact renamed = ltoMapping.get(a); + renamedNonLibraryLinkstampInputs.add(renamed == null ? a : renamed); + } + expandedNonLibraryLinkstampInputs = renamedNonLibraryLinkstampInputs.build(); } // getPrimaryInput returns the first element, and that is a public interface - therefore the @@ -1098,8 +1084,14 @@ public class CppLinkActionBuilder { if (linkCommandLine.getParamFile() != null) { inputsBuilder.add(ImmutableList.of(linkCommandLine.getParamFile())); // Pass along tree artifacts, so they can be properly expanded. - Iterable expandedNonLibraryTreeArtifactInputs = - Iterables.filter(expandedNonLibraryInputs, a -> a.isTreeArtifact()); + ImmutableSet expandedNonLibraryTreeArtifactInputs = + ImmutableSet.builder() + .addAll(expandedNonLibraryInputs) + .addAll(expandedNonLibraryLinkstampInputs) + .build() + .stream() + .filter(a -> a.isTreeArtifact()) + .collect(ImmutableSet.toImmutableSet()); Action parameterFileWriteAction = new ParameterFileWriteAction( getOwner(), @@ -1123,6 +1115,44 @@ public class CppLinkActionBuilder { featureConfiguration.getToolForAction(getActionName()).getExecutionRequirements()); } + if (!isLtoIndexing) { + for (Entry linkstampEntry : linkstampMap.entrySet()) { + analysisEnvironment.registerAction( + CppLinkstampCompileHelper.createLinkstampCompileAction( + ruleContext, + linkstampEntry.getKey().getArtifact(), + linkstampEntry.getValue(), + linkstampEntry.getKey().getDeclaredIncludeSrcs(), + NestedSetBuilder.stableOrder() + .addAll(nonCodeInputs) + // We don't want to add outputs of this linkstamp compilation action to + // inputsBuilder before this line, since that would introduce a cycle in the + // graph. + .addAll(inputsBuilder.deduplicate().build()) + .build(), + buildInfoHeaderArtifacts, + additionalLinkstampDefines, + toolchain, + configuration.isCodeCoverageEnabled(), + cppConfiguration, + CppHelper.getFdoBuildStamp(ruleContext, fdoSupport.getFdoSupport()), + featureConfiguration, + cppConfiguration.forcePic() + || (linkType == LinkTargetType.DYNAMIC_LIBRARY + && toolchain.toolchainNeedsPic()), + Matcher.quoteReplacement( + isNativeDeps && cppConfiguration.shareNativeDeps() + ? output.getExecPathString() + : Label.print(getOwner().getLabel())), + Matcher.quoteReplacement(output.getExecPathString()), + cppSemantics)); + } + + inputsBuilder.add(linkstampMap.values()); + } + + inputsBuilder.add(expandedNonLibraryLinkstampInputs); + return new CppLinkAction( getOwner(), mnemonic, 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 index 49be968256..f04101b13c 100644 --- 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 @@ -33,7 +33,7 @@ public class CppLinkstampCompileHelper { Artifact sourceFile, Artifact outputFile, Iterable compilationInputs, - ImmutableSet nonCodeInputs, + Iterable nonCodeInputs, ImmutableList buildInfoHeaderArtifacts, Iterable additionalLinkstampDefines, CcToolchainProvider ccToolchainProvider, -- cgit v1.2.3