From a22fb12669743bdfb78dfbf71a63db898f026dce Mon Sep 17 00:00:00 2001 From: hlopko Date: Fri, 15 Sep 2017 16:47:31 +0200 Subject: Automated rollback of commit f26e8694ae78599b3e2004e3360eaf3443fa53a6. *** Reason for rollback *** Breaks clang_tidy. *** Original change description *** Introduce -c source_file -o output_file build variables Prior to this cl CompileCommandLine would (almost) unconditionally emit -c and -o flags. This cl removes this logic and relies on crosstool to emit these flags. This is another small step towards platform independent C++ rules. Memory use is not affected, since the build variables used by this cl are already exposed, this cl just forces crosstools to use it. RELNOTES: None. PiperOrigin-RevId: 168834576 --- .../devtools/build/lib/actions/CommandAction.java | 2 - .../devtools/build/lib/rules/cpp/CcCommon.java | 1 + .../build/lib/rules/cpp/CompileCommandLine.java | 45 +++++++++++-- .../build/lib/rules/cpp/CppActionConfigs.java | 76 ---------------------- .../build/lib/rules/cpp/CppCompileAction.java | 28 +++++--- .../build/lib/rules/cpp/CppRuleClasses.java | 7 ++ .../build/lib/rules/cpp/FakeCppCompileAction.java | 6 +- .../build/lib/rules/cpp/SpawnGccStrategy.java | 23 ++++--- .../rules/objc/CrosstoolCompilationSupport.java | 1 + 9 files changed, 82 insertions(+), 107 deletions(-) (limited to 'src/main/java/com/google/devtools/build') diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandAction.java b/src/main/java/com/google/devtools/build/lib/actions/CommandAction.java index 1ac7da832d..e8d5a93d1c 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandAction.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandAction.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.actions; -import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableMap; import java.util.List; @@ -34,6 +33,5 @@ public interface CommandAction extends Action, ExecutionInfoSpecifier { ImmutableMap getEnvironment(); /** Returns inputs to this action, including inputs that may be pruned. */ - @VisibleForTesting // productionVisibility = Visibility.PRIVATE Iterable getPossibleInputsForTesting(); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java index da4ec05b07..0a253d34c5 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java @@ -90,6 +90,7 @@ public final class CcCommon { private static final ImmutableSet DEFAULT_FEATURES = ImmutableSet.of( CppRuleClasses.DEPENDENCY_FILE, + CppRuleClasses.COMPILE_ACTION_FLAGS_IN_FLAG_SET, CppRuleClasses.RANDOM_SEED, CppRuleClasses.MODULE_MAPS, CppRuleClasses.MODULE_MAP_HOME_CWD, 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 73bf3e85bb..3215cbb36e 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 @@ -20,8 +20,10 @@ import com.google.devtools.build.lib.actions.Artifact; 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.CppCompileAction.DotdFile; +import com.google.devtools.build.lib.util.FileType; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.Preconditions; +import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -31,6 +33,7 @@ import javax.annotation.Nullable; public final class CompileCommandLine { private final Artifact sourceFile; + private final Artifact outputFile; private final Predicate coptsFilter; private final FeatureConfiguration featureConfiguration; private final CcToolchainFeatures.Variables variables; @@ -40,6 +43,7 @@ public final class CompileCommandLine { private CompileCommandLine( Artifact sourceFile, + Artifact outputFile, Predicate coptsFilter, FeatureConfiguration featureConfiguration, CppConfiguration cppConfiguration, @@ -47,6 +51,7 @@ public final class CompileCommandLine { String actionName, DotdFile dotdFile) { this.sourceFile = Preconditions.checkNotNull(sourceFile); + this.outputFile = Preconditions.checkNotNull(outputFile); this.coptsFilter = coptsFilter; this.featureConfiguration = Preconditions.checkNotNull(featureConfiguration); this.cppConfiguration = Preconditions.checkNotNull(cppConfiguration); @@ -66,12 +71,8 @@ public final class CompileCommandLine { return featureConfiguration.getEnvironmentVariables(actionName, variables); } - /** - * @param overwrittenVariables: Variables that will overwrite original build variables. When null, - * unmodified original variables are used. - */ - protected List getArguments( - @Nullable CcToolchainFeatures.Variables overwrittenVariables) { + protected List getArgv( + PathFragment outputFile, CcToolchainFeatures.Variables overwrittenVariables) { List commandLine = new ArrayList<>(); // first: The command name. @@ -86,6 +87,17 @@ public final class CompileCommandLine { // second: The compiler options. commandLine.addAll(getCompilerOptions(overwrittenVariables)); + + if (!featureConfiguration.isEnabled("compile_action_flags_in_flag_set")) { + // third: The file to compile! + commandLine.add("-c"); + commandLine.add(sourceFile.getExecPathString()); + + // finally: The output file. (Prefixed with -o). + commandLine.add("-o"); + commandLine.add(outputFile.getPathString()); + } + return commandLine; } @@ -104,6 +116,19 @@ public final class CompileCommandLine { addFilteredOptions( options, featureConfiguration.getPerFeatureExpansions(actionName, updatedVariables)); + if (!featureConfiguration.isEnabled("compile_action_flags_in_flag_set")) { + if (FileType.contains(outputFile, CppFileTypes.ASSEMBLER, CppFileTypes.PIC_ASSEMBLER)) { + options.add("-S"); + } else if (FileType.contains( + outputFile, + CppFileTypes.PREPROCESSED_C, + CppFileTypes.PREPROCESSED_CPP, + CppFileTypes.PIC_PREPROCESSED_C, + CppFileTypes.PIC_PREPROCESSED_CPP)) { + options.add("-E"); + } + } + return options; } @@ -149,16 +174,19 @@ public final class CompileCommandLine { public static Builder builder( Artifact sourceFile, + Artifact outputFile, Predicate coptsFilter, String actionName, CppConfiguration cppConfiguration, DotdFile dotdFile) { - return new Builder(sourceFile, coptsFilter, actionName, cppConfiguration, dotdFile); + return new Builder( + sourceFile, outputFile, coptsFilter, actionName, cppConfiguration, dotdFile); } /** A builder for a {@link CompileCommandLine}. */ public static final class Builder { private final Artifact sourceFile; + private final Artifact outputFile; private Predicate coptsFilter; private FeatureConfiguration featureConfiguration; private CcToolchainFeatures.Variables variables = Variables.EMPTY; @@ -169,6 +197,7 @@ public final class CompileCommandLine { public CompileCommandLine build() { return new CompileCommandLine( Preconditions.checkNotNull(sourceFile), + Preconditions.checkNotNull(outputFile), Preconditions.checkNotNull(coptsFilter), Preconditions.checkNotNull(featureConfiguration), Preconditions.checkNotNull(cppConfiguration), @@ -179,11 +208,13 @@ public final class CompileCommandLine { private Builder( Artifact sourceFile, + Artifact outputFile, Predicate coptsFilter, String actionName, CppConfiguration cppConfiguration, DotdFile dotdFile) { this.sourceFile = sourceFile; + this.outputFile = outputFile; this.coptsFilter = coptsFilter; this.actionName = actionName; this.cppConfiguration = cppConfiguration; diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java index 29a00d0142..e965d4a82f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java @@ -70,8 +70,6 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", - " implies: 'compiler_input_flags'", - " implies: 'compiler_output_flags'", "}", "action_config {", " config_name: 'preprocess-assemble'", @@ -83,8 +81,6 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", - " implies: 'compiler_input_flags'", - " implies: 'compiler_output_flags'", "}", "action_config {", " config_name: 'c-compile'", @@ -96,8 +92,6 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", - " implies: 'compiler_input_flags'", - " implies: 'compiler_output_flags'", "}", "action_config {", " config_name: 'c++-compile'", @@ -109,8 +103,6 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", - " implies: 'compiler_input_flags'", - " implies: 'compiler_output_flags'", "}", "action_config {", " config_name: 'c++-header-parsing'", @@ -122,8 +114,6 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", - " implies: 'compiler_input_flags'", - " implies: 'compiler_output_flags'", "}", "action_config {", " config_name: 'c++-header-preprocessing'", @@ -135,8 +125,6 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", - " implies: 'compiler_input_flags'", - " implies: 'compiler_output_flags'", "}", "action_config {", " config_name: 'c++-module-compile'", @@ -148,8 +136,6 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", - " implies: 'compiler_input_flags'", - " implies: 'compiler_output_flags'", "}", "action_config {", " config_name: 'c++-module-codegen'", @@ -161,8 +147,6 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", - " implies: 'compiler_input_flags'", - " implies: 'compiler_output_flags'", "}", ifTrue( !existingFeatureNames.contains(CppRuleClasses.LEGACY_COMPILE_FLAGS), @@ -1043,66 +1027,6 @@ public class CppActionConfigs { " flag: '@%{linker_param_file}'", " }", " }", - "}"), - ifTrue( - !existingFeatureNames.contains("compiler_input_flags"), - "feature {", - " name: 'compiler_input_flags'", - " enabled: true", - " flag_set {", - " action: 'assemble'", - " action: 'preprocess-assemble'", - " action: 'c-compile'", - " action: 'c++-compile'", - " action: 'c++-module-compile'", - " action: 'c++-module-codegen'", - " action: 'objc-compile'", - " action: 'objc++-compile'", - " action: 'c++-header-preprocessing'", - " action: 'c++-header-parsing'", - " action: 'lto-backend'", - " expand_if_all_available: 'source_file'", - " flag_group {", - " flag: '-c'", - " flag: '%{source_file}'", - " }", - " }", - "}"), - ifTrue( - !existingFeatureNames.contains("compiler_output_flags"), - "feature {", - " name: 'compiler_output_flags'", - " enabled: true", - " flag_set {", - " action: 'assemble'", - " action: 'preprocess-assemble'", - " action: 'c-compile'", - " action: 'c++-compile'", - " action: 'c++-module-compile'", - " action: 'c++-module-codegen'", - " action: 'objc-compile'", - " action: 'objc++-compile'", - " action: 'c++-header-preprocessing'", - " action: 'c++-header-parsing'", - " action: 'lto-backend'", - " flag_group {", - " expand_if_all_available: 'output_object_file'", - " flag: '-o'", - " flag: '%{output_object_file}'", - " }", - " flag_group {", - " expand_if_all_available: 'output_assembly_file'", - " flag: '-o'", - " flag: '%{output_assembly_file}'", - " flag: '-S'", - " }", - " flag_group {", - " expand_if_all_available: 'output_preprocess_file'", - " flag: '-o'", - " flag: '%{output_preprocess_file}'", - " flag: '-E'", - " }", - " }", "}"))); } 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 735e24ecb8..0de697be74 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 @@ -341,6 +341,7 @@ public class CppCompileAction extends AbstractAction this.compileCommandLine = CompileCommandLine.builder( sourceFile, + outputFile, coptsFilter, actionName, cppConfiguration, @@ -662,7 +663,7 @@ public class CppCompileAction extends AbstractAction @Override public List getCmdlineIncludes() { ImmutableList.Builder cmdlineIncludes = ImmutableList.builder(); - List args = getArguments(); + List args = getArgv(); for (Iterator argi = args.iterator(); argi.hasNext();) { String arg = argi.next(); if (arg.equals("-include") && argi.hasNext()) { @@ -724,9 +725,21 @@ public class CppCompileAction extends AbstractAction return ImmutableMap.copyOf(environment); } + /** + * Returns a new, mutable list of command and arguments (argv) to be passed + * to the gcc subprocess. + */ + public final List getArgv() { + return getArgv(getInternalOutputFile()); + } + @Override public List getArguments() { - return compileCommandLine.getArguments(overwrittenVariables); + return getArgv(); + } + + protected final List getArgv(PathFragment outputFile) { + return compileCommandLine.getArgv(outputFile, overwrittenVariables); } @Override @@ -1087,8 +1100,7 @@ public class CppCompileAction extends AbstractAction public ResourceSet estimateResourceConsumptionLocal() { // We use a local compile, so much of the time is spent waiting for IO, // but there is still significant CPU; hence we estimate 50% cpu usage. - return ResourceSet.createWithRamCpuIo( - /* memoryMb= */ 200, /* cpuUsage= */ 0.5, /* ioUsage= */ 0.0); + return ResourceSet.createWithRamCpuIo(/*memoryMb=*/200, /*cpuUsage=*/0.5, /*ioUsage=*/0.0); } @Override @@ -1105,10 +1117,10 @@ public class CppCompileAction extends AbstractAction // itself is fully determined by the input source files and module maps. // A better long-term solution would be to make the compiler to find them automatically and // never hand in the .pcm files explicitly on the command line in the first place. - f.addStrings(compileCommandLine.getArguments(/* overwrittenVariables= */ null)); + f.addStrings(compileCommandLine.getArgv(getInternalOutputFile(), null)); /* - * getArguments() above captures all changes which affect the compilation + * getArgv() above captures all changes which affect the compilation * command and hence the contents of the object file. But we need to * also make sure that we reexecute the action if any of the fields * that affect whether validateIncludes() will report an error or warning @@ -1336,9 +1348,9 @@ public class CppCompileAction extends AbstractAction message.append(getProgressMessage()); message.append('\n'); // Outputting one argument per line makes it easier to diff the results. - // The first element in getArguments() is actually the command to execute. + // The first element in getArgv() is actually the command to execute. String legend = " Command: "; - for (String argument : ShellEscaper.escapeAll(getArguments())) { + for (String argument : ShellEscaper.escapeAll(getArgv())) { message.append(legend); message.append(argument); message.append('\n'); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java index 4e8139e096..5239a2f5eb 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java @@ -189,6 +189,13 @@ public class CppRuleClasses { */ public static final String RANDOM_SEED = "random_seed"; + /** + * A string constant for the compile_action_flags_in_flag_set feature. This feature is just a + * transitional feature which helps telling whether -c and -o options are already in flag_set of + * action_config in CROSSTOOL file. Once the transition is done, it should be removed. + */ + public static final String COMPILE_ACTION_FLAGS_IN_FLAG_SET = "compile_action_flags_in_flag_set"; + /** * A string constant for the dependency_file feature. This feature generates the .d file. */ 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 24afe83de2..1daec4300b 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 @@ -199,12 +199,14 @@ public class FakeCppCompileAction extends CppCompileAction { // line to write to $TEST_TMPDIR instead. final String outputPrefix = "$TEST_TMPDIR/"; String argv = - getArguments() + getArgv(outputFile.getExecPath()) .stream() .map( input -> { String result = ShellEscaper.escapeString(input); - // Replace -c so it's -c . + // Once -c and -o options are added into action_config, the argument of + // getArgv(outputFile.getExecPath()) won't be used anymore. There will always be + // -c , but here it has to be outputFile, so we replace it. if (input.equals(tempOutputFile.getPathString())) { result = outputPrefix + ShellEscaper.escapeString(outputFile.getExecPathString()); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java index cf8612a89e..f24be92101 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java @@ -53,18 +53,17 @@ public class SpawnGccStrategy implements CppCompileActionContext { + action.getPrimaryInput().getExecPathString()); } Iterable inputs = Iterables.concat(action.getInputs(), action.getAdditionalInputs()); - Spawn spawn = - new SimpleSpawn( - action, - ImmutableList.copyOf(action.getArguments()), - ImmutableMap.copyOf(action.getEnvironment()), - ImmutableMap.copyOf(action.getExecutionInfo()), - EmptyRunfilesSupplier.INSTANCE, - ImmutableList.copyOf(inputs), - /* tools= */ ImmutableList.of(), - /* filesetManifests= */ ImmutableList.of(), - action.getOutputs().asList(), - action.estimateResourceConsumptionLocal()); + Spawn spawn = new SimpleSpawn( + action, + ImmutableList.copyOf(action.getArgv()), + ImmutableMap.copyOf(action.getEnvironment()), + ImmutableMap.copyOf(action.getExecutionInfo()), + EmptyRunfilesSupplier.INSTANCE, + ImmutableList.copyOf(inputs), + /*tools=*/ImmutableList.of(), + /*filesetManifests=*/ImmutableList.of(), + action.getOutputs().asList(), + action.estimateResourceConsumptionLocal()); actionExecutionContext .getSpawnActionContext(action.getMnemonic()) diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CrosstoolCompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CrosstoolCompilationSupport.java index 69d071eeee..8241b68cfb 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/CrosstoolCompilationSupport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CrosstoolCompilationSupport.java @@ -502,6 +502,7 @@ public class CrosstoolCompilationSupport extends CompilationSupport { .add(CppRuleClasses.COMPILE_ALL_MODULES) .add(CppRuleClasses.EXCLUDE_PRIVATE_HEADERS_IN_MODULE_MAPS) .add(CppRuleClasses.ONLY_DOTH_HEADERS_IN_MODULE_MAPS) + .add(CppRuleClasses.COMPILE_ACTION_FLAGS_IN_FLAG_SET) .add(CppRuleClasses.DEPENDENCY_FILE) .add(CppRuleClasses.INCLUDE_PATHS) .add(isHost ? "host" : "nonhost") -- cgit v1.2.3