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

Fix #10701 & 10838 #642

Merged
merged 4 commits into from
Oct 31, 2013
Merged

Fix #10701 & 10838 #642

merged 4 commits into from
Oct 31, 2013

Conversation

Safety0ff
Copy link
Contributor

@ghost
Copy link

ghost commented Oct 25, 2013

When I was debugging these issues, conclusion was that it is dmd optimization bug related to bit shifting (I was unable to reproduce bugs when druntime was compiled without -O -inline).

@Safety0ff
Copy link
Contributor Author

My conclusion is pn wraps around (goes negative due to substraction) in the case when bins == B_FREE and causes a segfault in the test(size_t) call via biti.

{
pn -= pool.bPageOffsets[pn];
biti = pn * (PAGESIZE >> pool.shiftBy);
}
else // bins == B_FREE
Copy link
Member

Choose a reason for hiding this comment

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

This should be an assert(bins == B_FREE).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I was just following examples from https://github.com/D-Programming-Language/druntime/blob/master/src/gc/gc.d#L1635 and https://github.com/D-Programming-Language/druntime/blob/master/src/gc/gc.d#L2298 I suppose they're making the assumption of the position of B_FREE in the anonymous enum.
I'll add the assert.

Copy link
Member

Choose a reason for hiding this comment

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

Implicit assumptions are never a good idea.
If I had added an assertion for bins == B_PAGEPLUS when writing isMarked we would have found the bug earlier.

- The test isn't 100% reliable because it depends
  on the GC and array append cache state.
@MartinNowak
Copy link
Member

I send you a pull request which adds a regression test (Safety0ff#1).

MartinNowak added a commit that referenced this pull request Oct 31, 2013
@MartinNowak MartinNowak merged commit 545b44b into dlang:master Oct 31, 2013
@Safety0ff Safety0ff deleted the gcismarkedfix branch May 25, 2014 17:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants