[Support] Move the static initializer install_out_memory_new_handler to InitLLVM

[MaskRay]

Summary

An application linking against LLVMSupport should not get the gratuitous
set::std_new_handler call.

[MaskRay]

Update comment and delete unnecessary out_of_memory_new_handler_installed

std::set_new_handler just sets a global state. It is very cheap. Not really necessary to guard it with another variable.

[MaskRay]

It was reported that this piece of innocent OpenGL code http://paste.debian.net/1091081/ errors LLVM ERROR: out of memory when AMD GPU is used...

[jfb]

Does it make sense to reset any previous handler in llvm_shutdown?

[rnk]

In D64505#1578699, @jfb wrote:

Does it make sense to reset any previous handler in llvm_shutdown?

Doing so would require saving the current handler somewhere, if one exists.

InitLLVM really only works if you call it in main at the start of the program (see how it rewrites argv). If a user wants to initialize LLVM as a library and then shut it down, I would expect the user to avoid InitLLVM and instead use llvm_shutdown directly. With this change, they won't get LLVM's OOM handler, but that seems desirable. It's easy for them to set their own to one that is equivalent to LLVM's if desired.

[jfb]

In D64505#1579335, @rnk wrote:

In D64505#1578699, @jfb wrote:

Does it make sense to reset any previous handler in llvm_shutdown?

Doing so would require saving the current handler somewhere, if one exists.

That's easy.

InitLLVM really only works if you call it in main at the start of the program (see how it rewrites argv). If a user wants to initialize LLVM as a library and then shut it down, I would expect the user to avoid InitLLVM and instead use llvm_shutdown directly. With this change, they won't get LLVM's OOM handler, but that seems desirable. It's easy for them to set their own to one that is equivalent to LLVM's if desired.

So you're saying: maybe it's a bug if we install one but there was already one installed?

[rnk]

In D64505#1579577, @jfb wrote:

So you're saying: maybe it's a bug if we install one but there was already one installed?

Yes. And the user can call llvm_shutdown without setting creating an InitLLVM object, so llvm_shutdown shouldn't assume that LLVM's new handler is in use.

So, I think this change is fine as is:

• LLVM tools want this OOM handler, and they use InitLLVM to set up crash handlers
• Users of LLVM as a library should avoid InitLLVM and set their own OOM handler if they want one

[jfb]

In D64505#1579592, @rnk wrote:

In D64505#1579577, @jfb wrote:

So you're saying: maybe it's a bug if we install one but there was already one installed?

Yes. And the user can call llvm_shutdown without setting creating an InitLLVM object, so llvm_shutdown shouldn't assume that LLVM's new handler is in use.

So, I think this change is fine as is:

• LLVM tools want this OOM handler, and they use InitLLVM to set up crash handlers
• Users of LLVM as a library should avoid InitLLVM and set their own OOM handler if they want one

Is there a diagnostic that we can add to catch misuses for InitLLVM?

[MaskRay]

In D64505#1579593, @jfb wrote:

In D64505#1579592, @rnk wrote:

In D64505#1579577, @jfb wrote:

So you're saying: maybe it's a bug if we install one but there was already one installed?

Yes. And the user can call llvm_shutdown without setting creating an InitLLVM object, so llvm_shutdown shouldn't assume that LLVM's new handler is in use.

Yes. I could check std::get_new_handler, but I think that is not really necessary. Beside manging argv on Windows, InitLLVM installs signal handlers, which may not be desired in application code. I agree that InitLLVM should only be used by LLVM tools.

So, I think this change is fine as is:

• LLVM tools want this OOM handler, and they use InitLLVM to set up crash handlers
• Users of LLVM as a library should avoid InitLLVM and set their own OOM handler if they want one

Is there a diagnostic that we can add to catch misuses for InitLLVM?

I can imagine some out-of-tree LLVM tools don't bother doing their own signal handlers or new handler. I don't know how such tools (where LLVM is an integral part of them) can be differentiated from other tools (e.g. the AMD GPU driver that revealed the problem). So, I don't think a diagnostic is possible...

