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

Heapster array permission bug fixes #1659

Merged
merged 8 commits into from
May 7, 2022
Merged

Conversation

eddywestbrook
Copy link
Contributor

This PR re-works the proveVarLLVMArray function to address a few infinite loops and other bugs in proving array permissions. It also adds more comments to document what that function is doing. Additionally, the new example alloc_sum_array_test has been added to arrays.c to exercise the fixes.

@eddywestbrook eddywestbrook added the Heapster Issues specifically related to the Heapster sub-system label May 5, 2022
@eddywestbrook eddywestbrook requested a review from m-yac May 5, 2022 23:53
…rows it computes as it computes them; fixed case 4 of proveVarLLVMArrayH so that it only uses the unborrowed ranges for permissions on the LHS
Copy link
Contributor

@m-yac m-yac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome! Your documentation comment above proveVarLLVMArrayH was incredibly helpful for me in understanding your algorithm. I did get confused in a few places though, and I noted what I thought might've helped me follow along better what was going on. No necessary changes though, if just want to get this merged.

heapster-saw/src/Verifier/SAW/Heapster/Implication.hs Outdated Show resolved Hide resolved
heapster-saw/src/Verifier/SAW/Heapster/Implication.hs Outdated Show resolved Hide resolved
heapster-saw/src/Verifier/SAW/Heapster/Implication.hs Outdated Show resolved Hide resolved
heapster-saw/src/Verifier/SAW/Heapster/Implication.hs Outdated Show resolved Hide resolved
heapster-saw/src/Verifier/SAW/Heapster/Implication.hs Outdated Show resolved Hide resolved
Copy link
Contributor

@m-yac m-yac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! The "ways" of the top-level comment not lining up with the "cases" makes everything make so much more sense. Also I really like how you re-worded the comment for case 3. LGTM!

@m-yac m-yac added the ready-to-merge If at least one person approves this PR, automatically merge it when CI finishes successfully. label May 6, 2022
@mergify mergify bot merged commit 2f48685 into master May 7, 2022
@mergify mergify bot deleted the heapster/debug-array-perms branch May 7, 2022 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Heapster Issues specifically related to the Heapster sub-system ready-to-merge If at least one person approves this PR, automatically merge it when CI finishes successfully.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants