Skip to content

Commit

Permalink
Expose cc_common.create_compile_build_variables
Browse files Browse the repository at this point in the history 
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 bazelbuild#4571.

This is an encore of bazelbuild@c4fc620 after fixing memory regression
(CompileBuildVariables.getSafePathStrings fixed to return ImmutableList, which
has smaller memory footprint).

RELNOTES: None
PiperOrigin-RevId: 196797581
  • Loading branch information
@hlopko
hlopko authored and Copybara-Service committed May 16, 2018
1 parent e440d59 commit 280175d
Show file tree
Hide file tree
Showing 4 changed files with 92 additions and 74 deletions.
16 changes: 10 additions & 6 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 @@ -1509,16 +1509,16 @@ private CcToolchainVariables setupCompileBuildVariables(
ruleContext,
featureConfiguration,
ccToolchain,
sourceFile,
builder.getOutputFile(),
gcnoFile,
dwoFile,
ltoIndexingFile,
toPathString(sourceFile),
toPathString(builder.getOutputFile()),
toPathString(gcnoFile),
toPathString(dwoFile),
toPathString(ltoIndexingFile),
ImmutableList.of(),
userCompileFlags.build(),
cppModuleMap,
usePic,
builder.getRealOutputFilePath(),
builder.getTempOutputFile(),
CppHelper.getFdoBuildStamp(ruleContext, fdoSupport.getFdoSupport()),
dotdFileExecPath,
ImmutableList.copyOf(variablesExtensions),
Expand All @@ -1530,6 +1530,10 @@ 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
143 changes: 78 additions & 65 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,7 +27,6 @@
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 @@ -108,24 +107,24 @@ public static CcToolchainVariables setupVariablesOrReportRuleError(
RuleContext ruleContext,
FeatureConfiguration featureConfiguration,
CcToolchainProvider ccToolchainProvider,
Artifact sourceFile,
Artifact outputFile,
Artifact gcnoFile,
Artifact dwoFile,
Artifact ltoIndexingFile,
String sourceFile,
String outputFile,
String gcnoFile,
String dwoFile,
String ltoIndexingFile,
ImmutableList<String> includes,
ImmutableList<String> userCompileFlags,
Iterable<String> userCompileFlags,
CppModuleMap cppModuleMap,
boolean usePic,
PathFragment realOutputFilePath,
PathFragment fakeOutputFile,
String fdoStamp,
String dotdFileExecPath,
ImmutableList<VariablesExtension> variablesExtensions,
ImmutableMap<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
Collection<PathFragment> includeDirs,
Collection<PathFragment> quoteIncludeDirs,
Collection<PathFragment> systemIncludeDirs,
Iterable<PathFragment> includeDirs,
Iterable<PathFragment> quoteIncludeDirs,
Iterable<PathFragment> systemIncludeDirs,
Iterable<String> defines) {
try {
return setupVariablesOrThrowEvalException(
Expand All @@ -140,16 +139,20 @@ public static CcToolchainVariables setupVariablesOrReportRuleError(
userCompileFlags,
cppModuleMap,
usePic,
realOutputFilePath,
toPathString(fakeOutputFile),
fdoStamp,
dotdFileExecPath,
variablesExtensions,
additionalBuildVariables,
directModuleMaps,
includeDirs,
quoteIncludeDirs,
systemIncludeDirs,
defines);
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));
} catch (EvalException e) {
ruleContext.ruleError(e.getMessage());
return CcToolchainVariables.EMPTY;
Expand All @@ -159,25 +162,28 @@ public static CcToolchainVariables setupVariablesOrReportRuleError(
public static CcToolchainVariables setupVariablesOrThrowEvalException(
FeatureConfiguration featureConfiguration,
CcToolchainProvider ccToolchainProvider,
Artifact sourceFile,
Artifact outputFile,
Artifact gcnoFile,
Artifact dwoFile,
Artifact ltoIndexingFile,
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,
ImmutableList<String> includes,
ImmutableList<String> userCompileFlags,
Iterable<String> userCompileFlags,
CppModuleMap cppModuleMap,
boolean usePic,
PathFragment realOutputFilePath,
String fakeOutputFile,
String fdoStamp,
String dotdFileExecPath,
ImmutableList<VariablesExtension> variablesExtensions,
ImmutableMap<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
Iterable<PathFragment> includeDirs,
Iterable<PathFragment> quoteIncludeDirs,
Iterable<PathFragment> systemIncludeDirs,
Iterable<String> defines)
Iterable<String> includeDirs,
Iterable<String> quoteIncludeDirs,
Iterable<String> systemIncludeDirs,
Iterable<String> defines,
boolean addLegacyCxxOptions)
throws EvalException {
Preconditions.checkNotNull(directModuleMaps);
Preconditions.checkNotNull(includeDirs);
Expand All @@ -190,34 +196,42 @@ 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, sourceFilename));
getLegacyCompileFlagsSupplier(ccToolchainProvider, addLegacyCxxOptions));

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

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

