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

Overflow behaviour relies on Optimization #5449

Closed
vaivaswatha opened this issue Jan 10, 2024 · 1 comment · Fixed by #5510
Closed

Overflow behaviour relies on Optimization #5449

vaivaswatha opened this issue Jan 10, 2024 · 1 comment · Fixed by #5510
Assignees
Labels
bug Something isn't working compiler: ir IRgen and sway-ir including optimization passes

Comments

@vaivaswatha
Copy link
Contributor

vaivaswatha commented Jan 10, 2024

While implementing #5385, I found that when optimizations are disabled, (in particular, constcombine and/or mem2reg for this bug), the IR we're generating for addition doesn't check for overflows correctly.

This is the IR for should_fail/arith_overflow/u8_add_overflow (note that there's no mem2reg or constcombine):

script {
    entry fn main() -> bool, !1 {
        local u8 a
        local u8 b
        local u8 res
        local u8 result

        entry():
        v0 = get_local ptr u8, a, !2
        v1 = const u8 255, !4
        store v1 to v0, !2
        v2 = get_local ptr u8, b, !5
        v3 = const u8 1, !6
        store v3 to v2, !5
        v4 = get_local ptr u8, a, !7
        v5 = load v4
        v6 = get_local ptr u8, b, !8
        v7 = load v6
        v8 = add v5, v7, !9
        v9 = get_local ptr u8, res, !12
        store v8 to v9, !13
        v10 = get_local ptr u8, res, !15
        v11 = load v10, !9
        v12 = const u8 255, !4
        v13 = cmp gt v11 v12, !9
        cbr v13, add_1_block0(), add_1_block1(), !17

        add_1_block0():
        v14 = const u64 0, !18
        revert v14, !20

        add_1_block1():
        ...
    }
   ...

Because we're using byte stores and loads for u8, we end up ignoring the overflowed part in the u64 register, thus not triggering the revert.

When this issue is fixed, we should be enabling the tests should_pass/arith_overflow/u8_add_overflow and should_pass/arith_overflow/u8_mul_overflow which were both disabled in #5385.

I suspect the bug got introduced with #4929.

@vaivaswatha
Copy link
Contributor Author

Easy repro without dependence on #5385

script;

fn main() {

}

fn add(a: u8, b: u8) -> u8 {
   a + b
}

#[test]
fn should_overflow_revert() {
   let a: u8 = u8::max();
   let b: u8 = 1;
   add(a, b);
}

On master, forc test hits a revert, as it should, but with mem2reg (by adding Return Ok(false) at the beginning of promote_to_registers), forc test just passes (but it shouldn't have).

@vaivaswatha vaivaswatha added bug Something isn't working compiler: ir IRgen and sway-ir including optimization passes labels Jan 10, 2024
@vaivaswatha vaivaswatha self-assigned this Jan 16, 2024
vaivaswatha added a commit that referenced this issue Jan 20, 2024
## Description
This PR implements `-o` flags so that we can disable optimizations. This
will be useful as we are working towards debugger support and
optimizations pollute source map generated.

Provide option to the e2e test binary to specify build profile. For
tests that have their ABI tested, or rely on their compiled
hashes (for deployment), since that changes with build profile, the
tests are marked as unsupported for debug profile.
(better to test with and have expected results for release than the
debug profile).

Add testing release profile in CI since the default is now debug. Two
tests in `should_fail` that are expected to fail due to
overflows are disabled because there's a bug in our IR gen. This is 
tracked by #5449 

---------

Co-authored-by: Vaivaswatha Nagaraj <vaivaswatha.nagaraj@fuel.sh>
Co-authored-by: Vaivaswatha N <vaivaswatha@users.noreply.github.com>
Co-authored-by: Sophie Dankel <47993817+sdankel@users.noreply.github.com>
sdankel added a commit that referenced this issue Jan 24, 2024
This PR implements `-o` flags so that we can disable optimizations. This
will be useful as we are working towards debugger support and
optimizations pollute source map generated.

Provide option to the e2e test binary to specify build profile. For
tests that have their ABI tested, or rely on their compiled
hashes (for deployment), since that changes with build profile, the
tests are marked as unsupported for debug profile.
(better to test with and have expected results for release than the
debug profile).

Add testing release profile in CI since the default is now debug. Two
tests in `should_fail` that are expected to fail due to
overflows are disabled because there's a bug in our IR gen. This is
tracked by #5449

---------

Co-authored-by: Vaivaswatha Nagaraj <vaivaswatha.nagaraj@fuel.sh>
Co-authored-by: Vaivaswatha N <vaivaswatha@users.noreply.github.com>
Co-authored-by: Sophie Dankel <47993817+sdankel@users.noreply.github.com>
vaivaswatha added a commit that referenced this issue Jan 24, 2024
Fixes #5449

Without this fix, because we use `u8` load and store instructions,
the overflow check (which relies on `u64` arithmetic) gets nullified.
vaivaswatha added a commit that referenced this issue Jan 30, 2024
Fixes #5449

Without this fix, because we use `u8` load and store instructions, the
overflow check (which relies on `u64` arithmetic) gets nullified.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler: ir IRgen and sway-ir including optimization passes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant