Implement support for generators#1378
Conversation
celinval
left a comment
There was a problem hiding this comment.
Can you please add comments to your code and maybe a high level explanation of how you modeled generators in the PR body?
|
@celinval I added both a comment in the code and the PR. |
| if layout.is_zst() || dst_type.sizeof_in_bits(&self.symbol_table) == 0 { | ||
| // We ignore assignment for all zero size types | ||
| // Ignore generators too for now: | ||
| // https://github.com/model-checking/kani/issues/416 |
There was a problem hiding this comment.
I believe this comment was outdated because the if condition only checks for ZST.
I'll try to take a look at this tomorrow. Remove my change request since you have added a great description to this issue. Thanks!
tests/kani/Generator/rustc-generator-tests/size-moved-locals.rs
Outdated
Show resolved
Hide resolved
celinval
left a comment
There was a problem hiding this comment.
This is my first pass. I'll keep taking a look at it tomorrow. Thanks!
tests/kani/Generator/main.rs
Outdated
| // as long as the path is not dynamically used. Full Generator support | ||
| // tracked in: https://github.com/model-checking/kani/issues/416 | ||
|
|
||
| // when the path is not dynamically used. |
There was a problem hiding this comment.
The same here. Can you please make this test more meaningful.
There was a problem hiding this comment.
I don't think this test is really necessary anymore, but I'm hesitant to remove tests. It used to test that a file containing generators worked as long as generators are not actually used in the execution of the program. Should I remove it?
There was a problem hiding this comment.
Either remove it or add a check at the end
There was a problem hiding this comment.
I've changed the test completely. It now checks whether generators work with resume types that are not (). I don't think the original test makes sense anymore, now that generators are properly implemented.
| pub fn fn_sig_of_instance(&self, instance: Instance<'tcx>) -> Option<ty::PolyFnSig<'tcx>> { | ||
| // Adapted from `fn_sig_for_fn_abi` in compiler/rustc_middle/src/ty/layout.rs | ||
| // Code duplication tracked here: https://github.com/model-checking/kani/issues/1365 | ||
| fn generator_sig( |
There was a problem hiding this comment.
Can you please document what you are doing here?
There was a problem hiding this comment.
To be honest, I don't really understand what's happening there (or in closure_sig, which also has no explanatory comment). Both are copied from compiler/rustc_middle/src/ty/layout.rs. That's why I added the tracking issue: #1365
| ty::Generator(..) => { | ||
| (self.generator_variant_name(idx), TypeOrVariant::GeneratorVariant(idx)) | ||
| } | ||
| _ => unreachable!("it's a bug to reach here! {:?}", &t.kind()), |
There was a problem hiding this comment.
⛏️ error message could be more informative
| let layout = self.layout_of(t); | ||
| let expr = match &layout.variants { | ||
| Variants::Single { .. } => { | ||
| assert!(!t.is_generator()); |
There was a problem hiding this comment.
Might be clearer to lift these to a precondition
if t.is_generator {
assert!(matches!(layout.variants, Variants::Single { .. });
etc ...
}There was a problem hiding this comment.
Created a function check_generator_layout_assumptions because this code was repeated in a few places.
| // Contrary to generators, currently enums have only one field (the discriminant), the rest are in the variants: | ||
| assert!(layout.fields.count() <= 1); | ||
| // Contrary to generators, the discriminant is the first (and only) field for enums: | ||
| assert_eq!(*tag_field, 0); |
There was a problem hiding this comment.
What happens in a Niche encoding
There was a problem hiding this comment.
It seems that they have a tag_field anyway: https://doc.rust-lang.org/stable/nightly-rustc/rustc_target/abi/enum.Variants.html#variant.Multiple.field.tag_field. So this assertion should not be a problem (and the regression tests pass).
|
Regarding the assertions checking that the layout of a generator is multiple variants, with direct encoding: I created a function |
| (self.generator_variant_name(idx), TypeOrVariant::GeneratorVariant(idx)) | ||
| } | ||
| _ => unreachable!( | ||
| "cannot downcast {:?} to a varian (only enums and generators can)", |
There was a problem hiding this comment.
That's a much nicer error message. :)
@celinval I first thought this was a good idea and I just tried it, but it turns out that 5 of the other tests rely on |
I have decided to remove the additional checks and only check the assumption once, when we create the generator. This is what we do for other assumptions as well: check them on creation, not every time the datatype is used. |
| take(x); | ||
| }; | ||
|
|
||
| // FIXME: for some reason, these asserts are very hard for CBMC to figure out |
There was a problem hiding this comment.
Can you please create an issue to take a look at the performance of this test?
Description of changes:
This implements the generator feature for Rust. We translate a generator type similarly to an enum with a variant for each suspend point.
Consider the following generator:
Rustc compiles this to something similar to the following enum (but there are differences, see below!):
However, its layout may differ from normal Rust enums in the following ways:
This means that we CANNOT use the enum translation, which would be roughly as follows:
Instead, we use the following translation:
Of course, if the generator has any other top-level/direct fields, they'd be included in the
DirectFieldsstruct as well.Resolved issues:
Resolves #416
Call-outs:
ignore_var_tybecame redundant, so I removed it.codegen_enum. I'm not sure how to remove it becausecodegen_enumrelies on anAdtDef, which is not available for generators (because the relevant enum is generated in MIR).codegen_enumthat document assumptions that we currently make about enum layouts. (I don't know if they're guaranteed for enums, but they don't hold for generators, so I thought it would be good to be explicit about it.)discriminant.rs, taken from rustc) takes a bit over a minute on my laptop. Is that too much?Testing:
How is this change tested? 3 existing regression tests (that now work) + 16 test cases from rustc (
src/test/ui/generator/)Is this a refactor change? No
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.