Skip to content
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

Fix - Upcoming arithmetic_side_effects lints #33000

Merged
merged 6 commits into from
Aug 29, 2023

Conversation

Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Aug 25, 2023

Problem

This PR is another preparation for #32961.
saturating and wrapping versions of div and rem will no longer be accepted.

Summary of Changes

Replaces them with their checked version.

apfitzge
apfitzge previously approved these changes Aug 25, 2023
perf/src/sigverify.rs Outdated Show resolved Hide resolved
@t-nelson
Copy link
Contributor

i'd like to review some of the introduced unwraps here before merging

@apfitzge
Copy link
Contributor

i'd like to review some of the introduced unwraps here before merging

I think the main ones that stood out to me are the ones with the compute budget. Those appear to be defined as constants initialized in ComputeBudget::new, but not entirely clear those aren't modified and set to 0.

Other unwraps involving align_of or constants should be safe as those cannot or are not 0.

@Lichtso Lichtso force-pushed the fix/arithmetic_side_effects branch 2 times, most recently from a32b94d to 9a84379 Compare August 28, 2023 17:06
@Lichtso
Copy link
Contributor Author

Lichtso commented Aug 28, 2023

I made all the divisions which have a non-constant divisor (those used in CU consumption) to be .unwrap_or(u64::MAX). Meaning that if the divisor becomes 0 the program definitely fails because it consumes all CUs.

t-nelson
t-nelson previously approved these changes Aug 29, 2023
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed a few more commits to wrap up arithmetic_side_effects compatibility

perf/src/sigverify.rs Outdated Show resolved Hide resolved
@@ -763,7 +773,7 @@ declare_syscall!(
let cost = compute_budget.mem_op_base_cost.max(
compute_budget
.sha256_byte_cost
.saturating_mul((val.len() as u64).saturating_div(2)),
.saturating_mul((val.len() as u64).checked_div(2).unwrap()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's annoying that the lint can't resolve non-zero literals. compiler can know this div is safe

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it generate a linter warning?
At -O it is compiled to a bit shift:

pub fn square(num: u64) -> u64 {
    num.checked_div(2).unwrap()
}
example::square:
        mov     rax, rdi
        shr     rax
        ret

https://godbolt.org/z/xz8cPjccP

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that happens in LLVM, not in rustc. In other words, rustc is probably unaware.

@t-nelson t-nelson force-pushed the fix/arithmetic_side_effects branch from 513596b to 0b2e923 Compare August 29, 2023 04:10
perf/src/sigverify.rs Outdated Show resolved Hide resolved
@Lichtso Lichtso force-pushed the fix/arithmetic_side_effects branch from 0b2e923 to 7643cbb Compare August 29, 2023 09:22
perf/src/perf_libs.rs Outdated Show resolved Hide resolved
@t-nelson
Copy link
Contributor

was the commit that switched unwraps to expects dropped intentionally/

@t-nelson
Copy link
Contributor

was the commit that switched unwraps to expects dropped intentionally/

n/m, i see that it was squashed in. more rebase hell than i was hoping for this early in the day 😅

@Lichtso
Copy link
Contributor Author

Lichtso commented Aug 29, 2023

Yes, I reordered and squashed the commits to reduce the forward / backward changes.

@t-nelson t-nelson force-pushed the fix/arithmetic_side_effects branch from 7643cbb to bbc8981 Compare August 29, 2023 17:35
@Lichtso Lichtso merged commit 150a798 into solana-labs:master Aug 29, 2023
@Lichtso Lichtso deleted the fix/arithmetic_side_effects branch August 29, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants