Message ID | 20240823122919.3325906-1-aperez@igalia.com |
---|---|
State | Changes Requested |
Headers | show |
Series | package/wpewebkit: disable JIT for all MIPS CPUs | expand |
Hello Adrian, On Fri, 23 Aug 2024 15:29:16 +0300 Adrian Perez de Castro <aperez@igalia.com> wrote: > -# JIT is not supported for MIPS r6, but the WebKit build system does not > -# have a check for these processors. The same goes for ARMv5 and ARMv6. > -# Disable JIT forcibly here and use the CLoop interpreter instead. > +# JIT is not supported for MIPS, ARMv5, and ARMv6, but the WebKit build > +# system does not have a check for these processors. > # > +# Disable JIT forcibly here and use the CLoop interpreter instead. > # Also, we have to disable the sampling profiler and WebAssembly, which > # do NOT work with ENABLE_C_LOOP. > # > -# Upstream bugs: https://bugs.webkit.org/show_bug.cgi?id=191258 > -# https://bugs.webkit.org/show_bug.cgi?id=172765 > -# https://bugs.webkit.org/show_bug.cgi?id=265218 > +# Upstream bug: https://bugs.webkit.org/show_bug.cgi?id=278559 > # > -ifeq ($(BR2_ARM_CPU_ARMV5)$(BR2_ARM_CPU_ARMV6)$(BR2_MIPS_CPU_MIPS32R6)$(BR2_MIPS_CPU_MIPS64R6),y) > +ifeq ($(BR2_ARM_CPU_ARMV5)$(BR2_ARM_CPU_ARMV6)$(BR2_mips)$(BR2_mipsel),y) But then do we actually need a change? Keep in mind that here we are only disabling JIT explicitly for ARMv5, ARMv6 and MIPS32R6/MIPS64R6 because they were misdetected as supported by wpewebkit. Indeed wpewekit supported ARM and MIPS, but not some variants of it. For all other architectures, we do nothing, and wpewebit properly decides whether it supports JIT or not. So if JIT support for MIPS has been dropped I believe the correct change is: -ifeq ($(BR2_ARM_CPU_ARMV5)$(BR2_ARM_CPU_ARMV6)$(BR2_MIPS_CPU_MIPS32R6)$(BR2_MIPS_CPU_MIPS64R6),y) +ifeq ($(BR2_ARM_CPU_ARMV5)$(BR2_ARM_CPU_ARMV6),y) but only of course once we use a version of wpewebkit that really has dropped MIPS support. As such, your change would potentially break: it re-enables JIT on BR2_MIPS_CPU_MIPS64R6=y, because you disable for BR2_mips and BR2_mipsel but there are the 32-bit variants of MIPS, and so BR2_MIPS_CPU_MIPS64R6=y would again allow JIT. Could you clarify if we have already updated to a version of wpewebkit that has dropped MIPS support for JIT? If so, we should simply apply the simpler change above. Thanks! Thomas
Hello Thomas, On Fri, 23 Aug 2024 16:56:40 +0200 Thomas Petazzoni via buildroot <buildroot@buildroot.org> wrote: > On Fri, 23 Aug 2024 15:29:16 +0300 > Adrian Perez de Castro <aperez@igalia.com> wrote: > > > -# JIT is not supported for MIPS r6, but the WebKit build system does not > > -# have a check for these processors. The same goes for ARMv5 and ARMv6. > > -# Disable JIT forcibly here and use the CLoop interpreter instead. > > +# JIT is not supported for MIPS, ARMv5, and ARMv6, but the WebKit build > > +# system does not have a check for these processors. > > # > > +# Disable JIT forcibly here and use the CLoop interpreter instead. > > # Also, we have to disable the sampling profiler and WebAssembly, which > > # do NOT work with ENABLE_C_LOOP. > > # > > -# Upstream bugs: https://bugs.webkit.org/show_bug.cgi?id=191258 > > -# https://bugs.webkit.org/show_bug.cgi?id=172765 > > -# https://bugs.webkit.org/show_bug.cgi?id=265218 > > +# Upstream bug: https://bugs.webkit.org/show_bug.cgi?id=278559 > > # > > -ifeq ($(BR2_ARM_CPU_ARMV5)$(BR2_ARM_CPU_ARMV6)$(BR2_MIPS_CPU_MIPS32R6)$(BR2_MIPS_CPU_MIPS64R6),y) > > +ifeq ($(BR2_ARM_CPU_ARMV5)$(BR2_ARM_CPU_ARMV6)$(BR2_mips)$(BR2_mipsel),y) > > But then do we actually need a change? > > Keep in mind that here we are only disabling JIT explicitly for ARMv5, > ARMv6 and MIPS32R6/MIPS64R6 because they were misdetected as supported > by wpewebkit. Indeed wpewekit supported ARM and MIPS, but not some > variants of it. > > For all other architectures, we do nothing, and wpewebit properly > decides whether it supports JIT or not. So if JIT support for MIPS has > been dropped I believe the correct change is: > > -ifeq ($(BR2_ARM_CPU_ARMV5)$(BR2_ARM_CPU_ARMV6)$(BR2_MIPS_CPU_MIPS32R6)$(BR2_MIPS_CPU_MIPS64R6),y) > +ifeq ($(BR2_ARM_CPU_ARMV5)$(BR2_ARM_CPU_ARMV6),y) > > but only of course once we use a version of wpewebkit that really has > dropped MIPS support. I am confident that the 2.44.x does not include MIPS JIT support in JavaScriptCore, but when working this I could not just remove the $(BR2_MIPS_*) checks because the build failed. Which is another reason why I filed https://bugs.webkit.org/show_bug.cgi?id=278559 to update the checks in WebKit's CMake build system. > As such, your change would potentially break: it re-enables JIT on > BR2_MIPS_CPU_MIPS64R6=y, because you disable for BR2_mips and > BR2_mipsel but there are the 32-bit variants of MIPS, and so > BR2_MIPS_CPU_MIPS64R6=y would again allow JIT. My intention was to always make the Buildroot packaging disable JIT support on both 32- and 64-bit MIPS. I didn't notice that BR2_mips[el] only covered the 32-bit ones. > Could you clarify if we have already updated to a version of wpewebkit > that has dropped MIPS support for JIT? If so, we should simply apply > the simpler change above. I'll double check again but I am not 100% sure we can use the simpler check. Ideally, we shouldn't need a check in the Buildroot packaging and my ultimate goal is to have WebKit's CMake build system configure itself correctly so we can remove the ad-hoc checks in Buildroot. Cheers, —Adrián
diff --git a/package/wpewebkit/wpewebkit.mk b/package/wpewebkit/wpewebkit.mk index 948f260e35..d71ade577d 100644 --- a/package/wpewebkit/wpewebkit.mk +++ b/package/wpewebkit/wpewebkit.mk @@ -128,18 +128,16 @@ else WPEWEBKIT_CONF_OPTS += -DUSE_GBM=OFF endif -# JIT is not supported for MIPS r6, but the WebKit build system does not -# have a check for these processors. The same goes for ARMv5 and ARMv6. -# Disable JIT forcibly here and use the CLoop interpreter instead. +# JIT is not supported for MIPS, ARMv5, and ARMv6, but the WebKit build +# system does not have a check for these processors. # +# Disable JIT forcibly here and use the CLoop interpreter instead. # Also, we have to disable the sampling profiler and WebAssembly, which # do NOT work with ENABLE_C_LOOP. # -# Upstream bugs: https://bugs.webkit.org/show_bug.cgi?id=191258 -# https://bugs.webkit.org/show_bug.cgi?id=172765 -# https://bugs.webkit.org/show_bug.cgi?id=265218 +# Upstream bug: https://bugs.webkit.org/show_bug.cgi?id=278559 # -ifeq ($(BR2_ARM_CPU_ARMV5)$(BR2_ARM_CPU_ARMV6)$(BR2_MIPS_CPU_MIPS32R6)$(BR2_MIPS_CPU_MIPS64R6),y) +ifeq ($(BR2_ARM_CPU_ARMV5)$(BR2_ARM_CPU_ARMV6)$(BR2_mips)$(BR2_mipsel),y) WPEWEBKIT_CONF_OPTS += \ -DENABLE_JIT=OFF \ -DENABLE_C_LOOP=ON \
Change the mips32r6 into a blanket BR2_mips[el] check to disable the JSC JIT. Upstream removed JIT support for all MIPS processors in January 2024 [0], and the change trickled down to stable releases starting on version 2.44.0 [1]. While at it, change the upstream bug links to point to a more appropriate bug report. [0] https://commits.webkit.org/272866@main [1] https://lists.webkit.org/pipermail/webkit-wpe/2023-September/000612.html Signed-off-by: Adrian Perez de Castro <aperez@igalia.com> --- package/wpewebkit/wpewebkit.mk | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)