[jfb]

Let me rephrase: does it ever make sense to call InitLLVM if there's already a new handler installed? Or is it always a sign of trouble if that's done?

[rnk]

In D64505#1580928, @jfb wrote:

Let me rephrase: does it ever make sense to call InitLLVM if there's already a new handler installed? Or is it always a sign of trouble if that's done?

I think it would be reasonable to assert that get_new_handler returns null when we install ours. It's also worth checking the documentation for InitLLVM again:
http://llvm-cs.pcc.me.uk/include/llvm/Support/InitLLVM.h#19
We can mention the new handler after the signal handler there.

[MaskRay]

Add doc to InitLLVM.h
assert set_new_handler returns nullptr

llvm/trunk/include/llvm/Support/InitLLVM.h

 #include "llvm/Support/PrettyStackTrace.h"
 
 // The main() functions in typical LLVM tools start with InitLLVM which does
 // the following one-time initializations:
 //
 //  1. Setting up a signal handler so that pretty stack trace is printed out
 //     if a process crashes.
 //
-//  2. If running on Windows, obtain command line arguments using a
+//  2. Set up the global new-handler which is called when a memory allocation
+//     attempt fails.
+//
+//  3. If running on Windows, obtain command line arguments using a
 //     multibyte character-aware API and convert arguments into UTF-8
 //     encoding, so that you can assume that command line arguments are
 //     always encoded in UTF-8 on any platform.
 //
 // InitLLVM calls llvm_shutdown() on destruction, which cleans up
 // ManagedStatic objects.
 namespace llvm {
 class InitLLVM {

llvm/trunk/lib/Support/ErrorHandling.cpp

 }
 #else
 // Causes crash on allocation failure. It is called prior to the handler set by
 // 'install_bad_alloc_error_handler'.
 static void out_of_memory_new_handler() {
   llvm::report_bad_alloc_error("Allocation failed");
 }
 
-// Installs new handler that causes crash on allocation failure. It does not
-// need to be called explicitly, if this file is linked to application, because
-// in this case it is called during construction of 'new_handler_installer'.
+// Installs new handler that causes crash on allocation failure. It is called by
+// InitLLVM.
 void llvm::install_out_of_memory_new_handler() {
-  static bool out_of_memory_new_handler_installed = false;
-  if (!out_of_memory_new_handler_installed) {
-    std::set_new_handler(out_of_memory_new_handler);
-    out_of_memory_new_handler_installed = true;
-  }
-}
-
-// Static object that causes installation of 'out_of_memory_new_handler' before
-// execution of 'main'.
-static class NewHandlerInstaller {
-public:
-  NewHandlerInstaller() {
-    install_out_of_memory_new_handler();
-  }
-} new_handler_installer;
+  std::new_handler old = std::set_new_handler(out_of_memory_new_handler);
+  (void)old;
+  assert(old == nullptr && "new-handler already installed");
 #endif
 
 void llvm::llvm_unreachable_internal(const char *msg, const char *file,
                                      unsigned line) {
   // This code intentionally doesn't call the ErrorHandler callback, because
   // llvm_unreachable is intended to be used to indicate "impossible"
   // situations, and not legitimate runtime errors.
   if (msg)

llvm/trunk/lib/Support/InitLLVM.cpp

 #include "Windows/WindowsSupport.h"
 #endif
 
 using namespace llvm;
 using namespace llvm::sys;
 
 InitLLVM::InitLLVM(int &Argc, const char **&Argv) : StackPrinter(Argc, Argv) {
   sys::PrintStackTraceOnErrorSignal(Argv[0]);
+  install_out_of_memory_new_handler();
 
 #ifdef _WIN32
   // We use UTF-8 as the internal character encoding. On Windows,
   // arguments passed to main() may not be encoded in UTF-8. In order
   // to reliably detect encoding of command line arguments, we use an
   // Windows API to obtain arguments, convert them to UTF-8, and then
   // write them back to the Argv vector.
   //