Bug 159759

Created attachment 283632 [details] Patch

Comment on attachment 283632 [details] Patch Clearing flags on attachment: 283632 Committed r203272: <http://trac.webkit.org/changeset/203272>

All reviewed patches have been landed. Closing bug.

Reopening to attach new patch.

Created attachment 284851 [details] Patch Let's disable MathIC's too until proper fix. The ARM traditional port have been broken 6 weeks ago and more fixes are needed, for example bug160295, bug159720.

Comment on attachment 284851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284851&action=review > Source/JavaScriptCore/jit/JITMathIC.h:71 > +#if CPU(ARM_TRADITIONAL) > + // FIXME: Remove this workaround once the proper fixes are landed. > + // [ARM] Disable Inline Caching on ARMv7 traditional until proper fix > + // https://bugs.webkit.org/show_bug.cgi?id=159759 > + return false; > +#endif Actually, I realized a better solution is to force the result to be JITMathICInlineResult::GenerateFullSnippet. You can accomplish that by having a compile-time conditional around m_generator.generateInline(...) below. This will revert to the behavior of the old code. This will be much faster than forcing a call on every add/mul/etc.

(In reply to comment #6) > Comment on attachment 284851 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=284851&action=review > > > Source/JavaScriptCore/jit/JITMathIC.h:71 > > +#if CPU(ARM_TRADITIONAL) > > + // FIXME: Remove this workaround once the proper fixes are landed. > > + // [ARM] Disable Inline Caching on ARMv7 traditional until proper fix > > + // https://bugs.webkit.org/show_bug.cgi?id=159759 > > + return false; > > +#endif > > Actually, I realized a better solution is to force the result to be > JITMathICInlineResult::GenerateFullSnippet. > You can accomplish that by having a compile-time conditional around > m_generator.generateInline(...) below. > This will revert to the behavior of the old code. This will be much faster > than forcing a call on every add/mul/etc. Did you mean a similar workaround? (of course with CPU(ARM_TRADITIONAL) guard) - JITMathICInlineResult result = m_generator.generateInline(jit, state); + JITMathICInlineResult result = JITMathICInlineResult::GenerateFullSnippet; I made a quick test, it decreased the number of crashes from 4500 to 3500, but it is so far from the optimal number zero. :( I'll check what causes the crashes on a debug build in 1-2 hours.

I got the following crash backtraces with the patch you suggested: ASSERTION FAILED: linkBuffer.isValid() ../../Source/JavaScriptCore/jit/JITMathIC.h(130) : JSC::JITMathIC<Generator>::generateOutOfLine(JSC::VM&, JSC::CdeBlock*, JSC::FunctionPtr)::<lambda()> [with GeneratorType = JSC::JITAddGenerator] 1 0x41c53ad8 WTFCrash 2 0x41764034 JSC::JITMathIC<JSC::JITAddGenerator>::generateOutOfLine(JSC::VM&, JSC::CodeBlock*, JSC::FunctionPr)::{lambda()#1}::operator()() const 3 0x417643fc JSC::JITMathIC<JSC::JITAddGenerator>::generateOutOfLine(JSC::VM&, JSC::CodeBlock*, JSC::FunctionPr) 4 0x4175acb4 Illegal instruction ERROR: Unexpected exit code: 132 ---------------------- ASSERTION FAILED: static_cast<ptrdiff_t>(inlineSize) <= MacroAssembler::maxJumpReplacementSize() ../../Source/JavaScriptCore/jit/JITMathIC.h(81) : bool JSC::JITMathIC<Generator>::generateInline(JSC::CCallHelpers&, JSC::athICGenerationState&, bool) [with GeneratorType = JSC::JITAddGenerator] 1 0x41b65ad8 WTFCrash 2 0x41441788 JSC::JITMathIC<JSC::JITAddGenerator>::generateInline(JSC::CCallHelpers&, JSC::MathICGenerationState&, bool) 3 0x41643f50 void JSC::JIT::emitMathICFast<JSC::JITAddGenerator, long long (*)(JSC::ExecState*, long long, long long, JS::ArithProfile*), long long (*)(JSC::ExecState*, long long, long long)>(JSC::JITMathIC<JSC::JITAddGenerator>*, JSC::Instruction*, long long (*)(JSC::ExecStat*, long long, long long, JSC::ArithProfile*), long long (*)(JSC::ExecState*, long long, long long)) 4 0x41641148 JSC::JIT::emit_op_add(JSC::Instruction*) 5 0x4162f710 JSC::JIT::privateCompileMainPass() 6 0x41632868 JSC::JIT::compileWithoutLinking(JSC::JITCompilationEffort) 7 0x41633e34 JSC::JIT::privateCompile(JSC::JITCompilationEffort) 8 0x41691630 JSC::JIT::compile(JSC::VM*, JSC::CodeBlock*, JSC::JITCompilationEffort) 9 0x41861300 10 0x418614d8 JSC::ScriptExecutable::prepareForExecutionImpl(JSC::ExecState*, JSC::JSFunction*, JSC::JSScope*, JSC::CodeSecializationKind) 11 0x415fb35c JSC::ScriptExecutable::prepareForExecution(JSC::ExecState*, JSC::JSFunction*, JSC::JSScope*, JSC::CodeSpecilizationKind) 12 0x4166636c Illegal instruction ERROR: Unexpected exit code: 132 Do you have a better idea how to workaround this regression? Or should we push the previous change I suggested?

Comment on attachment 284851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284851&action=review >>> Source/JavaScriptCore/jit/JITMathIC.h:71 >>> +#endif >> >> Actually, I realized a better solution is to force the result to be JITMathICInlineResult::GenerateFullSnippet. >> You can accomplish that by having a compile-time conditional around m_generator.generateInline(...) below. >> This will revert to the behavior of the old code. This will be much faster than forcing a call on every add/mul/etc. > > Did you mean a similar workaround? (of course with CPU(ARM_TRADITIONAL) guard) > - JITMathICInlineResult result = m_generator.generateInline(jit, state); > + JITMathICInlineResult result = JITMathICInlineResult::GenerateFullSnippet; > > I made a quick test, it decreased the number of crashes from > 4500 to 3500, but it is so far from the optimal number zero. :( > I'll check what causes the crashes on a debug build in 1-2 hours. Sorry, so you want to both do that and then also put the if (ArithProfile* ...) branch behind a compile flag so it doesn't execute in arm traditional

Created attachment 285097 [details] Patch

(In reply to comment #10) > Created attachment 285097 [details] > Patch Did you mean this workaround? Unfortunately it doesn't help too lot. There are still 1066 crashes with this patch, it means that all mozilla-tests.yaml/****.js.mozilla-baseline tests crashes because of "Illegal instruction".

(In reply to comment #11) > (In reply to comment #10) > > Created attachment 285097 [details] > > Patch > > Did you mean this workaround? Unfortunately it doesn't help too lot. > There are still 1066 crashes with this patch, it means that all > mozilla-tests.yaml/****.js.mozilla-baseline tests crashes because > of "Illegal instruction". I still get the following assertions with this workaround: ASSERTION FAILED: linkBuffer.isValid() ../../Source/JavaScriptCore/jit/JITMathIC.h(137) : JSC::JITMathIC<Generator>::generateOutOfLine(JSC::VM&, JSC::CodeBlock*, JSC::FunctionPtr)::<lambda()> [with GeneratorType = JSC::JITAddGenerator] 1 0xb638eb44 WTFCrash 2 0xb5e9e0e4 JSC::JITMathIC<JSC::JITAddGenerator>::generateOutOfLine(JSC::VM&, JSC::CodeBlock*, JSC::FunctionPtr)::{lambda()#1}::operator()() const 3 0xb5e9e778 JSC::JITMathIC<JSC::JITAddGenerator>::generateOutOfLine(JSC::VM&, JSC::CodeBlock*, JSC::FunctionPtr) 4 0xb5e94ab4 Segmentation fault ERROR: Unexpected exit code: 139

Comment on attachment 284851 [details] Patch The ARM assembler is broken since 6 weeks and I will take my holiday since now until the end of August and don't want to left ARM assembler broken for this long period. Please reconsider your previous r- on this workaround.

Comment on attachment 285097 [details] Patch clearing r? , because it doesn't work

Comment on attachment 284851 [details] Patch Clearing flags on attachment: 284851 Committed r204025: <http://trac.webkit.org/changeset/204025>

All reviewed patches have been landed. Closing bug.