-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove the use of usrToCell in gcMark [backport:1.2] #17709
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Recently, we've discovered a GC crash resulting from inlining of the memory allocation procs that allowed the compiler to avoid maintaining any references to the "user pointer" on the stack. Instead, a "cell pointer" appeared there and all field accesses were performed with adjusted offsets. This interfered with the ability of the GC to mark the correct cell in the conservative stack scans which lead to premature collection of objects. More details here: status-im@af69b3c This commit closes another theoretical loophole that may lead to the same problem. If a short proc is accessing both the object and its reference count in a short sequence of instructions, the compiler may be enticed to reduce the number of registers being used by storing only a single pointer to the object and using offsets when reading and writing fields. A perfectly good strategy would be to store only the cell pointer, so the reference count updates can be performed without applying offsets. Accessing the fields of the object requires offsets anyway, but these can be adjusted at compile-time without any loss. Following this strategy will lead to the same problem of marking a wrong cell during the conservative stack scan, leading to premature collection. The problem is avoided by not using `usrToCell` in `gcMark`. Since the cell discovery logic can already handle interior pointers, the user pointers don't need to be adjusted for the GC to function correctly.
narimiran
pushed a commit
that referenced
this pull request
Apr 14, 2021
* Remove the use of usrToCell in gcMark [backport:1.2] Recently, we've discovered a GC crash resulting from inlining of the memory allocation procs that allowed the compiler to avoid maintaining any references to the "user pointer" on the stack. Instead, a "cell pointer" appeared there and all field accesses were performed with adjusted offsets. This interfered with the ability of the GC to mark the correct cell in the conservative stack scans which lead to premature collection of objects. More details here: status-im@af69b3c This commit closes another theoretical loophole that may lead to the same problem. If a short proc is accessing both the object and its reference count in a short sequence of instructions, the compiler may be enticed to reduce the number of registers being used by storing only a single pointer to the object and using offsets when reading and writing fields. A perfectly good strategy would be to store only the cell pointer, so the reference count updates can be performed without applying offsets. Accessing the fields of the object requires offsets anyway, but these can be adjusted at compile-time without any loss. Following this strategy will lead to the same problem of marking a wrong cell during the conservative stack scan, leading to premature collection. The problem is avoided by not using `usrToCell` in `gcMark`. Since the cell discovery logic can already handle interior pointers, the user pointers don't need to be adjusted for the GC to function correctly. (cherry picked from commit 3b47a68)
narimiran
pushed a commit
that referenced
this pull request
Apr 14, 2021
* Remove the use of usrToCell in gcMark [backport:1.2] Recently, we've discovered a GC crash resulting from inlining of the memory allocation procs that allowed the compiler to avoid maintaining any references to the "user pointer" on the stack. Instead, a "cell pointer" appeared there and all field accesses were performed with adjusted offsets. This interfered with the ability of the GC to mark the correct cell in the conservative stack scans which lead to premature collection of objects. More details here: status-im@af69b3c This commit closes another theoretical loophole that may lead to the same problem. If a short proc is accessing both the object and its reference count in a short sequence of instructions, the compiler may be enticed to reduce the number of registers being used by storing only a single pointer to the object and using offsets when reading and writing fields. A perfectly good strategy would be to store only the cell pointer, so the reference count updates can be performed without applying offsets. Accessing the fields of the object requires offsets anyway, but these can be adjusted at compile-time without any loss. Following this strategy will lead to the same problem of marking a wrong cell during the conservative stack scan, leading to premature collection. The problem is avoided by not using `usrToCell` in `gcMark`. Since the cell discovery logic can already handle interior pointers, the user pointers don't need to be adjusted for the GC to function correctly. (cherry picked from commit 3b47a68)
sthagen
added a commit
to sthagen/nim-lang-Nim
that referenced
this pull request
Apr 14, 2021
Remove the use of usrToCell in gcMark [backport:1.2] (nim-lang#17709)
PMunch
pushed a commit
to PMunch/Nim
that referenced
this pull request
Mar 28, 2022
* Remove the use of usrToCell in gcMark [backport:1.2] Recently, we've discovered a GC crash resulting from inlining of the memory allocation procs that allowed the compiler to avoid maintaining any references to the "user pointer" on the stack. Instead, a "cell pointer" appeared there and all field accesses were performed with adjusted offsets. This interfered with the ability of the GC to mark the correct cell in the conservative stack scans which lead to premature collection of objects. More details here: status-im@af69b3c This commit closes another theoretical loophole that may lead to the same problem. If a short proc is accessing both the object and its reference count in a short sequence of instructions, the compiler may be enticed to reduce the number of registers being used by storing only a single pointer to the object and using offsets when reading and writing fields. A perfectly good strategy would be to store only the cell pointer, so the reference count updates can be performed without applying offsets. Accessing the fields of the object requires offsets anyway, but these can be adjusted at compile-time without any loss. Following this strategy will lead to the same problem of marking a wrong cell during the conservative stack scan, leading to premature collection. The problem is avoided by not using `usrToCell` in `gcMark`. Since the cell discovery logic can already handle interior pointers, the user pointers don't need to be adjusted for the GC to function correctly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Recently, we've discovered a GC crash resulting from inlining of
the memory allocation procs that allowed the compiler to avoid
maintaining any references to the "user pointer" on the stack.
Instead, a "cell pointer" appeared there and all field accesses
were performed with adjusted offsets. This interfered with the
ability of the GC to mark the correct cell in the conservative
stack scans which lead to premature collection of objects.
More details here:
status-im@af69b3c
This commit closes another theoretical loophole that may lead to
the same problem. If a short proc is accessing both the object and
its reference count in a short sequence of instructions, the compiler
may be enticed to reduce the number of registers being used by storing
only a single pointer to the object and using offsets when reading
and writing fields. A perfectly good strategy would be to store only
the cell pointer, so the reference count updates can be performed
without applying offsets. Accessing the fields of the object requires
offsets anyway, but these can be adjusted at compile-time without any
loss. Following this strategy will lead to the same problem of marking
a wrong cell during the conservative stack scan, leading to premature
collection.
The problem is avoided by not using
usrToCell
ingcMark
. Sincethe cell discovery logic can already handle interior pointers, the
user pointers don't need to be adjusted for the GC to function correctly.