1706314 - Assertion failure: def->type() == phi->type(), at jit/IonAnalysis.cpp:1861

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20210420-a916ade0ae29 (debug build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off):

function main() {
  let v0 = 0;
  while (v0 < 10) {
    for (let v14 = 2; v14 < 100; v14++) {
        for (const v17 of "1073741824") {}
        const v19 = this;
        for (let v26 = 0; v26 < 10; v26++) {}
    }
  }
}
main();

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x00005555578c4b10 in (anonymous namespace)::TypeAnalyzer::analyze() ()
#1  0x00005555578b93f1 in js::jit::ApplyTypeInformation(js::jit::MIRGenerator*, js::jit::MIRGraph&) ()
#2  0x00005555578b2cd8 in js::jit::OptimizeMIR(js::jit::MIRGenerator*) ()
#3  0x00005555578bc48c in js::jit::CompileBackEnd(js::jit::MIRGenerator*, js::jit::WarpSnapshot*) ()
#4  0x00005555578bdd72 in js::jit::Compile(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned char*) ()
#5  0x00005555578be792 in IonCompileScriptForBaseline(JSContext*, js::jit::BaselineFrame*, unsigned char*) ()
#6  0x00005555578befaa in js::jit::IonCompileScriptForBaselineOSR(JSContext*, js::jit::BaselineFrame*, unsigned int, unsigned char*, js::jit::IonOsrTempData**) ()
#7  0x00001d410ff0fa77 in ?? ()
#8  0xfffe0dd52880d010 in ?? ()
#9  0x00007fffffffba00 in ?? ()
#10 0x0000000000000000 in ?? ()
rax	0x55555571170e	93824994055950
rbx	0x7ffff6051ef8	140737320918776
rcx	0x555558024188	93825037123976
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffb0e0	140737488335072
rsp	0x7fffffffb060	140737488334944
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f99840	140737353717824
r10	0x0	0
r11	0x0	0
r12	0x7ffff604f840	140737320908864
r13	0x2	2
r14	0x7ffff60ea3e8	140737321542632
r15	0x7ffff60501a0	140737320911264
rip	0x5555578c4b10 <(anonymous namespace)::TypeAnalyzer::analyze()+9424>
=> 0x5555578c4b10 <_ZN12_GLOBAL__N_112TypeAnalyzer7analyzeEv+9424>:	movl   $0x745,0x0
   0x5555578c4b1b <_ZN12_GLOBAL__N_112TypeAnalyzer7analyzeEv+9435>:	callq  0x555556a8413e <abort>

Marking s-s since this is a JIT assert.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20210420095122-a916ade0ae29.
The bug appears to have been introduced in the following build range:

Start: 863a9ed73a15e633e80c91994d351e7957d13093 (20210414170625)
End: d0210270c7e821f637a92a50c591e5d20bb48f90 (20210414170727)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=863a9ed73a15e633e80c91994d351e7957d13093&tochange=d0210270c7e821f637a92a50c591e5d20bb48f90

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1706314: Don't specialize OSR phis after pruning path to preheader r=jandem

This bug is an extension of bug 1704467. That bug was triggered when we had a phi with one undefined operand and one OSRValue operand; this bug is triggered if we have a phi with one undefined operand and a second phi operand, which in turn has OSRValue operands. I've updated the testcase to test the broader problem.

It's relatively simple to fix this, but at this point I'm tired of playing whack-a-mole with this code. All of these problems (this bug, bug 1704467, bug 1701208) happen when we try to specialize OSR phis after branch pruning has removed the path from the entry block to the preheader. The cleanest fix, which I probably should have done a while ago, is to just disable OSR phi specialization in this case. It's not clear that it's even useful: for example, in this testcase, which isn't particularly weird, we end up specializing o1 and o2 to null because that's the only information we have, which will cause us to immediately bail out and recompile.

After disabling OSR specialization, we can also remove the extra code we added to handle this weird case.

Comment 4

[Andrew McCreight [:mccr8]]

From your description, I'm going to mark this as a regression from bug 1702306 and not from bug 1704467, particularly because it sounds like it could fix even more issues with the former that haven't been discovered yet.

Comment 5

[Daniel Veditz [:dveditz]]

This bug is an extension of bug 1704467.

Does that mean it's a "probably not a security bug" like that one and we can rate it sec-low

Comment 6

[Iain Ireland [:iain]]

Yeah, security-wise there's no practical difference between the two.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Don't specialize OSR phis after pruning path to preheader r=jandem
https://hg.mozilla.org/integration/autoland/rev/0f0dcd677e6568cca6cb93c4c7298c641071c895
https://hg.mozilla.org/mozilla-central/rev/0f0dcd677e65

Comment 8

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20210422030450-a74febc3cc23.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:iain, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 10

[Iain Ireland [:iain]]

Bluh. According to bug 1707544, this part was too optimistic:

It's not clear that it's even useful: for example, in this testcase, which isn't particularly weird, we end up specializing o1 and o2 to null because that's the only information we have, which will cause us to immediately bail out and recompile.

It looks like OSR phi specialization matters in some real-world cases with branch pruning. We also can't turn off branch pruning here because that would affect (for example) scalar replacement in for-of.

I guess the best solution is to resurrect the code I removed and fix the bug. I already had a patch written before I decided to go with the more direct approach.

Comment 11

[Iain Ireland [:iain]]

Attachment: Bug 1706314: Back out previous fix r=jandem

Comment 12

[Iain Ireland [:iain]]

Attachment: Bug 1706314: Add OSR guards for mismatched phis r=jandem!

Depends on D113416

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Back out previous fix r=jandem
https://hg.mozilla.org/integration/autoland/rev/fef6eb0de42b7f8fb3e50dbc454611f3a04b0e8f
Add OSR guards for mismatched phis r=jandem
https://hg.mozilla.org/integration/autoland/rev/e986a5e437c233adf150ebf045e23a5c27380236

https://hg.mozilla.org/mozilla-central/rev/fef6eb0de42b
https://hg.mozilla.org/mozilla-central/rev/e986a5e437c2

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Does this need a Beta uplift request?

Comment 15

[Iain Ireland [:iain]]

Comment on attachment 9218476 [details]
Bug 1706314: Add OSR guards for mismatched phis r=jandem!

Beta/Release Uplift Approval Request

• User impact if declined: Potentially incorrect results (we could end up incorrectly folding a result to null or undefined).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The fix is well understood, and the patch has been baking in Nightly for two weeks without any problems.

The alternative would be to leave this unfixed: I'm not aware of any cases where this bug has been triggered outside of a fuzz bug.

• String changes made/needed:

Comment 16

[Iain Ireland [:iain]]

AFAICT this isn't a security concern, but it might be a correctness issue. The requirements for triggering it are less niche than many fuzz bugs: we need a loop using a variable that is only assigned a non undefined/null value before the loop, plus some conditions about how it's used inside the loop. I'm not aware of any cases of this being an issue in the wild, though, so it would also be reasonable to choose not to uplift it.

Comment 17

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9218476 [details]
Bug 1706314: Add OSR guards for mismatched phis r=jandem!

Given that it is a sec-low, that this doesn't fix an identified end-user issue and that we are in our last week of betas, I think we can indeed let it ride the trains, thanks!