Page MenuHomePhabricator

[Support] Move the static initializer install_out_memory_new_handler to InitLLVM
ClosedPublic

Authored by MaskRay on Jul 10 2019, 9:10 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Jul 10 2019, 9:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2019, 9:10 AM
MaskRay updated this revision to Diff 208985.Jul 10 2019, 9:19 AM

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 added a comment.EditedJul 10 2019, 9:21 AM

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 added a subscriber: jfb.Jul 10 2019, 9:47 AM

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

rnk added a comment.Jul 10 2019, 2:22 PM
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 added a comment.Jul 10 2019, 3:43 PM
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 added a comment.Jul 10 2019, 3:50 PM
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 added a comment.Jul 10 2019, 3:53 PM
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?

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 added a comment.Jul 11 2019, 10:07 AM

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 added a comment.Jul 11 2019, 11:38 AM
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 updated this revision to Diff 209395.Jul 11 2019, 6:49 PM

Add doc to InitLLVM.h
assert set_new_handler returns nullptr

MaskRay added a reviewer: jfb.Jul 11 2019, 6:49 PM
jfb accepted this revision.Jul 12 2019, 9:14 AM
This revision is now accepted and ready to land.Jul 12 2019, 9:14 AM
This revision was automatically updated to reflect the committed changes.