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

Lithium: a Rust-native back end #1321

Open
wants to merge 55 commits into
base: master
Choose a base branch
from
Open

Conversation

JakuJ
Copy link
Contributor

@JakuJ JakuJ commented Feb 13, 2023

This is a draft PR outlining the work on a new Rust-native back end for Prusti, developed in connection to my Master's thesis. The aim is to, among others, evaluate the performance benefits of going straight from CFG to SMT-LIB and decoupling from Viper.

The project is in its initial stages and so the code is highly volatile. Everything is subject to change. The purpose of this PR is to give Prusti developers an overview of the work being done in order to avoid double work, conflicts etc.

@JakuJ JakuJ marked this pull request as draft February 13, 2023 22:59
@fpoli fpoli marked this pull request as ready for review February 14, 2023 09:24
@fpoli fpoli marked this pull request as draft February 14, 2023 09:24
@fpoli
Copy link
Member

fpoli commented Feb 14, 2023

Wow, thanks! I'm not sure why the CI is not running. Even if it's a draft, I was wondering if you considered moving native_verifier outside viper, because the API of a Viper and a native one have various differences (e.g. parsing command line parameters, the type of the Program, depending on the JVM...).

@JakuJ
Copy link
Contributor Author

JakuJ commented Feb 14, 2023

I think the CI doesn't run because of the merge conflicts. I'll move the code to a separate create as soon as I figure out how to avoid circular dependencies.

viper/src/verifier.rs Outdated Show resolved Hide resolved
@vakaras
Copy link
Contributor

vakaras commented Feb 14, 2023

The PR looks like a good start. I think that once it is cleaned up, we should merge it in with a stub native verifier so that we do not need to worry about getting large conflicts.

cc @Aurel300 @JonasAlaif @fpoli

@Aurel300
Copy link
Member

We'll probably soon get some merge conflicts in prusti-server since Cedric and Joseph will be finishing the IDE improvements project soon.

@vakaras
Copy link
Contributor

vakaras commented Feb 15, 2023

@JakuJ In our meeting, we discussed what would be the best way to proceed and agreed that the work should be split into two PRs:

  1. A clean-up PR that prepares code to allow easily adding a new backend. This PR should be merged as soon as possible to avoid conflicts.
  2. A PR that implements the actual backend – this PR would be merged only if this project worked out.

The clean-up PR should add VerificationBackend enum to prusti-server that abstracts over Viper and your new backend. We decided that an enum will be a cleaner solution for now than having a Verifier trait, which I suggested before.

enum VerificationBackend {
    Viper(viper::Viper),
//    Lithium(lithium::Lithium), – only added later once the backend is ready
}

impl VerificationBackend {
    pub fn verify(&self, program: vir::Program) -> VerificationResult {
        match self { /* delegate */ }
    }
}

Notice that the verify method takes the VIR program (that is either legacy or low) as input. This means that all JAVA stuff should happen inside the verify method.

@fpoli
Copy link
Member

fpoli commented Feb 28, 2023

  • Use git rebase -i master to remove 4b055b7 and 5484ca7.
  • Remove .DS_Store (from the history of the PR, not in a new commit) and add it to .gitignore.

@fpoli
Copy link
Member

fpoli commented Feb 28, 2023

Is there a clear advantage in splitting backend-common out of viper? All its current content (java exceptions, verification results, silicon counterexamples) can still be seen as part of Viper, and I'd avoid one more top-level folder if it's not necessary.

@JakuJ JakuJ changed the title [WIP] New Rust-native back-end Lithium: a Rust-native back end Jul 7, 2023
@JakuJ JakuJ marked this pull request as ready for review September 1, 2023 14:50
@JakuJ
Copy link
Contributor Author

JakuJ commented Sep 1, 2023

A test or two are still failing, but I'm going to switch from draft mode now. I suppose some quality-of-life improvements are necessary for the general public to find this usable, in particular:

  • Better error handling and error messages
  • A single configuration flag to run the native back end, instead of modifying half the config.rs file

I'll be chipping away at these improvements when I have the time, and in the meantime I would appreciate a code review.

@fpoli
Copy link
Member

fpoli commented Sep 4, 2023

I notice that the PR runs cargo fmt on prusti-viper, but from the diff it's difficult to see if you made more changes in there. Could you try to reformat prusti-viper on the master branch, opening a single-commit PR that we can immediately merge, and rebasing at the same time this PR on top of that commit?

@JakuJ JakuJ mentioned this pull request Sep 15, 2023
prusti-server/src/backend.rs Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this script into the scripts folder?

Comment on lines +194 to +204
// 0. Apply Lithium overrides if necessary
if let Ok(true) = env::var("PRUSTI_NATIVE_VERIFIER").map(|s| s.parse::<bool>().unwrap_or(false)) {
settings.set_default("viper_backend", "Lithium").unwrap();
settings.set_default("verify_specifications_backend", "Lithium").unwrap();
settings.set_default("encode_bitvectors", true).unwrap();
settings.set_default("enable_purification_optimization", true).unwrap();
settings.set_default("unsafe_core_proof", true).unwrap();
settings.set_default("verify_core_proof", false).unwrap();
settings.set_default("inline_caller_for", true).unwrap();
}

Copy link
Member

@fpoli fpoli Oct 27, 2023

Choose a reason for hiding this comment

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

What is the use case of this new PRUSTI_NATIVE_VERIFIER?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Grouping all required flags into one, to facilitate switching between the backends. Alternatively I could check if viper_backend == "Lithium".

Why an environment variable? For some reason if I do

if settings.get_bool("native_verifier").unwrap_or(false) {
   ...
}

at the end (after the -P arguments) and provide it via cargo-prusti -- -Pnative_verifier=true it breaks everything, including the building of prusti-contracts 🤨

Copy link
Member

Choose a reason for hiding this comment

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

There are various reasons why I'd suggest to undo this change and look for another solution:

  • The native_verifier and viper_backend flags are partially overlapping.
  • The code checks the environment variable PRUSTI_NATIVE_VERIFIER, but does not work if the same flag is set via -P. This goes against the current design of the configuration flags.
  • Changing the default of some flags depending on the value of some flags looks like the kind of thing that easily leads to surprises and hard-to-debug issues. We were forced to do it in one case (DEFAULT_PRUSTI_*) because of the way cargo works, but in this PR I think that we have more freedom to choose a better solution.

If Lithium requires a specific set of flags, what about just implementing runtime checks like we do here? Moreover, if a set of tests require a long combination of flags, what about adding one entry here, with a new test folder such as prusti-tests/tests/verify_lithium?

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.

4 participants