Warn on clippy::cast_possible_truncation#3557
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
JBYoshiforce-pushed the clippy-cast-possible-truncation branch from ffcd821 to 0dfbf3aCompare
JonathanWoollett-Light added Priority: Low JonathanWoollett-Light changed the title clippy::cast_possible_truncationJBYoshiforce-pushed the clippy-cast-possible-truncation branch 2 times, most recently from 794d0d6 to fa3a8cbCompare
I've updated to the latest main commit. All tests are passing on my end, though the coverage for the case without io_uring has decreased due to the increased line count in io_uring. My dev machine uses an Intel chip so I updated the coverage for that; I don't have an AMD or ARM machine to test on, so those coverage numbers may still need to be fixed. |
JBYoshiforce-pushed the clippy-cast-possible-truncation branch 2 times, most recently from e47a81d to e85a4aaCompareUpdated to fix AMD test coverage and ARM compile errors. |
Please squash these changes into 1 commit. |
JBYoshiforce-pushed the clippy-cast-possible-truncation branch 2 times, most recently from c72f1e3 to 6bec8a2CompareSquashed, and I fixed a few more bugs in the build. |
JBYoshiforce-pushed the clippy-cast-possible-truncation branch from 6bec8a2 to 2523fe9CompareFixed a few more styling points and updated coverage numbers. I'm noticing that the Git commit message lint is failing because my sign-off line is longer than the maximum line length. Is there a way to work around this? |
JBYoshiforce-pushed the clippy-cast-possible-truncation branch from b5d62a0 to fb21477CompareUpdated to add comments to the other "allow" markers. |
@JBYoshi Can you please resolve merge conflicts and rebase. |
JBYoshiforce-pushed the clippy-cast-possible-truncation branch from fb21477 to 99f2ec8Compare@JonathanWoollett-Light Updated. |
JBYoshiforce-pushed the clippy-cast-possible-truncation branch from 99f2ec8 to 1eb79ecCompareFixed Clippy error from merge. |
@JBYoshi Could you please rebase. |
JBYoshiforce-pushed the clippy-cast-possible-truncation branch from 41d8ccd to 407bc6dCompare@JonathanWoollett-Light I've rebased to the latest changes as far as I can test. The coverage numbers may still need to be updated though. |
JBYoshiforce-pushed the clippy-cast-possible-truncation branch 2 times, most recently from 0138434 to 690abb3Compare
In vmm/builder.rs, the CPU count can be fetched from the VM configuration without having to do a type conversion. In the serial device and the networking code, I added explicit checks to ensure that values fit in bounds. In the vCPU code, the existing code uses floats for tolerance values. We can replace these all with integer multiplications and divisions. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
We can use usize in place of u64 here to reduce type conversion warnings. On 64-bit systems the types are equivalent, but Clippy allows automatic conversions in the one direction but not in the other. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
Some places in the code don't allow us to re-type values to let the compiler verify that those conversions are safe, particularly with length values. All of these are marked with comments justifying why they are safe. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
Some places in the code don't allow us to re-type values to let the compiler verify that those conversions are safe, particularly with length values. Since these are for test code, I don't expect overflows to happen, so I've just inserted unwraps here. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
In this commit, I changed several places that refer to memory addresses that we generate and that are bounded by limitations of the system to use explicit unwraps. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
Some of Rust's time measurements use u128 for outputs but u64 for inputs. I don't expect we'll overflow a u64 - 2^64 nanoseconds is over 500 years - so I've added try_from()/unwrap() calls to those conversions. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
This code shoud already be safe because of debug-mode assertions and formal proofs done ahead of time. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
These values can be changed to smaller types without causing problems. I've done this in a separate commit because my dev setup doesn't use ARM. In case something ARM-specific breaks in CI, I'd like to be able to keep those changes in their own commit so I can debug more easily. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
For equalities and inequalities, we can change the types on each side to always convert to the larger type, which ensures all conversions are safe. I've done this in a separate commit because my dev setup doesn't use ARM. In case something ARM-specific breaks in CI, I'd like to be able to keep those changes in their own commit so I can debug more easily. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
These values can be changed to smaller types without causing problems. I've done this in a separate commit because my dev setup doesn't use ARM. In case something ARM-specific breaks in CI, I'd like to be able to keep those changes in their own commit so I can debug more easily. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
In these cases, values are intentionally truncated, either to split them into multiple values or to ensure that user-supplied data does not cause Firecracker to panic with a try_from/unwrap failure. Clippy will allow us to use "as" when we use bit masks to limit the size of an explicitly-sized value (u16, u32, u64). It doesn't work on usize values or on some more complicated expressions. I've done this in a separate commit because my dev setup doesn't use ARM. In case something ARM-specific breaks in CI, I'd like to be able to keep those changes in their own commits so I can debug more easily. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
Some places in the code don't allow us to re-type values to let the compiler verify that those conversions are safe, particularly with length values. All of these are marked with comments justifying why they are safe. I've done this in a separate commit because my dev setup doesn't use ARM. In case something ARM-specific breaks in CI, I'd like to be able to keep those changes in their own commits so I can debug more easily. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
Some places in the code don't allow us to re-type values to let the compiler verify that those conversions are safe, particularly with length values. Since these are for test code, I don't expect overflows to happen, so I've just inserted unwraps here. I've done this in a separate commit because my dev setup doesn't use ARM. In case something ARM-specific breaks in CI, I'd like to be able to keep those changes in their own commits so I can debug more easily. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
Errors here appear to just emit warnings, so I added a check here to ensure the offset is in bounds. I've done this in a separate commit because my dev setup doesn't use ARM. In case something ARM-specific breaks in CI, I'd like to be able to keep those changes in their own commits so I can debug more easily. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
Interrupt statuses are stored as 32-bit values in the system, but as 64-bit values in the snapshots. This commit changes the snapshot type to a u32 to remove those conversions. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
This commit enables Clippy lint checks for truncating conversions. All code changes to conform to this are included in earlier commits. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
JBYoshiforce-pushed the clippy-cast-possible-truncation branch 2 times, most recently from 4d55f2d to b6bf058CompareI found the problem with the performance tests (I had mixed up |
In these tests, the tolerance constants all originally rounded to 0, which meant they weren't testing the correct behavior (everything was always under the tolerance level). This fixes the tests to properly put everything over the tolerance threshold. Signed-off-by: Jonathan Browne <12983479+JBYoshi@users.noreply.github.com>
JBYoshiforce-pushed the clippy-cast-possible-truncation branch from b6bf058 to d0deb48Compare
JonathanWoollett-Light approved these changes Oct 9, 2023
Great work, this turned into a surprisingly herculean effort, thank you for all your work. |
roypat added a commit to roypat/firecracker that referenced this pull request Oct 12, 2023
It seems a rebase gone awry caused the version bump from firecracker-microvm#3557 to be considered as part of 1.5, even though it will only land with 1.6. Swap the two lines to unblock CI. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
9 tasks
roypat added a commit that referenced this pull request Oct 12, 2023
It seems a rebase gone awry caused the version bump from #3557 to be considered as part of 1.5, even though it will only land with 1.6. Swap the two lines to unblock CI. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds checks for
clippy::cast_possible_truncationas per #3197.Changes
I added
-Dclippy::cast_possible_truncationto the Rust configuration and wrapped most unchecked casts withtry_from(...).unwrap(). In a few cases like tests or where the truncation behavior was intentional, I updated the types or added explicit allow markers.Reason
Fixes#3197.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following
Developer Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
CHANGELOG.md.TODOs link to an issue.rust-vmm.