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

Make termination checks for exec functions, add support for mutable references in parameters, and emit a warning that the check doesn't account for loops #238

Merged
merged 5 commits into from Aug 8, 2022

Conversation

utaal
Copy link
Collaborator

@utaal utaal commented Aug 4, 2022

I do not believe this opens to any potential unsoundness, especially because it's already possible to write a non-terminating recursive exec function with a loop.

Reported by @matthias-brun.

@utaal
Copy link
Collaborator Author

utaal commented Aug 4, 2022

I am not confident this is okay.

@utaal utaal force-pushed the exec-recursion-no-decrease branch from c860972 to 51d4d46 Compare August 4, 2022 12:07
@utaal utaal requested a review from tjhance August 4, 2022 16:43
@Chris-Hawblitzel
Copy link
Collaborator

Can you say what the motivation for this change is? I could imagine having an option that either disables termination checks for both exec recursive functions and exec loops, or enables termination checks for both exec recursive functions and exec loops.

@tjhance
Copy link
Collaborator

tjhance commented Aug 4, 2022

I was already under the impression we didn't support termination checking for exec functions, seeing as, exec functions support while loops, which don't support termination checking. As far as I can tell, Verus never complains about the existence of loops and there isn't support for opting in to exec-mode termination checking (at the moment) so permitting decreases on an exec function would be misleading.

@Chris-Hawblitzel
Copy link
Collaborator

Won't some people want termination checking for exec loops in the future, though? It's not implemented yet, but I think that's just because it wasn't a high priority to implement. And the syntax for decreases on loops is implemented, even if the check isn't yet. I don't think exec checking would have to be turned on by default, but in cases where people explicitly want the checking, it seems harmless to allow it. Let me propose a way to allow it when requested:

  • if some writes decreases on a loop or exec function, it is checked
  • if the command-line option --check-exec-termination is provided, then exec recursive functions and exec loops must have decreases clauses

@tjhance
Copy link
Collaborator

tjhance commented Aug 4, 2022

well, i don't object to supporting it in the future. I'm just saying Andrea's change makes sense now and until such support does exist.

@Chris-Hawblitzel
Copy link
Collaborator

I think in the design I'm proposing, though, we wouldn't be disallowing and removing the decreases clauses that already exist.

@Chris-Hawblitzel
Copy link
Collaborator

We would, however, remove by default the requirement that exec functions have decreases clauses. I support that part of the change.

@utaal utaal marked this pull request as draft August 4, 2022 18:17
@utaal utaal force-pushed the exec-recursion-no-decrease branch from 12c07b4 to 51c6815 Compare August 5, 2022 11:51
…d support for mutable references in parameters of exec functions with decreases clauses
@utaal utaal force-pushed the exec-recursion-no-decrease branch from 51c6815 to dc87c2f Compare August 5, 2022 12:09
@utaal utaal changed the title Remove and disallow termination checks for exec functions Make termination checks for exec functions, add support for mutable references in parameters, and emit a warning that the check doesn't account for loops Aug 5, 2022
@utaal
Copy link
Collaborator Author

utaal commented Aug 5, 2022

The initial impetus was due to the fact that the support was incomplete: decreases did not work for functions with mutable references as arguments. That turned out to be a simple fix, so those are now supported.

The need to check termination and the guarantees provided by the check were unclear to me. From the discussion:

  • termination checks for exec functions aren't necessary for soundness,
  • the current check does not take into account loops,
  • we'd like to support optional full termination checks for exec functions in the future.

So, this PR, now:

  • makes the termination check for exec functions optional (only enabled when there's a decreases clause),
  • warns the user that a termination check on an exec passing doesn't guarantee that the function terminates if there are loops.

@utaal utaal marked this pull request as ready for review August 5, 2022 13:36
@utaal utaal force-pushed the exec-recursion-no-decrease branch from 8efeb17 to 3449856 Compare August 5, 2022 13:44
@utaal utaal force-pushed the exec-recursion-no-decrease branch from 3449856 to 3cf3430 Compare August 5, 2022 13:55
@tjhance
Copy link
Collaborator

tjhance commented Aug 5, 2022

can you add a warning or error if the user tries to use decreases on a loop? It looks like that's still missing, I think.

@utaal utaal merged commit 3154937 into main Aug 8, 2022
@utaal utaal deleted the exec-recursion-no-decrease branch August 8, 2022 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants