825966 - JM: JSOP_CASE/JSOP_DEFAULT targets not marked as safepoint

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

This is causing a jit-test failure with my patch for bug 825892.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1: remove unused Bytecode::switchTarget

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2: Mark CASE/DEFAULT target as safepoint

This matches what we do for TABLESWITCH and LOOKUPSWITCH cases and fixes the testcase.

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 697079 [details] [diff] [review]
Part 2: Mark CASE/DEFAULT target as safepoint

Review of attachment 697079 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, this is pretty strange as the reason we mark safepoints for tableswitch targets is that they can be indirectly branched to (via the dispatch table) and need a consistent synced register state.  CASE/DEFAULT though are just dressed up branches and gotos, so seem like they should be handled by the existing machinery for such jumps.  Moreover, we already need to handle CASE/DEFAULT for unoptimizable switches even without the bug 825892 patch, so this suggests either there is a preexisting problem with the handling of these opcodes or that there is a bug in the bug 825892 patch.  How does this test fail without the patch?

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #3)
> Moreover, we already need to handle
> CASE/DEFAULT for unoptimizable switches even without the bug 825892 patch,
> so this suggests either there is a preexisting problem with the handling of
> these opcodes or that there is a bug in the bug 825892 patch.  How does this
> test fail without the patch?

Yeah, it's a preexisting problem. It's the same bug you fixed for LOOKUPSWITCH in bug 751320, if you tweak jit-test/tests/jaeger/bug751320.js so that it uses CONDSWITCH (add a "case Math:") it fails without the patch for bug 825892. Would you mind taking a look?

Comment 5

[Brian Hackett [Laid off!]]

Comment on attachment 697079 [details] [diff] [review]
Part 2: Mark CASE/DEFAULT target as safepoint

Review of attachment 697079 [details] [diff] [review]:
-----------------------------------------------------------------

OK, thanks for the explanation.

Comment 6

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5b2384539cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae47afdbb7d1

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a5b2384539cd
https://hg.mozilla.org/mozilla-central/rev/ae47afdbb7d1