Gracefully handle assertion errors during runtime startup/teardown

[dnadlinger]

assert() is used throughout the various bits and pieces involved in druntime startup and teardown. However, the default assert handler (throwing an AssertError) requires the GC and exception handling to be up and running, which is not the case for failures e.g. in the module registry code.

Of course, these assertions should never be triggered in shipped versions of druntime. But they might be during development, and this change ensures that a sensible error message is printed instead of hitting an infinite recursion or a GC deadlock.

@MartinNowak

[dnadlinger]

Fixed the line-number-sensitive test case.

[dnadlinger]

The coverage hit can't really be avoided unless we want to add a special flag to make druntime assert during startup.

[WalterBright]

The coverage hit can't really be avoided unless we want to add a special flag to make druntime assert during startup.

That's why the coverage test is advisory, not definitive!

[WalterBright]

There's no reason to new an assert error anyway. The program is aborting; no collection will ever be done. The data can be allocated via malloc, it can be statically allocated, or it can even just throw AssertError.init.

[MartinNowak]

Can we fix/ban the usage of assert instead of patching the assertHandler for everyone? I also find that adding even more things to the startup order becomes problematic. We could replace asserts w/ some internal_assert helper and use throw staticError!AssertError for failures. Not sure about compiler generated asserts though, are those a problem?

[WalterWaldron]

This type of issue doesn't only arise for startup / teardown. I had found some asserts in critical sections of thread.d, which I intend to finalize a PR for (but got delayed.) Doing anything other than abort'ing in those cases is extremely questionable (you can't do async signal unsafe things like call malloc.)

I think we should create a policy / checklist regarding use of assert within druntime and review existing uses.

[rainers]

I think calling abort if the GC isn't available is the right approach. Using a statically allocated AssertError could be ok, too, but should probably be thread local. It needs to disable exception chaining omehow, though.

Can we fix/ban the usage of assert instead of patching the assertHandler for everyone? We could replace asserts w/ some internal_assert helper

That doesn't work if the assert is in some commonly used function called both at startup and during normal operation.

I had found some asserts in critical sections of thread.d

The asserts inside the GC are also pretty bad, because they get hidden by a different error or corrupt the GC.

A couple of issues regardig this PR:

• needs a rebase
• shared module Ctors/Dtors should be executed with normal assertions, they are user code, not part of the runtime startup
• there is an assert in the Windows version of abort, that won't work well.

BTW: IMO making errors part of exception handling was not the best decision. An abort function that can be hooked with a (thread local) handler function would work just as well without confusion about non-existing guarantees of stack cleanup.

[PetarKirov]

Using a statically allocated AssertError could be ok, too, but should probably be thread local. It needs to disable exception chaining somehow, though.

Could something along the lines of https://github.com/dlang/druntime/blob/v2.075.0-b4/src/rt/deh.d#L22 and https://github.com/dlang/druntime/blob/v2.075.0-b4/src/core/exception.d#L702 be used to detect if an exception is statically allocated, so that the rest of the runtime would treat it appropriately?

[rainers]

Could something along the lines of https://github.com/dlang/druntime/blob/v2.075.0-b4/src/rt/deh.d#L22

Using the initializer is just one possibilty, even a pretty bad one, because you have to modify it to store location information and message. That precludes putting the initializer into readonly memory.

and https://github.com/dlang/druntime/blob/v2.075.0-b4/src/core/exception.d#L702 be used

staticError is better.

so that the rest of the runtime would treat it appropriately?

What would be appropriate? Aborting immediately if trying to chain the same exception again?

[nemanja-boric-sociomantic]

Yeah, the exception chaining is a no-go for staticError. I've experienced the same problem in #1872 - chaining staticError causes entire exception handling to get stuck in the infinite loop, traversing the chain. I think aborting in exception handler when trying to chain the throwable to itself is a good option.

[PetarKirov]

@rainers I meant that we need to do something on top of what staticError currently does, so that exception handling part of the runtime is staticError aware. I.e. someway to tag the exception object, so the runtime does not try to GC allocate memory, chain exceptions, etc. May be that way we could support truly immutable exceptions that the runtime does not modify in any way (e.g. setting the file/line/msg/next fields).