rt.sections_elf_shared: Replace (almost) all runtime assertions by abort()#2324
rt.sections_elf_shared: Replace (almost) all runtime assertions by abort()#2324PetarKirov merged 1 commit intodlang:masterfrom
Conversation
|
Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + druntime#2324" |
kinke
force-pushed
the
sections_upstream2
branch
from
757302b to
cf808c2
Compare
| { | ||
| auto tdata = findThreadDSO(pdso); | ||
| tdata !is null || assert(0); | ||
| !decAdd || tdata._addCnt > 0 || assert(0, "Mismatching rt_unloadLibrary call."); |
There was a problem hiding this comment.
What do you think of using the style condition || abort("message")? abort already does the trick of auto-filling the file name and line number.
There was a problem hiding this comment.
I don't like it too much (and haven't seen it much in druntime/Phobos), that's why I decided to wrap it with an extra helper function.
src/rt/sections_elf_shared.d
Outdated
| import rt.util.container.array; | ||
| import rt.util.container.hashtab; | ||
|
|
||
| void safeAssert(bool condition, scope string msg, scope string filename = __FILE__, size_t line = __LINE__) @nogc nothrow @safe |
There was a problem hiding this comment.
This would be a sensible place to include your comment
Normal assertions (throwing AssertErrors) aren't really usable as the GC may not be initialized (yet or anymore).
n8sh
left a comment
n8sh
left a comment
There was a problem hiding this comment.
The only objection I can imagine is that this is heavier than assert(0) but none of this appears to be performance-critical.
|
@kinke : That looks like a perfect use case for dlang/dmd#8718 |
|
@Geod24 Actually no, because the switch affects all asserts in compiled modules and we need something specific to code that runs before |
Geod24
left a comment
Geod24
left a comment
There was a problem hiding this comment.
Just a minor nit on the helper function
src/rt/sections_elf_shared.d
Outdated
| import rt.util.container.array; | ||
| import rt.util.container.hashtab; | ||
|
|
||
| void safeAssert(bool condition, scope string msg, scope string filename = __FILE__, size_t line = __LINE__) @nogc nothrow @safe |
|
|
Nevermind, not editable by third parties. |
|
Nits addressed.
(Of course) not disallowed from my side, 'allow edits from maintainers' is checked. |
|
For whatever reason I can't squash this. As soon as that's done this should be merged. |
…ort() An assert(0) in the release libs yields an "invalid instruction" error, which isn't very helpful for troubleshooting. Normal assertions (throwing AssertErrors) aren't really usable as the GC may not be initialized (yet or anymore).
kinke
force-pushed
the
sections_upstream2
branch
from
f8f0107 to
9061ae9
Compare
|
Squashed manually. |
|
Auto-merge toggled on |
5 participants
An
assert(0)in the release libs yields an "invalid instruction" error, which isn't very helpful for troubleshooting.Normal assertions (throwing
AssertErrors) aren't really usable as the GC may not be initialized (yet or anymore).