Bug 172766

Created attachment 311617 [details] Patch

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.

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

Created attachment 311704 [details] Patch

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.

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.

Comment on attachment 314589 [details] Patch Unfortunately, this patch no longer applies.