Replace CLANG_SPAWN_CC1 env var with a driver mode flag

[thakis]

Summary

Flags are clang's default UI.

We can have an env var in addition to that, but in D69825 nobody has yet mentioned why this needs an env var, so omit it for now.
If someone needs to set the flag via env var, the existing CCC_OVERRIDE_OPTIONS mechanism works
for it (set CCC_OVERRIDE_OPTIONS=+-fno-integrated-cc1 for example).

Also mention the cc1-in-process change in the release notes.

Also spruce up the test a bit so it actually tests something :)

[aganea]

I guess CLANG_SPAWN_CC1 is/was mostly for debugging purposes. We have otherwise no use for that env var in production, other than for occasional testing.

You could run your build, and toggle that env var without changing the build options.
In the same way, I was also using it when building LLVM with ninja check-all to ensure things work in the same way (when CLANG_SPAWN_CC1=0 or =1). You would otherwise need to fully rebuild LLVM if there's no env var.

The other issue I was seeing with a flag is that, it could be enabled on some libraries, but not others, depending on how the build system is setup. For example, if you're using an external package which is generated from an external script, but compiled within your project, with your clang-cl, you could easily miss that flag (especially when you have tens of external packages to update).

[thakis]

In D72769#1821938, @aganea wrote:

I guess CLANG_SPAWN_CC1 is/was mostly for debugging purposes. We have otherwise no use for that env var in production, other than for occasional testing.

It's a pretty major change in how clang runs compilations, and I think we need some way to turn it off in case it causes issues somewhere (distcc or similar). Every build system can easily change flags. Some can't easily add env vars. We definitely need a flag for this.

You could run your build, and toggle that env var without changing the build options.
In the same way, I was also using it when building LLVM with ninja check-all to ensure things work in the same way (when CLANG_SPAWN_CC1=0 or =1). You would otherwise need to fully rebuild LLVM if there's no env var.

That's a good point. Clang has an existing mechanism for this (set CCC_OVERRIDE_OPTIONS=^-fno-integrated-cc1 like you would for any other flag), but it didn't work for this CL. It now does.

The other issue I was seeing with a flag is that, it could be enabled on some libraries, but not others, depending on how the build system is setup. For example, if you're using an external package which is generated from an external script, but compiled within your project, with your clang-cl, you could easily miss that flag (especially when you have tens of external packages to update).

That's true for every flag under the sun though :)

[thakis]

make flag work with CCC_OVERRIDE_OPTIONS

[thakis]

add CCC_OVERRIDE_OPTIONS=^-fno-integrated-cc1 test

[hans]

lgtm (with comment)

Having a flag makes sense, and those wanting an env var can use CCC_OVERRIDE_OPTIONS, so everyone is happy :-)

clang/tools/driver/driver.cpp

288

This has the same arg passed twice?

[thakis]

clang/tools/driver/driver.cpp

288

Errrrrr. Fixed, and added a test for this too. Thanks!

[thakis]

8e5018e990b

[thakis]

cc cfe-commits for completeness

[sammccall]

Thanks for fixing this.

As I understand, the original patch made the new behaviour on-by-default and will make the release, so we want this flag on the release branch too, right?

(Happy to file a bug, just want to check first)

[hans]

In D72769#1823473, @sammccall wrote:

As I understand, the original patch made the new behaviour on-by-default and will make the release, so we want this flag on the release branch too, right?

Yes, this is on my list. Thanks for keeping the release branch in mind :-)

[hans]

In D72769#1823488, @hans wrote:

In D72769#1823473, @sammccall wrote:

As I understand, the original patch made the new behaviour on-by-default and will make the release, so we want this flag on the release branch too, right?

Yes, this is on my list. Thanks for keeping the release branch in mind :-)

Cherry-picked to 10.x in c4a134a

[hans]

Oh, and removed the release note from trunk in 00c74d0b644

clang/docs/ReleaseNotes.rst

 Some of the major new features and improvements to Clang are listed
 here. Generic improvements to Clang as a whole or to its underlying
 infrastructure are described first, followed by language-specific
 sections with improvements to Clang's support for those languages.
 
 Major New Features
 ------------------
 
