diff mbox series

[2/3] package/poke: disable minimal-threading with thumb or thumb2

Message ID 20220129142352.1197168-2-romain.naour@gmail.com
State Superseded
Headers show
Series [1/3] package/poke: install libpoke.so to staging | expand

Commit Message

Romain Naour Jan. 29, 2022, 2:23 p.m. UTC
Disable minimal-threading when thumb or thumb2 is enabled due to build and runtime issues.

After fixing the build issue "Error: invalid swi expression" with
upstream, poke fail to run with the following error:

 # poke
 ERROR: specialized instruction beghl/retR (opcode 25) is defective but has no replacement
 [...]

Fixes:
https://gitlab.com/kubu93/buildroot/-/jobs/2030046871

Reported upstream [1].

[1] https://lists.gnu.org/archive/html/poke-devel/2022-01/msg00162.html

Signed-off-by: Romain Naour <romain.naour@gmail.com>
---
 package/poke/poke.mk | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Thomas Petazzoni Jan. 30, 2022, 2:46 p.m. UTC | #1
Hello,

On Sat, 29 Jan 2022 15:23:51 +0100
Romain Naour <romain.naour@gmail.com> wrote:

> +# Disable minimal-threading when thumb or thumb2 is enabled
> +# due to runtime issue.
> +# https://lists.gnu.org/archive/html/poke-devel/2022-01/msg00162.html
> +ifeq ($(BR2_ARM_CPU_HAS_THUMB)$(BR2_ARM_CPU_HAS_THUMB2),y)
> +POKE_CONF_OPTS += --disable-dispatch-minimal-threading
> +else
> +POKE_CONF_OPTS += --enable-dispatch-minimal-threading
> +endif

I don't understand this patch, because minimal-threading is already
disabled by default (at least in poke 1.4, I haven't checked in poke
2.0). jitter/configure.ac contains:

# Is the minimal-threading dispatch enabled?  Check the default and the
# configure command-line option.
AC_MSG_CHECKING([if minimal-threading dispatch is enabled])
# FIXME: reenable this when I make defect handling reliable.
#default=$(test "x$JITTER_HAVE_ACTUAL_GCC" = "xyes" && echo yes || echo no)
default=no
AC_ARG_ENABLE([dispatch-minimal-threading],
              AS_HELP_STRING([--enable-dispatch-minimal-threading],
              [enable minimal threading dispatch: default yes if and
only if GCC (not an imitation) is used]),
              jitter_enable_dispatch_minimal_threading="$enableval",
              jitter_enable_dispatch_minimal_threading="$default")
if test "x$jitter_enable_dispatch_minimal_threading" = "xyes"; then
  jitter_best_dispatch_model="minimal-threading"
  jitter_enabled_dispatch_models="$jitter_enabled_dispatch_models minimal-threading"
fi
AC_MSG_RESULT([$jitter_enable_dispatch_minimal_threading$jitter_subpackage_disclaimer])
# FIXME: remove this warning message when I make defect handling reliable.
if test "x$jitter_enable_dispatch_minimal_threading" = "xyes"; then
  AC_MSG_WARN([minimal-threading dispatch is currently unreliable, and should
  not be used in production until defective instruction replacement is fully
  implemented])
fi

See the "default=no" ?

Also, when building with the following defconfig:

BR2_arm=y
BR2_cortex_a8=y
BR2_ARM_EABI=y
BR2_ARM_INSTRUCTIONS_THUMB2=y
BR2_TOOLCHAIN_EXTERNAL=y
BR2_INIT_NONE=y
BR2_SYSTEM_BIN_SH_NONE=y
# BR2_PACKAGE_BUSYBOX is not set
BR2_PACKAGE_POKE=y
# BR2_TARGET_ROOTFS_TAR is not set

The minimal threading is already disabled:

checking if switch dispatching is enabled... yes, provisionally (sub-package mode)
checking if direct-threading dispatch is enabled... yes, provisionally (sub-package mode)
checking if minimal-threading dispatch is enabled... no, provisionally (sub-package mode)
checking if no-threading dispatch is enabled... no, provisionally (sub-package mode)
configure: sub-package mode: disabling every dispatch except
direct-threading
configure:   (the following dispatches could have been enabled:
  switch direct-threading)
configure: enabled dispatching models are:
  direct-threading
configure: the best enabled dispatching model is:
  direct-threading

So what this patch is doing is in fact slightly the opposite of what he
says: instead of disabling minimal threading for thumb or thumb2, it
enables it for any other platform.

Are you sure this patch was needed for poke 1.4, and not for poke 2.0 ?

Best regards,

Thomas
Romain Naour Jan. 30, 2022, 5:32 p.m. UTC | #2
Hello Thomas,

