Skip to content

Commit

Permalink
Automated rollback of commit c4fc620.
Browse files Browse the repository at this point in the history 
*** Reason for rollback ***

Caused a memory regression

*** Original change description ***

Expose cc_common.create_compile_build_variables

This cl enabled skylark rules to create build variables used for C++ compile
actions (in a limited form, e.g. build variables for modules are not exposed
etc).

Working towards #4571.

RELNOTES: None
PiperOrigin-RevId: 196566686
  • Loading branch information
mstaib authored and Copybara-Service committed May 14, 2018
1 parent 17fb867 commit 2c493e8
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 89 deletions.
16 changes: 6 additions & 10 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1517,16 +1517,16 @@ private CcToolchainVariables setupCompileBuildVariables(
ruleContext,
featureConfiguration,
ccToolchain,
toPathString(sourceFile),
toPathString(builder.getOutputFile()),
toPathString(gcnoFile),
toPathString(dwoFile),
toPathString(ltoIndexingFile),
sourceFile,
builder.getOutputFile(),
gcnoFile,
dwoFile,
ltoIndexingFile,
ImmutableList.of(),
userCompileFlags.build(),
cppModuleMap,
usePic,
builder.getTempOutputFile(),
builder.getRealOutputFilePath(),
CppHelper.getFdoBuildStamp(ruleContext, fdoSupport.getFdoSupport()),
dotdFileExecPath,
ImmutableList.copyOf(variablesExtensions),
Expand All @@ -1538,10 +1538,6 @@ private CcToolchainVariables setupCompileBuildVariables(
ccCompilationContext.getDefines());
}

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

