fix bug in src/reassign.c - #640
Open
mpadge wants to merge 1 commit into
Open
Conversation
The struct copy in the C code wires 'new's formals/body/env directly into 'old_fun' without going through R's normal SET_FORMALS/SET_BODY/ SET_CLOENV setters. Those setters exist not just to assign the pointer, but also to run the generational-GC write barrier (so that an old-generation object such as 'old_fun', which may already have survived several collections, is flagged as now referencing potentially younger objects). Skipping that barrier is safe as long as the referenced objects can never be reclaimed while 'old_fun' still points to them. 'R_PreserveObject' is the public, API-stable way to guarantee that: it keeps an object alive for the remainder of the session regardless of the normal generational reachability scan. That permanently retains these (typically small) formals/body/env objects, which is an acceptable trade-off for a tracing tool to avoid the alternative: a GC collecting them out from under 'old_fun' the next time a minor collection runs, corrupting the traced function.
|
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Credit here, like it or not, to Claude for uncovering this one. I used to also use
SET_BODYin one package of mine. When R removed export of theSET_functions fromRinternals.h, I looked here for how you got around it, and ended up wholesale copying your approach - with full attribution!I encountered an odd bug in my code, and set Claude on a debugging session. It came up with this solution, which indeed works. This is an edited version of the Claude comment:
The struct copy in the C code wires 'new's formals/body/env directly into 'old_fun' without going through R's normal SET_FORMALS/SET_BODY/ SET_CLOENV setters. Those setters exist not just to assign the pointer, but also to run the generational-GC write barrier (so that an old-generation object such as 'old_fun', which may already have survived several collections, is flagged as now referencing potentially younger objects). Skipping that barrier is safe as long as the referenced objects can never be reclaimed while 'old_fun' still points to them. 'R_PreserveObject' is the public, API-stable way to guarantee that: it keeps an object alive for the remainder of the session regardless of the normal generational reachability scan. That permanently retains these (typically small) formals/body/env objects, which is an acceptable trade-off for a tracing tool to avoid the alternative: a GC collecting them out from under 'old_fun' the next time a minor collection runs, corrupting the traced function.