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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 230143.
aganea marked 10 inline comments as done.
aganea edited the summary of this revision.
aganea added a comment.

Addressed @hans' comments.

I've also added a script in the summary, if you'd like to reproduce the results 
on your end, and to ensure we all run the same thing.


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

https://reviews.llvm.org/D69825

Files:
  clang/include/clang/Driver/Driver.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/Job.cpp
  clang/tools/driver/driver.cpp
  llvm/include/llvm/Support/CrashRecoveryContext.h
  llvm/include/llvm/Support/Signals.h
  llvm/lib/Support/CrashRecoveryContext.cpp
  llvm/lib/Support/Unix/Signals.inc
  llvm/lib/Support/Windows/Signals.inc

Index: llvm/lib/Support/Windows/Signals.inc
===
--- llvm/lib/Support/Windows/Signals.inc
+++ llvm/lib/Support/Windows/Signals.inc
@@ -521,10 +521,13 @@
 extern "C" VOID WINAPI RtlCaptureContext(PCONTEXT ContextRecord);
 #endif
 
-void llvm::sys::PrintStackTrace(raw_ostream ) {
-  STACKFRAME64 StackFrame = {};
-  CONTEXT Context = {};
-  ::RtlCaptureContext();
+static void LocalPrintStackTrace(raw_ostream , PCONTEXT C) {
+  STACKFRAME64 StackFrame{};
+  CONTEXT Context{};
+  if (!C) {
+::RtlCaptureContext();
+C = 
+  }
 #if defined(_M_X64)
   StackFrame.AddrPC.Offset = Context.Rip;
   StackFrame.AddrStack.Offset = Context.Rsp;
@@ -546,9 +549,12 @@
   StackFrame.AddrStack.Mode = AddrModeFlat;
   StackFrame.AddrFrame.Mode = AddrModeFlat;
   PrintStackTraceForThread(OS, GetCurrentProcess(), GetCurrentThread(),
-   StackFrame, );
+   StackFrame, C);
 }
 
+void llvm::sys::PrintStackTrace(raw_ostream ) {
+  LocalPrintStackTrace(OS, nullptr);
+}
 
 void llvm::sys::SetInterruptFunction(void (*IF)()) {
   RegisterHandler();
@@ -792,6 +798,10 @@
   return std::error_code();
 }
 
+signed sys::InvokeExceptionHandler(int &, void *ExceptionInfo) {
+  return LLVMUnhandledExceptionFilter((LPEXCEPTION_POINTERS)ExceptionInfo);
+}
+
 static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep) {
   Cleanup();
 
@@ -810,42 +820,9 @@
<< "\n";
   }
 
-  // Initialize the STACKFRAME structure.
-  STACKFRAME64 StackFrame = {};
-
-#if defined(_M_X64)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Rip;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Rsp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Rbp;
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#elif defined(_M_IX86)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Eip;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Esp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Ebp;
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#elif defined(_M_ARM64) || defined(_M_ARM)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Pc;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Sp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-#if defined(_M_ARM64)
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Fp;
-#else
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->R11;
-#endif
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#endif
-
-  HANDLE hProcess = GetCurrentProcess();
-  HANDLE hThread = GetCurrentThread();
-  PrintStackTraceForThread(llvm::errs(), hProcess, hThread, StackFrame,
-   ep->ContextRecord);
+  LocalPrintStackTrace(llvm::errs(), ep ? ep->ContextRecord : nullptr);
 
-  _exit(ep->ExceptionRecord->ExceptionCode);
+  return EXCEPTION_EXECUTE_HANDLER;
 }
 
 static BOOL WINAPI LLVMConsoleCtrlHandler(DWORD dwCtrlType) {
Index: llvm/lib/Support/Unix/Signals.inc
===
--- llvm/lib/Support/Unix/Signals.inc
+++ llvm/lib/Support/Unix/Signals.inc
@@ -345,6 +345,17 @@
   FileToRemoveList::removeAllFiles(FilesToRemove);
 }
 
