-
Notifications
You must be signed in to change notification settings - Fork 832
wasm-ctor-eval: Do not emit invalid code after hitting a GC cycle #8188
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
Conversation
| ;; A circular reference using a non-nullable (but mutable) field. Unlike cases | ||
| ;; above, we cannot break up such cycles, and must give up. We should at least | ||
| ;; not error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this test show that continuing after the invalid state is cleared works as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is what the global.set does. That change is not updated, because we throw all the state away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I would have expected to see changes from precomputation that happens after the invalid state is cleared. Or am I misunderstanding whether that is expected to be possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't continue after we hit the cycle. In general, when we hit something we can't fully precompute, we halt. (Though in theory, later code could undo a cycle... but we don't bet on such luck.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. "losing any evalling work thus far" in the description made me think that there might be some later evalling work would not be lost.
We break up cycles during serialization of GC data, but we can't always do
so: if the cycle uses immutable and non-nullable fields, we can't write a null
first and close the loop later. Before this PR, we would error inside the
topological sort utility; instead, catch an error from there, and stop our
work.
Unfortunately, it is complicated to detect this situation ahead of time: we
only hit a problem on the specific type of cycle mentioned before, and we
do so while already modifying the module state. Rather than do a large
refactoring to handle this, just give up on the state in memory, clearing it,
losing any evalling work thus far. If this becomes important enough, we
can consider handling it more gracefully later.