From d7297887c764e9a9ee906202d988bd353e5f5ecf Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 10 May 2017 14:18:03 -0400 Subject: Automated g4 rollback of commit 6879d7ceff0e118fdecb0cabe5134979030b7cb8. *** Reason for rollback *** Fixes memory issue that caused this CL to be rolled back. *** Original change description *** Automated g4 rollback of commit cbbb423663b154d82e3dfa5e9a56839583987999. *** Reason for rollback *** Need to roll this back as part of http://b/38171368 *** Original change description *** RELNOTES: Effectively remove sysroot from CppConfiguration and allow it to use select statements. PiperOrigin-RevId: 155651879 --- .../devtools/build/lib/rules/cpp/CcBinary.java | 3 +- .../devtools/build/lib/rules/cpp/CcToolchain.java | 27 +- .../build/lib/rules/cpp/CcToolchainProvider.java | 14 +- .../build/lib/rules/cpp/CcToolchainRule.java | 6 +- .../build/lib/rules/cpp/CompileCommandLine.java | 7 +- .../build/lib/rules/cpp/CppCompileAction.java | 30 ++- .../lib/rules/cpp/CppCompileActionBuilder.java | 6 +- .../build/lib/rules/cpp/CppConfiguration.java | 297 +++++++-------------- .../lib/rules/cpp/CppConfigurationLoader.java | 44 ++- .../devtools/build/lib/rules/cpp/CppHelper.java | 2 +- .../build/lib/rules/cpp/CppLinkActionBuilder.java | 4 +- .../build/lib/rules/cpp/FakeCppCompileAction.java | 6 +- .../build/lib/rules/cpp/LinkCommandLine.java | 38 +-- 13 files changed, 225 insertions(+), 259 deletions(-) (limited to 'src/main/java/com/google/devtools/build/lib/rules') diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java index ca98d7bf61..7e15d9e462 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java @@ -523,8 +523,7 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory { private static final boolean dashStaticInLinkopts(List linkopts, CppConfiguration cppConfiguration) { - return linkopts.contains("-static") - || cppConfiguration.getLinkOptions().contains("-static"); + return linkopts.contains("-static") || cppConfiguration.hasStaticLinkOption(); } private static final LinkStaticness getLinkStaticness(RuleContext context, diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java index 6fc4c92938..da27ada49e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java @@ -36,6 +36,7 @@ import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.analysis.actions.SpawnAction; import com.google.devtools.build.lib.analysis.actions.SymlinkAction; import com.google.devtools.build.lib.analysis.config.CompilationMode; +import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; @@ -284,6 +285,15 @@ public class CcToolchain implements RuleConfiguredTargetFactory { coverage = crosstool; } + PathFragment sysroot = calculateSysroot(ruleContext); + + ImmutableList builtInIncludeDirectories = null; + try { + builtInIncludeDirectories = cppConfiguration.getBuiltInIncludeDirectories(sysroot); + } catch (InvalidConfigurationException e) { + ruleContext.ruleError(e.getMessage()); + } + CcToolchainProvider ccProvider = new CcToolchainProvider( cppConfiguration, @@ -310,9 +320,8 @@ public class CcToolchain implements RuleConfiguredTargetFactory { coverageEnvironment.build(), ruleContext.getPrerequisiteArtifact("$link_dynamic_library_tool", Mode.HOST), getEnvironment(ruleContext), - cppConfiguration.getBuiltInIncludeDirectories()); - - PathFragment sysroot = cppConfiguration.getSysroot(); + builtInIncludeDirectories, + sysroot); MakeVariableProvider makeVariableProvider = createMakeVariableProvider(cppConfiguration, sysroot); @@ -481,4 +490,16 @@ public class CcToolchain implements RuleConfiguredTargetFactory { protected ImmutableMap getEnvironment(RuleContext ruleContext) { return ImmutableMap.of(); } + + private PathFragment calculateSysroot(RuleContext ruleContext) { + + TransitiveInfoCollection sysrootTarget = ruleContext.getPrerequisite(":sysroot", Mode.TARGET); + if (sysrootTarget == null) { + CppConfiguration cppConfiguration = + Preconditions.checkNotNull(ruleContext.getFragment(CppConfiguration.class)); + return cppConfiguration.getDefaultSysroot(); + } + + return sysrootTarget.getLabel().getPackageFragment(); + } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java index 26d4c38b67..89beafe21d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java @@ -75,7 +75,8 @@ public final class CcToolchainProvider NestedSetBuilder.>emptySet(Order.COMPILE_ORDER), null, ImmutableMap.of(), - ImmutableList.of()); + ImmutableList.of(), + null); @Nullable private final CppConfiguration cppConfiguration; private final NestedSet crosstool; @@ -102,6 +103,7 @@ public final class CcToolchainProvider @Nullable private final Artifact linkDynamicLibraryTool; private final ImmutableMap environment; private final ImmutableList builtInIncludeDirectories; + @Nullable private final PathFragment sysroot; public CcToolchainProvider( @Nullable CppConfiguration cppConfiguration, @@ -128,7 +130,8 @@ public final class CcToolchainProvider NestedSet> coverageEnvironment, Artifact linkDynamicLibraryTool, ImmutableMap environment, - ImmutableList builtInIncludeDirectories) { + ImmutableList builtInIncludeDirectories, + @Nullable PathFragment sysroot) { super(SKYLARK_CONSTRUCTOR, ImmutableMap.of()); this.cppConfiguration = cppConfiguration; this.crosstool = Preconditions.checkNotNull(crosstool); @@ -155,6 +158,7 @@ public final class CcToolchainProvider this.linkDynamicLibraryTool = linkDynamicLibraryTool; this.environment = environment; this.builtInIncludeDirectories = builtInIncludeDirectories; + this.sysroot = sysroot; } @SkylarkCallable( @@ -352,7 +356,7 @@ public final class CcToolchainProvider + "this method returns None." ) public PathFragment getSysroot() { - return cppConfiguration.getSysroot(); + return sysroot; } @SkylarkCallable( @@ -362,7 +366,7 @@ public final class CcToolchainProvider + "rules. These should be appended to the command line after filtering." ) public ImmutableList getUnfilteredCompilerOptions(Iterable features) { - return cppConfiguration.getUnfilteredCompilerOptions(features); + return cppConfiguration.getUnfilteredCompilerOptions(features, sysroot); } @SkylarkCallable( @@ -373,6 +377,6 @@ public final class CcToolchainProvider + "inferred from the command-line options." ) public ImmutableList getLinkOptions() { - return cppConfiguration.getLinkOptions(); + return cppConfiguration.getLinkOptions(sysroot); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainRule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainRule.java index a1922b8553..8b7dbe92ff 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainRule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainRule.java @@ -51,9 +51,8 @@ public final class CcToolchainRule implements RuleDefinition { private static final LateBoundLabel LIBC_TOP = new LateBoundLabel(CppConfiguration.class) { @Override - public Label resolve( - Rule rule, AttributeMap attributes, BuildConfiguration configuration) { - return configuration.getFragment(CppConfiguration.class).getLibcLabel(); + public Label resolve(Rule rule, AttributeMap attributes, BuildConfiguration configuration) { + return configuration.getFragment(CppConfiguration.class).getSysrootLabel(); } }; @@ -88,6 +87,7 @@ public final class CcToolchainRule implements RuleDefinition { .value(env.getToolsLabel("//tools/cpp:link_dynamic_library"))) // TODO(bazel-team): Should be using the TARGET configuration. .add(attr(":libc_top", LABEL).cfg(HOST).value(LIBC_TOP)) + .add(attr(":sysroot", LABEL).value(LIBC_TOP)) .add( attr(":lipo_context_collector", LABEL) .cfg(LipoTransition.LIPO_COLLECTOR) 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 f9fc4c3f6b..3224702bb2 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 @@ -45,6 +45,7 @@ public final class CompileCommandLine { @VisibleForTesting public final CcToolchainFeatures.Variables variables; private final String actionName; private final CppConfiguration cppConfiguration; + private final CcToolchainProvider cppProvider; public CompileCommandLine( Artifact sourceFile, @@ -57,7 +58,8 @@ public final class CompileCommandLine { CppConfiguration cppConfiguration, CcToolchainFeatures.Variables variables, String actionName, - DotdFile dotdFile) { + DotdFile dotdFile, + CcToolchainProvider cppProvider) { this.sourceFile = Preconditions.checkNotNull(sourceFile); this.outputFile = Preconditions.checkNotNull(outputFile); this.sourceLabel = Preconditions.checkNotNull(sourceLabel); @@ -69,6 +71,7 @@ public final class CompileCommandLine { this.variables = variables; this.actionName = actionName; this.dotdFile = isGenerateDotdFile(sourceFile) ? Preconditions.checkNotNull(dotdFile) : null; + this.cppProvider = cppProvider; } /** Returns true if Dotd file should be generated. */ @@ -161,7 +164,7 @@ public final class CompileCommandLine { // the user provided options, otherwise users adding include paths will not pick up their // own include paths first. if (!isObjcCompile(actionName)) { - options.addAll(toolchain.getUnfilteredCompilerOptions(features)); + options.addAll(cppProvider.getUnfilteredCompilerOptions(features)); } // Add the options of --per_file_copt, if the label or the base name of the source file 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 1b1884ce8d..5bc7e3647a 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 @@ -217,6 +217,8 @@ public class CppCompileAction extends AbstractAction /** Whether this action needs to discover inputs. */ private final boolean discoversInputs; + private final ImmutableList builtInIncludeDirectories; + /** * Set when the action prepares for execution. Used to preserve state between preparation and * execution. @@ -266,11 +268,10 @@ public class CppCompileAction extends AbstractAction * @param additionalIncludeScannables list of additional artifacts to include-scan * @param actionClassId TODO(bazel-team): Add parameter description * @param environment TODO(bazel-team): Add parameter description - * @param builtinIncludeFiles List of include files that may be included even if they are not - * mentioned in the source file or any of the headers included by it * @param actionName a string giving the name of this action for the purpose of toolchain * evaluation * @param cppSemantics C++ compilation semantics + * @param cppProvider - CcToolchainProvider with configuration-dependent information. */ protected CppCompileAction( ActionOwner owner, @@ -307,8 +308,8 @@ public class CppCompileAction extends AbstractAction ImmutableMap executionInfo, ImmutableMap environment, String actionName, - Iterable builtinIncludeFiles, - CppSemantics cppSemantics) { + CppSemantics cppSemantics, + CcToolchainProvider cppProvider) { super( owner, allInputs, @@ -349,7 +350,8 @@ public class CppCompileAction extends AbstractAction cppConfiguration, variables, actionName, - dotdFile); + dotdFile, + cppProvider); this.actionContext = actionContext; this.lipoScannables = lipoScannables; this.actionClassId = actionClassId; @@ -360,9 +362,11 @@ public class CppCompileAction extends AbstractAction // artifact and will definitely exist prior to this action execution. this.mandatoryInputs = mandatoryInputs; this.prunableInputs = prunableInputs; - this.builtinIncludeFiles = ImmutableList.copyOf(builtinIncludeFiles); + this.builtinIncludeFiles = ImmutableList.copyOf(cppProvider.getBuiltinIncludeFiles()); this.cppSemantics = cppSemantics; this.additionalIncludeScannables = ImmutableList.copyOf(additionalIncludeScannables); + this.builtInIncludeDirectories = + ImmutableList.copyOf(cppProvider.getBuiltInIncludeDirectories()); } /** @@ -381,7 +385,7 @@ public class CppCompileAction extends AbstractAction @Override public List getBuiltInIncludeDirectories() { - return cppConfiguration.getBuiltInIncludeDirectories(); + return builtInIncludeDirectories; } @Nullable @@ -835,9 +839,10 @@ public class CppCompileAction extends AbstractAction if (optionalSourceFile != null) { allowedIncludes.add(optionalSourceFile); } - Iterable ignoreDirs = cppConfiguration.isStrictSystemIncludes() - ? cppConfiguration.getBuiltInIncludeDirectories() - : getValidationIgnoredDirs(); + Iterable ignoreDirs = + cppConfiguration.isStrictSystemIncludes() + ? getBuiltInIncludeDirectories() + : getValidationIgnoredDirs(); // Copy the sets to hash sets for fast contains checking. // Avoid immutable sets here to limit memory churn. @@ -906,7 +911,7 @@ public class CppCompileAction extends AbstractAction } Iterable getValidationIgnoredDirs() { - List cxxSystemIncludeDirs = cppConfiguration.getBuiltInIncludeDirectories(); + List cxxSystemIncludeDirs = getBuiltInIncludeDirectories(); return Iterables.concat( cxxSystemIncludeDirs, context.getSystemIncludeDirs()); } @@ -1259,9 +1264,8 @@ public class CppCompileAction extends AbstractAction } public List getPermittedSystemIncludePrefixes(Path execRoot) { - CppConfiguration toolchain = cppConfiguration; List systemIncludePrefixes = new ArrayList<>(); - for (PathFragment includePath : toolchain.getBuiltInIncludeDirectories()) { + for (PathFragment includePath : getBuiltInIncludeDirectories()) { if (includePath.isAbsolute()) { systemIncludePrefixes.add(execRoot.getFileSystem().getPath(includePath)); } 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 333361f193..596bd3af2a 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 @@ -384,8 +384,8 @@ public class CppCompileActionBuilder { ImmutableList.copyOf(copts), getNocoptPredicate(nocopts), getLipoScannables(realMandatoryInputs), - ccToolchain.getBuiltinIncludeFiles(), cppSemantics, + ccToolchain, ImmutableMap.copyOf(executionInfo)); } else { return new CppCompileAction( @@ -421,8 +421,8 @@ public class CppCompileActionBuilder { ImmutableMap.copyOf(executionInfo), ImmutableMap.copyOf(environment), getActionName(), - ccToolchain.getBuiltinIncludeFiles(), - cppSemantics); + cppSemantics, + ccToolchain); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java index 4388571311..765f4d6f6d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java @@ -25,12 +25,10 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; import com.google.common.collect.Maps; -import com.google.devtools.build.lib.analysis.RedirectChaser; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.CompilationMode; -import com.google.devtools.build.lib.analysis.config.ConfigurationEnvironment; import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; import com.google.devtools.build.lib.analysis.config.PatchTransition; import com.google.devtools.build.lib.analysis.config.PerLabelOptions; @@ -60,7 +58,6 @@ import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CTool import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.LinkingModeFlags; import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.LipoMode; import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.ToolPath; -import com.google.devtools.common.options.OptionsParsingException; import com.google.protobuf.TextFormat; import com.google.protobuf.TextFormat.ParseException; import java.io.Serializable; @@ -162,127 +159,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment { } } - /** Storage for the libc label, if given. */ - public static class LibcTop implements Serializable { - private final Label label; - - /** - * Result of trying to create LibcTop. Bundles whether or not it still needs values that aren't - * loaded yet. - */ - public static class Result { - private final LibcTop libcTop; - private final boolean valuesMissing; - - Result(LibcTop libcTop, boolean valuesMissing) { - this.libcTop = libcTop; - this.valuesMissing = valuesMissing; - } - - @Nullable - public LibcTop getLibcTop() { - return libcTop; - } - - public boolean valuesMissing() { - return valuesMissing; - } - }; - - /** Tries to create a LibcTop object and returns whether or not all were any missing values. */ - public static LibcTop.Result createLibcTop( - CppOptions cppOptions, ConfigurationEnvironment env, CrosstoolConfig.CToolchain toolchain) - throws InterruptedException, InvalidConfigurationException { - Preconditions.checkArgument(env != null); - - PathFragment defaultSysroot = - toolchain.getBuiltinSysroot().length() == 0 - ? null - : PathFragment.create(toolchain.getBuiltinSysroot()); - if ((defaultSysroot != null) && !defaultSysroot.isNormalized()) { - throw new InvalidConfigurationException( - "The built-in sysroot '" + defaultSysroot + "' is not normalized."); - } - - if ((cppOptions.libcTopLabel != null) && (defaultSysroot == null)) { - throw new InvalidConfigurationException( - "The selected toolchain " - + toolchain.getToolchainIdentifier() - + " does not support setting --grte_top."); - } - - LibcTop libcTop = null; - if (cppOptions.libcTopLabel != null) { - libcTop = createLibcTop(cppOptions.libcTopLabel, env); - if (libcTop == null) { - return new Result(libcTop, true); - } - } else if (!toolchain.getDefaultGrteTop().isEmpty()) { - Label grteTopLabel = null; - try { - grteTopLabel = - new CppOptions.LibcTopLabelConverter().convert(toolchain.getDefaultGrteTop()); - } catch (OptionsParsingException e) { - throw new InvalidConfigurationException(e.getMessage(), e); - } - - libcTop = createLibcTop(grteTopLabel, env); - if (libcTop == null) { - return new Result(libcTop, true); - } - } - return new Result(libcTop, false); - } - - @Nullable - private static LibcTop createLibcTop(Label label, ConfigurationEnvironment env) - throws InvalidConfigurationException, InterruptedException { - Preconditions.checkArgument(label != null); - Label trueLabel = label; - // Allow the sysroot to follow any redirects. - trueLabel = RedirectChaser.followRedirects(env, label, "libc_top"); - if (trueLabel == null) { - // Happens if there's a reference to package that hasn't been loaded yet. - return null; - } - return new LibcTop(trueLabel); - } - - private LibcTop(Label label) { - Preconditions.checkArgument(label != null); - this.label = label; - } - - public Label getLabel() { - return label; - } - - public PathFragment getSysroot() { - return label.getPackageFragment(); - } - - @Override - public String toString() { - return label.toString(); - } - - @Override - public boolean equals(Object other) { - if (this == other) { - return true; - } else if (other instanceof LibcTop) { - return label.equals(((LibcTop) other).label); - } else { - return false; - } - } - - @Override - public int hashCode() { - return label.hashCode(); - } - } - /** * This macro will be passed as a command-line parameter (eg. -DBUILD_FDO_TYPE="LIPO"). * For possible values see {@code CppModel.getFdoBuildStamp()}. @@ -381,15 +257,17 @@ public class CppConfiguration extends BuildConfiguration.Fragment { // TODO(bazel-team): All these labels (except for ccCompilerRuleLabel) can be removed once the // transition to the cc_compiler rule is complete. - private final Label libcLabel; private final Label staticRuntimeLibsLabel; private final Label dynamicRuntimeLibsLabel; private final Label ccToolchainLabel; private final Label stlLabel; - private final PathFragment sysroot; + // TODO(kmensah): This is temporary until all the Skylark functions that need this can be removed. + private final PathFragment nonConfiguredSysroot; + private final Label sysrootLabel; + private final PathFragment defaultSysroot; private final PathFragment runtimeSysroot; - private final ImmutableList builtInIncludeDirectories; + private final ImmutableList rawBuiltInIncludeDirectories; private final Map toolPaths; private final PathFragment ldExecutable; @@ -614,29 +492,14 @@ public class CppConfiguration extends BuildConfiguration.Fragment { this.abiGlibcVersion = toolchain.getAbiLibcVersion(); // The default value for optional string attributes is the empty string. - PathFragment defaultSysroot = toolchain.getBuiltinSysroot().length() == 0 - ? null - : PathFragment.create(toolchain.getBuiltinSysroot()); - if ((defaultSysroot != null) && !defaultSysroot.isNormalized()) { - throw new IllegalArgumentException("The built-in sysroot '" + defaultSysroot - + "' is not normalized."); - } + this.defaultSysroot = computeDefaultSysroot(toolchain); - LibcTop libcTop = params.libcTop; - if ((libcTop != null) && (libcTop.getLabel() != null)) { - libcLabel = libcTop.getLabel(); - } else { - libcLabel = null; - } + this.sysrootLabel = params.sysrootLabel; + this.nonConfiguredSysroot = + params.sysrootLabel == null ? defaultSysroot : params.sysrootLabel.getPackageFragment(); - ImmutableList.Builder builtInIncludeDirectoriesBuilder - = ImmutableList.builder(); - sysroot = libcTop == null ? defaultSysroot : libcTop.getSysroot(); - for (String s : toolchain.getCxxBuiltinIncludeDirectoryList()) { - builtInIncludeDirectoriesBuilder.add( - resolveIncludeDir(s, sysroot, crosstoolTopPathFragment)); - } - builtInIncludeDirectories = builtInIncludeDirectoriesBuilder.build(); + rawBuiltInIncludeDirectories = + ImmutableList.copyOf(toolchain.getCxxBuiltinIncludeDirectoryList()); // The runtime sysroot should really be set from --grte_top. However, currently libc has no // way to set the sysroot. The CROSSTOOL file does set the runtime sysroot, in the @@ -644,17 +507,8 @@ public class CppConfiguration extends BuildConfiguration.Fragment { // and libc versions, you must always choose compatible ones. runtimeSysroot = defaultSysroot; - String sysrootFlag; - if (sysroot != null) { - sysrootFlag = "--sysroot=" + sysroot; - } else { - sysrootFlag = null; - } - ImmutableList.Builder unfilteredCoptsBuilder = ImmutableList.builder(); - if (sysrootFlag != null) { - unfilteredCoptsBuilder.add(sysrootFlag); - } + unfilteredCoptsBuilder.addAll(toolchain.getUnfilteredCxxFlagList()); unfilteredCompilerFlags = new FlagList( unfilteredCoptsBuilder.build(), @@ -666,9 +520,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment { if (cppOptions.experimentalOmitfp) { linkoptsBuilder.add("-Wl,--eh-frame-hdr"); } - if (sysrootFlag != null) { - linkoptsBuilder.add(sysrootFlag); - } this.linkOptions = linkoptsBuilder.build(); ImmutableList.Builder ltoindexoptsBuilder = ImmutableList.builder(); @@ -730,12 +581,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment { for (CrosstoolConfig.MakeVariable variable : toolchain.getMakeVariableList()) { makeVariablesBuilder.put(variable.getName(), variable.getValue()); } - // TODO(kmensah): Remove once targets can depend on the cc_toolchain in skylark. - if (sysrootFlag != null) { - String ccFlags = makeVariablesBuilder.get("CC_FLAGS"); - ccFlags = ccFlags.isEmpty() ? sysrootFlag : ccFlags + " " + sysrootFlag; - makeVariablesBuilder.put("CC_FLAGS", ccFlags); - } this.additionalMakeVariables = ImmutableMap.copyOf(makeVariablesBuilder); } @@ -1277,14 +1122,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment { return toolPaths.get(tool.getNamePart()); } - /** - * Returns a label that forms a dependency to the files required for the - * sysroot that is used. - */ - public Label getLibcLabel() { - return libcLabel; - } - /** * Returns a label that references the library files needed to statically * link the C++ runtime (i.e. libgcc.a, libgcc_eh.a, libstdc++.a) for the @@ -1433,17 +1270,30 @@ public class CppConfiguration extends BuildConfiguration.Fragment { } /** - * Returns the built-in list of system include paths for the toolchain - * compiler. All paths in this list should be relative to the exec directory. - * They may be absolute if they are also installed on the remote build nodes or - * for local compilation. + * Returns the built-in list of system include paths for the toolchain compiler. All paths in this + * list should be relative to the exec directory. They may be absolute if they are also installed + * on the remote build nodes or for local compilation. */ - @SkylarkCallable(name = "built_in_include_directories", structField = true, - doc = "Built-in system include paths for the toolchain compiler. All paths in this list" - + " should be relative to the exec directory. They may be absolute if they are also installed" - + " on the remote build nodes or for local compilation.") - public ImmutableList getBuiltInIncludeDirectories() { - return builtInIncludeDirectories; + @SkylarkCallable( + name = "built_in_include_directories", + structField = true, + doc = + "Built-in system include paths for the toolchain compiler. All paths in this list" + + " should be relative to the exec directory. They may be absolute if they are also" + + " installed on the remote build nodes or for local compilation." + ) + public ImmutableList getBuiltInIncludeDirectories() + throws InvalidConfigurationException { + return getBuiltInIncludeDirectories(nonConfiguredSysroot); + } + + public ImmutableList getBuiltInIncludeDirectories(PathFragment sysroot) + throws InvalidConfigurationException { + ImmutableList.Builder builtInIncludeDirectoriesBuilder = ImmutableList.builder(); + for (String s : rawBuiltInIncludeDirectories) { + builtInIncludeDirectoriesBuilder.add(resolveIncludeDir(s, sysroot, crosstoolTopPathFragment)); + } + return builtInIncludeDirectoriesBuilder.build(); } /** @@ -1456,7 +1306,11 @@ public class CppConfiguration extends BuildConfiguration.Fragment { + "different sysroots, or the sysroot is the same as the default sysroot, then " + "this method returns None.") public PathFragment getSysroot() { - return sysroot; + return nonConfiguredSysroot; + } + + public Label getSysrootLabel() { + return sysrootLabel; } /** @@ -1516,8 +1370,8 @@ public class CppConfiguration extends BuildConfiguration.Fragment { } /** - * Returns the default list of options which cannot be filtered by BUILD - * rules. These should be appended to the command line after filtering. + * Returns the default list of options which cannot be filtered by BUILD rules. These should be + * appended to the command line after filtering. */ @SkylarkCallable( name = "unfiltered_compiler_options", @@ -1526,21 +1380,56 @@ public class CppConfiguration extends BuildConfiguration.Fragment { + "rules. These should be appended to the command line after filtering." ) public ImmutableList getUnfilteredCompilerOptions(Iterable features) { - return unfilteredCompilerFlags.evaluate(features); + return getUnfilteredCompilerOptions(features, nonConfiguredSysroot); + } + + public ImmutableList getUnfilteredCompilerOptions( + Iterable features, PathFragment sysroot) { + if (sysroot == null) { + return unfilteredCompilerFlags.evaluate(features); + } else { + return ImmutableList.builder() + .add("--sysroot=" + sysroot) + .addAll(unfilteredCompilerFlags.evaluate(features)) + .build(); + } } /** - * Returns the set of command-line linker options, including any flags - * inferred from the command-line options. + * Returns the set of command-line linker options, including any flags inferred from the + * command-line options. * * @see Link */ // TODO(bazel-team): Clean up the linker options computation! - @SkylarkCallable(name = "link_options", structField = true, - doc = "Returns the set of command-line linker options, including any flags " - + "inferred from the command-line options.") + @SkylarkCallable( + name = "link_options", + structField = true, + doc = + "Returns the set of command-line linker options, including any flags " + + "inferred from the command-line options." + ) public ImmutableList getLinkOptions() { - return linkOptions; + return getLinkOptions(nonConfiguredSysroot); + } + + public ImmutableList getLinkOptions(PathFragment sysroot) { + if (sysroot == null) { + return linkOptions; + } else { + return ImmutableList.builder() + .addAll(linkOptions) + .add("--sysroot=" + sysroot) + .build(); + } + } + + public boolean hasStaticLinkOption() { + return linkOptions.contains("-static"); + } + + public boolean hasSharedLinkOption() { + return linkOptions.contains("-shared"); } /** Returns the set of command-line LTO indexing options. */ @@ -2170,6 +2059,8 @@ public class CppConfiguration extends BuildConfiguration.Fragment { // TODO(bazel-team): delete all of these. globalMakeEnvBuilder.put("CROSSTOOLTOP", crosstoolTopPathFragment.getPathString()); + // TODO(kmensah): Remove when skylark dependencies can be updated to rely on + // CcToolchainProvider. globalMakeEnvBuilder.putAll(getAdditionalMakeVariables()); globalMakeEnvBuilder.put("ABI_GLIBC_VERSION", getAbiGlibcVersion()); @@ -2252,6 +2143,22 @@ public class CppConfiguration extends BuildConfiguration.Fragment { return requestedFeatures.build(); } + public static PathFragment computeDefaultSysroot(CToolchain toolchain) { + PathFragment defaultSysroot = + toolchain.getBuiltinSysroot().length() == 0 + ? null + : PathFragment.create(toolchain.getBuiltinSysroot()); + if ((defaultSysroot != null) && !defaultSysroot.isNormalized()) { + throw new IllegalArgumentException( + "The built-in sysroot '" + defaultSysroot + "' is not normalized."); + } + return defaultSysroot; + } + + public PathFragment getDefaultSysroot() { + return defaultSysroot; + } + @Override public PatchTransition getArtifactOwnerTransition() { return isLipoContextCollector() ? ContextCollectorOwnerTransition.INSTANCE : null; diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java index 9a98181724..b334b10923 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java @@ -36,10 +36,11 @@ import com.google.devtools.build.lib.packages.NoSuchThingException; import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.Target; -import com.google.devtools.build.lib.rules.cpp.CppConfiguration.LibcTop; 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 com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig; +import com.google.devtools.common.options.OptionsParsingException; import javax.annotation.Nullable; /** @@ -89,7 +90,7 @@ public class CppConfigurationLoader implements ConfigurationFragmentFactory { protected final Label ccToolchainLabel; protected final Label stlLabel; protected final Path fdoZip; - protected final LibcTop libcTop; + protected final Label sysrootLabel; CppConfigurationParameters( CrosstoolConfig.CToolchain toolchain, @@ -99,7 +100,7 @@ public class CppConfigurationLoader implements ConfigurationFragmentFactory { Label crosstoolTop, Label ccToolchainLabel, Label stlLabel, - LibcTop libcTop) { + Label sysrootLabel) { this.toolchain = toolchain; this.cacheKeySuffix = cacheKeySuffix; this.commonOptions = buildOptions.get(BuildConfiguration.Options.class); @@ -108,7 +109,7 @@ public class CppConfigurationLoader implements ConfigurationFragmentFactory { this.crosstoolTop = crosstoolTop; this.ccToolchainLabel = ccToolchainLabel; this.stlLabel = stlLabel; - this.libcTop = libcTop; + this.sysrootLabel = sysrootLabel; } } @@ -219,10 +220,7 @@ public class CppConfigurationLoader implements ConfigurationFragmentFactory { "The label '%s' is not a cc_toolchain rule", ccToolchainLabel)); } - LibcTop.Result libcTopResult = LibcTop.createLibcTop(cppOptions, env, toolchain); - if (libcTopResult.valuesMissing()) { - return null; - } + Label sysrootLabel = getSysrootLabel(toolchain, cppOptions.libcTopLabel); return new CppConfigurationParameters( toolchain, @@ -232,6 +230,34 @@ public class CppConfigurationLoader implements ConfigurationFragmentFactory { crosstoolTopLabel, ccToolchainLabel, stlLabel, - libcTopResult.getLibcTop()); + sysrootLabel); + } + + @Nullable + public static Label getSysrootLabel(CrosstoolConfig.CToolchain toolchain, Label libcTopLabel) + throws InvalidConfigurationException { + PathFragment defaultSysroot = CppConfiguration.computeDefaultSysroot(toolchain); + + if ((libcTopLabel != null) && (defaultSysroot == null)) { + throw new InvalidConfigurationException( + "The selected toolchain " + + toolchain.getToolchainIdentifier() + + " does not support setting --grte_top."); + } + + if (libcTopLabel != null) { + return libcTopLabel; + } + + if (!toolchain.getDefaultGrteTop().isEmpty()) { + try { + Label grteTopLabel = + new CppOptions.LibcTopLabelConverter().convert(toolchain.getDefaultGrteTop()); + return grteTopLabel; + } catch (OptionsParsingException e) { + throw new InvalidConfigurationException(e.getMessage(), e); + } + } + return null; } } 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 aa719c80ed..ea3fd42ddd 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 @@ -592,7 +592,7 @@ public class CppHelper { public static void maybeAddStaticLinkMarkerProvider(RuleConfiguredTargetBuilder builder, RuleContext ruleContext) { boolean staticallyLinked = false; - if (ruleContext.getFragment(CppConfiguration.class).getLinkOptions().contains("-static")) { + if (ruleContext.getFragment(CppConfiguration.class).hasStaticLinkOption()) { staticallyLinked = true; } else if (ruleContext.attributes().has("linkopts", Type.STRING_LIST) && ruleContext.attributes().get("linkopts", Type.STRING_LIST).contains("-static")) { 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 0c640ff818..18d7102092 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 @@ -813,7 +813,7 @@ public class CppLinkActionBuilder { } else if (isLTOIndexing && allLTOArtifacts != null) { for (LTOBackendArtifacts a : allLTOArtifacts) { List argv = new ArrayList<>(); - argv.addAll(cppConfiguration.getLinkOptions()); + argv.addAll(toolchain.getLinkOptions()); argv.addAll(cppConfiguration.getCompilerOptions(features)); a.setCommandLine(argv); @@ -914,7 +914,7 @@ public class CppLinkActionBuilder { boolean sharedLinkopts = type == LinkTargetType.DYNAMIC_LIBRARY || linkopts.contains("-shared") - || cppConfig.getLinkOptions().contains("-shared"); + || cppConfig.hasSharedLinkOption(); return (isNativeDeps || cppConfig.legacyWholeArchive()) && (fullyStatic || mostlyStatic) && sharedLinkopts; 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 f35954589d..31363c3cf1 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 @@ -80,8 +80,8 @@ public class FakeCppCompileAction extends CppCompileAction { ImmutableList copts, Predicate nocopts, Iterable lipoScannables, - Iterable builtinIncludeFiles, CppSemantics cppSemantics, + CcToolchainProvider cppProvider, ImmutableMap executionInfo) { super( owner, @@ -123,8 +123,8 @@ public class FakeCppCompileAction extends CppCompileAction { executionInfo, ImmutableMap.of(), CppCompileAction.CPP_COMPILE, - builtinIncludeFiles, - cppSemantics); + cppSemantics, + cppProvider); this.tempOutputFile = Preconditions.checkNotNull(tempOutputFile); } 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 bbb3b4ddb8..491e481322 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 @@ -74,6 +74,7 @@ public final class LinkCommandLine extends CommandLine { @Nullable private final PathFragment runtimeSolibDir; private final boolean nativeDeps; private final boolean useTestOnlyFlags; + private final CcToolchainProvider ccProvider; @Nullable private final Artifact paramFile; @@ -98,7 +99,8 @@ public final class LinkCommandLine extends CommandLine { boolean useTestOnlyFlags, @Nullable Artifact paramFile, CcToolchainFeatures.Variables variables, - @Nullable FeatureConfiguration featureConfiguration) { + @Nullable FeatureConfiguration featureConfiguration, + CcToolchainProvider ccProvider) { this.actionName = actionName; this.toolPath = toolPath; @@ -125,6 +127,7 @@ public final class LinkCommandLine extends CommandLine { this.nativeDeps = nativeDeps; this.useTestOnlyFlags = useTestOnlyFlags; this.paramFile = paramFile; + this.ccProvider = ccProvider; } @Nullable @@ -322,7 +325,7 @@ public final class LinkCommandLine extends CommandLine { boolean sharedLinkopts = linkTargetType == LinkTargetType.DYNAMIC_LIBRARY || linkopts.contains("-shared") - || cppConfiguration.getLinkOptions().contains("-shared"); + || cppConfiguration.hasSharedLinkOption(); List toolchainFlags = new ArrayList<>(); @@ -353,7 +356,7 @@ public final class LinkCommandLine extends CommandLine { toolchainFlags.addAll(cppConfiguration.getTestOnlyLinkOptions()); } - toolchainFlags.addAll(cppConfiguration.getLinkOptions()); + toolchainFlags.addAll(ccProvider.getLinkOptions()); // -pie is not compatible with shared and should be // removed when the latter is part of the link command. Should we need to further @@ -572,7 +575,7 @@ public final class LinkCommandLine extends CommandLine { optionList.add("-I."); // Add sysroot. - PathFragment sysroot = cppConfiguration.getSysroot(); + PathFragment sysroot = ccProvider.getSysroot(); if (sysroot != null) { optionList.add("--sysroot=" + sysroot.getPathString()); } @@ -580,7 +583,7 @@ public final class LinkCommandLine extends CommandLine { // Add toolchain compiler options. optionList.addAll(cppConfiguration.getCompilerOptions(features)); optionList.addAll(cppConfiguration.getCOptions()); - optionList.addAll(cppConfiguration.getUnfilteredCompilerOptions(features)); + optionList.addAll(ccProvider.getUnfilteredCompilerOptions(features)); if (CppFileTypes.CPP_SOURCE.matches(linkstamp.getKey().getExecPath())) { optionList.addAll(cppConfiguration.getCxxOptions(features)); } @@ -634,7 +637,7 @@ public final class LinkCommandLine extends CommandLine { private final BuildConfiguration configuration; private final ActionOwner owner; - @Nullable private final RuleContext ruleContext; + private final RuleContext ruleContext; @Nullable private String toolPath; @Nullable private Artifact output; @@ -651,7 +654,7 @@ public final class LinkCommandLine extends CommandLine { private boolean nativeDeps; private boolean useTestOnlyFlags; @Nullable private Artifact paramFile; - @Nullable private CcToolchainProvider toolchain; + private CcToolchainProvider toolchain; private FdoSupport fdoSupport; private Variables variables; private FeatureConfiguration featureConfiguration; @@ -659,8 +662,7 @@ public final class LinkCommandLine extends CommandLine { // 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, ActionOwner owner, @Nullable RuleContext ruleContext) { + public Builder(BuildConfiguration configuration, ActionOwner owner, RuleContext ruleContext) { this.configuration = configuration; this.owner = owner; this.ruleContext = ruleContext; @@ -689,17 +691,16 @@ public final class LinkCommandLine extends CommandLine { Iterables.concat(DEFAULT_LINKSTAMP_OPTIONS, linkstampCompileOptions)); } + if (toolchain == null) { + toolchain = + Preconditions.checkNotNull(CppHelper.getToolchain(ruleContext, ":cc_toolchain")); + } + // The ruleContext can be null for some tests. if (ruleContext != null) { if (featureConfiguration == null) { - if (toolchain != null) { - featureConfiguration = - CcCommon.configureFeatures( - ruleContext, toolchain, CcLibraryHelper.SourceCategory.CC); - } else { - CcToolchainProvider ccToolchain = CppHelper.getToolchain(ruleContext, ":cc_toolchain"); - featureConfiguration = CcCommon.configureFeatures(ruleContext, ccToolchain); - } + featureConfiguration = + CcCommon.configureFeatures(ruleContext, toolchain, CcLibraryHelper.SourceCategory.CC); } if (fdoSupport == null) { @@ -734,7 +735,8 @@ public final class LinkCommandLine extends CommandLine { useTestOnlyFlags, paramFile, variables, - featureConfiguration); + featureConfiguration, + toolchain); } /** -- cgit v1.2.3