Stream: git-wasmtime

Topic: wasmtime / PR #4507 cranelift: Fix Switch bug when emitti...

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 10:17):

afonso360 opened PR #4507 from switch-fix to main:

:wave: Hey,

In #4502 we discovered a bug in the switch api where it would emit icmp_imm's with types that were not able to fully represent the destination index.

The fix for this is to extend the input type to a type suitable for representing the largest index possible.

We also remove a check preventing small types from being compared. This has been fixed in the x86 backend for a while.

CC: #4502
CC: @jameysharp @bjorn3

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 11:34):

bjorn3 created PR review comment:

This is not a valid input IMHO. I don't think we should support cases larger than what the maximum value of the input (in this case 255 as it uses an i8). They can't be hit anyway.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 11:34):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 11:57):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 11:57):

afonso360 created PR review comment:

Makes sense. I'll reject these inputs instead.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 12:06):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 12:06):

afonso360 created PR review comment:

Wait, what about if we have something like this setup!(1, [10, 0x4100_0000_00bf_d470,]);

We are able to hit one of the outputs, does it make sense to reject that as well?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 12:07):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 12:09):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 12:34):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 12:34):

bjorn3 created PR review comment:

I think so too. I think it should require all cases to be in bounds.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 12:59):

afonso360 updated PR #4507 from switch-fix to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 13:00):

afonso360 updated PR #4507 from switch-fix to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 13:16):

afonso360 edited PR #4507 from switch-fix to main:

:wave: Hey,

In #4502 we discovered a bug in the switch api where it would emit icmp_imm's with types that were not able to fully represent the destination index.

We now reject these inputs. The index val must always have a type that is capable of addressing the entire range of inputs.

We also remove a check preventing small types from being compared. This has been fixed in the x86 backend for a while.

CC: #4502
CC: @jameysharp @bjorn3

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 13:45):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 17:54):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 17:55):

jameysharp merged PR #4507.

Last updated: Nov 22 2024 at 16:03 UTC