Bug 172766 – [JSC][ARMv6][LLInt] Fix ARMv6 support into LLInt layer

• Component: JavaScriptCore
• Status: RESOLVED WONTFIX

[Caio Lima]

...

[Caio Lima]

Created attachment 311617 [details]
Patch

[Mark Lam]

Comment on attachment 311617 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311617&action=review

> Source/JavaScriptCore/runtime/Options.h:466
> +#if CPU(ARM)
> +#define JSC_USE_TAIL_CALLS false
> +#else
> +#define JSC_USE_TAIL_CALLS true
> +#endif

What is this for?  I couldn't find any uses of JSC_USE_TAIL_CALLS anywhere.  Also, I don't think this is the right place to enable / disable a feature.  That is usually done in Platform.h.

[Caio Lima]

(In reply to Mark Lam from comment #2)
> Comment on attachment 311617 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311617&action=review
> 
> > Source/JavaScriptCore/runtime/Options.h:466
> > +#if CPU(ARM)
> > +#define JSC_USE_TAIL_CALLS false
> > +#else
> > +#define JSC_USE_TAIL_CALLS true
> > +#endif
> 
> What is this for?  I couldn't find any uses of JSC_USE_TAIL_CALLS anywhere. 
> Also, I don't think this is the right place to enable / disable a feature. 
> That is usually done in Platform.h.

Oops. It should be used in useTailCalls option, but I override it in last merge conflict solving. But I'm going to move to the right place as well.

[Caio Lima]

Created attachment 311704 [details]
Patch

[Caio Lima]

Created attachment 314589 [details]
Patch

Rebased the code with master.

The "mcr" discussion in https://bugs.webkit.org/show_bug.cgi?id=172767 also can be applied here to replace "dmb sy" with "mcr p15, 0, r6, c7, c10, 5", since it's DMB equivalence in ARMv6.

[Mark Lam]

Comment on attachment 314589 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314589&action=review

> Source/JavaScriptCore/ChangeLog:14
> +        We also disabled tail calls by default for ARMv6 because they aren't properly
> +        handled in this architecrue right now.

This is not related to "Fix ARMv6 support into LLInt layer".  I think you should do this in a different patch if needed.

> Source/JavaScriptCore/offlineasm/arm.rb:640
> +                $asm.puts "mcr p15, 0, r6, c7, c10, 5"

I'm still uncomfortable with the choice of r6 in mcr since we don't yet know what the mcr instruction expects from that register for the DMB operation.

> Source/JavaScriptCore/runtime/Options.h:32
> +#include <wtf/Platform.h>

Don't need this.  Platform.h is automatically included by config.h in .cpp files.

> Source/JavaScriptCore/runtime/Options.h:146
> -    v(bool, useTailCalls, true, Normal, nullptr) \
> +    v(bool, useTailCalls, JSC_USE_TAIL_CALLS, Normal, nullptr) \

There are 2 things wrong with this:
1. This is a debugging option to check if tail calls is the source of a bug.  It is not an option to be disabled for certain ports.  To do so would make that port not ES6 compliant.
2. We normally override the defaults in overrideDefaults() in Options.cpp.

If you are disabling it temporarily for ARMv6, please add a FIXME and a bug in overrideDefaults() where you disable it so that it will be fixed later.

Why do you need to disable tail calls anyway?  The LLInt supports tail calls.  See op_tail_call, op_tail_call_varargs, and op_tail_call_forward_arguments.  Hence, this is contrary to what your ChangeLog claims this patch does.

[Maciej Stachowiak]

Comment on attachment 314589 [details]
Patch

Unfortunately, this patch no longer applies.