Skip to content
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

fix: add inc for double resets #4228

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

anfelor
Copy link

@anfelor anfelor commented May 20, 2024

@bollu and I also ran into the double reset bug a while ago (sorry for not reporting this earlier -- we didn't have a good MWE and put it off at first). However, we fixed it slightly differently: The reference counting can still be sound if there are two reset instructions on the same variable, but then an inc has to be generated before the first reset.

The underlying reason for the bug is thus that Figure 5 of https://pp.ipd.kit.edu/uploads/publikationen/ullrich19counting.pdf has a wrong reset case, where the O^+ function is not called on resets, even though the variable to be reset should be owned (compare to the Ty-Reset rule of https://lean-lang.org/papers/beans_appendix.pdf). In contrast, Koka used to call O^+ in this case before we switched to drop-guided reuse analysis.

This PR fixes the issue by calling O^+ on reset. This might make the fix in #4028 easier to maintain since it prevents segfaults even if there are two reset instructions on the same variable.

@anfelor anfelor requested a review from leodemoura as a code owner May 20, 2024 12:24
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label May 20, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

Mathlib CI status (docs):

  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 42215cc0728fbee0705acf87995a8720ec8bcc0d --onto f53b778c0d4a474383eab709d67db5e12357f39d. (2024-05-20 12:42:14)

@anfelor anfelor changed the title Add inc for double resets fix: add inc for double resets May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants