zlib Illegal Instruction on CPU without SSSE3 support #32553

Strum355 · 2020-03-29T20:01:30Z

Node.js Version: 13 (works on 12) from nodesource ubuntu repos
OS: Ubuntu 16.04
Scope (install, code, runtime, meta, other?): npm install

Running npm i for the following package.json:

  "devDependencies": {
    "@types/react": "^16.9.26",
    "@types/react-dom": "^16.9.5",
    "@types/styled-components": "^5.0.1",
    "parcel": "^1.12.4",
    "typescript": "^3.8.3"
  },
  "dependencies": {
    "react": "^16.13.1",
    "react-dom": "^16.13.1",
    "styled-components": "^5.0.1"
  }

gives Illegal Instruction. In gdb, the backtrace is

(gdb) bt
#0  0x000000000132a53f in fill_window ()
nodejs/help#1  0x000000000132bdf0 in deflate_slow ()
nodejs/help#2  0x000000000132d06f in deflate ()
nodejs/help#3  0x0000000000ac098c in node::(anonymous namespace)::ZlibContext::DoThreadPoolWork() ()
nodejs/help#4  0x000000000134170e in worker (arg=0x0) at ../deps/uv/src/threadpool.c:122
nodejs/help#5  0x00007ffff711c6ba in start_thread (arg=0x7fffde7fc700) at pthread_create.c:333
nodejs/help#6  0x00007ffff6e5241d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109

With specifically >│0x132a53f <fill_window+1007> pshufb 0xebc708(%rip),%xmm0 # 0x21e6c50 being the issue, where pshufb is an SSSE3 instruction not supported by the CPU

The text was updated successfully, but these errors were encountered:

addaleax · 2020-03-29T20:03:19Z

@mscdex This was almost certainly introduced in #31201.

Adenilson · 2020-03-30T04:08:29Z

