sway icon indicating copy to clipboard operation
sway copied to clipboard

Correctly check for overflow in add, mul and pow

Open SwayStar123 opened this issue 1 year ago • 1 comments

Description

Adds flag checks for overflow in core lib, properly cap values if overflow is enabled

Checklist

  • [ ] I have linked to any relevant issues.
  • [ ] I have commented my code, particularly in hard-to-understand areas.
  • [ ] I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • [ ] I have added tests that prove my fix is effective or that my feature works.
  • [ ] I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • [ ] I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • [ ] I have requested a review from the relevant team or maintainers.

SwayStar123 avatar Aug 22 '24 06:08 SwayStar123

This is not a vm issue, the instruction that's supposed to revert is getting optimized away. Likely the compiler doesn't consider panic-on-invalid-input side effect. Not sure if the compiler behavior is expected or not.

Investigation

if the function looks like this:

#[test(should_revert)]
fn math_0th_root_fail() {
    let _res = asm(r1: 100, r2: 0, r3) {
        log flag r1 r2 zero;
        mroo r3 r1 r2;
        log flag r1 r2 r3;
        log one one one one;
        r3: u8
    };
}

Then the test passes as expected.

However, if we remove the middle log, i.e.:

#[test(should_revert)]
fn math_0th_root_fail() {
    let _res = asm(r1: 100, r2: 0, r3) {
        log flag r1 r2 zero;
        mroo r3 r1 r2;
        log one one one one;
        r3: u8
    };
}

Then the test fails.

Let's look at the bytecode. Compile with forc build --output-bin prog.bin --tests (in test/src/in_language_tests/test_programs/math_inline_tests). Then we can find the relevant log instructions with forc parse-bytecode prog.bin | rg -C 1 '\bLOG \{'

For the first version, i.e. without logging r3 , the bytecode looks like this:

      18230   72920   MOVE { dst: 0x11, src: 0x0 }                                               1a 44 00 00
      18231   72924   LOG { a: 0xf, b: 0x10, c: 0x11, d: 0x0 }                                   33 3d 04 40
      18232   72928   LOG { a: 0x1, b: 0x1, c: 0x1, d: 0x1 }                                     33 04 10 41
      18233   72932   RET { value: 0x0 }                                                         24 00 00 00

However, when we introduce the log in the code in the second version, we get:

      18230   72920   MOVE { dst: 0x11, src: 0x0 }                                               1a 44 00 00
      18231   72924   LOG { a: 0xf, b: 0x10, c: 0x11, d: 0x0 }                                   33 3d 04 40
      18232   72928   MROO { dst: 0x12, lhs: 0x10, rhs: 0x11 }                                   18 49 04 40
      18233   72932   LOG { a: 0xf, b: 0x10, c: 0x11, d: 0x12 }                                  33 3d 04 52
      18234   72936   LOG { a: 0x1, b: 0x1, c: 0x1, d: 0x1 }                                     33 04 10 41
      18235   72940   RET { value: 0x0 }                                                         24 00 00 00

So it seems like the mroo is completely missing in the first version.

Dentosal avatar Aug 23 '24 11:08 Dentosal