diff mbox series

package/wpewebkit: disable JIT for all MIPS CPUs

Message ID 20240823122919.3325906-1-aperez@igalia.com
State Changes Requested
Headers show
Series package/wpewebkit: disable JIT for all MIPS CPUs | expand

Commit Message

Adrian Perez de Castro Aug. 23, 2024, 12:29 p.m. UTC
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(-)

Comments

Thomas Petazzoni Aug. 23, 2024, 2:56 p.m. UTC | #1
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
Adrian Perez de Castro Sept. 2, 2024, 8:29 p.m. UTC | #2
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 mbox series

Patch

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 \