/**
* Returns a {@code CppCompileActionBuilder} with the common fields for a C++ compile action being
* initialized.
Expand Down
134 changes: 59 additions & 75 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariables.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.util.FileType;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Collection;

/** Enum covering all build variables we create for all various {@link CppCompileAction}. */
public enum CompileBuildVariables {
Expand Down Expand Up @@ -107,24 +108,24 @@ public static CcToolchainVariables setupVariablesOrReportRuleError(
RuleContext ruleContext,
FeatureConfiguration featureConfiguration,
CcToolchainProvider ccToolchainProvider,
String sourceFile,
String outputFile,
String gcnoFile,
String dwoFile,
String ltoIndexingFile,
Artifact sourceFile,
Artifact outputFile,
Artifact gcnoFile,
Artifact dwoFile,
Artifact ltoIndexingFile,
ImmutableList<String> includes,
Iterable<String> userCompileFlags,
ImmutableList<String> userCompileFlags,
CppModuleMap cppModuleMap,
boolean usePic,
PathFragment fakeOutputFile,
PathFragment realOutputFilePath,
String fdoStamp,
String dotdFileExecPath,
ImmutableList<VariablesExtension> variablesExtensions,
ImmutableMap<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
Iterable<PathFragment> includeDirs,
Iterable<PathFragment> quoteIncludeDirs,
Iterable<PathFragment> systemIncludeDirs,
Collection<PathFragment> includeDirs,
Collection<PathFragment> quoteIncludeDirs,
Collection<PathFragment> systemIncludeDirs,
Iterable<String> defines) {
try {
return setupVariablesOrThrowEvalException(
Expand All @@ -139,20 +140,16 @@ public static CcToolchainVariables setupVariablesOrReportRuleError(
userCompileFlags,
cppModuleMap,
usePic,
toPathString(fakeOutputFile),
realOutputFilePath,
fdoStamp,
dotdFileExecPath,
variablesExtensions,
additionalBuildVariables,
directModuleMaps,
getSafePathStrings(includeDirs),
getSafePathStrings(quoteIncludeDirs),
getSafePathStrings(systemIncludeDirs),
defines,
/* addLegacyCxxOptions= */ CppFileTypes.CPP_SOURCE.matches(sourceFile)
|| CppFileTypes.CPP_HEADER.matches(sourceFile)
|| CppFileTypes.CPP_MODULE_MAP.matches(sourceFile)
|| CppFileTypes.CLIF_INPUT_PROTO.matches(sourceFile));
includeDirs,
quoteIncludeDirs,
systemIncludeDirs,
defines);
} catch (EvalException e) {
ruleContext.ruleError(e.getMessage());
return CcToolchainVariables.EMPTY;
Expand All @@ -162,28 +159,25 @@ public static CcToolchainVariables setupVariablesOrReportRuleError(
public static CcToolchainVariables setupVariablesOrThrowEvalException(
FeatureConfiguration featureConfiguration,
CcToolchainProvider ccToolchainProvider,
String sourceFile,
// TODO(b/76195763): Remove once blaze with cl/189769259 is released and crosstools are
// updated.
String outputFile,
String gcnoFile,
String dwoFile,
String ltoIndexingFile,
Artifact sourceFile,
Artifact outputFile,
Artifact gcnoFile,
Artifact dwoFile,
Artifact ltoIndexingFile,
ImmutableList<String> includes,
Iterable<String> userCompileFlags,
ImmutableList<String> userCompileFlags,
CppModuleMap cppModuleMap,
boolean usePic,
String fakeOutputFile,
PathFragment realOutputFilePath,
String fdoStamp,
String dotdFileExecPath,
ImmutableList<VariablesExtension> variablesExtensions,
ImmutableMap<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
Iterable<String> includeDirs,
Iterable<String> quoteIncludeDirs,
Iterable<String> systemIncludeDirs,
Iterable<String> defines,
boolean addLegacyCxxOptions)
Iterable<PathFragment> includeDirs,
Iterable<PathFragment> quoteIncludeDirs,
Iterable<PathFragment> systemIncludeDirs,
Iterable<String> defines)
throws EvalException {
Preconditions.checkNotNull(directModuleMaps);
Preconditions.checkNotNull(includeDirs);
Expand All @@ -196,42 +190,34 @@ public static CcToolchainVariables setupVariablesOrThrowEvalException(
buildVariables.addStringSequenceVariable(
USER_COMPILE_FLAGS.getVariableName(), userCompileFlags);

buildVariables.addStringVariable(SOURCE_FILE.getVariableName(), sourceFile.getExecPathString());

String sourceFilename = sourceFile.getExecPathString();
buildVariables.addLazyStringSequenceVariable(
LEGACY_COMPILE_FLAGS.getVariableName(),
getLegacyCompileFlagsSupplier(ccToolchainProvider, addLegacyCxxOptions));
getLegacyCompileFlagsSupplier(ccToolchainProvider, sourceFilename));

if (sourceFile != null) {
buildVariables.addStringVariable(SOURCE_FILE.getVariableName(), sourceFile);
}

if (sourceFile == null
|| (!CppFileTypes.OBJC_SOURCE.matches(sourceFile)
&& !CppFileTypes.OBJCPP_SOURCE.matches(sourceFile))) {
if (!CppFileTypes.OBJC_SOURCE.matches(sourceFilename)
&& !CppFileTypes.OBJCPP_SOURCE.matches(sourceFilename)) {
buildVariables.addLazyStringSequenceVariable(
UNFILTERED_COMPILE_FLAGS.getVariableName(),
getUnfilteredCompileFlagsSupplier(ccToolchainProvider));
}

String fakeOutputFileOrRealOutputFile = fakeOutputFile != null ? fakeOutputFile : outputFile;

if (outputFile != null) {
// TODO(b/76195763): Remove once blaze with cl/189769259 is released and crosstools are
// updated.
if (!FileType.contains(
PathFragment.create(outputFile),
CppFileTypes.ASSEMBLER,
CppFileTypes.PIC_ASSEMBLER,
CppFileTypes.PREPROCESSED_C,
CppFileTypes.PREPROCESSED_CPP,
CppFileTypes.PIC_PREPROCESSED_C,
CppFileTypes.PIC_PREPROCESSED_CPP)) {
buildVariables.addStringVariable(
OUTPUT_OBJECT_FILE.getVariableName(), fakeOutputFileOrRealOutputFile);
}

// TODO(b/76195763): Remove once blaze with cl/189769259 is released and crosstools are updated.
if (!FileType.contains(
outputFile,
CppFileTypes.ASSEMBLER,
CppFileTypes.PIC_ASSEMBLER,
CppFileTypes.PREPROCESSED_C,
CppFileTypes.PREPROCESSED_CPP,
CppFileTypes.PIC_PREPROCESSED_C,
CppFileTypes.PIC_PREPROCESSED_CPP)) {
buildVariables.addStringVariable(
OUTPUT_FILE.getVariableName(), fakeOutputFileOrRealOutputFile);
OUTPUT_OBJECT_FILE.getVariableName(), realOutputFilePath.getSafePathString());
}
buildVariables.addStringVariable(
OUTPUT_FILE.getVariableName(), realOutputFilePath.getSafePathString());

// Set dependency_file to enable <object>.d file generation.
if (dotdFileExecPath != null) {
Expand All @@ -255,11 +241,12 @@ public static CcToolchainVariables setupVariablesOrThrowEvalException(
buildVariables.addStringSequenceVariable(MODULE_FILES.getVariableName(), ImmutableSet.of());
}
if (featureConfiguration.isEnabled(CppRuleClasses.INCLUDE_PATHS)) {
buildVariables.addStringSequenceVariable(INCLUDE_PATHS.getVariableName(), includeDirs);
buildVariables.addStringSequenceVariable(
QUOTE_INCLUDE_PATHS.getVariableName(), quoteIncludeDirs);
INCLUDE_PATHS.getVariableName(), getSafePathStrings(includeDirs));
buildVariables.addStringSequenceVariable(
QUOTE_INCLUDE_PATHS.getVariableName(), getSafePathStrings(quoteIncludeDirs));
buildVariables.addStringSequenceVariable(
SYSTEM_INCLUDE_PATHS.getVariableName(), systemIncludeDirs);
SYSTEM_INCLUDE_PATHS.getVariableName(), getSafePathStrings(systemIncludeDirs));
}

if (!includes.isEmpty()) {
Expand Down Expand Up @@ -290,16 +277,18 @@ public static CcToolchainVariables setupVariablesOrThrowEvalException(
}

if (gcnoFile != null) {
buildVariables.addStringVariable(GCOV_GCNO_FILE.getVariableName(), gcnoFile);
buildVariables.addStringVariable(
GCOV_GCNO_FILE.getVariableName(), gcnoFile.getExecPathString());
}

if (dwoFile != null) {
buildVariables.addStringVariable(PER_OBJECT_DEBUG_INFO_FILE.getVariableName(), dwoFile);
buildVariables.addStringVariable(
PER_OBJECT_DEBUG_INFO_FILE.getVariableName(), dwoFile.getExecPathString());
}

if (ltoIndexingFile != null) {
buildVariables.addStringVariable(
LTO_INDEXING_BITCODE_FILE.getVariableName(), ltoIndexingFile);
LTO_INDEXING_BITCODE_FILE.getVariableName(), ltoIndexingFile.getExecPathString());
}

buildVariables.addAllStringVariables(additionalBuildVariables);
Expand All @@ -326,11 +315,14 @@ private static ImmutableSet<String> getSafePathStrings(Iterable<PathFragment> pa
* to arguments (to prevent accidental capture of enclosing instance which could regress memory).
*/
private static Supplier<ImmutableList<String>> getLegacyCompileFlagsSupplier(
CcToolchainProvider toolchain, boolean addLegacyCxxOptions) {
CcToolchainProvider toolchain, String sourceFilename) {
return () -> {
ImmutableList.Builder<String> legacyCompileFlags = ImmutableList.builder();
legacyCompileFlags.addAll(toolchain.getLegacyCompileOptions());
if (addLegacyCxxOptions) {
if (CppFileTypes.CPP_SOURCE.matches(sourceFilename)
|| CppFileTypes.CPP_HEADER.matches(sourceFilename)
|| CppFileTypes.CPP_MODULE_MAP.matches(sourceFilename)
|| CppFileTypes.CLIF_INPUT_PROTO.matches(sourceFilename)) {
legacyCompileFlags.addAll(toolchain.getLegacyCxxOptions());
}
return legacyCompileFlags.build();
Expand All @@ -348,14 +340,6 @@ private static Supplier<ImmutableList<String>> getUnfilteredCompileFlagsSupplier
return () -> ccToolchain.getUnfilteredCompilerOptions();
}

private static String toPathString(PathFragment a) {
if (a == null) {
return null;
}

return a.getSafePathString();
}

public String getVariableName() {
return variableName;
}
Expand Down
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,6 @@ public static String getCppActionConfigs(
" action: 'c++-module-codegen'",
" action: 'c++-module-compile'",
" flag_group {",
" expand_if_all_available: 'output_file'",
" flag: '-frandom-seed=%{output_file}'",
" }",
" }",
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkstampCompileHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,8 @@ private static CcToolchainVariables getVariables(
ruleContext,
featureConfiguration,
ccToolchainProvider,
sourceFile.getExecPathString(),
outputFile.getExecPathString(),
sourceFile,
outputFile,
/* gcnoFile= */ null,
/* dwoFile= */ null,
/* ltoIndexingFile= */ null,
Expand All @@ -158,7 +158,7 @@ private static CcToolchainVariables getVariables(
CcCompilationHelper.getCoptsFromOptions(cppConfiguration, sourceFile.getExecPathString()),
/* cppModuleMap= */ null,
needsPic,
/* fakeOutputFile= */ null,
outputFile.getExecPath(),
fdoBuildStamp,
/* dotdFileExecPath= */ null,
/* variablesExtensions= */ ImmutableList.of(),
Expand Down

0 comments on commit 2c493e8

Please sign in to comment.