-
Notifications
You must be signed in to change notification settings - Fork 144
Fix generator layout and sizes #1607
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1130,7 +1130,7 @@ impl<'tcx> GotocCtx<'tcx> { | |
| name: field_name, | ||
| typ: ctx.codegen_ty(field_ty), | ||
| }); | ||
| offset += field_size; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the core of the fix right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that was the bug. |
||
| offset = field_offset + field_size; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please add a comment here? You really only care about the last field, right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What kind of comment do you mean? We also use the offset earlier to figure out how much padding to insert before a field.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha. That's fine, thanks |
||
| } | ||
| fields.extend(ctx.codegen_alignment_padding( | ||
| offset, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| use std::{ | ||
| future::Future, | ||
| pin::Pin, | ||
| sync::{ | ||
| atomic::{AtomicI64, Ordering}, | ||
| Arc, | ||
| }, | ||
| }; | ||
|
|
||
| #[kani::proof] | ||
| fn issue_1593() { | ||
| let x = Arc::new(AtomicI64::new(0)); | ||
| let x2 = x.clone(); | ||
| let gen = async move { | ||
| async {}.await; | ||
| x2.fetch_add(1, Ordering::Relaxed); | ||
| }; | ||
| assert_eq!(std::mem::size_of_val(&gen), 16); | ||
| let pinbox = Box::pin(gen); // check that vtables work | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,12 +79,10 @@ fn overlap_x_and_y() -> impl Generator<Yield = (), Return = ()> { | |
|
|
||
| #[kani::proof] | ||
| fn main() { | ||
| // FIXME: size of generators does not work reliably (https://github.com/model-checking/kani/issues/1395) | ||
| assert_eq!(1025, std::mem::size_of_val(&move_before_yield())); | ||
| // The following assertion fails for some reason (tracking issue: https://github.com/model-checking/kani/issues/1395). | ||
| // But it also fails for WASM (https://github.com/rust-lang/rust/issues/62807), | ||
| // so it is probably not a big problem: | ||
| assert_eq!(1026, std::mem::size_of_val(&move_before_yield_with_noop())); | ||
| // With panic=unwind, the following assertion passes: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please improve this doc. Maybe add as a document of the harness itself.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| // assert_eq!(1026, std::mem::size_of_val(&move_before_yield_with_noop())); | ||
| assert_eq!(1025, std::mem::size_of_val(&move_before_yield_with_noop())); | ||
| assert_eq!(2051, std::mem::size_of_val(&overlap_move_points())); | ||
| assert_eq!(1026, std::mem::size_of_val(&overlap_x_and_y())); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this previously missed because sitting inside a macro made it hard to find references? Just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the macro had no effect on this as far as I can tell.