Skip to content

Conversation

thanhnguyen-aws
Copy link
Contributor

@thanhnguyen-aws thanhnguyen-aws commented May 28, 2025

This PR fixes the bug: Static union values can panic Kani.

The reason behind the bug is Kani's translation of Union type is not consistent with its layout defined by Rust. Specifically, the size of the Union is not just the max of its fields but that maximum should be round up to a multiple of its alignment.
See:
https://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/reference/type-layout.html#:~:text=The%20layout%20of%20a%20type,also%20part%20of%20type%20layout

In this PR, I add another variant for DataComponent: DataComponent::UnionField, with a padded_type of a Union (a struct which contains a padding and the real field type) to fix the translated layout and support reasoning about transmute function for union type.

Resolves #4097

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

@thanhnguyen-aws thanhnguyen-aws requested a review from a team as a code owner May 28, 2025 00:41
@github-actions github-actions bot added the Z-EndToEndBenchCI Tag a PR to run benchmark CI label May 28, 2025
@thanhnguyen-aws thanhnguyen-aws enabled auto-merge May 28, 2025 14:47
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.

I'm wondering if we can do something similar to #3401 instead of adding a padding field?

In particular, we already have logic for codegen_alignment_padding, so I'd prefer not to introduce a second way of reasoning about alignment and padding if we can avoid it.

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.

I'm not following why we need a separate DatatypeComponent field for unions. Can't we represent the Type as a vector of DatatypeComponent::Field and DatatypeComponent::Padding like we do for structs and enums? So in the example of this test, we'd end up with a Type constructed from a vector of DataTypeComponents where the first item is the [u8; 3] field, the second element is 1 byte of padding, the third element is the u16 field, and the fourth element is 2 bytes of padding.

Happy to take this discussion offline if that's easier. I would prefer to use the same padding infrastructure that we have for structs and enums if possible. Perhaps it isn't possible, but if it's not I'm not seeing why.

@thanhnguyen-aws
Copy link
Contributor Author

I'm not following why we need a separate DatatypeComponent field for unions. Can't we represent the Type as a vector of DatatypeComponent::Field and DatatypeComponent::Padding like we do for structs and enums? So in the example of this test, we'd end up with a Type constructed from a vector of DataTypeComponents where the first item is the [u8; 3] field, the second element is 1 byte of padding, the third element is the u16 field, and the fourth element is 2 bytes of padding.

Happy to take this discussion offline if that's easier. I would prefer to use the same padding infrastructure that we have for structs and enums if possible. Perhaps it isn't possible, but if it's not I'm not seeing why.

It is difficult to come up with a short answer for it, but after trying several ways of implementation, I feel that this is the most convenient way, for the following observations:

  1. There are two things that we need to deal with: projection and transmute. For projection, we need to convert the union to the type of the field (with the layout of that type). But for transmute, we need to wrap the type in a struct with some padding, and use the exact layout of the union.
  2. It is not convenient to use Vec of Padding and Field like the case of struct, because in union, when we want to deal with transmute or calculate the length, for each Field we need the information of its following Padding, so Vec.iter() is not convenient.
  3. If we wrap a Field and a Padding into a struct, and use it as the type of the union field, it is not convenient to implement projection because we need to deal with union separately (not consistent with other types). Many related functions: lookup_field_type, lookup_field do not take the parent type as argument, so modifying them just for union does not look pretty. In contrast, for enum, although it is translated into union, we can do the wrapping because all if its variants are translated into struct types and we can add the padding inside it directly.
  4. The wrapped struct (the padded_typ of UnionField) is only used in to_irep, so keeping the typ field to be the original mir type makes dealing with union consistent with other types everywhere else.

@thanhnguyen-aws thanhnguyen-aws added this pull request to the merge queue Jun 13, 2025
Merged via the queue into model-checking:main with commit 2ad7573 Jun 13, 2025
25 of 26 checks passed
@thanhnguyen-aws thanhnguyen-aws deleted the fixunion branch June 18, 2025 15:42
github-merge-queue bot pushed a commit that referenced this pull request Jul 3, 2025
Auto generated release notes:

## What's Changed
* Edit quantifiers' documentation. by @thanhnguyen-aws in
#4142
* Fix the bug of using multiple hidden variables for the prev of the
same Expr by @thanhnguyen-aws in
#4150
* Remove `assess` subcommand by @carolynzech in
#4111
* Optimize goto binary exporting in `cprover_bindings` by
@AlexanderPortland in #4148
* Add the option to generate performance flamegraphs by
@AlexanderPortland in #4138
* Fix the bug: Static union values can panic Kani by @thanhnguyen-aws in
#4112
* Update toolchain to 2025-06-13 by @carolynzech in
#4152
* Automatic cargo update to 2025-06-16 by @github-actions in
#4156
* Major-version update cargo dependencies by @tautschnig in
#4158
* Upgrade Rust toolchain to 2025-06-16 by @tautschnig in
#4157
* Bump tests/perf/s2n-quic from `3129ad5` to `c6e694e` by @dependabot in
#4160
* Bump tests/perf/s2n-quic from `c6e694e` to `b1b5bf8` by @dependabot in
#4164
* Upgrade Rust toolchain to 2025-06-17 by @tautschnig in
#4163
* Automatic cargo update to 2025-06-23 by @github-actions in
#4172
* Stub panics during MIR transformation by @AlexanderPortland in
#4169
* Bump tests/perf/s2n-quic from `b1b5bf8` to `32ba87d` by @dependabot in
#4175
* Handle enums with zero or one variants by @zhassan-aws in
#4171
* Introduce compiler timing script & CI job by @AlexanderPortland in
#4154
* Upgrade Rust toolchain to 2025-06-18 by @tautschnig in
#4166
* Cache dependencies for CI jobs by @AlexanderPortland in
#4181
* Autoharness: Derive `Arbitrary` for structs and enums by @carolynzech
in #4167
* Upgrade Rust toolchain to 2025-06-27 by @tautschnig in
#4182
* Include wget in dependencies by @zhassan-aws in
#4183
* Automatic cargo update to 2025-06-30 by @github-actions in
#4186
* Add support for loop assigns in loop contracts by @thanhnguyen-aws in
#4174
* Upgrade toolchain to 06/30 by @carolynzech in
#4188
* Optimize reachability with non-mutating global passes by
@AlexanderPortland in #4177
* Bump tests/perf/s2n-quic from `32ba87d` to `b8f8cca` by @dependabot in
#4190
* Bump ncipollo/release-action from 1.16.0 to 1.18.0 by @dependabot in
#4191
* Upgrade toolchain to 07/02 by @carolynzech in
#4195
* Automatic Derivation Fixes by @carolynzech in
#4194


**Full Changelog**:
kani-0.63.0...kani-0.64.0

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-EndToEndBenchCI Tag a PR to run benchmark CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Static union values can panic kani
2 participants