wasmtime / PR #6850 cranelift: Validate `iconst` ranges · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #6850 cranelift: Validate `iconst` ranges

Wasmtime GitHub notifications bot (Aug 16 2023 at 00:13):

timjrd edited PR #6850:

Implements the following checks:

iconst.i8 immediate must be within 0 .. 0xFF
iconst.i16 immediate must be within 0 .. 0xFFFF
iconst.i32 immediate must be within 0 .. 0xFFFFFFFF

fix #3059

Explain why this change is needed:

As mentioned in #3059, iconst currently allows any immediate within the range of an i64, even for iconst.i8, iconst.i16 or iconst.i32.

This breaks some tests!

Running cargo test in /cranelift/codegen returns successfully. I also added a few tests concerning the new checks.

Running cargo test in /cranelift returns some failures. For example:
1: - inst1 (v1 = iconst.i8 -1): constant immediate is out of bounds
[...]
failures:
    bugpoint::tests::test_reduce
    run::test::nop
Which is expected if we forbid negative immediates, @jameysharp could you please confirm?

Running cargo test at the root of this repo returns a lot of failures, possibly related to negative immediates.

This is my first contribution so this patch is probably broken.

Wasmtime GitHub notifications bot (Aug 22 2023 at 19:21):

timjrd updated PR #6850.

Wasmtime GitHub notifications bot (Aug 22 2023 at 20:15):

jameysharp submitted PR review:

I still don't think that we want to allow sign-extended values in iconst instructions, and I don't think we need to.

Most of the remaining test failures are in the precise-output tests for the riscv64 backend, so I hope @afonso360 has time to take a look. There is one similar failure in each of x64 and s390x, and none in aarch64.

In all the failing precise-output tests, I think the backend is emitting constants where the upper bits are subsequently ignored so it doesn't matter what value they have, and this has changed only those upper bits. We need to verify that though. And it looks like riscv64 is missing an opportunity to use shorter sequences of instructions when those high bits don't matter.

Wasmtime GitHub notifications bot (Aug 22 2023 at 20:15):

jameysharp submitted PR review:

I still don't think that we want to allow sign-extended values in iconst instructions, and I don't think we need to.

Most of the remaining test failures are in the precise-output tests for the riscv64 backend, so I hope @afonso360 has time to take a look. There is one similar failure in each of x64 and s390x, and none in aarch64.

In all the failing precise-output tests, I think the backend is emitting constants where the upper bits are subsequently ignored so it doesn't matter what value they have, and this has changed only those upper bits. We need to verify that though. And it looks like riscv64 is missing an opportunity to use shorter sequences of instructions when those high bits don't matter.

Wasmtime GitHub notifications bot (Aug 22 2023 at 20:15):

jameysharp created PR review comment:

The handful of test failures on your branch that are in filetests/filetests/egraph/ are because of this change: constants which were narrower than 64 bits were previously printed as unsigned, and are now printed as signed, but these tests expect the printed text to match what's in the test.

This change is harmless and I kind of like having these values printed the way you've done it, so you can fix the egraph tests by updating them to match the new format.

Wasmtime GitHub notifications bot (Aug 23 2023 at 22:11):

timjrd submitted PR review.

Wasmtime GitHub notifications bot (Aug 23 2023 at 22:11):

timjrd created PR review comment:

OK! :smile: The change in write.rs was needed to fix a test case, but ended up breaking some others, as you said. I'll update those outputs.

Wasmtime GitHub notifications bot (Aug 29 2023 at 23:02):

timjrd updated PR #6850.

Wasmtime GitHub notifications bot (Aug 29 2023 at 23:04):

timjrd edited PR #6850:

Add the following checks:

iconst.i8 immediate must be within 0 .. 2^8-1

iconst.i16 immediate must be within 0 .. 2^16-1

iconst.i32 immediate must be within 0 .. 2^32-1

Resolves #3059

Explain why this change is needed:

As mentioned in #3059, iconst currently allows any immediate within the range of an i64, even for iconst.i8, iconst.i16 or iconst.i32.