+- clang used to run the actual compilation in a subprocess ("clang -cc1").
+  Now compilations are done in-process by default. ``-fno-integrated-cc1``
+  restores the former behavior. The ``-v`` and ``-###`` flags will print
+  "(in-process)" when compilations are done in-process.
+
 - ...
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - ...
 
 Non-comprehensive list of changes in this release

clang/include/clang/Driver/Options.td

   MetaVarName<"<language>">;
 def y : Joined<["-"], "y">;
 
 def fintegrated_as : Flag<["-"], "fintegrated-as">, Flags<[DriverOption]>,
                      Group<f_Group>, HelpText<"Enable the integrated assembler">;
 def fno_integrated_as : Flag<["-"], "fno-integrated-as">,
                         Flags<[CC1Option, DriverOption]>, Group<f_Group>,
                         HelpText<"Disable the integrated assembler">;
+
+def fintegrated_cc1 : Flag<["-"], "fintegrated-cc1">,
+                      Flags<[CoreOption, DriverOption]>, Group<f_Group>,
+                      HelpText<"Run cc1 in-process">;
+def fno_integrated_cc1 : Flag<["-"], "fno-integrated-cc1">,
+                         Flags<[CoreOption, DriverOption]>, Group<f_Group>,
+                         HelpText<"Spawn a separate process for each cc1">;
+
 def : Flag<["-"], "integrated-as">, Alias<fintegrated_as>, Flags<[DriverOption]>;
 def : Flag<["-"], "no-integrated-as">, Alias<fno_integrated_as>,
       Flags<[CC1Option, DriverOption]>;
 
 def working_directory : JoinedOrSeparate<["-"], "working-directory">, Flags<[CC1Option]>,
   HelpText<"Resolve file paths relative to the specified directory">;
 def working_directory_EQ : Joined<["-"], "working-directory=">, Flags<[CC1Option]>,
   Alias<working_directory>;

clang/lib/Driver/Driver.cpp

   bool CCCPrintPhases;
 
   // Silence driver warnings if requested
   Diags.setIgnoreAllWarnings(Args.hasArg(options::OPT_w));
 
   // -no-canonical-prefixes is used very early in main.
   Args.ClaimAllArgs(options::OPT_no_canonical_prefixes);
 
+  // f(no-)integated-cc1 is also used very early in main.
+  Args.ClaimAllArgs(options::OPT_fintegrated_cc1);
+  Args.ClaimAllArgs(options::OPT_fno_integrated_cc1);
+
   // Ignore -pipe.
   Args.ClaimAllArgs(options::OPT_pipe);
 
   // Extract -ccc args.
   //
   // FIXME: We need to figure out where this behavior should live. Most of it
   // should be outside in the client; the parts that aren't should have proper
   // options, either by introducing new ones or by overloading gcc ones like -V

clang/test/Driver/cc1-spawnprocess.c

-// RUN: env CLANG_SPAWN_CC1= %clang -S %s -o /dev/null
-// RUN: env CLANG_SPAWN_CC1=0 %clang -S %s -o /dev/null
-// RUN: env CLANG_SPAWN_CC1=1 %clang -S %s -o /dev/null
-// RUN: env CLANG_SPAWN_CC1=test not %clang -S %s -o /dev/null
+// RUN: %clang -fintegrated-cc1 -### %s 2>&1 | FileCheck %s --check-prefix=YES
+// RUN: %clang -fno-integrated-cc1 -### %s 2>&1 | FileCheck %s --check-prefix=NO
+
+// RUN: %clang_cl -fintegrated-cc1 -### -- %s 2>&1 \
+// RUN:     | FileCheck %s --check-prefix=YES
+// RUN: %clang_cl -fno-integrated-cc1 -### -- %s 2>&1 \
+// RUN:     | FileCheck %s --check-prefix=NO
+
+// RUN: env CCC_OVERRIDE_OPTIONS=+-fintegrated-cc1 \
+// RUN:     %clang -fintegrated-cc1 -### %s 2>&1 \
+// RUN:     | FileCheck %s --check-prefix=YES
+// RUN: env CCC_OVERRIDE_OPTIONS=+-fno-integrated-cc1 \
+// RUN:     %clang -fintegrated-cc1 -### %s 2>&1 \
+// RUN:     | FileCheck %s --check-prefix=NO
+
+// YES: (in-process)
+// NO-NOT: (in-process)

