[Buildroot] [PATCH 0/3] NodeJS fixes

[Buildroot] [PATCH 0/3] NodeJS fixes

[Thomas Petazzoni]

Hello,

I finally took a closer look at two long standing NodeJS issues:

 - Issue with the host tool not finding libcrypto.so (bug #12211)

 - Issue with invalid floating point instructions being used on some
   ARM platforms/configurations (bug #12166)

This patch series fixes both issues.

Thanks,

Thomas

Thomas Petazzoni (3):
  package/nodejs: properly pass the --with-arm-float-abi on ARM
  package/nodejs: use --with-arm-fpu option on ARM
  package/nodejs: properly pass HOST_LDFLAGS when building host tools

 package/nodejs/nodejs.mk | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

-- 
2.23.0

[Buildroot] [PATCH 1/3] package/nodejs: properly pass the --with-arm-float-abi on ARM

[Thomas Petazzoni]

When commit 0064132ba032da39cefa4fffe59c31a71d1f1ddb introduced ARM64
support in nodejs.mk, it incorrectly kept the NODEJS_ARM_FP
definition. This variable is used to pass --with-arm-float-abi, which
in NodeJS's configure.py script is only used when --dest-cpu=arm, and
not when --dest-cpu=arm64.

So we are passing --with-arm-float-abi=<something> for ARM64, which
has no effect, and we are no longer passing it on ARM.

This commit fixes that by putting the NODEJS_ARM_FP definition back at
the right location.

Fixes:

   0064132ba032da39cefa4fffe59c31a71d1f1ddb

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 package/nodejs/nodejs.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/package/nodejs/nodejs.mk b/package/nodejs/nodejs.mk
index 6e595b32c5..be13a0da37 100644
--- a/package/nodejs/nodejs.mk
+++ b/package/nodejs/nodejs.mk
@@ -110,10 +110,10 @@ else ifeq ($(BR2_mipsel),y)
 NODEJS_CPU = mipsel
 else ifeq ($(BR2_arm),y)
 NODEJS_CPU = arm
-else ifeq ($(BR2_aarch64),y)
-NODEJS_CPU = arm64
 # V8 needs to know what floating point ABI the target is using.
 NODEJS_ARM_FP = $(GCC_TARGET_FLOAT_ABI)
+else ifeq ($(BR2_aarch64),y)
+NODEJS_CPU = arm64
 endif
 
 # MIPS architecture specific options
-- 
2.23.0

[Buildroot] [PATCH 2/3] package/nodejs: use --with-arm-fpu option on ARM

[Thomas Petazzoni]

nodejs can use some FPU instructions on ARM, but it needs to know that
thanks to the --with-arm-fpu option. Without this, it may use the
wrong FPU setting, such as use VFPv3 even if only a VFPv3-D16 is
available. This has been reported as bug #12166, where the compiled
node binary had some floating point instructions using floating point
registers above 16 on a VFPv3-D16 system.

This commit makes sure we pass the appropriate --with-arm-fpu value
when it makes sense. Note that NodeJS only has explicit support for a
subset of the FPUs, for the ones that are not explicitly supported, we
simply pass no --with-arm-fpu value.

Fixes:

  https://bugs.busybox.net/show_bug.cgi?id=12166

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 package/nodejs/nodejs.mk | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/package/nodejs/nodejs.mk b/package/nodejs/nodejs.mk
index be13a0da37..7fb02970cf 100644
--- a/package/nodejs/nodejs.mk
+++ b/package/nodejs/nodejs.mk
@@ -112,6 +112,19 @@ else ifeq ($(BR2_arm),y)
 NODEJS_CPU = arm
 # V8 needs to know what floating point ABI the target is using.
 NODEJS_ARM_FP = $(GCC_TARGET_FLOAT_ABI)
+# it also wants to know which FPU to use, but only has support for
+# vfp, vfpv3, vfpv3-d16 and neon.
+ifeq ($(BR2_ARM_FPU_VFPV2),y)
+NODEJS_ARM_FPU = vfp
+# vfpv4 is a superset of vfpv3
+else ifeq ($(BR2_ARM_FPU_VFPV3)$(BR2_ARM_FPU_VFPV4),y)
+NODEJS_ARM_FPU = vfpv3
+# vfpv4-d16 is a superset of vfpv3-d16
+else ifeq ($(BR2_ARM_FPU_VFPV3D16)$(BR2_ARM_FPU_VFPV4D16),y)
+NODEJS_ARM_FPU = vfpv3-d16
+else ifeq ($(BR2_ARM_FPU_NEON),y)
+NODEJS_ARM_FPU = neon
+endif
 else ifeq ($(BR2_aarch64),y)
 NODEJS_CPU = arm64
 endif
@@ -148,6 +161,7 @@ define NODEJS_CONFIGURE_CMDS
 		--prefix=/usr \
 		--dest-cpu=$(NODEJS_CPU) \
 		$(if $(NODEJS_ARM_FP),--with-arm-float-abi=$(NODEJS_ARM_FP)) \
+		$(if $(NODEJS_ARM_FPU),--with-arm-fpu=$(NODEJS_ARM_FPU)) \
 		$(if $(NODEJS_MIPS_ARCH_VARIANT),--with-mips-arch-variant=$(NODEJS_MIPS_ARCH_VARIANT)) \
 		$(if $(NODEJS_MIPS_FPU_MODE),--with-mips-fpu-mode=$(NODEJS_MIPS_FPU_MODE)) \
 		$(NODEJS_CONF_OPTS) \
-- 
2.23.0

[Buildroot] [PATCH 3/3] package/nodejs: properly pass HOST_LDFLAGS when building host tools

[Thomas Petazzoni]

After building host tools, we currently run a pass of patchelf to add
the proper RPATH to these tools so that they are able to find the
libraries they depend on.

Unfortunately, the "torque" host tool is used during the build itself,
before we have a chance to run "patchelf" on it. Since it is linked
against libcrypto.so available in $(HOST_DIR)/lib, the build aborts
because the RPATH is not set.

To fix this, we make sure that $(HOST_LDFLAGS) are properly taken into
account: since they contain the -Wl,-rpath option, the host tools will
have the correct RPATH. This both fixes the build failure, and makes
the patchelf hack no longer necessary.

Fixes:

  https://bugs.busybox.net/show_bug.cgi?id=12211
  http://autobuild.buildroot.net/results/a1f5e336ddaf386ba08eb5a7a299a48e2bdfe2d9/

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 package/nodejs/nodejs.mk | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/package/nodejs/nodejs.mk b/package/nodejs/nodejs.mk
index 7fb02970cf..107e0b8d19 100644
--- a/package/nodejs/nodejs.mk
+++ b/package/nodejs/nodejs.mk
@@ -10,7 +10,7 @@ NODEJS_SITE = http://nodejs.org/dist/v$(NODEJS_VERSION)
 NODEJS_DEPENDENCIES = host-python host-nodejs c-ares \
 	libuv zlib nghttp2 \
 	$(call qstrip,$(BR2_PACKAGE_NODEJS_MODULES_ADDITIONAL_DEPS))
-HOST_NODEJS_DEPENDENCIES = host-libopenssl host-python host-zlib host-patchelf
+HOST_NODEJS_DEPENDENCIES = host-libopenssl host-python host-zlib
 NODEJS_LICENSE = MIT (core code); MIT, Apache and BSD family licenses (Bundled components)
 NODEJS_LICENSE_FILES = LICENSE
 
@@ -80,18 +80,16 @@ define HOST_NODEJS_BUILD_CMDS
 	$(HOST_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python2 \
 		$(MAKE) -C $(@D) \
 		$(HOST_CONFIGURE_OPTS) \
+		LDFLAGS.host="$(HOST_LDFLAGS)" \
 		NO_LOAD=cctest.target.mk \
 		PATH=$(@D)/bin:$(BR_PATH)
-
-	$(foreach f,$(NODEJS_HOST_TOOLS), \
-		$(HOST_DIR)/bin/patchelf --set-rpath $(HOST_DIR)/lib $(@D)/out/Release/$(f)
-	)
 endef
 
 define HOST_NODEJS_INSTALL_CMDS
 	$(HOST_MAKE_ENV) PYTHON=$(HOST_DIR)/bin/python2 \
 		$(MAKE) -C $(@D) install \
 		$(HOST_CONFIGURE_OPTS) \
+		LDFLAGS.host="$(HOST_LDFLAGS)" \
 		NO_LOAD=cctest.target.mk \
 		PATH=$(@D)/bin:$(BR_PATH)
 
-- 
2.23.0

[Buildroot] [PATCH 1/3] package/nodejs: properly pass the --with-arm-float-abi on ARM

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:

 > When commit 0064132ba032da39cefa4fffe59c31a71d1f1ddb introduced ARM64
 > support in nodejs.mk, it incorrectly kept the NODEJS_ARM_FP
 > definition. This variable is used to pass --with-arm-float-abi, which
 > in NodeJS's configure.py script is only used when --dest-cpu=arm, and
 > not when --dest-cpu=arm64.

 > So we are passing --with-arm-float-abi=<something> for ARM64, which
 > has no effect, and we are no longer passing it on ARM.

 > This commit fixes that by putting the NODEJS_ARM_FP definition back at
 > the right location.

 > Fixes:

 >    0064132ba032da39cefa4fffe59c31a71d1f1ddb

 > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 2/3] package/nodejs: use --with-arm-fpu option on ARM

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:

 > nodejs can use some FPU instructions on ARM, but it needs to know that
 > thanks to the --with-arm-fpu option. Without this, it may use the
 > wrong FPU setting, such as use VFPv3 even if only a VFPv3-D16 is
 > available. This has been reported as bug #12166, where the compiled
 > node binary had some floating point instructions using floating point
 > registers above 16 on a VFPv3-D16 system.

 > This commit makes sure we pass the appropriate --with-arm-fpu value
 > when it makes sense. Note that NodeJS only has explicit support for a
 > subset of the FPUs, for the ones that are not explicitly supported, we
 > simply pass no --with-arm-fpu value.

 > Fixes:

 >   https://bugs.busybox.net/show_bug.cgi?id=12166

 > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 3/3] package/nodejs: properly pass HOST_LDFLAGS when building host tools

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:

 > After building host tools, we currently run a pass of patchelf to add
 > the proper RPATH to these tools so that they are able to find the
 > libraries they depend on.

 > Unfortunately, the "torque" host tool is used during the build itself,
 > before we have a chance to run "patchelf" on it. Since it is linked
 > against libcrypto.so available in $(HOST_DIR)/lib, the build aborts
 > because the RPATH is not set.

 > To fix this, we make sure that $(HOST_LDFLAGS) are properly taken into
 > account: since they contain the -Wl,-rpath option, the host tools will
 > have the correct RPATH. This both fixes the build failure, and makes
 > the patchelf hack no longer necessary.

 > Fixes:

 >   https://bugs.busybox.net/show_bug.cgi?id=12211
 >   http://autobuild.buildroot.net/results/a1f5e336ddaf386ba08eb5a7a299a48e2bdfe2d9/

 > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 2/3] package/nodejs: use --with-arm-fpu option on ARM

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:

 > nodejs can use some FPU instructions on ARM, but it needs to know that
 > thanks to the --with-arm-fpu option. Without this, it may use the
 > wrong FPU setting, such as use VFPv3 even if only a VFPv3-D16 is
 > available. This has been reported as bug #12166, where the compiled
 > node binary had some floating point instructions using floating point
 > registers above 16 on a VFPv3-D16 system.

 > This commit makes sure we pass the appropriate --with-arm-fpu value
 > when it makes sense. Note that NodeJS only has explicit support for a
 > subset of the FPUs, for the ones that are not explicitly supported, we
 > simply pass no --with-arm-fpu value.

 > Fixes:

 >   https://bugs.busybox.net/show_bug.cgi?id=12166

 > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Committed to 2019.02.x and 2019.08.x, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/3] package/nodejs: properly pass the --with-arm-float-abi on ARM

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:

 > When commit 0064132ba032da39cefa4fffe59c31a71d1f1ddb introduced ARM64
 > support in nodejs.mk, it incorrectly kept the NODEJS_ARM_FP
 > definition. This variable is used to pass --with-arm-float-abi, which
 > in NodeJS's configure.py script is only used when --dest-cpu=arm, and
 > not when --dest-cpu=arm64.

 > So we are passing --with-arm-float-abi=<something> for ARM64, which
 > has no effect, and we are no longer passing it on ARM.

 > This commit fixes that by putting the NODEJS_ARM_FP definition back at
 > the right location.

 > Fixes:

 >    0064132ba032da39cefa4fffe59c31a71d1f1ddb

 > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Committed to 2019.02.x and 2019.08.x, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 3/3] package/nodejs: properly pass HOST_LDFLAGS when building host tools

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:

 > After building host tools, we currently run a pass of patchelf to add
 > the proper RPATH to these tools so that they are able to find the
 > libraries they depend on.

 > Unfortunately, the "torque" host tool is used during the build itself,
 > before we have a chance to run "patchelf" on it. Since it is linked
 > against libcrypto.so available in $(HOST_DIR)/lib, the build aborts
 > because the RPATH is not set.

 > To fix this, we make sure that $(HOST_LDFLAGS) are properly taken into
 > account: since they contain the -Wl,-rpath option, the host tools will
 > have the correct RPATH. This both fixes the build failure, and makes
 > the patchelf hack no longer necessary.

 > Fixes:

 >   https://bugs.busybox.net/show_bug.cgi?id=12211
 >   http://autobuild.buildroot.net/results/a1f5e336ddaf386ba08eb5a7a299a48e2bdfe2d9/

 > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Committed to 2019.02.x and 2019.08.x, thanks.

-- 
Bye, Peter Korsgaard