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.

Sign up for GitHub

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

WDCM and WQCM: reset $of and $err #844

Merged
merged 10 commits into from
Oct 2, 2024
Merged

WDCM and WQCM: reset $of and $err #844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
11530ef
WDCM and WDQM: clear an
acerone85 Sep 27, 2024
46692d8
Changelog
acerone85 Sep 27, 2024
1d765be
Add tests
acerone85 Sep 30, 2024
68ea647
Clippy does not like .get(0)
acerone85 Sep 30, 2024
ac5cb82
Address code review
acerone85 Sep 30, 2024
44b53e3
Use div instead of divi
acerone85 Sep 30, 2024
cd6eefd
Typo
acerone85 Sep 30, 2024
6e13541
Clippy does not like pushing after creating vec
acerone85 Sep 30, 2024
b626866
Fix tests
acerone85 Oct 1, 2024
025dad3
Remove println statement
acerone85 Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
#### Breaking
- [#829](https://github.com/FuelLabs/fuel-vm/pull/829): Updated `add_random_fee_input()` to accept an `rng` for true randomization. Introduced `add_fee_input()` to retain the previous behavior of `add_random_fee_input()`.
- [#845](https://github.com/FuelLabs/fuel-vm/pull/845): Removed `Default` implementation of `SecretKey`.
- [#844](https://github.com/FuelLabs/fuel-vm/pull/844): `WDCM` and `WQCM` reset `$of` and `$err`.

## [Version 0.57.1]

Expand Down
5 changes: 4 additions & 1 deletion fuel-vm/src/interpreter/alu/wideint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ macro_rules! wideint_ops {
c: Word,
args: CompareArgs,
) -> SimpleResult<()> {
let (SystemRegisters { pc, .. }, mut w) = split_registers(&mut self.registers);

let (SystemRegisters { mut of, mut err, pc, .. }, mut w) = split_registers(&mut self.registers);
let dest: &mut Word = &mut w[ra.try_into()?];

// LHS argument is always indirect, load it
Expand All @@ -89,6 +90,8 @@ macro_rules! wideint_ops {
};

*dest = [<cmp_ $t:lower>](lhs, rhs, args.mode);
*of = 0;
*err = 0;
Comment on lines +93 to +94
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I remove these lines, tests still passes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can confirm, but I don't have any idea why.
I have checked, and log does not reset the registers, so the only option is that the registers are reset already somewhere while executing the wideint compare instruction. Will check more in depth

Copy link
Member

Choose a reason for hiding this comment

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

It's because code generated by make_u128 and make_u256 used in the tests reset $err and $of as they use movi internally.

You should first create all values, and only after that call the opcodes that access flags.

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 wish I looked at this comment three hours ago :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed tests in 18b6008 and verified that they fail before applying the changes to the wideint cmp alu operation


inc_pc(pc)?;
Ok(())
Expand Down
222 changes: 221 additions & 1 deletion fuel-vm/src/tests/wideint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,116 @@ fn cmp_u128(
}
}

#[test]
fn cmp_u128_resets_of() {
// Given
let mut ops = vec![
op::movi(0x20, Flags::WRAPPING.bits() as u32),
op::flag(0x20),
];

// Prepare the operands used for the cmp_u128 operation before
// performing the division. This is needed because make_u128
// uses `movi` internally, which resets both RegId::OF and RegId::ERR.
ops.extend(make_u128(0x21, 0));
ops.extend(make_u128(0x22, 0));

// Perform a mul operation that overflows and log the value of RegId::OF
ops.push(op::not(0x20, 0));
ops.push(op::mul(0x20, 0x20, 0x20));
ops.push(op::log(RegId::OF, RegId::ZERO, RegId::ZERO, RegId::ZERO));

// Now push a cmp_u128 operation and log the value of RegId::OF again
ops.push(op::wdcm_args(
0x23,
0x21,
0x22,
CompareArgs {
indirect_rhs: true,
mode: CompareMode::EQ,
},
));
ops.push(op::log(RegId::OF, RegId::ZERO, RegId::ZERO, RegId::ZERO));
ops.push(op::ret(RegId::ONE));

// When
let receipts: Vec<Receipt> = run_script(ops);

let Receipt::Log {
ra: reg_of_before_cmp,
..
} = receipts.first().unwrap()
else {
panic!("Expected log receipt");
};

let Receipt::Log {
ra: reg_of_after_cmp,
..
} = receipts.get(1).unwrap()
else {
panic!("Expected log receipt");
};

// Then
assert!(*reg_of_before_cmp != 0);
assert_eq!(*reg_of_after_cmp, 0);
}

#[test]
fn cmp_u128_resets_err() {
// Given
let mut ops = vec![
op::movi(0x20, Flags::UNSAFEMATH.bits() as u32),
op::flag(0x20),
];
// Prepare the operands used for the cmp_u128 operation before
// performing the division. This is needed because make_u128
// uses `movi` internally, which resets both RegId::OF and RegId::ERR.
ops.extend(make_u128(0x21, 0));
ops.extend(make_u128(0x22, 0));

// Perform the division and log the value of RegId::ERR
ops.push(op::div(0x10, RegId::ONE, RegId::ZERO));
ops.push(op::log(RegId::ERR, RegId::ZERO, RegId::ZERO, RegId::ZERO));

// Push a cmp_u128 operation and log the value of RegId::ERR again
ops.push(op::wdcm_args(
0x23,
0x21,
0x22,
CompareArgs {
indirect_rhs: true,
mode: CompareMode::EQ,
},
));
ops.push(op::log(RegId::ERR, RegId::ZERO, RegId::ZERO, RegId::ZERO));
ops.push(op::ret(RegId::ONE));

// When
let receipts: Vec<Receipt> = run_script(ops);

let Receipt::Log {
ra: reg_err_before_cmp,
..
} = receipts.first().unwrap()
else {
panic!("Expected log receipt");
};

let Receipt::Log {
ra: reg_err_after_cmp,
..
} = receipts.get(1).unwrap()
else {
panic!("Expected log receipt");
};

// Then
assert_eq!(*reg_err_before_cmp, 1);
assert_eq!(*reg_err_after_cmp, 0);
}

#[rstest::rstest]
fn cmp_u256(
#[values(0u64.into(), 1u64.into(), 2u64.into(), u64::MAX.into(), ((u64::MAX as u128) + 1).into(), u128::MAX.into())]
Expand Down Expand Up @@ -128,7 +238,7 @@ fn cmp_u256(
ops.push(op::log(0x22, RegId::ZERO, RegId::ZERO, RegId::ZERO));
ops.push(op::ret(RegId::ONE));

let receipts = run_script(ops);
let receipts: Vec<Receipt> = run_script(ops);

if let Receipt::Log { ra, .. } = receipts.first().unwrap() {
let expected = match mode {
Expand All @@ -146,6 +256,116 @@ fn cmp_u256(
}
}

#[test]
fn cmp_u256_resets_of() {
// Given
let mut ops = vec![
op::movi(0x20, Flags::WRAPPING.bits() as u32),
op::flag(0x20),
];

// Prepare the operands used for the cmp_u256 operation before
// performing the division. This is needed because make_u256
// uses `movi` internally, which resets both RegId::OF and RegId::ERR.
ops.extend(make_u256(0x21, 0u64.into()));
ops.extend(make_u256(0x22, 0u64.into()));

// Perform a mul operation that overflows and log the value of RegId::OF
ops.push(op::not(0x20, 0));
ops.push(op::mul(0x20, 0x20, 0x20));
ops.push(op::log(RegId::OF, RegId::ZERO, RegId::ZERO, RegId::ZERO));

// Now push a cmp_u256 operation and log the value of RegId::OF again
ops.push(op::wqcm_args(
0x23,
0x21,
0x22,
CompareArgs {
indirect_rhs: true,
mode: CompareMode::EQ,
},
));
ops.push(op::log(RegId::OF, RegId::ZERO, RegId::ZERO, RegId::ZERO));
ops.push(op::ret(RegId::ONE));

// When
let receipts: Vec<Receipt> = run_script(ops);

let Receipt::Log {
ra: reg_of_before_cmp,
..
} = receipts.first().unwrap()
else {
panic!("Expected log receipt");
};

let Receipt::Log {
ra: reg_of_after_cmp,
..
} = receipts.get(1).unwrap()
else {
panic!("Expected log receipt");
};

// Then
assert!(*reg_of_before_cmp != 0);
assert_eq!(*reg_of_after_cmp, 0);
}

#[test]
fn cmp_u256_resets_err() {
// Given
let mut ops = vec![
op::movi(0x20, Flags::UNSAFEMATH.bits() as u32),
op::flag(0x20),
];
// Prepare the operands used for the cmp_u256 operation before
// performing the division. This is needed because make_u256
// uses `movi` internally, which resets both RegId::OF and RegId::ERR.
ops.extend(make_u256(0x21, 0u64.into()));
ops.extend(make_u256(0x22, 0u64.into()));

// Perform the division and log the value of the RegId::ERR
ops.push(op::div(0x10, RegId::ONE, RegId::ZERO));
ops.push(op::log(RegId::ERR, RegId::ZERO, RegId::ZERO, RegId::ZERO));

// Push a cmp_u256 operation and log the value of RegId::ERR again
ops.push(op::wqcm_args(
0x23,
0x21,
0x22,
CompareArgs {
indirect_rhs: true,
mode: CompareMode::EQ,
},
));
ops.push(op::log(RegId::ERR, RegId::ZERO, RegId::ZERO, RegId::ZERO));
ops.push(op::ret(RegId::ONE));

// When
let receipts: Vec<Receipt> = run_script(ops);

let Receipt::Log {
ra: reg_err_before_cmp,
..
} = receipts.first().unwrap()
else {
panic!("Expected log receipt");
};

let Receipt::Log {
ra: reg_err_after_cmp,
..
} = receipts.get(1).unwrap()
else {
panic!("Expected log receipt");
};

// Then
assert_eq!(*reg_err_before_cmp, 1);
assert_eq!(*reg_err_after_cmp, 0);
}

#[rstest::rstest]
fn incr_u128(
#[values(0, 1, 2, u64::MAX as u128, (u64::MAX as u128) + 1, u128::MAX - 1)] v: u128,
Expand Down
Loading