wasmtime / PR #1697 Fix missing modification of jump tabl... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #1697 Fix missing modification of jump tabl...

Wasmtime GitHub notifications bot (May 14 2020 at 14:09):

Y-Nak opened PR #1697 from fix-licm-jt-bug to master:

Resolve #1648 and resolve #1080.

[x] This has been discussed in issue #1648 and #1080

[x] A short description of what this does, why it is needed;

Fix inconsistency between branch_destination and branch_destination_mut (Just adding IndirectJump case to branch_destination_mut).

Modify jump table entry if it points at loop header, and also modify destination if it points at loop header.

[x] This PR contains test cases, if meaningful.

[x] A reviewer from the core maintainer team has been assigned for this PR.
@iximeow Could you review this please? I know you are aware of the issue, so please feel free to close this PR if you have started looking into.

Wasmtime GitHub notifications bot (May 14 2020 at 14:48):

Y-Nak submitted PR Review.

Wasmtime GitHub notifications bot (May 14 2020 at 14:48):

Y-Nak created PR Review Comment:

Oh, thanks!

Wasmtime GitHub notifications bot (May 14 2020 at 14:48):

Y-Nak updated PR #1697 from fix-licm-jt-bug to master:

Resolve #1648 and resolve #1080.

[x] This has been discussed in issue #1648 and #1080

[x] A short description of what this does, why it is needed;

Fix inconsistency between branch_destination and branch_destination_mut (Just adding IndirectJump case to branch_destination_mut).

Modify jump table entry if it points at loop header, and also modify destination if it points at loop header.

[x] This PR contains test cases, if meaningful.

[x] A reviewer from the core maintainer team has been assigned for this PR.
@iximeow Could you review this please? I know you are aware of the issue, so please feel free to close this PR if you have started looking into.

Wasmtime GitHub notifications bot (Jun 25 2020 at 18:49):

alexcrichton edited PR #1697 from fix-licm-jt-bug to main:

Resolve #1648 and resolve #1080.

[x] This has been discussed in issue #1648 and #1080

[x] A short description of what this does, why it is needed;

Fix inconsistency between branch_destination and branch_destination_mut (Just adding IndirectJump case to branch_destination_mut).

Modify jump table entry if it points at loop header, and also modify destination if it points at loop header.

[x] This PR contains test cases, if meaningful.

[x] A reviewer from the core maintainer team has been assigned for this PR.
@iximeow Could you review this please? I know you are aware of the issue, so please feel free to close this PR if you have started looking into.

Wasmtime GitHub notifications bot (Dec 07 2020 at 17:20):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Dec 07 2020 at 17:20):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Dec 07 2020 at 17:20):

cfallin created PR Review Comment:

It seems we could fold this into a helper alongside change_branch_destination on Function, by defining a rewrite_branch_destination(inst, old_target, new_target) that replaces any old_target target in the instruction with new_target, doing the replacement on any target(s) that exist in the instruction. Thoughts? This would also remove the awkward match below.

Wasmtime GitHub notifications bot (Dec 07 2020 at 17:20):

cfallin created PR Review Comment:

Can we remove the encodings here? They'll cause issues when we switch to the new backend by default and I don't think they're needed for the test.

Wasmtime GitHub notifications bot (Dec 08 2020 at 07:26):

Y-Nak submitted PR Review.

Wasmtime GitHub notifications bot (Dec 08 2020 at 07:26):

Y-Nak created PR Review Comment:

We can remove the encodings iff the test runs with the new backend, otherwise Verifier::verify_encodings fails because some jump insts with encodings is inserted here.
https://github.com/bytecodealliance/wasmtime/blob/2cec20aa57037ecdfaef46176f31fe8f6b33c2d8/cranelift/codegen/src/licm.rs#L94-L98
So, I'm inclined to remove all encodings from the test and change the target machine to aarch64 from x86-64, what do you think about it?

Wasmtime GitHub notifications bot (Dec 08 2020 at 07:26):

Y-Nak submitted PR Review.

Wasmtime GitHub notifications bot (Dec 08 2020 at 07:26):

Y-Nak created PR Review Comment:

Exactly! Thanks for pointing out.
I'll add Function::rewrite_branch_destination.

Wasmtime GitHub notifications bot (Dec 08 2020 at 08:40):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Dec 08 2020 at 08:40):

cfallin created PR Review Comment:

Yes, that sounds good, thanks!

Wasmtime GitHub notifications bot (Dec 09 2020 at 02:24):

Y-Nak updated PR #1697 from fix-licm-jt-bug to main:

Resolve #1648 and resolve #1080.

[x] This has been discussed in issue #1648 and #1080

[x] A short description of what this does, why it is needed;

Fix inconsistency between branch_destination and branch_destination_mut (Just adding IndirectJump case to branch_destination_mut).

Modify jump table entry if it points at loop header, and also modify destination if it points at loop header.

[x] This PR contains test cases, if meaningful.

[x] A reviewer from the core maintainer team has been assigned for this PR.
@iximeow Could you review this please? I know you are aware of the issue, so please feel free to close this PR if you have started looking into.

Wasmtime GitHub notifications bot (Dec 09 2020 at 04:18):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Dec 09 2020 at 04:19):

cfallin merged PR #1697.

Last updated: Dec 23 2024 at 12:05 UTC