This breaks some tests!

Running cargo test in /cranelift/codegen returns successfully. I also added a few tests concerning the new checks.

Running cargo test in /cranelift returns some failures. For example:
1: - inst1 (v1 = iconst.i8 -1): constant immediate is out of bounds
[...]
failures:
    bugpoint::tests::test_reduce
    run::test::nop
Which is expected if we forbid negative immediates, @jameysharp could you please confirm?

Running cargo test at the root of this repo returns a lot of failures, possibly related to negative immediates.

This is my first contribution so this patch is probably broken.

Wasmtime GitHub notifications bot (Aug 29 2023 at 23:11):

timjrd updated PR #6850.

Wasmtime GitHub notifications bot (Aug 29 2023 at 23:29):

timjrd requested jameysharp for a review on PR #6850.

Wasmtime GitHub notifications bot (Aug 29 2023 at 23:34):

timjrd submitted PR review.

Wasmtime GitHub notifications bot (Aug 29 2023 at 23:34):

timjrd created PR review comment:

@jameysharp this is not super clean, the parser could not re-parse this output, I don't know if it's a big deal

Wasmtime GitHub notifications bot (Aug 29 2023 at 23:50):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 29 2023 at 23:50):

jameysharp created PR review comment:

Yeah, something's a little off about these tests, which appear to be sign-extending 32-bit constants to 64-bit when printing them. I think there's a bug in the writer but I haven't looked closely at it to figure out what that might be.

Wasmtime GitHub notifications bot (Aug 30 2023 at 00:26):

timjrd submitted PR review.

Wasmtime GitHub notifications bot (Aug 30 2023 at 00:26):

timjrd created PR review comment:

I didn't check but most probably the Display instance for Imm64 is falling back to hex representation for big values. This particular example is a valid negative i32 value, but a big one, so maybe the Display implementation is falling back to unsigned 64-bit hexadecimal as it can't know it's a 32-bit value. We could add range checks in this Display instance and emit the right amount of hex digits; or I could remove my changes in the CLIF writer altogether and figure out another way of fixing the tests it solved.

Wasmtime GitHub notifications bot (Aug 30 2023 at 00:55):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 30 2023 at 00:55):

jameysharp created PR review comment:

I see now. Before this PR, for constants narrower than 64 bits we were preserving whether the input was written as a negative number or not based on whether the Imm64 was sign-extended or zero-extended. So some of the egraph tests used small negative numbers on input and got negative numbers in the test expectations output; others used large enough positive numbers to have the high bit set, which were printed as positive hex numbers before but get sign-extended by your change to the writer now.

I think we should revert your change to the writer so it doesn't try to sign-extend constants, and then we'll probably just need to update the test expectations to reflect that some negative constants are printed as large positive hex values now.

Wasmtime GitHub notifications bot (Aug 31 2023 at 21:47):

timjrd updated PR #6850.

Wasmtime GitHub notifications bot (Aug 31 2023 at 21:58):

jameysharp submitted PR review:

Looks great to me! Let's see if this all passes CI and go ahead and merge it if so. Nice work!

Wasmtime GitHub notifications bot (Aug 31 2023 at 22:32):

timjrd submitted PR review.

Wasmtime GitHub notifications bot (Aug 31 2023 at 22:32):

timjrd created PR review comment:

The instructions are different. Is this expected?

Wasmtime GitHub notifications bot (Aug 31 2023 at 22:32):

timjrd created PR review comment:

Same as above.

Wasmtime GitHub notifications bot (Aug 31 2023 at 22:32):

timjrd submitted PR review.

Wasmtime GitHub notifications bot (Aug 31 2023 at 22:39):

timjrd has marked PR #6850 as ready for review.

Wasmtime GitHub notifications bot (Aug 31 2023 at 22:39):

timjrd requested wasmtime-compiler-reviewers for a review on PR #6850.

Wasmtime GitHub notifications bot (Aug 31 2023 at 23:57):

jameysharp merged PR #6850.

Last updated: Nov 22 2024 at 16:03 UTC