Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zlib brotli fails unit tests #25568

Closed
AdamMajer opened this issue Jan 18, 2019 · 9 comments
Closed

zlib brotli fails unit tests #25568

AdamMajer opened this issue Jan 18, 2019 · 9 comments
Labels
brotli Issues and PRs related to the brotli dependency. test Issues and PRs related to the tests. zlib Issues and PRs related to the zlib subsystem.

Comments

@AdamMajer
Copy link
Contributor

  • Version: v11.x-staging, master
  • Platform: openSUSE Tumbleweed i586
  • Subsystem:

Problem only happens with i586 platform. It's not visible on other architectures. You can see the complete buildlog,

https://build.opensuse.org/public/build/devel:languages:nodejs:staging/openSUSE_Tumbleweed/i586/nodejs11/_log

[ 1447s] not ok 2059 parallel/test-zlib-brotli-from-string
[ 1447s]   ---
[ 1447s]   duration_ms: 2.512
[ 1447s]   severity: fail
[ 1447s]   exitcode: 1
[ 1447s]   stack: |-
[ 1447s]     assert.js:86
[ 1447s]       throw new AssertionError(obj);
[ 1447s]       ^
[ 1447s]     
[ 1447s]     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
[ 1447s]     + actual - expected
[ 1447s]     
[ 1447s]     + 'G/gBQBwHdky2aHV5KK9Snf05//1pPdmNw/7232fnIm1IBK1AA2Td3fAg/P/+HQqStshSigva4jwMAg9qm5sIgqt95AXhlmWj13XEoDvKxv1fQxNkko2Xh3BK1gEN3RqsfodMzf62zqW4UbFeFW3XqQAl7oeeOghDOcva1EvOAh0u8PInzamBSVzR1MHLbI21cmGSFZCCa2j/kTqQebe5PErgwrguHRDKoWyKE0gkbnJMPMtAdS9pT02Sk81T5qxA0FbxfrXwoL+ICzl8OYaSYR3W/nmWOh6lAZmayDpbo//UcwAxRCQ8a9UkjF52eZ0tB2vo6v8SqVfNMkBmmhhr0OT9LkYF69aEjlYzj7IEKmEUQf1HBogRYiGkW0ZTomgQDoKZnREi7w=='
[ 1447s]     - 'G/gBQBwHdky2aHV5KK9Snf05//1pPdmNw/7232fnIm1IBK1AA8RsN8OB8Nb7Lpgk3UWWUlzQXZyHQeBBbXMTQXC1j7wg3LJs9LqOGHRH2bj/a2iCTLLx8hBOyTqgoVuD1e+Qqdnf1rkUNyrWq6LtOhWgxP3QUwdhKGdZm3rJWaDDBV7+pDk1MIkrmjp4ma2xVi5MsgJScA3tP1I7mXeby6MELozrwoBQDmVTnEAicZNj4lkGqntJe2qSnGyeMmcFgraK94vCg/4iLuTw5RhKhnVY++dZ6niUBmRqIutsjf5TzwF5iAg8a9UkjF52eZ0tB2vo6v8SqVfNMkBmmhxr0NT9LkYF69aEjlYzj7IEKmEUQf1HBogRYhFIt4ymRNEgHAIzOyNEsQM='
[ 1447s]         at BrotliCompress.zlib.brotliCompress.common.mustCall (/home/abuild/rpmbuild/BUILD/node-git.8e84ccb502/test/parallel/test-zlib-brotli-from-string.js:28:10)
[ 1447s]         at BrotliCompress.cb (/home/abuild/rpmbuild/BUILD/node-git.8e84ccb502/test/common/index.js:389:15)
[ 1447s]         at BrotliCompress.zlibBufferOnEnd (zlib.js:131:10)
[ 1447s]         at BrotliCompress.emit (events.js:193:15)
[ 1447s]         at endReadableNT (_stream_readable.js:1129:12)
[ 1447s]         at processTicksAndRejections (internal/process/next_tick.js:76:17)
[ 1447s]   ...
@addaleax addaleax added zlib Issues and PRs related to the zlib subsystem. brotli Issues and PRs related to the brotli dependency. labels Jan 18, 2019
@addaleax
Copy link
Member