That is a bit odd, since we guard the optimization on a flag that will be set only if the CPU supports the required instructions (https://cs.chromium.org/chromium/src/third_party/zlib/deflate.c?l=1525).

It seems that SSE3 was introduced in early 2004 (https://en.wikipedia.org/wiki/SSE3), or 16 years ago.

Unfortunately I don't have any hardware available without SSE3 to debug.

Could you provide further details about the environment? (i.e. CPU model, etc)

addaleax · 2020-03-30T08:10:51Z

@Adenilson It looks like it’s fill_window_c, the plain C version of fill_window, and not fill_window_sse that’s leading to the pshufb being generated. It’s a bit ironic, but I think it makes sense given that the file is compiled with -mssse3 – that leaves the compiler the option to use SSSE3, whether it’s used through intrinsics or not.

I would have assumed that instead of passing compiler flags, the optimized zlib code uses __attribute__((target(...))) to specify that a function may contain code for certain CPU features. Is changing that an option for you?

(Also, minor side note just so we’re on the same page: SSSE3 !== SSE3.)

Strum355 · 2020-03-30T11:47:40Z

CPU seems to be a Xeon 7140M http://www.cpu-world.com/CPUs/Xeon/Intel-Xeon%20MP%207140M%20-%20LF80550KG096007%20(BX805507140M).html

bnoordhuis · 2020-03-30T11:59:22Z

That makes sense, the 7100 series were probably the last Xeons without SSSE3 and SSE4.2 support.

I'm not sure what to do here. This kind of breakage in a release line isn't great but on the other hand, we're talking 14 year old systems here.

I'm inclined to mark the PR as "don't backport to the LTS branches" (edit: or remove the -mssse3 -msse4.2 -mpclmul flags when backporting) and leave it at that. Thoughts?

mmarchini · 2020-03-30T16:45:19Z

Agreed @bnoordhuis, even though we don't have any guarantees on which CPUs are supported within a major release (at least not according to https://github.com/nodejs/node/blob/master/BUILDING.md), we should prevent this breaking change at least on LTS.

or remove the -mssse3 -msse4.2 -mpclmul flags when backporting

We should test this firsts to make sure it works. Any tips on how @Strum355 can test it?

Strum355 · 2020-03-30T16:53:38Z

A docker image would probably be a convenient way for me to test this if that suits :)

Adenilson · 2020-03-30T17:08:22Z

The portable C code should work fine without SSSE3 flags.

Chromium has SSE2 as minimal requirements (https://support.google.com/chrome/a/answer/7100626?hl=en), so crashing on a Xeon without SSSE3 is not acceptable.

@addaleax yes, we are moving towards using attribute(target()) (e.g. https://cs.chromium.org/chromium/src/third_party/zlib/contrib/optimizations/insert_string.h?l=23) which I believe is better than using explicit compiler switches.

IIRC, node has its own build system so it should be simple to fix (i.e. don't pass the compiler flag to deflate.c).

If that requires further changes (i.e. moving code around), we can coordinate the work to make sure that upstream (Chromium zlib) adopts the same fixes.

I will have breakfast and start looking on which changes would be required on Chromium side.

Adenilson · 2020-03-30T17:12:00Z

Would be ok to share a test binary here so we validate if it is crashing (or not) on a machine without SSSE3?

Or maybe someone could provide me remote access to such machine?

addaleax · 2020-03-30T17:16:21Z

Fwiw, here’s the somewhat straightforward diff I put together for testing this:

diff in the fold

diff --git a/deps/zlib/adler32_simd.c b/deps/zlib/adler32_simd.c
index 1354915cc099..0170c8df870d 100644
--- a/deps/zlib/adler32_simd.c
+++ b/deps/zlib/adler32_simd.c
@@ -53,6 +53,7 @@
 
 #include <tmmintrin.h>
 
+__attribute__((target("ssse3")))
 uint32_t ZLIB_INTERNAL adler32_simd_(  /* SSSE3 */
     uint32_t adler,
     const unsigned char *buf,
diff --git a/deps/zlib/crc32_simd.c b/deps/zlib/crc32_simd.c
index c8e5592f38ef..a77d629f2288 100644
--- a/deps/zlib/crc32_simd.c
+++ b/deps/zlib/crc32_simd.c
@@ -21,6 +21,7 @@
 #include <smmintrin.h>
 #include <wmmintrin.h>
 
+__attribute__((target("sse4.2,pclmul")))
 uint32_t ZLIB_INTERNAL crc32_sse42_simd_(  /* SSE4.2+PCLMUL */
     const unsigned char *buf,
     z_size_t len,
diff --git a/deps/zlib/crc_folding.c b/deps/zlib/crc_folding.c
index 48d77744aaf4..24621d6c5449 100644
--- a/deps/zlib/crc_folding.c
+++ b/deps/zlib/crc_folding.c
@@ -39,6 +39,7 @@
         _mm_storeu_si128((__m128i *)s->crc0 + 4, xmm_crc_part);\
     } while (0);
 
+__attribute__((target("sse4.2,pclmul")))
 ZLIB_INTERNAL void crc_fold_init(deflate_state *const s)
 {
     CRC_LOAD(s)
@@ -53,6 +54,7 @@ ZLIB_INTERNAL void crc_fold_init(deflate_state *const s)
     s->strm->adler = 0;
 }
 
+__attribute__((target("sse4.2,pclmul")))
 local void fold_1(deflate_state *const s,
         __m128i *xmm_crc0, __m128i *xmm_crc1,
         __m128i *xmm_crc2, __m128i *xmm_crc3)
@@ -79,6 +81,7 @@ local void fold_1(deflate_state *const s,
     *xmm_crc3 = _mm_castps_si128(ps_res);
 }
 
+__attribute__((target("sse4.2,pclmul")))
 local void fold_2(deflate_state *const s,
         __m128i *xmm_crc0, __m128i *xmm_crc1,
         __m128i *xmm_crc2, __m128i *xmm_crc3)
@@ -113,6 +116,7 @@ local void fold_2(deflate_state *const s,
     *xmm_crc3 = _mm_castps_si128(ps_res31);
 }
 
+__attribute__((target("sse4.2,pclmul")))
 local void fold_3(deflate_state *const s,
         __m128i *xmm_crc0, __m128i *xmm_crc1,
         __m128i *xmm_crc2, __m128i *xmm_crc3)
@@ -153,6 +157,7 @@ local void fold_3(deflate_state *const s,
     *xmm_crc3 = _mm_castps_si128(ps_res32);
 }
 
+__attribute__((target("sse4.2,pclmul")))
 local void fold_4(deflate_state *const s,
         __m128i *xmm_crc0, __m128i *xmm_crc1,
         __m128i *xmm_crc2, __m128i *xmm_crc3)
@@ -219,6 +224,7 @@ local const unsigned zalign(32) pshufb_shf_table[60] = {
 	0x0201008f,0x06050403,0x0a090807,0x0e0d0c0b  /* shl  1 (16 -15)/shr15*/
 };
 
+__attribute__((target("sse4.2,pclmul")))
 local void partial_fold(deflate_state *const s, const size_t len,
         __m128i *xmm_crc0, __m128i *xmm_crc1,
         __m128i *xmm_crc2, __m128i *xmm_crc3,
@@ -269,6 +275,7 @@ local void partial_fold(deflate_state *const s, const size_t len,
     *xmm_crc3 = _mm_castps_si128(ps_res);
 }
 
+__attribute__((target("sse4.2,pclmul")))
 ZLIB_INTERNAL void crc_fold_copy(deflate_state *const s,
         unsigned char *dst, const unsigned char *src, long len)
 {
@@ -425,6 +432,7 @@ local const unsigned zalign(16) crc_mask2[4] = {
     0x00000000, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF
 };
 
+__attribute__((target("sse4.2,pclmul")))
 unsigned ZLIB_INTERNAL crc_fold_512to32(deflate_state *const s)
 {
     const __m128i xmm_mask  = _mm_load_si128((__m128i *)crc_mask);
diff --git a/deps/zlib/zlib.gyp b/deps/zlib/zlib.gyp
index c73b9adba902..6f5a8ce3464b 100644
--- a/deps/zlib/zlib.gyp
+++ b/deps/zlib/zlib.gyp
@@ -91,20 +91,6 @@
                 'x86.c',
               ],
               'conditions': [
-                ['OS!="win" or llvm_version!="0.0"', {
-                  'cflags': [
-                    '-mssse3',
-                    '-msse4.2',
-                    '-mpclmul',
-                  ],
-                  'xcode_settings': {
-                    'OTHER_CFLAGS': [
-                      '-mssse3',
-                      '-msse4.2',
-                      '-mpclmul',
-                    ],
-                  },
-                }],
                 ['target_arch=="x64"', {
                   'defines': [ 'INFLATE_CHUNK_READ_64LE' ],
                 }],

I assume that would need to be polished up a bit, but I can upload the node binary that I’m getting from it, maybe that helps: https://addaleax.net/node-32553.gz

Strum355 · 2020-03-30T17:32:11Z

@addaleax any chance that could be built with glibc 2.23? Ubuntu 16.04 only provides 2.23 in the repos, upgrading to 2.31 fails because locales requires a higher libc-bin version, which requires a higher libc6 version which requires a higher locales version...

addaleax · 2020-03-30T17:36:28Z

@Strum355 I … probably don’t want to downgrade glibc on my laptop. 😅

It probably doesn’t make sense to put a test build together from this, but I can see if I could build that patch on one of the Ubuntu 16.04 CI machines.

Adenilson · 2020-03-30T17:37:27Z

Upon a closer look, it doesn't seem to be an issue upstream.

We have separated modules with their own flags, for optimized modules (https://cs.chromium.org/chromium/src/third_party/zlib/BUILD.gn?l=201) and vanilla ones (https://cs.chromium.org/chromium/src/third_party/zlib/BUILD.gn?l=246).

My GYP-fu is a bit rusty, but my guess is that you shouldn't use this compiler switches (https://github.com/nodejs/node/blob/master/deps/zlib/zlib.gyp#L94) on the portable code.

While we work upstream towards removing compiler switches, I guess the quick fix (without diverging from upstream) for Node would be to follow the same build strategy as Chromium (i.e. use source modules with the same compiler switches).

Does that sound reasonable?

addaleax · 2020-03-30T17:43:35Z

Yeah, if somebody from @nodejs/build-files knows how to specify build flags only for specific files, I guess that works too.

Strum355 · 2020-03-30T17:45:51Z

@addaleax had the better idea of just running an ubuntu:18.04 container with glib 2.27, the binary you provided didnt SIGILL 👍

richardlau · 2020-03-30T17:56:58Z

Yeah, if somebody from @nodejs/build-files knows how to specify build flags only for specific files, I guess that works too.

I think we'd have to split the files into a new static library target and set the flags for that.

cc @nodejs/gyp

bnoordhuis · 2020-03-30T18:35:19Z

That's right. cflags are per-target.

Adenilson · 2020-03-31T00:23:36Z

Yeah, if somebody from @nodejs/build-files knows how to specify build flags only for specific files, I guess that works too.

Excelent! Please feel free to reach out if you guys run into any unexpected issue.
:-)

Fix the compile flags so that zlib can run on CPUs that do not have SSSE3/SSE4.2/etc. Do not compile zlib with flags that indicate that those features are available, and instead enable them selectively for functions that use them. There are probably better way to do this, e.g. through gyp file modifications as suggested in the issue. However, this patch should do just fine until that happens. Fixes: nodejs#32553

addaleax · 2020-04-03T02:30:19Z

Opened #32627 which is mostly the same as my patch above, since that has been confirmed to work for two people now. If somebody comes up with a gyp-only solution, great, but I’d like to get this resolved in time for v14.0.0 one way or another.

Fix the compile flags so that zlib can run on CPUs that do not have SSSE3/SSE4.2/etc. Do not compile zlib with flags that indicate that those features are available, and instead enable them selectively for functions that use them. There are probably better way to do this, e.g. through gyp file modifications as suggested in the issue. However, this patch should do just fine until that happens. Fixes: #32553 PR-URL: #32627 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fix the compile flags so that zlib can run on CPUs that do not have SSSE3/SSE4.2/etc. Do not compile zlib with flags that indicate that those features are available, and instead enable them selectively for functions that use them. There are probably better way to do this, e.g. through gyp file modifications as suggested in the issue. However, this patch should do just fine until that happens. Fixes: nodejs#32553 PR-URL: nodejs#32627 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fix the compile flags so that zlib can run on CPUs that do not have SSSE3/SSE4.2/etc. Do not compile zlib with flags that indicate that those features are available, and instead enable them selectively for functions that use them. There are probably better way to do this, e.g. through gyp file modifications as suggested in the issue. However, this patch should do just fine until that happens. Fixes: #32553 PR-URL: #32627 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fix the compile flags so that zlib can run on CPUs that do not have SSSE3/SSE4.2/etc. Do not compile zlib with flags that indicate that those features are available, and instead enable them selectively for functions that use them. There are probably better way to do this, e.g. through gyp file modifications as suggested in the issue. However, this patch should do just fine until that happens. Fixes: nodejs#32553 PR-URL: nodejs#32627 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fix the compile flags so that zlib can run on CPUs that do not have SSSE3/SSE4.2/etc. Do not compile zlib with flags that indicate that those features are available, and instead enable them selectively for functions that use them. There are probably better way to do this, e.g. through gyp file modifications as suggested in the issue. However, this patch should do just fine until that happens. Fixes: #32553 PR-URL: #32627 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> PR-URL: #45387 Reviewed-By: Rafael Gonzaga <[email protected]>

addaleax transferred this issue from nodejs/help Mar 29, 2020

addaleax added the zlib Issues and PRs related to the zlib subsystem. label Mar 29, 2020

addaleax mentioned this issue Mar 29, 2020

v12.16.2 proposal #32313

Merged

Strum355 mentioned this issue Mar 29, 2020

Port Chromium optimizations to zlib madler/zlib#346

Open

addaleax mentioned this issue Apr 3, 2020

Getting 'illegal instruction' error on old AMD Phenom II #32625

Closed

addaleax added this to the 14.0.0 milestone Apr 3, 2020

addaleax mentioned this issue Apr 3, 2020

deps: fix zlib compilation for CPUs without SIMD features #32627

Closed

2 tasks

addaleax closed this as completed in dccdc51 Apr 9, 2020

richardlau mentioned this issue Apr 24, 2020

build: rework gyp files for zlib #33044

Closed

3 tasks

Cubelrti mentioned this issue Mar 1, 2021

zlib Illegal Instruction on CPU without SSSE3/SSE4.2 support nwjs/nw.js#7688

Open

nodejs-github-bot mentioned this issue Nov 16, 2022

deps: update zlib to upstream 8bbd6c31 #45387

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zlib Illegal Instruction on CPU without SSSE3 support #32553

zlib Illegal Instruction on CPU without SSSE3 support #32553

Strum355 commented Mar 29, 2020 •

edited

Loading

addaleax commented Mar 29, 2020

Adenilson commented Mar 30, 2020

addaleax commented Mar 30, 2020

Strum355 commented Mar 30, 2020

bnoordhuis commented Mar 30, 2020 •

edited

Loading

mmarchini commented Mar 30, 2020

Strum355 commented Mar 30, 2020

Adenilson commented Mar 30, 2020

Adenilson commented Mar 30, 2020

addaleax commented Mar 30, 2020

Strum355 commented Mar 30, 2020

addaleax commented Mar 30, 2020

Adenilson commented Mar 30, 2020

addaleax commented Mar 30, 2020

Strum355 commented Mar 30, 2020

richardlau commented Mar 30, 2020

bnoordhuis commented Mar 30, 2020

Adenilson commented Mar 31, 2020

addaleax commented Apr 3, 2020

zlib Illegal Instruction on CPU without SSSE3 support #32553

zlib Illegal Instruction on CPU without SSSE3 support #32553

Comments

Strum355 commented Mar 29, 2020 • edited Loading

addaleax commented Mar 29, 2020

Adenilson commented Mar 30, 2020

addaleax commented Mar 30, 2020

Strum355 commented Mar 30, 2020

bnoordhuis commented Mar 30, 2020 • edited Loading

mmarchini commented Mar 30, 2020

Strum355 commented Mar 30, 2020

Adenilson commented Mar 30, 2020

Adenilson commented Mar 30, 2020

addaleax commented Mar 30, 2020

Strum355 commented Mar 30, 2020

addaleax commented Mar 30, 2020

Adenilson commented Mar 30, 2020

addaleax commented Mar 30, 2020

Strum355 commented Mar 30, 2020

richardlau commented Mar 30, 2020

bnoordhuis commented Mar 30, 2020

Adenilson commented Mar 31, 2020

addaleax commented Apr 3, 2020

Strum355 commented Mar 29, 2020 •

edited

Loading

bnoordhuis commented Mar 30, 2020 •

edited

Loading