// 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)) {
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);
}

buildVariables.addStringVariable(
OUTPUT_OBJECT_FILE.getVariableName(), realOutputFilePath.getSafePathString());
OUTPUT_FILE.getVariableName(), fakeOutputFileOrRealOutputFile);
}
buildVariables.addStringVariable(
OUTPUT_FILE.getVariableName(), realOutputFilePath.getSafePathString());

// Set dependency_file to enable <object>.d file generation.
if (dotdFileExecPath != null) {
Expand All @@ -241,12 +255,11 @@ 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(
INCLUDE_PATHS.getVariableName(), getSafePathStrings(includeDirs));
QUOTE_INCLUDE_PATHS.getVariableName(), quoteIncludeDirs);
buildVariables.addStringSequenceVariable(
QUOTE_INCLUDE_PATHS.getVariableName(), getSafePathStrings(quoteIncludeDirs));
buildVariables.addStringSequenceVariable(
SYSTEM_INCLUDE_PATHS.getVariableName(), getSafePathStrings(systemIncludeDirs));
SYSTEM_INCLUDE_PATHS.getVariableName(), systemIncludeDirs);
}

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

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

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

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

buildVariables.addAllStringVariables(additionalBuildVariables);
Expand All @@ -300,12 +311,13 @@ public static CcToolchainVariables setupVariablesOrThrowEvalException(
}

/** Get the safe path strings for a list of paths to use in the build variables. */
private static ImmutableSet<String> getSafePathStrings(Iterable<PathFragment> paths) {
ImmutableSet.Builder<String> result = ImmutableSet.builder();
for (PathFragment path : paths) {
result.add(path.getSafePathString());
}
return result.build();
private static ImmutableList<String> getSafePathStrings(Iterable<PathFragment> paths) {
// Using ImmutableSet first to remove duplicates, then ImmutableList for smaller memory
// footprint.
return ImmutableSet.copyOf(paths)
.stream()
.map(PathFragment::getSafePathString)
.collect(ImmutableList.toImmutableList());
}

/**
Expand All @@ -315,14 +327,11 @@ 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, String sourceFilename) {
CcToolchainProvider toolchain, boolean addLegacyCxxOptions) {
return () -> {
ImmutableList.Builder<String> legacyCompileFlags = ImmutableList.builder();
legacyCompileFlags.addAll(toolchain.getLegacyCompileOptions());
if (CppFileTypes.CPP_SOURCE.matches(sourceFilename)
|| CppFileTypes.CPP_HEADER.matches(sourceFilename)
|| CppFileTypes.CPP_MODULE_MAP.matches(sourceFilename)
|| CppFileTypes.CLIF_INPUT_PROTO.matches(sourceFilename)) {
if (addLegacyCxxOptions) {
legacyCompileFlags.addAll(toolchain.getLegacyCxxOptions());
}
return legacyCompileFlags.build();
Expand All @@ -340,6 +349,10 @@ private static Supplier<ImmutableList<String>> getUnfilteredCompileFlagsSupplier
return () -> ccToolchain.getUnfilteredCompilerOptions();
}

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

public String getVariableName() {
return variableName;
}
Expand Down
1 change: 1 addition & 0 deletions 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,6 +254,7 @@ 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,
outputFile,
sourceFile.getExecPathString(),
outputFile.getExecPathString(),
/* gcnoFile= */ null,
/* dwoFile= */ null,
/* ltoIndexingFile= */ null,
Expand All @@ -158,7 +158,7 @@ private static CcToolchainVariables getVariables(
CcCompilationHelper.getCoptsFromOptions(cppConfiguration, sourceFile.getExecPathString()),
/* cppModuleMap= */ null,
needsPic,
outputFile.getExecPath(),
/* fakeOutputFile= */ null,
fdoBuildStamp,
/* dotdFileExecPath= */ null,
/* variablesExtensions= */ ImmutableList.of(),
Expand Down

0 comments on commit 280175d

Please sign in to comment.