[PATCH] D69825: [Clang][Driver] Re-use the calling process instead of creating a new process for the cc1 invocation

[Hans Wennborg via Phabricator via cfe-commits]

hans marked an inline comment as done.
hans added a comment.

I'm basically happy with this, just some minor comments.



================
Comment at: clang/lib/Driver/Job.cpp:376
+                       CrashReportInfo *CrashInfo) const {
+  OS << " Running the following in ExecuteCC1Tool():\n";
+  Command::Print(OS, Terminator, Quote, CrashInfo);
----------------
I think this gets printed by "clang -###" so maybe just print something short here. Perhaps just an "(in-process)" prefix? Then we don't need to change so many test expectations also.


================
Comment at: clang/lib/Driver/Job.cpp:394
+
+  llvm::CrashRecoveryContext::Enable();
+
----------------
Do we need to disable afterwards?


================
Comment at: clang/tools/driver/driver.cpp:313
+  llvm::cl::ResetAllOptionOccurrences();
+  StringRef Tool = argv[1] + 4;
   void *GetExecutablePathVP = (void *)(intptr_t) GetExecutablePath;
----------------
aganea wrote:
> hans wrote:
> > This feels a little risky to me. Do we still need to rely on argv[1], now that we have a nice CC1Command abstraction?
> Risky in what way? What would you suggest? The contents of `argv` are exactly the same in both cases: when called from the CC1Command, or explicitly from the command-line.
You're right, I suppose the code was already doing this.


================
Comment at: clang/tools/driver/driver.cpp:267
+
+  StringRef SpawnCC1Str = ::getenv("CLANG_SPAWN_CC1");
+  if (!SpawnCC1Str.empty()) {
----------------
Maybe just do the "!!" thing like for the environment variables above? It's not pretty, but at least that would be consistent, and it avoids the problem of deciding what values mean "on" and what mean "off".


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69825/new/

https://reviews.llvm.org/D69825