clang/tools/driver/driver.cpp

static void SetBackdoorDriverOutputsFromEnvVars(Driver &TheDriver) {
   TheDriver.CCPrintHeaders = !!::getenv("CC_PRINT_HEADERS");
   if (TheDriver.CCPrintHeaders)
     TheDriver.CCPrintHeadersFilename = ::getenv("CC_PRINT_HEADERS_FILE");
 
   // Handle CC_LOG_DIAGNOSTICS and CC_LOG_DIAGNOSTICS_FILE.
   TheDriver.CCLogDiagnostics = !!::getenv("CC_LOG_DIAGNOSTICS");
   if (TheDriver.CCLogDiagnostics)
     TheDriver.CCLogDiagnosticsFilename = ::getenv("CC_LOG_DIAGNOSTICS_FILE");
-
-  // Whether the cc1 tool should be called inside the current process, or if we
-  // should spawn a new clang process (old behavior).
-  // Not having an additional process saves some execution time of Windows,
-  // and makes debugging easier.
-  bool UseNewCC1Process = CLANG_SPAWN_CC1;
-
-  StringRef SpawnCC1Str = ::getenv("CLANG_SPAWN_CC1");
-  if (!SpawnCC1Str.empty()) {
-    if (SpawnCC1Str != "0" && SpawnCC1Str != "1") {
-      llvm::errs() << "error: the value of the environment variable "
-                      "CLANG_SPAWN_CC1 must be either 0 or 1.\n";
-      ::exit(1);
-    }
-    UseNewCC1Process = SpawnCC1Str[0] - '0';
-  }
-  if (!UseNewCC1Process) {
-    TheDriver.CC1Main = &ExecuteCC1Tool;
-    // Ensure the CC1Command actually catches cc1 crashes
-    llvm::CrashRecoveryContext::Enable();
-  }
 }
 
 static void FixupDiagPrefixExeName(TextDiagnosticPrinter *DiagClient,
                                    const std::string &Path) {
   // If the clang binary happens to be named cl.exe for compatibility reasons,
   // use clang-cl.exe as the prefix to avoid confusion between clang and MSVC.
   StringRef ExeBasename(llvm::sys::path::stem(Path));
   if (ExeBasename.equals_lower("cl"))
     ExeBasename = "clang-cl";
   DiagClient->setPrefix(ExeBasename);
 }
 
 // This lets us create the DiagnosticsEngine with a properly-filled-out
 // DiagnosticOptions instance.
 static DiagnosticOptions *
