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

Update LLBC backend for Trait support and translation of projection #3807

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

Conversation

thanhnguyen-aws
Copy link
Contributor

@thanhnguyen-aws thanhnguyen-aws commented Jan 2, 2025

  1. Update LLBC backend to adapt to the changes of Charon submodule to AeneasVerif/charon@adc0a85: Changing translate_place to fit with the current place representation of Charon.
  2. Support translation of programs that use Trait, but translating the trait impl functions into monomorphized functions.

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

Copy link
Contributor

@carolynzech carolynzech left a comment

Choose a reason for hiding this comment

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

(Had a bit of a race condition with Zyad reviewing, so there may be some repetition with the comments here.)

As a more high-level comment, it'd be great to write some more thorough documentation for each function so that people unfamiliar with Charon can have a basic intuition for what's going on. Nothing too crazy--just 1/2 sentences above each function would help a lot.

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

Now that #3801 is merged, please change the title of this PR and its description.

@thanhnguyen-aws thanhnguyen-aws changed the title Update charon submodule version Update LLBC backend for new version of Charon Jan 3, 2025
@zhassan-aws
Copy link
Contributor

Update LLBC backend for new version of Charon

I'm not sure the updated title accurately captures what this PR is doing. Are any of the updates needed for the new version of Charon? I thought all such updates were made in #3801.

It seems to me that this PR is primarily about adding support for traits? If so, the title and description should capture that.

@thanhnguyen-aws thanhnguyen-aws changed the title Update LLBC backend for new version of Charon Update LLBC backend for Trait support Jan 6, 2025
@thanhnguyen-aws thanhnguyen-aws changed the title Update LLBC backend for Trait support Update LLBC backend for Trait support and translation of projection Jan 6, 2025
Copy link
Contributor

@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 second @carolynzech's request of adding doc comments to functions, even if it is just a simple description to help the reader understand what a function is doing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this change.

Copy link
Contributor

@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.

Looks good! Can you revert the change to the s2n-quic submodule as @carolynzech pointed out?

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.

3 participants