Le 30/01/2022 à 15:46, Thomas Petazzoni a écrit :
> Hello,
> 
> On Sat, 29 Jan 2022 15:23:51 +0100
> Romain Naour <romain.naour@gmail.com> wrote:
> 
>> +# Disable minimal-threading when thumb or thumb2 is enabled
>> +# due to runtime issue.
>> +# https://lists.gnu.org/archive/html/poke-devel/2022-01/msg00162.html
>> +ifeq ($(BR2_ARM_CPU_HAS_THUMB)$(BR2_ARM_CPU_HAS_THUMB2),y)
>> +POKE_CONF_OPTS += --disable-dispatch-minimal-threading
>> +else
>> +POKE_CONF_OPTS += --enable-dispatch-minimal-threading
>> +endif
> 
> I don't understand this patch, because minimal-threading is already
> disabled by default (at least in poke 1.4, I haven't checked in poke
> 2.0). jitter/configure.ac contains:
> 
> # Is the minimal-threading dispatch enabled?  Check the default and the
> # configure command-line option.
> AC_MSG_CHECKING([if minimal-threading dispatch is enabled])
> # FIXME: reenable this when I make defect handling reliable.
> #default=$(test "x$JITTER_HAVE_ACTUAL_GCC" = "xyes" && echo yes || echo no)
> default=no
> AC_ARG_ENABLE([dispatch-minimal-threading],
>               AS_HELP_STRING([--enable-dispatch-minimal-threading],
>               [enable minimal threading dispatch: default yes if and
> only if GCC (not an imitation) is used]),
>               jitter_enable_dispatch_minimal_threading="$enableval",
>               jitter_enable_dispatch_minimal_threading="$default")
> if test "x$jitter_enable_dispatch_minimal_threading" = "xyes"; then
>   jitter_best_dispatch_model="minimal-threading"
>   jitter_enabled_dispatch_models="$jitter_enabled_dispatch_models minimal-threading"
> fi
> AC_MSG_RESULT([$jitter_enable_dispatch_minimal_threading$jitter_subpackage_disclaimer])
> # FIXME: remove this warning message when I make defect handling reliable.
> if test "x$jitter_enable_dispatch_minimal_threading" = "xyes"; then
>   AC_MSG_WARN([minimal-threading dispatch is currently unreliable, and should
>   not be used in production until defective instruction replacement is fully
>   implemented])
> fi
> 
> See the "default=no" ?
> 
> Also, when building with the following defconfig:
> 
> BR2_arm=y
> BR2_cortex_a8=y
> BR2_ARM_EABI=y
> BR2_ARM_INSTRUCTIONS_THUMB2=y
> BR2_TOOLCHAIN_EXTERNAL=y
> BR2_INIT_NONE=y
> BR2_SYSTEM_BIN_SH_NONE=y
> # BR2_PACKAGE_BUSYBOX is not set
> BR2_PACKAGE_POKE=y
> # BR2_TARGET_ROOTFS_TAR is not set
> 
> The minimal threading is already disabled:
> 
> checking if switch dispatching is enabled... yes, provisionally (sub-package mode)
> checking if direct-threading dispatch is enabled... yes, provisionally (sub-package mode)
> checking if minimal-threading dispatch is enabled... no, provisionally (sub-package mode)
> checking if no-threading dispatch is enabled... no, provisionally (sub-package mode)
> configure: sub-package mode: disabling every dispatch except
> direct-threading
> configure:   (the following dispatches could have been enabled:
>   switch direct-threading)
> configure: enabled dispatching models are:
>   direct-threading
> configure: the best enabled dispatching model is:
>   direct-threading
> 
> So what this patch is doing is in fact slightly the opposite of what he
> says: instead of disabling minimal threading for thumb or thumb2, it
> enables it for any other platform.
> 
> Are you sure this patch was needed for poke 1.4, and not for poke 2.0 ?

I overlooked that was disabled by default in poke 1.4.
I only checked if this option was present in poke 1.4.
So this patch must be merged in the poke version bump.

Best regards,
Romain


> 
> Best regards,
> 
> Thomas
>
diff mbox series

Patch

diff --git a/package/poke/poke.mk b/package/poke/poke.mk
index ddd03ae3b0..91518f5854 100644
--- a/package/poke/poke.mk
+++ b/package/poke/poke.mk
@@ -46,6 +46,15 @@  POKE_CONF_OPTS = \
 	--disable-libnbd \
 	--with-libreadline-prefix=$(STAGING_DIR)
 
+# Disable minimal-threading when thumb or thumb2 is enabled
+# due to runtime issue.
+# https://lists.gnu.org/archive/html/poke-devel/2022-01/msg00162.html
+ifeq ($(BR2_ARM_CPU_HAS_THUMB)$(BR2_ARM_CPU_HAS_THUMB2),y)
+POKE_CONF_OPTS += --disable-dispatch-minimal-threading
+else
+POKE_CONF_OPTS += --enable-dispatch-minimal-threading
+endif
+
 ifeq ($(BR2_PACKAGE_JSON_C),y)
 POKE_DEPENDENCIES += json-c
 POKE_CONF_OPTS += --enable-mi