-CreateAndPopulateDiagOpts(ArrayRef<const char *> argv) {
+CreateAndPopulateDiagOpts(ArrayRef<const char *> argv, bool &UseNewCC1Process) {
   auto *DiagOpts = new DiagnosticOptions;
   unsigned MissingArgIndex, MissingArgCount;
   InputArgList Args = getDriverOptTable().ParseArgs(
       argv.slice(1), MissingArgIndex, MissingArgCount);
   // We ignore MissingArgCount and the return value of ParseDiagnosticArgs.
   // Any errors that would be diagnosed here will also be diagnosed later,
   // when the DiagnosticsEngine actually exists.
   (void)ParseDiagnosticArgs(*DiagOpts, Args);
+
+  UseNewCC1Process =
+      Args.hasFlag(clang::driver::options::OPT_fno_integrated_cc1,
+                   clang::driver::options::OPT_fno_integrated_cc1,
+                   /*Default=*/CLANG_SPAWN_CC1);
+
   return DiagOpts;
 }
 
 static void SetInstallDir(SmallVectorImpl<const char *> &argv,
                           Driver &TheDriver, bool CanonicalPrefixes) {
   // Attempt to find the original path used to invoke the driver, to determine
   // the installed path. We do this manually, because we want to support that
   // path being a symlink.
static void SetInstallDir(SmallVectorImpl<const char *> &argv,
 
   StringRef InstalledPathParent(llvm::sys::path::parent_path(InstalledPath));
   if (llvm::sys::fs::exists(InstalledPathParent))
     TheDriver.setInstalledDir(InstalledPathParent);
 }
 
 static int ExecuteCC1Tool(ArrayRef<const char *> argv) {
   // If we call the cc1 tool from the clangDriver library (through
-  // Driver::CC1Main), we need to cleanup the options usage count. The options
+  // Driver::CC1Main), we need to clean up the options usage count. The options
   // are currently global, and they might have been used previously by the
   // driver.
   llvm::cl::ResetAllOptionOccurrences();
   StringRef Tool = argv[1];
   void *GetExecutablePathVP = (void *)(intptr_t) GetExecutablePath;
   if (Tool == "-cc1")
     return cc1_main(argv.slice(2), argv[0], GetExecutablePathVP);
   if (Tool == "-cc1as")
if (FirstArg != argv.end() && StringRef(*FirstArg).startswith("-cc1")) {
     // If -cc1 came from a response file, remove the EOL sentinels.
     if (MarkEOLs) {
       auto newEnd = std::remove(argv.begin(), argv.end(), nullptr);
       argv.resize(newEnd - argv.begin());
     }
     return ExecuteCC1Tool(argv);
   }
 
+  // Handle options that need handling before the real command line parsing in
+  // Driver::BuildCompilation()
   bool CanonicalPrefixes = true;
   for (int i = 1, size = argv.size(); i < size; ++i) {
     // Skip end-of-line response file markers
     if (argv[i] == nullptr)
       continue;
     if (StringRef(argv[i]) == "-no-canonical-prefixes") {
       CanonicalPrefixes = false;
       break;
int main(int argc_, const char **argv_) {
   // scenes.
   if (const char *OverrideStr = ::getenv("CCC_OVERRIDE_OPTIONS")) {
     // FIXME: Driver shouldn't take extra initial argument.
     ApplyQAOverride(argv, OverrideStr, SavedStrings);
   }
 
   std::string Path = GetExecutablePath(argv[0], CanonicalPrefixes);
 
+  // Whether the cc1 tool should be called inside the current process, or if we
+  // should spawn a new clang subprocess (old behavior).
+  // Not having an additional process saves some execution time of Windows,
+  // and makes debugging and profiling easier.
+  bool UseNewCC1Process;
+
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts =
-      CreateAndPopulateDiagOpts(argv);
+      CreateAndPopulateDiagOpts(argv, UseNewCC1Process);
 
   TextDiagnosticPrinter *DiagClient
     = new TextDiagnosticPrinter(llvm::errs(), &*DiagOpts);
   FixupDiagPrefixExeName(DiagClient, Path);
 
   IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
 
   DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagClient);
int main(int argc_, const char **argv_) {
   Driver TheDriver(Path, llvm::sys::getDefaultTargetTriple(), Diags);
   SetInstallDir(argv, TheDriver, CanonicalPrefixes);
   TheDriver.setTargetAndMode(TargetAndMode);
 
   insertTargetAndModeArgs(TargetAndMode, argv, SavedStrings);
 
   SetBackdoorDriverOutputsFromEnvVars(TheDriver);
 
+  if (!UseNewCC1Process) {
+    TheDriver.CC1Main = &ExecuteCC1Tool;
+    // Ensure the CC1Command actually catches cc1 crashes
+    llvm::CrashRecoveryContext::Enable();
+  }
+
   std::unique_ptr<Compilation> C(TheDriver.BuildCompilation(argv));
   int Res = 1;
   if (C && !C->containsError()) {
     SmallVector<std::pair<int, const Command *>, 4> FailingCommands;
     Res = TheDriver.ExecuteCompilation(*C, FailingCommands);
 
     // Force a crash to test the diagnostics.
     if (TheDriver.GenReproducer) {