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

Add an LLBC backend #3514

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

zhassan-aws
Copy link
Contributor

@zhassan-aws zhassan-aws commented Sep 14, 2024

This PR adds a new codegen backend that generates low-level borrow calculus (LLBC), which is the format defined by Charon and Aeneas.

The backend can be invoked using -Zaeneas, and will generate a .llbc file. This file can then be passed to Aeneas to generate Lean for example. Currently, Aeneas needs to be manually run on the generated LLBC.

The PR translates stable MIR to unstructured LLBC (ULLBC) and then uses the Charon library to apply transformations that reconstruct the control flow to regain some of the high-level structure (loops, if statements, etc.).

In this PR, very few MIR constructs are handled, but the PR is already pretty big. More support to come in subsequent PRs.

Call-outs:

  • Charon is currently not released on crates.io, so the library is added as a dependency through GitHub with a particular commit.
  • The Kani driver is currenty heavily tailored towards CBMC, so currently it's expecting a goto file, and thus errors out when the Aeneas backend is invoked.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

@zhassan-aws zhassan-aws requested a review from a team as a code owner September 14, 2024 00:12
@github-actions github-actions bot added the Z-BenchCI Tag a PR to run benchmark CI label Sep 14, 2024
Copy link
Member

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

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

Approving with the following understanding:

  1. Some cleanup is needed as indicated by the comments.
  2. The codegen for Aeneas does need further work, but this doesn't impact any existing user who continues to use the cprover back-end.

kani-driver/src/call_single_file.rs Outdated Show resolved Hide resolved
kani-compiler/src/kani_compiler.rs Outdated Show resolved Hide resolved
kani-compiler/src/kani_compiler.rs Outdated Show resolved Hide resolved
@@ -10,6 +10,7 @@ publish = false

[dependencies]
cbmc = { path = "../cprover_bindings", package = "cprover_bindings", optional = true }
charon = { git = "https://github.com/AeneasVerif/charon", optional = true, default-features = false, rev = "cdc1dcde447a50cbc20336c79b21b42ac977b7fd" }
Copy link
Member

Choose a reason for hiding this comment

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

Do we need some way to automatically update this commit id? Something akin to the CBMC update job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. Currently, there are a lot of breaking changes though, so most updates will need to be handled manually.

Comment on lines +80 to +81
## Charon/Aeneas LLBC files
*.llbc
Copy link
Member

Choose a reason for hiding this comment

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

How would those even come about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are emitted by the Kani compiler for the Aeneas backend. They're the equivalent of goto binaries for the CBMC backend. Kani makes sure to delete the goto binaries, but in the current PR, it doesn't yet delete the llbc ones.

Comment on lines 152 to 164
info!("# LLBC resulting from control-flow reconstruction:\n\n{}\n", ccx);

// Run the micro-passes that clean up bodies.
for pass in charon_lib::transform::LLBC_PASSES.iter() {
pass.transform_ctx(&mut ccx)
}

println!("# Final LLBC before serialization:\n\n{}\n", ccx);

// Display an error report about the external dependencies, if necessary
ccx.errors.report_external_deps_errors();

trace!("Done");
Copy link
Member

Choose a reason for hiding this comment

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

That's info, println, trace - seems a bit mixed up? Should there be any use of println at all? (Also applies elsewhere.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the printing of the final LLBC dependent on a new option (--print-llbc) that is used in the current expected tests. Now, by default, nothing will get printed except the name of the LLBC file.

parents.into_iter().rev().collect()
};

// Rk.: below we try to be as tight as possible with regards to sanity
Copy link
Member

Choose a reason for hiding this comment

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

What does "Rk." mean?

Copy link

@Nadrieril Nadrieril Sep 19, 2024

Choose a reason for hiding this comment

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

(Charon dev here) That's a comment copied from Charon, "Rk" is short for @R1kM I believe

Copy link

Choose a reason for hiding this comment

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

I believe it stands for "Remark" here

Copy link

@Nadrieril Nadrieril Sep 20, 2024

Choose a reason for hiding this comment

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

Hah that makes a lot more sense x)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fun conversation :)

I was hesitant to remove the comments because we may need to import any future upstream changes to this function, so I wanted to minimize the diff. @tautschnig Let me know if you think the comments create confusion, and I'll remove them.

}
DefPathData::Impl => todo!(),
DefPathData::OpaqueTy => {
// TODO: do nothing for now
Copy link
Member

Choose a reason for hiding this comment

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

When does this need to be changed?

Copy link

@Nadrieril Nadrieril Sep 19, 2024

Choose a reason for hiding this comment

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

This is copied over from Charon, I think the answer was "whenever we understand why we'd need the name of an opaque type". We could probably make this an error, I haven't tested if this can ever occur.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same comment here @tautschnig

kani-compiler/src/codegen_aeneas_llbc/mir_to_ullbc/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the backend code yet, but I'm curious about how this affects our released software. Are we shipping the new backend?

kani-compiler/src/args.rs Outdated Show resolved Hide resolved
kani-compiler/src/args.rs Show resolved Hide resolved
Copy link
Contributor Author

@zhassan-aws zhassan-aws left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the backend code yet, but I'm curious about how this affects our released software. Are we shipping the new backend?

That was the plan (it's under an unstable feature). If we prefer to not include it in the release at this point, we can disable the feature by default and have a separate set of regressions for this feature.

kani-compiler/src/args.rs Show resolved Hide resolved
kani-compiler/src/args.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-BenchCI Tag a PR to run benchmark CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants