Bug 159759 – [ARM] Disable Inline Caching on ARMv7 traditional until proper fix

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Csaba Osztrogonác]

https://trac.webkit.org/changeset/202214 broke inline caching
on ARMv7 traditional (ARM instruction set / ARMAssembler)

bug159408 is to fix this regression step by step.
But ARM traditional assembler is completely broken
since a month ago, so I propose disabling inline
caching on it until all fixes are reviewed and landed.

[Csaba Osztrogonác]

Created attachment 283632 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 283632 [details]
Patch

Clearing flags on attachment: 283632

Committed r203272: <http://trac.webkit.org/changeset/203272>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopening to attach new patch.

[Csaba Osztrogonác]

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.

[Saam Barati]

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.

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

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?

[Saam Barati]

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

[Csaba Osztrogonác]

Created attachment 285097 [details]
Patch

[Csaba Osztrogonác]

(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".

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Comment on attachment 285097 [details]
Patch

clearing r? , because it doesn't work

[WebKit Commit Bot]

Comment on attachment 284851 [details]
Patch

Clearing flags on attachment: 284851

Committed r204025: <http://trac.webkit.org/changeset/204025>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.