RESOLVED FIXED 236037
Wasm crash on https://copy.sh/v86/?profile=dsl
https://bugs.webkit.org/show_bug.cgi?id=236037
Summary Wasm crash on https://copy.sh/v86/?profile=dsl
Yusuke Suzuki
Reported 2022-02-02 13:54:55 PST
And in the other examples in https://github.com/copy/v86
Attachments
asm.txt (16.18 MB, text/plain)
2022-02-06 20:42 PST, Saam Barati
no flags
quick-and-dirty-fix (2.82 KB, patch)
2022-02-06 21:05 PST, Saam Barati
ews-feeder: commit-queue-
patch (5.32 KB, patch)
2022-02-07 15:57 PST, Saam Barati
mark.lam: review+
ews-feeder: commit-queue-
patch for landing (5.33 KB, patch)
2022-02-07 18:16 PST, Saam Barati
no flags
Alexey Proskuryakov
Comment 1 2022-02-03 16:45:22 PST
Saam Barati
Comment 2 2022-02-04 10:06:40 PST
Saam Barati
Comment 3 2022-02-04 11:38:54 PST
Turning off OMG makes the crash go away. Will try to configure more options to see what reproduces and what doesn't.
Saam Barati
Comment 4 2022-02-04 11:58:07 PST
it seems like an issue with OMG+fast memory. If I turn on OMG, and turn off fast memory, I don't crash.
Saam Barati
Comment 5 2022-02-04 15:42:31 PST
Reproduces with concurrent JIT off.
Saam Barati
Comment 6 2022-02-04 15:43:15 PST
Reproduces with OSR entry turned off
Saam Barati
Comment 7 2022-02-05 11:03:21 PST
seems like we need to be running with webAssemblyOMGOptimizationLevel=2 for this to reproduce. It also doesn't seem to be a bug in IRC, since running with O1 and forcing on IRC does not crash either
Yusuke Suzuki
Comment 8 2022-02-05 11:15:43 PST
(In reply to Saam Barati from comment #7) > seems like we need to be running with webAssemblyOMGOptimizationLevel=2 for > this to reproduce. It also doesn't seem to be a bug in IRC, since running > with O1 and forcing on IRC does not crash either Nice! In that case, typically I comment out several path in B3Generate.cpp to see which pass can trigger this problem :) 84 if (procedure.optLevel() >= 2) { 85 reduceDoubleToFloat(procedure); 86 reduceStrength(procedure); 87 // FIXME: Re-enable B3 hoistLoopInvariantValues 88 // https://bugs.webkit.org/show_bug.cgi?id=212651 89 if (Options::useB3HoistLoopInvariantValues()) 90 hoistLoopInvariantValues(procedure); 91 if (eliminateCommonSubexpressions(procedure)) 92 eliminateCommonSubexpressions(procedure); 93 eliminateDeadCode(procedure); 94 inferSwitches(procedure); 95 reduceLoopStrength(procedure); 96 if (Options::useB3TailDup()) 97 duplicateTails(procedure); 98 fixSSA(procedure); 99 foldPathConstants(procedure); 100 // FIXME: Add more optimizations here. 101 // https://bugs.webkit.org/show_bug.cgi?id=150507
Saam Barati
Comment 9 2022-02-06 20:33:38 PST
(In reply to Yusuke Suzuki from comment #8) > (In reply to Saam Barati from comment #7) > > seems like we need to be running with webAssemblyOMGOptimizationLevel=2 for > > this to reproduce. It also doesn't seem to be a bug in IRC, since running > > with O1 and forcing on IRC does not crash either > > Nice! In that case, typically I comment out several path in B3Generate.cpp > to see which pass can trigger this problem :) > > 84 if (procedure.optLevel() >= 2) { > 85 reduceDoubleToFloat(procedure); > 86 reduceStrength(procedure); > 87 // FIXME: Re-enable B3 hoistLoopInvariantValues > 88 // https://bugs.webkit.org/show_bug.cgi?id=212651 > 89 if (Options::useB3HoistLoopInvariantValues()) > 90 hoistLoopInvariantValues(procedure); > 91 if (eliminateCommonSubexpressions(procedure)) > 92 eliminateCommonSubexpressions(procedure); > 93 eliminateDeadCode(procedure); > 94 inferSwitches(procedure); > 95 reduceLoopStrength(procedure); > 96 if (Options::useB3TailDup()) > 97 duplicateTails(procedure); > 98 fixSSA(procedure); > 99 foldPathConstants(procedure); > 100 // FIXME: Add more optimizations here. > 101 // https://bugs.webkit.org/show_bug.cgi?id=150507 Yeah, I've been doing this. No luck so far. Though commenting out some stuff seems to change how frequently the crash reproduces.
Saam Barati
Comment 10 2022-02-06 20:40:42 PST
Unrelated, but we emit some silly code we should fix: ``` 0x120b5282c: str x0, [sp, #17128] 0x120b52830: ldr x0, [sp, #17128] ```
Saam Barati
Comment 11 2022-02-06 20:42:50 PST
Created attachment 451062 [details] asm.txt Attaching asm of a function we're crashing in. Noted which instruction we're crashing in with "CRASH". We have this code sequence: 0x120b52850: ldr x0, [sp, #17096] 0x120b52854: add x0, x22, x0 0x120b52858: movz x16, #0xc640 0x120b5285c: add x16, sp, x16, uxtx 0x120b52860: ldur x1, [x16] 0x120b52864: ldr w20, [x0, x1] <--- CRASH The interesting thing here is I can't find where we store to [sp, #17096], which makes me wonder if it's uninitialized. But there's a chance we're storing it somehow that I haven't been able to figure out by statically looking at the asm.
Saam Barati
Comment 12 2022-02-06 20:46:41 PST
(In reply to Saam Barati from comment #11) > Created attachment 451062 [details] > asm.txt > > Attaching asm of a function we're crashing in. Noted which instruction we're > crashing in with "CRASH". > > We have this code sequence: > > 0x120b52850: ldr x0, [sp, #17096] > 0x120b52854: add x0, x22, x0 > 0x120b52858: movz x16, #0xc640 > 0x120b5285c: add x16, sp, x16, uxtx > 0x120b52860: ldur x1, [x16] > 0x120b52864: ldr w20, [x0, x1] <--- CRASH > > > The interesting thing here is I can't find where we store to [sp, #17096], > which makes me wonder if it's uninitialized. But there's a chance we're > storing it somehow that I haven't been able to figure out by statically > looking at the asm. Nevermind, was happening right in front of my face: 0x120b52840: movz x16, #0x42c8 0x120b52844: add x16, sp, x16, uxtx 0x120b52848: stur w0, [x16] 0x120b5284c: stur wzr, [x16] in: 0x120b52840: movz x16, #0x42c8 0x120b52844: add x16, sp, x16, uxtx 0x120b52848: stur w0, [x16] 0x120b5284c: stur wzr, [x16] 0x120b52850: ldr x0, [sp, #17096] 0x120b52854: add x0, x22, x0 0x120b52858: movz x16, #0xc640 0x120b5285c: add x16, sp, x16, uxtx 0x120b52860: ldur x1, [x16] 0x120b52864: ldr w20, [x0, x1] <--- CRASH But it seems like we're storing zero to it right after storing something else. So weird.
Saam Barati
Comment 13 2022-02-06 20:51:41 PST
Check out these first two moves of constants followed by stores: 0x120b5283c: movz x16, #0x42cc 0x120b52840: movz x16, #0x42c8 0x120b52844: add x16, sp, x16, uxtx 0x120b52848: stur w0, [x16] 0x120b5284c: stur wzr, [x16] 0x120b52850: ldr x0, [sp, #17096] 0x120b52854: add x0, x22, x0 0x120b52858: movz x16, #0xc640 0x120b5285c: add x16, sp, x16, uxtx 0x120b52860: ldur x1, [x16] 0x120b52864: ldr w20, [x0, x1] <--- CRASH Can't get over the fact that it seems like we shouldn't be moving into x16 back to back like that, and somehow the code should've been ordered differently so we store to both locations independently.
Saam Barati
Comment 14 2022-02-06 20:56:18 PST
I'm pretty suspicious of a bug inside AirLowerStackArgs.
Saam Barati
Comment 15 2022-02-06 21:02:11 PST
(In reply to Saam Barati from comment #14) > I'm pretty suspicious of a bug inside AirLowerStackArgs. I found a bug inside AirLowerStackArgs. But I don't know if it's our crash or not.
Saam Barati
Comment 16 2022-02-06 21:05:32 PST
Created attachment 451064 [details] quick-and-dirty-fix this is the bug I spotted. Will make a nicer patch once I test it.
Saam Barati
Comment 17 2022-02-06 21:30:17 PST
(In reply to Saam Barati from comment #16) > Created attachment 451064 [details] > quick-and-dirty-fix > > this is the bug I spotted. Will make a nicer patch once I test it. This is the right fix. It also fixes a bug where the site just hangs when loading. We used to crash or hang 100% of the time, and this fixes that issue. However, I did encounter one crash that I still need to investigate. The crash happened ~1/20 loads. So maybe it's a different issue. I'll upload a patch to fix this tomorrow. Ima try to think about how to write a test case, too. It might be tricky. I'll open a separate bug to investigate the other issue tomorrow.
Saam Barati
Comment 18 2022-02-07 15:57:44 PST
Mark Lam
Comment 19 2022-02-07 17:11:59 PST
Comment on attachment 451165 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=451165&action=review r=me > Source/JavaScriptCore/ChangeLog:22 > + mov x16, k > + mov x16, k2 > + mov x1, [x16] > + mov zr, [x16] Did you mean this instead? ``` mov k, x16 mov k2, x16 mov x1, [x16] mov zr, [x16] ``` I find this example confusing: the first 2 instructions imply the dest is the left operand, while the last 2 instructions imply that the dest is the right operand. Can you make it consistent? > Source/JavaScriptCore/ChangeLog:28 > + mov x16, k > + mov x1, [x16] > + mov x16, k2 > + mov zr, [x16] Ditto.
Saam Barati
Comment 20 2022-02-07 18:12:13 PST
Comment on attachment 451165 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=451165&action=review >> Source/JavaScriptCore/ChangeLog:22 >> + mov zr, [x16] > > Did you mean this instead? > ``` > mov k, x16 > mov k2, x16 > mov x1, [x16] > mov zr, [x16] > ``` > I find this example confusing: the first 2 instructions imply the dest is the left operand, while the last 2 instructions imply that the dest is the right operand. Can you make it consistent? We're moving constants k and k2 into x16. Let me switch to using arm syntax and it'll clear things up.
Saam Barati
Comment 21 2022-02-07 18:16:24 PST
Created attachment 451189 [details] patch for landing
EWS
Comment 22 2022-02-07 19:01:40 PST
Committed r289354 (246942@main): <https://commits.webkit.org/246942@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451189 [details].
Saam Barati
Comment 23 2022-02-07 21:56:49 PST
Note You need to log in before you can comment on or make changes to this bug.