[PATCH] D69825: [Clang][Driver] Bypass cc1 process and re-enter driver main

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

In D69825#1747539 <https://reviews.llvm.org/D69825#1747539>, @aganea wrote:

> @hans : Simply because `ExecuteCC1Tool()` lives in the clang tool (`clang/tools/driver/driver.cpp`) and we're calling it from the clangDriver.lib (`clang/lib/Driver/Job.cpp`). The clangDriver.lib is linked into many other tools (clang-refactor, clang-rename, clang-diff, clang-check, libclang, ...) If I reference the cc1 function directly, we end of with linker errors in all those tools.
>  We could //maybe// move the tool code into the clangDriver.lib, but I'm not sure it's practical as the tool pulls on many other libraries. I thought the callback was the path of least resistance. Let me know if we could do it otherwise.


Oh, I see. I didn't think about that. That's annoying, but maybe using the variable makes sense then.



================
Comment at: clang/include/clang/Driver/Driver.h:207
 
+  /// Callback to the CC1 tool, if available
+  typedef int(*CC1ToolFunc)(ArrayRef<const char *> argv);
----------------
It's not really a callback though. How about just "Pointer to the ExecuteCC1Tool function, if available."
It would also be good if the comment explained why the pointer is needed.
And why does it need to be thread-local? Can different threads end up with different values for this?


================
Comment at: clang/include/clang/Driver/Job.h:136
+  /// instead of creating a new process. This is an optimization for speed.
+  static LLVM_THREAD_LOCAL bool ExecuteCC1Tool;
 };
----------------
Is there another way of avoiding in-process-cc1 after a crash? For example, could the Job know that it's being built for generating the crash report? It seems unfortunate to introduce this widely-scoped variable here.


================
Comment at: clang/lib/Driver/Job.cpp:339
 
-  if (ResponseFile == nullptr) {
+  Driver::CC1ToolFunc CC1Main{};
+
----------------
I don't think this form of initialization is common in LLVM code.


================
Comment at: clang/lib/Driver/Job.cpp:347
+    StringRef DriverExe = llvm::sys::path::stem(D.ClangExecutable);
+    if (CommandExe.equals_lower(DriverExe))
+        CC1Main = Driver::CC1Main;
----------------
Now that we're not calling main() anymore, but cc1 directly -- is this checking still necessary?


================
Comment at: clang/tools/driver/driver.cpp:308
+int ExecuteCC1Tool(ArrayRef<const char *> argv) {
+  // If we re-enter the cc1 tool from the driver process, we should cleanup the
+  // usage count for the driver options (which might be used in the cc1 tool)
----------------
It's not really re-entering. Maybe "If we call the cc1 tool directly in the driver process" is clearer?


================
Comment at: llvm/include/llvm/Support/CrashRecoveryContext.h:105
+  /// In case of a crash, this is the crash identifier
+  int RetCode{};
+
----------------
I don't think this form of initialization is common in LLVM. Same below.


================
Comment at: llvm/lib/Support/CrashRecoveryContext.cpp:23
+// In llvm/lib/Support/Windows/Signals.inc
+LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep);
+#endif
----------------
Can we move this declaration, and the one for SignalHandler, into some header file? Declaring it manually like this doesn't seem so nice.


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

https://reviews.llvm.org/D69825