Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

rt.sections_elf_shared: Replace (almost) all runtime assertions by abort() #2324

Merged
merged 1 commit into from
Oct 17, 2018
Merged

rt.sections_elf_shared: Replace (almost) all runtime assertions by abort() #2324

merged 1 commit into from
Oct 17, 2018

Conversation

kinke
Copy link
Contributor

@kinke kinke commented Oct 9, 2018

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 kinke requested a review from Burgos as a code owner October 9, 2018 21:15
@dlang-bot
Copy link
Contributor

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 verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

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 references

Your 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 locally

If 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 kinke force-pushed the sections_upstream2 branch from 757302b to cf808c2 Compare October 9, 2018 21:47
n8sh
n8sh reviewed Oct 9, 2018
View reviewed changes
src/rt/sections_elf_shared.d
@@ -522,8 +531,8 @@ version (Shared)
void decThreadRef(DSO* pdso, bool decAdd)
{
auto tdata = findThreadDSO(pdso);
tdata !is null || assert(0);
!decAdd || tdata._addCnt > 0 || assert(0, "Mismatching rt_unloadLibrary call.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

n8sh
n8sh reviewed Oct 9, 2018
View reviewed changes
src/rt/sections_elf_shared.d Outdated
@@ -59,6 +59,12 @@ import rt.minfo;
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
n8sh approved these changes Oct 9, 2018
View reviewed changes
Copy link
Member

@n8sh n8sh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only objection I can imagine is that this is heavier than assert(0) but none of this appears to be performance-critical.

@Geod24
Copy link
Member

Geod24 commented Oct 10, 2018

@kinke : That looks like a perfect use case for dlang/dmd#8718

@PetarKirov
Copy link
Member

PetarKirov commented Oct 10, 2018 •
edited
Loading

@Geod24 Actually no, because the switch affects all asserts in compiled modules and we need something specific to code that runs before rt_init() returns / while new threads are initialized.

Geod24
Geod24 approved these changes Oct 10, 2018
View reviewed changes
Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor nit on the helper function

src/rt/sections_elf_shared.d Outdated
@@ -59,6 +59,12 @@ import rt.minfo;
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private + DDOC would be perfect

@n8sh
Copy link
Member

n8sh commented Oct 10, 2018 •
edited
Loading

Will amend to include niggling suggestions then automerge.

@n8sh
Copy link
Member

n8sh commented Oct 10, 2018

Will amend to include niggling suggestions then automerge.

Nevermind, not editable by third parties.

@kinke
Copy link
Contributor Author

kinke commented Oct 10, 2018

Nits addressed.

not editable by third parties

(Of course) not disallowed from my side, 'allow edits from maintainers' is checked.

This was referenced Oct 10, 2018
Closed
Merged
@n8sh
Copy link
Member

n8sh commented Oct 17, 2018

For whatever reason I can't squash this. As soon as that's done this should be merged.

@kinke
rt.sections_elf_shared: Replace (almost) all runtime assertions by ab…
9061ae9 
…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 kinke force-pushed the sections_upstream2 branch from f8f0107 to 9061ae9 Compare October 17, 2018 18:55
@kinke
Copy link
Contributor Author

kinke commented Oct 17, 2018

Squashed manually.

@PetarKirov
Copy link
Member

Auto-merge toggled on

@PetarKirov PetarKirov merged commit aa00905 into dlang:master Oct 17, 2018
@kinke kinke deleted the sections_upstream2 branch October 17, 2018 23:39
@joakim-noah joakim-noah mentioned this pull request Oct 25, 2018
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@n8sh n8sh n8sh approved these changes

@PetarKirov PetarKirov PetarKirov approved these changes

@Geod24 Geod24 Geod24 approved these changes

@Burgos Burgos Awaiting requested review from Burgos

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kinke @dlang-bot @Geod24 @PetarKirov @n8sh