Bug ID: JDK-8204479 Bitwise AND on byte value sometimes produces wrong result

JDK-8204479 : Bitwise AND on byte value sometimes produces wrong result

Type: Bug
Component: hotspot
Sub-Component: compiler
Affected Version: 11

Priority: P1
Status: Closed
Resolution: Fixed
OS: os_x
CPU: x86_64

Submitted: 2018-06-06
Updated: 2018-08-16
Resolved: 2018-06-08

Versions (Unresolved/Resolved/Fixed)

The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.

JDK 11
11 b18Fixed

Related Reports

Relates :

JDK-8203628 - Optimize (masked) byte memory comparisons on x86

Description

We are seeing a number of test failures when running Nashorn test262 tests with JDK 11. The failures only happen when running a large chunk of the test suite, not when running just the failing tests alone.

The failures are caused by the Nashorn encodeURI function returning the wrong result. Specifically, the following condition at line 274 in src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/runtime/URIUtils.java sometimes evaluates to true when it should evaluate to false:

            if ((b[i] & 0xff) < 0x10) {

b being of type byte[] and b[i] being a negative byte value.
If I change above condition to the one below the failures disappear.

            if ((b[i] & 0xff) < 0x10 && b[i] >= 0) {

Steps to reproduce: 

cd make/nashorn
ant externals
ant test262parallel -Dtest262-test-sys-prop.test.js.roots=test/nashorn/script/external/test262/test/suite/ch15/15.1/15.1.3/

The last step runs a large enough subset to reproduce the problem. It should terminate without failures, but currently produces 11 failures:

     [java] FAILED TESTS
     [java] Test(compile, run): test/nashorn/script/external/test262/test/suite/ch15/15.1/15.1.3/15.1.3.3/S15.1.3.3_A2.3_T1.js
     [java] test/nashorn/script/external/test262/test/harness/sta.js:286:4 Error: #3629-D7FF 
     [java] Test(compile, run): test/nashorn/script/external/test262/test/suite/ch15/15.1/15.1.3/15.1.3.3/S15.1.3.3_A2.4_T1.js
     [java] test/nashorn/script/external/test262/test/harness/sta.js:286:4 Error: #D800-DBFF 
     [java] Test(compile, run): test/nashorn/script/external/test262/test/suite/ch15/15.1/15.1.3/15.1.3.3/S15.1.3.3_A2.4_T2.js
     [java] test/nashorn/script/external/test262/test/harness/sta.js:286:4 Error: #DC00-DFFF 
     [java] Test(compile, run): test/nashorn/script/external/test262/test/suite/ch15/15.1/15.1.3/15.1.3.3/S15.1.3.3_A2.5_T1.js
     [java] test/nashorn/script/external/test262/test/harness/sta.js:286:4 Error: #E000-FFFF 
     [java] Test(compile, run): test/nashorn/script/external/test262/test/suite/ch15/15.1/15.1.3/15.1.3.3/S15.1.3.3_A4_T2.js
     [java] test/nashorn/script/external/test262/test/harness/sta.js:286:4 Error: #1: http://ru.wikipedia.org/wiki/������������
     [java] Test(compile, run): test/nashorn/script/external/test262/test/suite/ch15/15.1/15.1.3/15.1.3.4/S15.1.3.4_A2.2_T1.js
     [java] test/nashorn/script/external/test262/test/harness/sta.js:286:4 Error: #0080-07FF 
     [java] Test(compile, run): test/nashorn/script/external/test262/test/suite/ch15/15.1/15.1.3/15.1.3.4/S15.1.3.4_A2.3_T1.js
     [java] test/nashorn/script/external/test262/test/harness/sta.js:286:4 Error: #0800-D7FF 
     [java] Test(compile, run): test/nashorn/script/external/test262/test/suite/ch15/15.1/15.1.3/15.1.3.4/S15.1.3.4_A2.4_T1.js
     [java] test/nashorn/script/external/test262/test/harness/sta.js:286:4 Error: #D800-DBFF 
     [java] Test(compile, run): test/nashorn/script/external/test262/test/suite/ch15/15.1/15.1.3/15.1.3.4/S15.1.3.4_A2.4_T2.js
     [java] test/nashorn/script/external/test262/test/harness/sta.js:286:4 Error: #DC00-DFFF 
     [java] Test(compile, run): test/nashorn/script/external/test262/test/suite/ch15/15.1/15.1.3/15.1.3.4/S15.1.3.4_A2.5_T1.js
     [java] test/nashorn/script/external/test262/test/harness/sta.js:286:4 Error: #E000-FFFF 
     [java] Test(compile, run): test/nashorn/script/external/test262/test/suite/ch15/15.1/15.1.3/15.1.3.4/S15.1.3.4_A4_T2.js
     [java] test/nashorn/script/external/test262/test/harness/sta.js:286:4 Error: #1: http://ru.wikipedia.org/wiki/������������
     [java] Tests run: 160/160 tests, passed: 149 (93.13%), failed: 11. Time elapsed: 1min 35s.

Comments

ILW = wrong code generation; regression; workaround in test as mentioned in Description = HHM = P1
08-06-2018
http://cr.openjdk.java.net/~kvn/8204479/webrev.00/
08-06-2018
So the problem is that cmpb/jl is signed compare which compares only one byte (which could be signed). Where the original Java code does byte load into Int (32-bit) value and then masking it with 0xFF. movzbl instruction does exactly that - loads only 8 bits without sign extension. After that it can do singed 32-bit compare. I think we should remove compUB_mem_imm() and testUB_mem_imm() mach instructions added by JDK-8203628.
08-06-2018
Looking on generated code in pre-loop: 026 B4: # B79 B5 <- B3 Freq: 0.499999 026 cmpb [RSI + #16 (8-bit)], #16 02a jl B79 P=0.000001 C=-1.000000 In main- and post- loop: 073 movzbl R10, [RSI + #16 + RBP] # ubyte 079 cmpl R10, #16 07d jl B47 P=0.000001 C=-1.000000 Based on this JDK-8203628 optimization was applied only in pre-loop for b[0].
08-06-2018
Thanks! It is extremely puzzling why only value -1 fails. Maybe it is a symptom of broader problem: it seems to always fail at index 0, but not other indexes that have value -1, which might indicate troubles with loop optimizations that throw this matcher under the bus? Test here: http://cr.openjdk.java.net/~shade/8204479/webrev.02/ Failed with: -1 at 0 <--- failed at index 0 Passed with: -2 at 1 Passed with: -3 at 2 ... Passed with: -125 at 124 Passed with: -126 at 125 Passed with: -1 at 126 <--- O_o Passed with: -2 at 127 ...
07-06-2018
Thank you for test. I will look what is happening.
07-06-2018
Yes, we have optimization which convert signed byte load with unsigned load when a mask masked only 8 bits: http://hg.openjdk.java.net/jdk/jdk/file/ccb2c0d5da93/src/hotspot/share/opto/mulnode.cpp#l491 In such case compare stays signed. But the optimization keeps AndI node which is missing in match rule in failing case.
07-06-2018
Ah, maybe not, and this just makes the offending matcher mismatch. rRegFlagsU should come with Cmp*U, but we have plain CmpI. Managed to produce the regression test, and it fails with matcher "Set cr (CmpI (LoadUB mem) imm)". It seems to fail when b[i] is -1, of all negative values. http://cr.openjdk.java.net/~shade/8204479/webrev.01/
07-06-2018
Reversal of JDK-8203628 helps to pass the tests for me. Now I vaguely remember we are parsing (b[i] & 0xFF) into (LoadUB mem). Looking closely at the original patch, I think it fails to recognize the difference between signed/unsigned flags. This patch helps to pass the tests too: http://cr.openjdk.java.net/~shade/8204479/candidate-1.patch Vladimir, does that make sense to you?
07-06-2018
Hm, but as far as I can see without the development machine handy, this pattern looks like (Set cr (CmpI (AndI (LoadB arr) 0xFF) 0x10)), which should not match to any new thing in JDK-8203628. Hannes, can you revert that patch and see if it passes these tests?
07-06-2018
This does indeed look suspicious. On vacation, can take a look on Monday.
07-06-2018
[~shade] Aleksey, please look.
07-06-2018
Could be related to recent change JDK-8203628.
07-06-2018