staticdata: Insert backedges recursively #57212
Conversation
In the new edges scheme, when we perform constant propagation, we create a dummy CodeInstance that is not inserted into the specializations cache, but instead simply serves as a container for all edges that were encountered during the constant propagation. These CodeInstances are not part of either the internal or external edges list collected during pkgimage generation. As such, while we were verifying edges recursively and would thus catch an invalidation prior to package image reload, we were failing to also insert backedges for these CodeInstance. We were thus failing to invalidate such methods if the method was redefined after re-load of the package image. Fix that by moving the storing of the backedges to the end of the validation code, so that it too happens recursively.
oscardssmith
added
bugfix
| end | ||
| if maxworld == current_world | ||
| Base.Compiler.store_backedges(child, child.edges) | ||
| @atomic :monotonic child.max_world = typemax(UInt) |
There was a problem hiding this comment.
The current_world may change while these backedge insertion is occurring. The part of this change that combined these separate atomics checks introduces an unsoundness here. This line needs to be put back into _insert_backedges: first all backedges must be inserted (for the cycle), and then, if the current_world is still the same, it can atomically change max_world => typemax to acknowledge that all of the backedges existed in the current world.
There was a problem hiding this comment.
Ok, and you're fine with capping the world age if that happened rather than revalidating?
There was a problem hiding this comment.
Well, yeah, we are supposed to revalidate later if we need this code later. We finally have the code for that written now (this file) but haven't finished hooking that up yet.
Addresses review comment in #57212 (comment). The key is that the hand-off of responsibility for verification between the loading code and the ordinary backedge mechanism happens under the world counter lock to ensure synchronization.
Addresses review comment in #57212 (comment). The key is that the hand-off of responsibility for verification between the loading code and the ordinary backedge mechanism happens under the world counter lock to ensure synchronization.
Addresses review comment in #57212 (comment). The key is that the hand-off of responsibility for verification between the loading code and the ordinary backedge mechanism happens under the world counter lock to ensure synchronization.
In the new edges scheme, when we perform constant propagation, we create a dummy CodeInstance that is not inserted into the specializations cache, but instead simply serves as a container for all edges that were encountered during the constant propagation. These CodeInstances are not part of either the internal or external edges list collected during pkgimage generation. As such, while we were verifying edges recursively and would thus catch an invalidation prior to package image reload, we were failing to also insert backedges for these CodeInstance. We were thus failing to invalidate such methods if the method was redefined after re-load of the package image. Fix that by moving the storing of the backedges to the end of the validation code, so that it too happens recursively.