+signed sys::InvokeExceptionHandler(int , void *) {
+  SignalHandler(RetCode);
+  // llvm/lib/Support/Unix/Program.inc:Wait() returns -2 if a crash occurs,
+  // not the actual error code. If we want to diagnose a crash in the same
+  // way as invoking/forking a new process (in
+  // clang/tools/driver/driver.cpp), we need to do this here.
+  if (WIFSIGNALED(RetCode))
+RetCode = -2;
+  return 0;
+}
+
 // The signal handler that runs.
 static RETSIGTYPE SignalHandler(int Sig) {
   // Restore the signal behavior to default, so that the program actually
@@ -361,8 +372,8 @@
   {
 RemoveFilesToRemove();
 
-if (std::find(std::begin(IntSigs), std::end(IntSigs), Sig)
-!= std::end(IntSigs)) {
+if (std::find(std::begin(IntSigs), std::end(IntSigs), Sig) !=
+std::end(IntSigs)) {
   if (auto OldInterruptFunction = 

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.



Comment at: clang/lib/Driver/Job.cpp:347
+StringRef DriverExe = llvm::sys::path::stem(D.ClangExecutable);
+if (CommandExe.equals_lower(DriverExe))
+CC1Main = Driver::CC1Main;

hans wrote:
> Now that we're not calling main() anymore, but cc1 directly -- is this 
> checking still necessary?
Yes it is - see my other comment just above.

The driver builds phases that do not always call the cc1 process. Simply 
stating `clang a.cpp` would invoke `clang -cc1`, then the linker, say 
`link.exe`. In this later case (invoking `link.exe`), even if we have 
`Driver::Main` it doesn't mean we should use it. There are a number of other 
edge cases of the same kind, such as `/fallback` or building cuda files, where 
a different executable is invoked along the way."


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

https://reviews.llvm.org/D69825



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In 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 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 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

@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.


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

https://reviews.llvm.org/D69825



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Thanks for the update! I think calling ExecuteCC1Tool instead of main is an 
improvement (there's no need to think about "re-entering" the process, it's 
really just a function call). I still don't see why it needs to go through a 
function pointer. I was hoping the code could be something like:

In Command::Execute:
if the tool we're going to execute is cc1 (not the gen-reproducer though),
then instead of forking, call ExecuteCC1Tool with the args directly (inside a 
context so that we can recover from a crash)

Do you think that's achievable?




Comment at: clang/include/clang/Driver/Driver.h:209
+  typedef int(*CC1ToolFunc)(ArrayRef argv);
+  static LLVM_THREAD_LOCAL CC1ToolFunc CC1Main;
+

Why do we need a variable for this? Couldn't the code just invoke the function 
directly?


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

https://reviews.llvm.org/D69825



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 229333.
aganea marked an inline comment as done.
aganea added a comment.

Call into `ExecuteCC1Tool()` directly, as suggested by @hans
Add more comments.
Remove `pThis` in `CrashRecoveryContext.cpp:RunSafely()` as suggested by 
@zturner


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

https://reviews.llvm.org/D69825

Files:
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/Job.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/Job.cpp
  clang/tools/driver/driver.cpp
  llvm/include/llvm/Support/CrashRecoveryContext.h
  llvm/lib/Support/CrashRecoveryContext.cpp
  llvm/lib/Support/Unix/Signals.inc
  llvm/lib/Support/Windows/Signals.inc

Index: llvm/lib/Support/Windows/Signals.inc
===
--- llvm/lib/Support/Windows/Signals.inc
+++ llvm/lib/Support/Windows/Signals.inc
@@ -187,7 +187,7 @@
 using namespace llvm;
 
 // Forward declare.
-static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep);
+LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep);
 static BOOL WINAPI LLVMConsoleCtrlHandler(DWORD dwCtrlType);
 
 // The function to call if ctrl-c is pressed.
@@ -521,10 +521,13 @@
 extern "C" VOID WINAPI RtlCaptureContext(PCONTEXT ContextRecord);
 #endif
 
-void llvm::sys::PrintStackTrace(raw_ostream ) {
-  STACKFRAME64 StackFrame = {};
-  CONTEXT Context = {};
-  ::RtlCaptureContext();
+static void LocalPrintStackTrace(raw_ostream , PCONTEXT C) {
+  STACKFRAME64 StackFrame{};
+  CONTEXT Context{};
+  if (!C) {
+::RtlCaptureContext();
+C = 
+  }
 #if defined(_M_X64)
   StackFrame.AddrPC.Offset = Context.Rip;
   StackFrame.AddrStack.Offset = Context.Rsp;
@@ -546,9 +549,12 @@
   StackFrame.AddrStack.Mode = AddrModeFlat;
   StackFrame.AddrFrame.Mode = AddrModeFlat;
   PrintStackTraceForThread(OS, GetCurrentProcess(), GetCurrentThread(),
-   StackFrame, );
+   StackFrame, C);
 }
 
+void llvm::sys::PrintStackTrace(raw_ostream ) {
+  LocalPrintStackTrace(OS, nullptr);
+}
 
 void llvm::sys::SetInterruptFunction(void (*IF)()) {
   RegisterHandler();
@@ -785,7 +791,7 @@
   return std::error_code();
 }
 
-static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep) {
+LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep) {
   Cleanup();
 
   // We'll automatically write a Minidump file here to help diagnose
@@ -803,42 +809,9 @@
<< "\n";
   }
 
-  // Initialize the STACKFRAME structure.
-  STACKFRAME64 StackFrame = {};
-
-#if defined(_M_X64)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Rip;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Rsp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Rbp;
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#elif defined(_M_IX86)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Eip;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Esp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Ebp;
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#elif defined(_M_ARM64) || defined(_M_ARM)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Pc;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Sp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-#if defined(_M_ARM64)
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Fp;
-#else
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->R11;
-#endif
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#endif
-
-  HANDLE hProcess = GetCurrentProcess();
-  HANDLE hThread = GetCurrentThread();
-  PrintStackTraceForThread(llvm::errs(), hProcess, hThread, StackFrame,
-   ep->ContextRecord);
+  LocalPrintStackTrace(llvm::errs(), ep ? ep->ContextRecord : nullptr);
 
-  _exit(ep->ExceptionRecord->ExceptionCode);
+  return EXCEPTION_EXECUTE_HANDLER;
 }
 
 static BOOL WINAPI LLVMConsoleCtrlHandler(DWORD dwCtrlType) {
Index: llvm/lib/Support/Unix/Signals.inc
===
--- llvm/lib/Support/Unix/Signals.inc
+++ llvm/lib/Support/Unix/Signals.inc
@@ -79,7 +79,7 @@
 
 using namespace llvm;
 
-static RETSIGTYPE SignalHandler(int Sig);  // defined below.
+RETSIGTYPE SignalHandler(int Sig);  // defined below.
 static RETSIGTYPE InfoSignalHandler(int Sig);  // defined below.
 
 using SignalHandlerFunctionType = void (*)();
@@ -341,7 +341,7 @@
 }
 
 // The signal handler that runs.
-static RETSIGTYPE SignalHandler(int Sig) {
+RETSIGTYPE SignalHandler(int Sig) {
   // Restore the signal behavior to default, so that the program actually
   // crashes when we return and the signal reissues.  This also ensures that if
   // we crash in our signal handler that the program will terminate immediately
@@ -356,8 +356,8 @@
   {
 

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

[Alexandre Ganea via Phabricator via cfe-commits]

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

In D69825#1742276 , @hans wrote:

> Instead of invoking main() (or a similar function like ClangDriverMain) as an 
> alternative to spinning up the cc1 process, would it be possible to call 
> ExecuteCC1Tool() directly? I guess it would be a little less general, but 
> maybe it would be more straight-forward?


Possibly, yes, I'll have to check. There might be a few edge cases where we 
re-enter `ClangDriverMain()` twice if I recall correctly - I'll check.

In D69825#1742621 , @zturner wrote:

> Does this change crash recovery semantics in any meaningful way?  Will we 
> still be able to get stack traces on all platforms when the compiler crashes?


Yes, the changes in `CrashRecoveryContext` allow the same side-effects as the 
global exception handler. You would see the callstack and minidump in the same 
way.

There's still a minimal risk of memory/heap/CRT corruption, even with 
`CrashRecoveryContext` (after a crash). But that would possibly affect 
`LLVMUnhandledExceptionFilter` as well, and right now (//without this patch//), 
that handler runs in the context of the **crashed process **(not the calling 
one) so it wouldn't be worse than what it is now.

I could write a follow-up patch to prepare //on startup// the cmd-line invoked 
by `Driver::generateCompilationDiagnostics()`, instead of preparing after a 
crash. We could also pre-allocate a few virtual pages on advance, and use that 
in a BumpAllocator, instead of allocating after the crash. We could also merge 
the feature of `llvm-symbolizer` into `clang.exe`, so that we could call-back 
into `clang.exe` to render the callstack on stdout, even if `llvm-symbolizer` 
is not present.

The only drawback I see now is that we don't get the coredump on Linux, because 
the program doesn't end through the kernel signal handler. However, given that 
@Meinersbur says he sees no improvement on his side, we could disable this 
optimization on non-win? (or hide it behind a disabled flag on non-win)




Comment at: clang/lib/Driver/Job.cpp:347
+StringRef DriverExe = llvm::sys::path::stem(D.ClangExecutable);
+if (CommandExe.equals_lower(DriverExe))
+  ClangDriverMain = Driver::Main;

Meinersbur wrote:
> Why is this check necessary? Why not assuming that if `Driver::Main` is set, 
> it will be the right one?
The driver builds phases that do not always call the cc1 process. Simply 
stating `clang a.cpp` would invoke `clang -cc1`, then the linker. In the later 
case, even if we have `Driver::Main` it doesn't mean we should use it. There 
are a number of other edge cases of the same kind, such as `/fallback` or build 
cuda files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Does this change crash recovery semantics in any meaningful way?  Will we still 
be able to get stack traces on all platforms when the compiler crashes?




Comment at: llvm/lib/Support/CrashRecoveryContext.cpp:207
+  // FIXME error: cannot compile this 'this' captured by SEH yet
+  CrashRecoveryContext *This = this;
   __try {

You can fix this by writing:

```
static bool wrap_function_call(function_ref Fn, bool 
EnableExceptionHandler, unsigned )
{
   __try {
 Fn();
 return true;
  } __except (EnableExceptionHandler
  ? LLVMUnhandledExceptionFilter(GetExceptionInformation())
  : 1) {
RetCode = GetExceptionCode();
return false;
  }
}

bool CrashRecoveryContext::RunSafely(function_ref Fn) {
...
bool Result = wrap_function_call(EnableExceptionHandler, Fn, RetCode);
...
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: clang/lib/Driver/Job.cpp:347
+StringRef DriverExe = llvm::sys::path::stem(D.ClangExecutable);
+if (CommandExe.equals_lower(DriverExe))
+  ClangDriverMain = Driver::Main;

Why is this check necessary? Why not assuming that if `Driver::Main` is set, it 
will be the right one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Thanks for sharing this!

I still need to take a proper look, but my first thought is this:

Instead of invoking main() (or a similar function like ClangDriverMain) as an 
alternative to spinning up the cc1 process, would it be possible to call 
ExecuteCC1Tool() directly? I guess it would be a little less general, but maybe 
it would be more straight-forward?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 228729.
aganea marked 2 inline comments as done.
aganea added a comment.

Addressed comments & finished the Linux part. All tests pass.

@Meinersbur : Just to show the difference between Windows & Linux, here's some 
timings for running the tests over the same `git checkout`, compiled with 
clang-9.
Everything was already built, I used `ninja check-all` and only the tests ran.
`LLVM_ENABLE_PROJECTS = "clang;lld;clang-tools-extra;compiler-rt"`

| 6-core Intel(R) Xeon(R) CPU E5-1650 0 @ 3.20GHz | Ubuntu 18.04  | 
770.07s (12 min 50 sec)  | 59182 tests ran |
| 6-core Intel(R) Xeon(R) W-2135 CPU @ 3.70GHz| Windows 10 build 1903 | 
1779.83s (29 min 39 sec) | 56589 tests ran |
|

This patch does not make much difference for running the tests on Windows (it's 
saving only 30 sec). The friction in the OS is so high, improving just 
clang.exe does not change much.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825

Files:
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/Job.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/Job.cpp
  clang/tools/driver/driver.cpp
  llvm/include/llvm/Support/CrashRecoveryContext.h
  llvm/lib/Support/CrashRecoveryContext.cpp
  llvm/lib/Support/Unix/Signals.inc
  llvm/lib/Support/Windows/Signals.inc

Index: llvm/lib/Support/Windows/Signals.inc
===
--- llvm/lib/Support/Windows/Signals.inc
+++ llvm/lib/Support/Windows/Signals.inc
@@ -187,7 +187,7 @@
 using namespace llvm;
 
 // Forward declare.
-static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep);
+LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep);
 static BOOL WINAPI LLVMConsoleCtrlHandler(DWORD dwCtrlType);
 
 // The function to call if ctrl-c is pressed.
@@ -521,10 +521,13 @@
 extern "C" VOID WINAPI RtlCaptureContext(PCONTEXT ContextRecord);
 #endif
 
-void llvm::sys::PrintStackTrace(raw_ostream ) {
-  STACKFRAME64 StackFrame = {};
-  CONTEXT Context = {};
-  ::RtlCaptureContext();
+static void LocalPrintStackTrace(raw_ostream , PCONTEXT C) {
+  STACKFRAME64 StackFrame{};
+  CONTEXT Context{};
+  if (!C) {
+::RtlCaptureContext();
+C = 
+  }
 #if defined(_M_X64)
   StackFrame.AddrPC.Offset = Context.Rip;
   StackFrame.AddrStack.Offset = Context.Rsp;
@@ -546,9 +549,12 @@
   StackFrame.AddrStack.Mode = AddrModeFlat;
   StackFrame.AddrFrame.Mode = AddrModeFlat;
   PrintStackTraceForThread(OS, GetCurrentProcess(), GetCurrentThread(),
-   StackFrame, );
+   StackFrame, C);
 }
 
+void llvm::sys::PrintStackTrace(raw_ostream ) {
+  LocalPrintStackTrace(OS, nullptr);
+}
 
 void llvm::sys::SetInterruptFunction(void (*IF)()) {
   RegisterHandler();
@@ -785,7 +791,7 @@
   return std::error_code();
 }
 
-static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep) {
+LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep) {
   Cleanup();
 
   // We'll automatically write a Minidump file here to help diagnose
@@ -803,42 +809,9 @@
<< "\n";
   }
 
-  // Initialize the STACKFRAME structure.
-  STACKFRAME64 StackFrame = {};
-
-#if defined(_M_X64)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Rip;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Rsp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Rbp;
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#elif defined(_M_IX86)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Eip;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Esp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Ebp;
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#elif defined(_M_ARM64) || defined(_M_ARM)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Pc;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Sp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-#if defined(_M_ARM64)
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Fp;
-#else
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->R11;
-#endif
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#endif
-
-  HANDLE hProcess = GetCurrentProcess();
-  HANDLE hThread = GetCurrentThread();
-  PrintStackTraceForThread(llvm::errs(), hProcess, hThread, StackFrame,
-   ep->ContextRecord);
+  LocalPrintStackTrace(llvm::errs(), ep ? ep->ContextRecord : nullptr);
 
-  _exit(ep->ExceptionRecord->ExceptionCode);
+  return EXCEPTION_EXECUTE_HANDLER;
 }
 
 static BOOL WINAPI LLVMConsoleCtrlHandler(DWORD dwCtrlType) {
Index: llvm/lib/Support/Unix/Signals.inc
===
--- llvm/lib/Support/Unix/Signals.inc
+++ llvm/lib/Support/Unix/Signals.inc
@@ 

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In D69825#1733958 , @thakis wrote:

> Thanks for sending this out!
>
> Instead of the dynamic lookup of that symbol, what do you think about passing 
> in the function via a normal api? That way, the type checker and linker help 
> us keep things working; dynamic lookup is always a bit subtle and hard to 
> grep for. (llvm-cs wouldn't know about the current use, for example.) See 
> https://reviews.llvm.org/D69848 for a rough sketch.


Absolutely, it make more sense, I can't remember why I used the dynamic lookup 
:-( I have this patch lying around since last year.

> For robust crash handling, I figured on non-win we'd have a signal handler 
> that on signal self-execs with a "actually, do use a cc1 process" and then 
> use the existing crash machinery, but maybe that's not needed.

Right now on Windows I'm doing about that, not in the exception handler, but a 
bit later on in `clang/lib/Driver/Driver.cpp, L1338`. The worse that can happen 
would be a memory corruption, which would crash 
`Driver::generateCompilationDiagnostics()` and prevent displaying the 
preprocessed file and the cmd-line. But you would still see the callstack 
(which is rendered now in the SEH, see 
`llvm/lib/Support/CrashRecoveryContext.cpp, L194` - I still need to hook that 
up for the posix implementation). Ideally we could render some of these things 
in advance (or pre-allocate memory at least) to minimize the risk of a crash 
when displaying crash diagnostics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Thanks for the feedback @Meinersbur!

This patch is mainly geared towards Windows users. I'm not expecting anything 
on Linux, you already have all the bells & whistles there :-) Although I 
definitely see improvements on my Linux box. Would the distro make a 
difference? Mine is:

  $ uname -a
  Linux (name edited) 5.0.0-29-generic #31~18.04.1-Ubuntu SMP Thu Sep 12 
18:29:21 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux



In D69825#1733254 , @Meinersbur wrote:

> This patch reduced the build time from 12 to 7 minutes? Sounds too good to be 
> true.


Starting up and cooling down a large process like clang on Windows is very 
expensive. A fair amount of cpu time is spent in the clang driver, essentially 
in the kernel, in the range of 30 - 100 ms per process:
//(timings below are before this patch)//
F10630335: procmon-clang-cl.PNG 

We don't have `fork()` on Windows meaning that the kernel needs to re-start a 
the cc1 process from scratch, allocate pages, remap the EXE, allocate heap, 
allocate TLS, start the CRT, etc. Also, `InitLLVM::InitLLVM` is expensive 
because it calls into `dbghelp.dll` and loads symbols - just that can sometimes 
take up to 10 ms per process (depending on the system load).
In addition, recent Windows builds have all sorts of kernel regressions related 
to process creation and virtual pages allocation. Bruce Dawson 
 has 
several blog entries about all this.
We're simply mitigating these issues. Upgrading the 36-core server to Windows 
10 build 1903 will probably decrease the gap (12 min -> 7 min). However I would 
still expect users of Clang on pre-1903 builds for a few years from now.

I will fix the other issues you've mentioned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Thanks for sending this out!

Instead of the dynamic lookup of that symbol, what do you think about passing 
in the function via a normal api? That way, the type checker and linker help us 
keep things working; dynamic lookup is always a bit subtle and hard to grep 
for. (llvm-cs wouldn't know about the current use, for example.) See 
https://reviews.llvm.org/D69848 for a rough sketch.

For robust crash handling, I figured on non-win we'd have a signal handler that 
on signal self-execs with a "actually, do use a cc1 process" and then use the 
existing crash machinery, but maybe that's not needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: clang/lib/Driver/Job.cpp:340-355
+  typedef int (*ClangDriverMainFunc)(SmallVectorImpl &);
+  ClangDriverMainFunc ClangDriverMain = nullptr;
+
+  if (ReenterTool) {
+StringRef F = llvm::sys::path::filename(Executable);
+if (F.endswith_lower(".exe"))
+  F = F.drop_back(4);

Meinersbur wrote:
> This looks fragile as it will break when the user chooses to rename the 
> executable (`clang-cuda`, `--driver-mode=...`,...). Why not moving the 
> ClangDriverMain logic into the clangDriver module. Or, at least, pass it 
> through a lambda. It would also make it easier to use clangDriver as a 
> library (if that was ever an intended use-case?!?)
I tried this on Linux and it did not work out-of-the box (`ClangDriverMain` 
being nullptr) due to the executable's name being `clang-10`. After fixing, 
`check-clang` failed with 5 errors in the Driver test.

The performance benefit was within noise: 6m20s vs. 6m18s on a 2-Socket 28 
cores-per-processor (112 SMT threads) Skylake-Xeon system for `ninja all 
check-clang`, assertions enabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

This patch reduced the build time from 12 to 7 minutes? Sounds too good to be 
true. Thanks for the work! I would primarily interested in this to avoid 
determining the `-cc1` command line when debugging.

However, the implementation of calling it's own main function gives the 
impression to be more like a hack. How much restructuring would be needed to 
properly create a CompilerInstance inside the driver instead?




Comment at: clang/lib/Driver/Job.cpp:340-355
+  typedef int (*ClangDriverMainFunc)(SmallVectorImpl &);
+  ClangDriverMainFunc ClangDriverMain = nullptr;
+
+  if (ReenterTool) {
+StringRef F = llvm::sys::path::filename(Executable);
+if (F.endswith_lower(".exe"))
+  F = F.drop_back(4);

This looks fragile as it will break when the user chooses to rename the 
executable (`clang-cuda`, `--driver-mode=...`,...). Why not moving the 
ClangDriverMain logic into the clangDriver module. Or, at least, pass it 
through a lambda. It would also make it easier to use clangDriver as a library 
(if that was ever an intended use-case?!?)



Comment at: clang/tools/driver/driver.cpp:337
+
+int ClangDriverMain(SmallVectorImpl ) {
+  static LLVM_THREAD_LOCAL bool EnterPE = true;

Could you add a comment for the purpose of this function? It's not obvious 
outside the context of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked 3 inline comments as done.
aganea added a comment.

A few remarks:




Comment at: clang/lib/Driver/Job.cpp:319
 
+LLVM_THREAD_LOCAL bool Command::ReenterTool = true;
+

See my other comment about `LLVM_THREAD_LOCAL`



Comment at: clang/tools/driver/driver.cpp:338
+int ClangDriverMain(SmallVectorImpl ) {
+  static LLVM_THREAD_LOCAL bool EnterPE = true;
+  if (EnterPE) {

`LLVM_THREAD_LOCAL` is here for the `llvm-buildozer` application I've presented 
at the LLVM conference. It's there because `main()` is entered from several 
threads, thus all clang needs to be thread-safe (I have several other patches 
to achieve that).
If you want this patch without `LLVM_THREAD_LOCAL` I'm fine with that.



Comment at: llvm/lib/Support/Windows/Signals.inc:814
+
+  return EXCEPTION_EXECUTE_HANDLER;
 }

When this function is entered through the global exception handler, returning 
`EXCEPTION_EXECUTE_HANDLER` lets the CRT do its own exit with the exception 
code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 227779.
aganea added a comment.
Herald added a subscriber: dexonsmith.

Fix missing `llvm::InitializeAllTargets()` in driver.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825

Files:
  clang/include/clang/Driver/Job.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/Job.cpp
  clang/tools/driver/driver.cpp
  llvm/include/llvm/Support/CrashRecoveryContext.h
  llvm/lib/Support/CrashRecoveryContext.cpp
  llvm/lib/Support/Windows/Signals.inc

Index: llvm/lib/Support/Windows/Signals.inc
===
--- llvm/lib/Support/Windows/Signals.inc
+++ llvm/lib/Support/Windows/Signals.inc
@@ -187,7 +187,7 @@
 using namespace llvm;
 
 // Forward declare.
-static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep);
+LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep);
 static BOOL WINAPI LLVMConsoleCtrlHandler(DWORD dwCtrlType);
 
 // The function to call if ctrl-c is pressed.
@@ -521,10 +521,13 @@
 extern "C" VOID WINAPI RtlCaptureContext(PCONTEXT ContextRecord);
 #endif
 
-void llvm::sys::PrintStackTrace(raw_ostream ) {
-  STACKFRAME64 StackFrame = {};
-  CONTEXT Context = {};
-  ::RtlCaptureContext();
+static void LocalPrintStackTrace(raw_ostream , PCONTEXT C) {
+  STACKFRAME64 StackFrame{};
+  CONTEXT Context{};
+  if (!C) {
+::RtlCaptureContext();
+C = 
+  }
 #if defined(_M_X64)
   StackFrame.AddrPC.Offset = Context.Rip;
   StackFrame.AddrStack.Offset = Context.Rsp;
@@ -546,9 +549,12 @@
   StackFrame.AddrStack.Mode = AddrModeFlat;
   StackFrame.AddrFrame.Mode = AddrModeFlat;
   PrintStackTraceForThread(OS, GetCurrentProcess(), GetCurrentThread(),
-   StackFrame, );
+   StackFrame, C);
 }
 
+void llvm::sys::PrintStackTrace(raw_ostream ) {
+  LocalPrintStackTrace(OS, nullptr);
+}
 
 void llvm::sys::SetInterruptFunction(void (*IF)()) {
   RegisterHandler();
@@ -785,7 +791,7 @@
   return std::error_code();
 }
 
-static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep) {
+LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep) {
   Cleanup();
 
   // We'll automatically write a Minidump file here to help diagnose
@@ -803,42 +809,9 @@
<< "\n";
   }
 
-  // Initialize the STACKFRAME structure.
-  STACKFRAME64 StackFrame = {};
-
-#if defined(_M_X64)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Rip;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Rsp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Rbp;
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#elif defined(_M_IX86)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Eip;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Esp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Ebp;
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#elif defined(_M_ARM64) || defined(_M_ARM)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Pc;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Sp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-#if defined(_M_ARM64)
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Fp;
-#else
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->R11;
-#endif
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#endif
-
-  HANDLE hProcess = GetCurrentProcess();
-  HANDLE hThread = GetCurrentThread();
-  PrintStackTraceForThread(llvm::errs(), hProcess, hThread, StackFrame,
-   ep->ContextRecord);
-
-  _exit(ep->ExceptionRecord->ExceptionCode);
+  LocalPrintStackTrace(llvm::errs(), ep->ContextRecord);
+
+  return EXCEPTION_EXECUTE_HANDLER;
 }
 
 static BOOL WINAPI LLVMConsoleCtrlHandler(DWORD dwCtrlType) {
Index: llvm/lib/Support/CrashRecoveryContext.cpp
===
--- llvm/lib/Support/CrashRecoveryContext.cpp
+++ llvm/lib/Support/CrashRecoveryContext.cpp
@@ -13,6 +13,9 @@
 #include "llvm/Support/ThreadLocal.h"
 #include 
 #include 
+#if defined(_WIN32)
+#include 
+#endif
 using namespace llvm;
 
 namespace {
@@ -54,7 +57,7 @@
 #endif
   }
 
-  void HandleCrash() {
+  void HandleCrash(int retCode) {
 // Eliminate the current context entry, to avoid re-entering in case the
 // cleanup code crashes.
 CurrentContext->set(Next);
@@ -62,6 +65,8 @@
 assert(!Failed && "Crash recovery context already failed!");
 Failed = true;
 
+CRC->RetCode = retCode;
+
 // FIXME: Stash the backtrace.
 
 // Jump back to the RunSafely we were called under.
@@ -171,6 +176,9 @@
 static void installExceptionOrSignalHandlers() {}
 static void uninstallExceptionOrSignalHandlers() {}
 
+// In Signals.inc
+LONG WINAPI 

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea created this revision.
aganea added reviewers: thakis, hans, rnk.
aganea added a project: clang.
Herald added subscribers: llvm-commits, cfe-commits, hiraditya.
Herald added a reviewer: jfb.
Herald added a project: LLVM.

This patch is an optimization for speed: it will bypass the cc1 process 
invocation when possible, and instead will re-enter the driver process.
On Windows, this has a great impact on build times:

F10626773: clang-bypass-cc1-6-core.png 

F10626775: clang-bypass-cc1-36-core.png 

CFG (control flow guard) is disabled on Windows.
//Side-note: I've also observed an improvement when switching to Windows 10 
build 1903: all builds are faster, generally by ~2-3 min on the 6-core 
machine.//

The cc1 bypass mildly improves build times on Linux as well (although it's a 
old machine):

F10626777: clang-bypass-cc1-6-core-linux.png 


All tests pass (ninja check-all) on Windows with the following config:

  cmake -G"Ninja" %ROOT%/llvm -DCMAKE_BUILD_TYPE=Release 
-DLLVM_OPTIMIZED_TABLEGEN=true -DLLVM_ENABLE_ASSERTIONS=ON 
-DLLVM_ENABLE_LIBXML2=OFF -DLLVM_USE_CRT_RELEASE=MT 
-DCMAKE_C_COMPILER="C:/Program Files/LLVM/bin/clang-cl.exe" 
-DCMAKE_CXX_COMPILER="C:/Program Files/LLVM/bin/clang-cl.exe" 
-DCMAKE_LINKER="c:/Program Files/LLVM/bin/lld-link.exe" -DLLVM_ENABLE_LLD=true 
-DCMAKE_CXX_FLAGS="/arch:AVX /GS- /D_ITERATOR_DEBUG_LEVEL=0" 
-DCMAKE_C_FLAGS="/arch:AVX /GS- /D_ITERATOR_DEBUG_LEVEL=0" 
-DLLVM_ENABLE_PDB=true 
-DLLVM_ENABLE_PROJECTS="clang;lld;clang-tools-extra;compiler-rt"

..then followed by a second stage build.

As for re-entering `main()` in the code:
In some cases, when using clang-as-a-library, we cannot re-enter `main()`; and 
we shouldn't take the address of `main()` as per [C++11: 3.6.1/3] (that's what 
I meant at the LLVM conf. by saying that "we shouldn't be doing that"). I 
therefore used `llvm::sys::DynamicLibrary::AddSymbol` to keep a weak link to 
`main()`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69825

Files:
  clang/include/clang/Driver/Job.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/Job.cpp
  clang/tools/driver/driver.cpp
  llvm/include/llvm/Support/CrashRecoveryContext.h
  llvm/lib/Support/CrashRecoveryContext.cpp
  llvm/lib/Support/Windows/Signals.inc

Index: llvm/lib/Support/Windows/Signals.inc
===
--- llvm/lib/Support/Windows/Signals.inc
+++ llvm/lib/Support/Windows/Signals.inc
@@ -187,7 +187,7 @@
 using namespace llvm;
 
 // Forward declare.
-static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep);
+LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep);
 static BOOL WINAPI LLVMConsoleCtrlHandler(DWORD dwCtrlType);
 
 // The function to call if ctrl-c is pressed.
@@ -521,10 +521,13 @@
 extern "C" VOID WINAPI RtlCaptureContext(PCONTEXT ContextRecord);
 #endif
 
-void llvm::sys::PrintStackTrace(raw_ostream ) {
-  STACKFRAME64 StackFrame = {};
-  CONTEXT Context = {};
-  ::RtlCaptureContext();
+static void LocalPrintStackTrace(raw_ostream , PCONTEXT C) {
+  STACKFRAME64 StackFrame{};
+  CONTEXT Context{};
+  if (!C) {
+::RtlCaptureContext();
+C = 
+  }
 #if defined(_M_X64)
   StackFrame.AddrPC.Offset = Context.Rip;
   StackFrame.AddrStack.Offset = Context.Rsp;
@@ -546,9 +549,12 @@
   StackFrame.AddrStack.Mode = AddrModeFlat;
   StackFrame.AddrFrame.Mode = AddrModeFlat;
   PrintStackTraceForThread(OS, GetCurrentProcess(), GetCurrentThread(),
-   StackFrame, );
+   StackFrame, C);
 }
 
+void llvm::sys::PrintStackTrace(raw_ostream ) {
+  LocalPrintStackTrace(OS, nullptr);
+}
 
 void llvm::sys::SetInterruptFunction(void (*IF)()) {
   RegisterHandler();
@@ -785,7 +791,7 @@
   return std::error_code();
 }
 
-static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep) {
+LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep) {
   Cleanup();
 
   // We'll automatically write a Minidump file here to help diagnose
@@ -803,42 +809,9 @@
<< "\n";
   }
 
-  // Initialize the STACKFRAME structure.
-  STACKFRAME64 StackFrame = {};
-
-#if defined(_M_X64)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Rip;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Rsp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Rbp;
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#elif defined(_M_IX86)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Eip;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Esp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Ebp;
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#elif defined(_M_ARM64) || defined(_M_ARM)
-  StackFrame.AddrPC.Offset =