Fix issue 16046 - ScopedAllocator does not set prev, causing segfaults #4742
Conversation
|
|
@andralex, thanks for your PR! By analyzing the annotation information on this pull request, we identified @9rnsr, @JesseKPhillips and @WalterBright to be potential reviewers. @9rnsr: The PR was automatically assigned to you, please reassign it if you were identified mistakenly. |
andralex
force-pushed
the
16046
branch
7 times, most recently
from
9e4d527
to
98a8e68
Compare
Current coverage is 88.79% (diff: 100%)@@ master #4742 diff @@
==========================================
Files 121 121
Lines 74159 74167 +8
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 65845 65859 +14
+ Misses 8314 8308 -6
Partials 0 0
|
|
How do I see the lines covered with the Codecov tool? |
Either click on the "0538d0c...1b96629" diff link above or if you want the annotation directly in the GitHub diff you can install their browser extension: https://github.com/codecov/browser-extension edit: the failures seem unrelated (maybe due to CircleCi failing), so perhaps just rebasing to the latest master branch will solve this? |
|
reviews pliz pliz |
| @@ -89,6 +89,8 @@ struct ScopedAllocator(ParentAllocator) | |||
| toInsert.prev = null; | |||
| toInsert.next = root; | |||
| toInsert.length = n; | |||
| assert(!root || !root.prev); | |||
There was a problem hiding this comment.
Quite recently there was a longer discussion on a PR about the ~200 HALTs in Phobos that come without message, but segfault. Maybe we could tell the user at least what has gone wrong?
e.g. "Invalid allocation. Root node isn't at the top of the stack"
There was a problem hiding this comment.
On top of the doubly-linked list actually ;-)
As we are at this, I'd suggest to add "Please, file an issue" or something like that, because if the assertion triggers, it's not a precondition gone wrong from the user, but rather an implementation bug.
There was a problem hiding this comment.
This will be a regular assert, not a HALT, because it's not compile-time zero.
|
Looks good, modulo the note by Sebastian. |
| auto bar = alloc.make!int(2).enforce; | ||
| alloc.dispose(foo); | ||
| alloc.dispose(bar); // segfault here | ||
| } |
There was a problem hiding this comment.
Maybe we can add another test and deallocate in non-list order?
e.g.
alloc.dispose(el2);
alloc.dispose(el3);
alloc.dispose(el1);
alloc.dispose(el4);
Yep this looks a lot better than #4702 and if I could, I would pull this one ;-) |
|
@wilzbach gave you rights |
Thanks! As this PR is quite young (1 day) and I don't want to mess up my first merge at Phobos, I'll give this PR another day, s.t. everyone has a fair opportunity to shout ;-) |
|
Auto-merge toggled on |
This is a simpler fix than #4702.