@AdamMajer Can you check whether other brotli compressors yield the same result on this machine (e.g. the Python one in https://github.com/google/brotli aka pip install brotli)?

@bnoordhuis
Copy link
Member

bnoordhuis commented Jan 21, 2019

i586 platform

Is that really i586? No SSE2, etc.? That's not a supported (let alone tested) configuration. I'm surprised it even builds.

@bnoordhuis
Copy link
Member

As there's been no follow-up, and because it looks like an unsupported configuration, I'm going to close out the issue. Let me know if I should reopen.

@bnoordhuis bnoordhuis added the wontfix Issues that will not be fixed. label Jan 25, 2019
@AdamMajer
Copy link
Contributor Author

Don't close it. This is not "unsupported configuration." It's running on a modern CPU and passes all the unit tests except for this one. I just didn't have time yet to reproduce the problem with the pip installed brotli. I should be able to do this shortly.

For example, this test fails in a 32-bit chroot (so no VM) on a Xeon E5-1620.

@addaleax addaleax added test Issues and PRs related to the tests. and removed wontfix Issues that will not be fixed. labels Jan 25, 2019
@addaleax
Copy link
Member

Fwiw, we could also just skip this particular test or allow two different outputs (as long as they decompress to the same result).

My request for checking another implementation is that it’s pretty uncommon for compressions algorithms to yield machine-dependent results?

@addaleax addaleax reopened this Jan 25, 2019
@AdamMajer
Copy link
Contributor Author

Ok, I've looked at the actual test now 😊 I think it was a little naive to assume that each machine will result in same output. Consider padding. But we should always get same output.

I'll make a quick patch that I think will satisfy everyone here.

@AdamMajer
Copy link
Contributor Author

Basically, on 32-bit x86, the compressed string is 1 byte shorter than the provided version. Of course, the 64-bit compressed string still decompresses correctly. The slightly modified test just verifies both.

I wonder if I should include the 32-bit compressed buffer as an alternative to test decompression on 64-bit machines? 😄

AdamMajer added a commit to AdamMajer/node that referenced this issue Jan 25, 2019
On different architectures, it's possible for the compression algorithm
to produce slightly different outputs. So, don't assume we will
always get the same compressed output on all architectures. Instead,
verify that the decompressing pre-compressed string functions
correctly.

Fixes: nodejs#25568
@bnoordhuis
Copy link
Member

This is not "unsupported configuration."

Can you elaborate on what 'i586' means? gcc with -mcpu=i586?

I thought that was the difference between opensuse's i586 and i686 flavors and the former is not a configuration we support (or even expect to work.)

@AdamMajer
Copy link
Contributor Author

If you look in the logs, v8 is compiled with these settings,

g++ -o /home/abuild/rpmbuild/BUILD/node-git.8e84ccb502/out/Release/obj.target/v8_base/deps/v8/src/compiler/graph-reducer.o ../deps/v8/src/compiler/graph-reducer.cc '-DNODE_OPENSSL_CERT_STORE' '-DV8_GYP_BUILD' '-DV8_TYPED_ARRAY_MAX_SIZE_IN_HEAP=0' '-DV8_TARGET_ARCH_IA32' '-DV8_EMBEDDER_STRING="-node.16"' '-DENABLE_DISASSEMBLER' '-DV8_PROMISE_INTERNAL_FIELD_COUNT=1' '-DENABLE_GDB_JIT_INTERFACE' '-DV8_INTL_SUPPORT' '-DV8_CONCURRENT_MARKING' '-DDISABLE_UNTRUSTED_CODE_MITIGATIONS' '-DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_STATIC' -I../deps/v8 -I../. -I/home/abuild/rpmbuild/BUILD/node-git.8e84ccb502/out/Release/obj/gen -I../deps/v8/include -pthread -Wall -Wextra -Wno-unused-parameter -m32 -msse2 -mfpmath=sse -mmmx -fno-strict-aliasing -m32 -O3 -fno-omit-frame-pointer -fdata-sections -ffunction-sections -O3 -fno-rtti -fno-exceptions -std=gnu++1y -MMD -MF /home/abuild/rpmbuild/BUILD/node-git.8e84ccb502/out/Release/.deps//home/abuild/rpmbuild/BUILD/node-git.8e84ccb502/out/Release/obj.target/v8_base/deps/v8/src/compiler/graph-reducer.o.d.raw -fomit-frame-pointer -O2 -Wall -D_FORTIFY_SOURCE=2 -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection -c

So, -m32

 gcc -dumpmachine
 i586-suse-linux

openSUSE doesn't really have i586 and i686 flavours anymore. There is only i586 which is 32-bit x86 and it's only available on Tumbleweed now. 32-bit not supported in SLE 15 anymore, for example, but it's good for testing corner cases.

Anyway, the point is that all unit tests pass except for this one. It seems to related to 1 byte difference in compressed output of this compression algorithm.

addaleax pushed a commit that referenced this issue Jan 28, 2019
On different architectures, it's possible for the compression algorithm
to produce slightly different outputs. So, don't assume we will
always get the same compressed output on all architectures. Instead,
verify that the decompressing pre-compressed string functions
correctly.

Fixes: #25568

PR-URL: #25697
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this issue May 13, 2019
On different architectures, it's possible for the compression algorithm
to produce slightly different outputs. So, don't assume we will
always get the same compressed output on all architectures. Instead,
verify that the decompressing pre-compressed string functions
correctly.

Fixes: nodejs#25568

PR-URL: nodejs#25697
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue May 16, 2019
On different architectures, it's possible for the compression algorithm
to produce slightly different outputs. So, don't assume we will
always get the same compressed output on all architectures. Instead,
verify that the decompressing pre-compressed string functions
correctly.

Fixes: #25568

Backport-PR-URL: #27681
PR-URL: #25697
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue May 16, 2019
On different architectures, it's possible for the compression algorithm
to produce slightly different outputs. So, don't assume we will
always get the same compressed output on all architectures. Instead,
verify that the decompressing pre-compressed string functions
correctly.

Fixes: #25568

Backport-PR-URL: #27681
PR-URL: #25697
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
brotli Issues and PRs related to the brotli dependency. test Issues and PRs related to the tests. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants