Warn on clippy::cast_possible_truncation
#3557
Merged
Conversation
This file contains 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
JBYoshi
force-pushed
the
clippy-cast-possible-truncation
branch
from
ffcd821
to
0dfbf3a
Compare
JonathanWoollett-Light
added
Priority: Low
JonathanWoollett-Light
changed the title
clippy::cast_possible_truncation
JBYoshi
force-pushed
the
clippy-cast-possible-truncation
branch
2 times, most recently
from
794d0d6
to
fa3a8cb
Compare
|
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. |
JBYoshi
force-pushed
the
clippy-cast-possible-truncation
branch
2 times, most recently
from
e47a81d
to
e85a4aa
Compare
|
Updated to fix AMD test coverage and ARM compile errors. |
|
Please squash these changes into 1 commit. |
JBYoshi
force-pushed
the
clippy-cast-possible-truncation
branch
2 times, most recently
from
c72f1e3
to
6bec8a2
Compare
|
Squashed, and I fixed a few more bugs in the build. |
JBYoshi
force-pushed
the
clippy-cast-possible-truncation
branch
from
6bec8a2
to
2523fe9
Compare
|
Fixed 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? |
JBYoshi
force-pushed
the
clippy-cast-possible-truncation
branch
from
b5d62a0
to
fb21477
Compare
|
Updated to add comments to the other "allow" markers. |
|
@JBYoshi Can you please resolve merge conflicts and rebase. |
JBYoshi
force-pushed
the
clippy-cast-possible-truncation
branch
from
fb21477
to
99f2ec8
Compare
|
@JonathanWoollett-Light Updated. |
JBYoshi
force-pushed
the
clippy-cast-possible-truncation
branch
from
99f2ec8
to
1eb79ec
Compare
|
Fixed Clippy error from merge. |
|
@JBYoshi Could you please rebase. |
JBYoshi
force-pushed
the
clippy-cast-possible-truncation
branch
from
41d8ccd
to
407bc6d
Compare
|
@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. |
JBYoshi
force-pushed
the
clippy-cast-possible-truncation
branch
2 times, most recently
from
0138434
to
690abb3
Compare
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
This code shoud already be safe because of debug-mode assertions and formal proofs done ahead of time. Signed-off-by: Jonathan Browne <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
JBYoshi
force-pushed
the
clippy-cast-possible-truncation
branch
2 times, most recently
from
4d55f2d
to
b6bf058
Compare
|
I 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 <[email protected]>
JBYoshi
force-pushed
the
clippy-cast-possible-truncation
branch
from
b6bf058
to
d0deb48
Compare
JonathanWoollett-Light
approved these changes
Oct 9, 2023
|
Great work, this turned into a surprisingly herculean effort, thank you for all your work. |
JonathanWoollett-Light
merged commit 5a6f28d
into
firecracker-microvm:main
6 of 7 checks passed
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 <[email protected]>
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 <[email protected]>
Sign up for free
to 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.