[jdk16] RFR: 8260927: StringBuilder::insert is incorrect without Compact Strings
Discovered it with ARM32 tier1 tests, which runs with -CompactStrings by default. But the bug is actually generic: $ CONF=linux-x86_64-server-fastdebug make run-test TEST=java/lang/StringBuilder/Insert.java TEST_VM_OPTS="-XX:-CompactStrings" test Insert.insertOffset(): failure java.lang.AssertionError: expected [??abc] but found [efabc] at org.testng.Assert.fail(Assert.java:94) at org.testng.Assert.failNotEquals(Assert.java:496) at org.testng.Assert.assertEquals(Assert.java:125) at org.testng.Assert.assertEquals(Assert.java:178) at org.testng.Assert.assertEquals(Assert.java:188) at Insert.insertOffset(Insert.java:45) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) I believe this is a regression from [JDK-8254082](https://bugs.openjdk.java.net/browse/JDK-8254082). Additional testing: - [x] Linux x86_64 `tier1` default, passes - [x] Linux x86_64 `tier1`, `-XX:-CompactStrings`, passes modulo two testbugs ([JDK-8260933](https://bugs.openjdk.java.net/browse/JDK-8260933), [JDK-8260934](https://bugs.openjdk.java.net/browse/JDK-8260934)) - [x] Linux ARM32, affected test now passes ------------- Commit messages: - 8260927: StringBuilder::insert is incorrect without Compact Strings Changes: https://git.openjdk.java.net/jdk16/pull/143/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk16&pr=143&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260927 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk16/pull/143.diff Fetch: git fetch https://git.openjdk.java.net/jdk16 pull/143/head:pull/143 PR: https://git.openjdk.java.net/jdk16/pull/143
On Tue, 2 Feb 2021 13:12:53 GMT, Aleksey Shipilev <shade@openjdk.org> wrote:
Discovered it with ARM32 tier1 tests, which runs with -CompactStrings by default. But the bug is actually generic:
$ CONF=linux-x86_64-server-fastdebug make run-test TEST=java/lang/StringBuilder/Insert.java TEST_VM_OPTS="-XX:-CompactStrings"
test Insert.insertOffset(): failure java.lang.AssertionError: expected [??abc] but found [efabc] at org.testng.Assert.fail(Assert.java:94) at org.testng.Assert.failNotEquals(Assert.java:496) at org.testng.Assert.assertEquals(Assert.java:125) at org.testng.Assert.assertEquals(Assert.java:178) at org.testng.Assert.assertEquals(Assert.java:188) at Insert.insertOffset(Insert.java:45) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
I believe this is a regression from [JDK-8254082](https://bugs.openjdk.java.net/browse/JDK-8254082).
void getBytes(byte[] dst, int srcPos, int dstBegin, byte coder, int length) { if (coder() == coder) { System.arraycopy(value, srcPos, dst, dstBegin << coder, length << coder()); } else { // this.coder == LATIN && coder == UTF16 StringLatin1.inflate(value, srcPos, dst, dstBegin, length); } }
When coder is `UTF16` (which it guaranteed to be without `CompactStrings`), then `srcPos` in `byte[]` array has to be adjusted by `coder` as well.
Additional testing: - [x] Linux x86_64 `tier1` default, passes - [x] Linux x86_64 `tier1`, `-XX:-CompactStrings`, passes modulo two testbugs ([JDK-8260933](https://bugs.openjdk.java.net/browse/JDK-8260933), [JDK-8260934](https://bugs.openjdk.java.net/browse/JDK-8260934)) - [x] Linux ARM32, affected test now passes
Looks good, thanks for fixing this. Should we add an explicit run with -CompactStrings to the Insert.java test? That'd have caught this issue sooner - perhaps already by the GA testing. ------------- Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk16/pull/143
On Tue, 2 Feb 2021 13:23:16 GMT, Claes Redestad <redestad@openjdk.org> wrote:
Should we add an explicit run with -CompactStrings to the Insert.java test? That'd have caught this issue sooner - perhaps already by the GA testing.
Yes, I thought about the same. Added and re-checked that it catches the failure. (I also added `-XX:-CompactStrings` jobs to my CI workflows). ------------- PR: https://git.openjdk.java.net/jdk16/pull/143
Discovered it with ARM32 tier1 tests, which runs with -CompactStrings by default. But the bug is actually generic:
$ CONF=linux-x86_64-server-fastdebug make run-test TEST=java/lang/StringBuilder/Insert.java TEST_VM_OPTS="-XX:-CompactStrings"
test Insert.insertOffset(): failure java.lang.AssertionError: expected [??abc] but found [efabc] at org.testng.Assert.fail(Assert.java:94) at org.testng.Assert.failNotEquals(Assert.java:496) at org.testng.Assert.assertEquals(Assert.java:125) at org.testng.Assert.assertEquals(Assert.java:178) at org.testng.Assert.assertEquals(Assert.java:188) at Insert.insertOffset(Insert.java:45) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
I believe this is a regression from [JDK-8254082](https://bugs.openjdk.java.net/browse/JDK-8254082).
void getBytes(byte[] dst, int srcPos, int dstBegin, byte coder, int length) { if (coder() == coder) { System.arraycopy(value, srcPos, dst, dstBegin << coder, length << coder()); } else { // this.coder == LATIN && coder == UTF16 StringLatin1.inflate(value, srcPos, dst, dstBegin, length); } }
When coder is `UTF16` (which it guaranteed to be without `CompactStrings`), then `srcPos` in `byte[]` array has to be adjusted by `coder` as well.
Additional testing: - [x] Linux ARM32, affected test now passes - [x] Linux x86_64, affected test now passes - [x] Linux x86_64 `tier1` default, passes - [x] Linux x86_64 `tier1`, `-XX:-CompactStrings`, passes modulo two testbugs ([JDK-8260933](https://bugs.openjdk.java.net/browse/JDK-8260933), [JDK-8260934](https://bugs.openjdk.java.net/browse/JDK-8260934))
Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision: Add the regression test case ------------- Changes: - all: https://git.openjdk.java.net/jdk16/pull/143/files - new: https://git.openjdk.java.net/jdk16/pull/143/files/e3c4da2b..0aa73e5f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk16&pr=143&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk16&pr=143&range=00-01 Stats: 3 lines in 1 file changed: 2 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk16/pull/143.diff Fetch: git fetch https://git.openjdk.java.net/jdk16 pull/143/head:pull/143 PR: https://git.openjdk.java.net/jdk16/pull/143
On Tue, 2 Feb 2021 13:31:58 GMT, Aleksey Shipilev <shade@openjdk.org> wrote:
Discovered it with ARM32 tier1 tests, which runs with -CompactStrings by default. But the bug is actually generic:
$ CONF=linux-x86_64-server-fastdebug make run-test TEST=java/lang/StringBuilder/Insert.java TEST_VM_OPTS="-XX:-CompactStrings"
test Insert.insertOffset(): failure java.lang.AssertionError: expected [??abc] but found [efabc] at org.testng.Assert.fail(Assert.java:94) at org.testng.Assert.failNotEquals(Assert.java:496) at org.testng.Assert.assertEquals(Assert.java:125) at org.testng.Assert.assertEquals(Assert.java:178) at org.testng.Assert.assertEquals(Assert.java:188) at Insert.insertOffset(Insert.java:45) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
I believe this is a regression from [JDK-8254082](https://bugs.openjdk.java.net/browse/JDK-8254082).
void getBytes(byte[] dst, int srcPos, int dstBegin, byte coder, int length) { if (coder() == coder) { System.arraycopy(value, srcPos, dst, dstBegin << coder, length << coder()); } else { // this.coder == LATIN && coder == UTF16 StringLatin1.inflate(value, srcPos, dst, dstBegin, length); } }
When coder is `UTF16` (which it guaranteed to be without `CompactStrings`), then `srcPos` in `byte[]` array has to be adjusted by `coder` as well.
Additional testing: - [x] Linux ARM32, affected test now passes - [x] Linux x86_64, affected test now passes - [x] Linux x86_64 `tier1` default, passes - [x] Linux x86_64 `tier1`, `-XX:-CompactStrings`, passes modulo two testbugs ([JDK-8260933](https://bugs.openjdk.java.net/browse/JDK-8260933), [JDK-8260934](https://bugs.openjdk.java.net/browse/JDK-8260934))
Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
Add the regression test case
Good ------------- Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk16/pull/143
On Tue, 2 Feb 2021 13:31:58 GMT, Aleksey Shipilev <shade@openjdk.org> wrote:
Discovered it with ARM32 tier1 tests, which runs with -CompactStrings by default. But the bug is actually generic:
$ CONF=linux-x86_64-server-fastdebug make run-test TEST=java/lang/StringBuilder/Insert.java TEST_VM_OPTS="-XX:-CompactStrings"
test Insert.insertOffset(): failure java.lang.AssertionError: expected [??abc] but found [efabc] at org.testng.Assert.fail(Assert.java:94) at org.testng.Assert.failNotEquals(Assert.java:496) at org.testng.Assert.assertEquals(Assert.java:125) at org.testng.Assert.assertEquals(Assert.java:178) at org.testng.Assert.assertEquals(Assert.java:188) at Insert.insertOffset(Insert.java:45) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
I believe this is a regression from [JDK-8254082](https://bugs.openjdk.java.net/browse/JDK-8254082).
void getBytes(byte[] dst, int srcPos, int dstBegin, byte coder, int length) { if (coder() == coder) { System.arraycopy(value, srcPos, dst, dstBegin << coder, length << coder()); } else { // this.coder == LATIN && coder == UTF16 StringLatin1.inflate(value, srcPos, dst, dstBegin, length); } }
When coder is `UTF16` (which it guaranteed to be without `CompactStrings`), then `srcPos` in `byte[]` array has to be adjusted by `coder` as well.
Additional testing: - [x] Linux ARM32, affected test now passes - [x] Linux x86_64, affected test now passes - [x] Linux x86_64 `tier1` default, passes - [x] Linux x86_64 `tier1`, `-XX:-CompactStrings`, passes modulo two testbugs ([JDK-8260933](https://bugs.openjdk.java.net/browse/JDK-8260933), [JDK-8260934](https://bugs.openjdk.java.net/browse/JDK-8260934))
Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
Add the regression test case
Marked as reviewed by alanb (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk16/pull/143
On Tue, 2 Feb 2021 13:31:58 GMT, Aleksey Shipilev <shade@openjdk.org> wrote:
Discovered it with ARM32 tier1 tests, which runs with -CompactStrings by default. But the bug is actually generic:
$ CONF=linux-x86_64-server-fastdebug make run-test TEST=java/lang/StringBuilder/Insert.java TEST_VM_OPTS="-XX:-CompactStrings"
test Insert.insertOffset(): failure java.lang.AssertionError: expected [??abc] but found [efabc] at org.testng.Assert.fail(Assert.java:94) at org.testng.Assert.failNotEquals(Assert.java:496) at org.testng.Assert.assertEquals(Assert.java:125) at org.testng.Assert.assertEquals(Assert.java:178) at org.testng.Assert.assertEquals(Assert.java:188) at Insert.insertOffset(Insert.java:45) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
I believe this is a regression from [JDK-8254082](https://bugs.openjdk.java.net/browse/JDK-8254082).
void getBytes(byte[] dst, int srcPos, int dstBegin, byte coder, int length) { if (coder() == coder) { System.arraycopy(value, srcPos, dst, dstBegin << coder, length << coder()); } else { // this.coder == LATIN && coder == UTF16 StringLatin1.inflate(value, srcPos, dst, dstBegin, length); } }
When coder is `UTF16` (which it guaranteed to be without `CompactStrings`), then `srcPos` in `byte[]` array has to be adjusted by `coder` as well.
Additional testing: - [x] Linux ARM32, affected test now passes - [x] Linux x86_64, affected test now passes - [x] Linux x86_64 `tier1` default, passes - [x] Linux x86_64 `tier1`, `-XX:-CompactStrings`, passes modulo two testbugs ([JDK-8260933](https://bugs.openjdk.java.net/browse/JDK-8260933), [JDK-8260934](https://bugs.openjdk.java.net/browse/JDK-8260934))
Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
Add the regression test case
Marked as reviewed by rriggs (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk16/pull/143
On Tue, 2 Feb 2021 13:31:58 GMT, Aleksey Shipilev <shade@openjdk.org> wrote:
Discovered it with ARM32 tier1 tests, which runs with -CompactStrings by default. But the bug is actually generic:
$ CONF=linux-x86_64-server-fastdebug make run-test TEST=java/lang/StringBuilder/Insert.java TEST_VM_OPTS="-XX:-CompactStrings"
test Insert.insertOffset(): failure java.lang.AssertionError: expected [??abc] but found [efabc] at org.testng.Assert.fail(Assert.java:94) at org.testng.Assert.failNotEquals(Assert.java:496) at org.testng.Assert.assertEquals(Assert.java:125) at org.testng.Assert.assertEquals(Assert.java:178) at org.testng.Assert.assertEquals(Assert.java:188) at Insert.insertOffset(Insert.java:45) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
I believe this is a regression from [JDK-8254082](https://bugs.openjdk.java.net/browse/JDK-8254082).
void getBytes(byte[] dst, int srcPos, int dstBegin, byte coder, int length) { if (coder() == coder) { System.arraycopy(value, srcPos, dst, dstBegin << coder, length << coder()); } else { // this.coder == LATIN && coder == UTF16 StringLatin1.inflate(value, srcPos, dst, dstBegin, length); } }
When coder is `UTF16` (which it guaranteed to be without `CompactStrings`), then `srcPos` in `byte[]` array has to be adjusted by `coder` as well.
Additional testing: - [x] Linux ARM32, affected test now passes - [x] Linux x86_64, affected test now passes - [x] Linux x86_64 `tier1` default, passes - [x] Linux x86_64 `tier1`, `-XX:-CompactStrings`, passes modulo two testbugs ([JDK-8260933](https://bugs.openjdk.java.net/browse/JDK-8260933), [JDK-8260934](https://bugs.openjdk.java.net/browse/JDK-8260934))
Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
Add the regression test case
Marked as reviewed by jlaskey (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk16/pull/143
On Tue, 2 Feb 2021 15:32:16 GMT, Jim Laskey <jlaskey@openjdk.org> wrote:
Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
Add the regression test case
Marked as reviewed by jlaskey (Reviewer).
Thanks. GHA are green, `tier1`, `tier2` are clean, JDK 16 fix approval is there. I think it checks all the boxes. ------------- PR: https://git.openjdk.java.net/jdk16/pull/143
On Tue, 2 Feb 2021 13:12:53 GMT, Aleksey Shipilev <shade@openjdk.org> wrote:
Discovered it with ARM32 tier1 tests, which runs with -CompactStrings by default. But the bug is actually generic:
$ CONF=linux-x86_64-server-fastdebug make run-test TEST=java/lang/StringBuilder/Insert.java TEST_VM_OPTS="-XX:-CompactStrings"
test Insert.insertOffset(): failure java.lang.AssertionError: expected [??abc] but found [efabc] at org.testng.Assert.fail(Assert.java:94) at org.testng.Assert.failNotEquals(Assert.java:496) at org.testng.Assert.assertEquals(Assert.java:125) at org.testng.Assert.assertEquals(Assert.java:178) at org.testng.Assert.assertEquals(Assert.java:188) at Insert.insertOffset(Insert.java:45) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
I believe this is a regression from [JDK-8254082](https://bugs.openjdk.java.net/browse/JDK-8254082).
void getBytes(byte[] dst, int srcPos, int dstBegin, byte coder, int length) { if (coder() == coder) { System.arraycopy(value, srcPos, dst, dstBegin << coder, length << coder()); } else { // this.coder == LATIN && coder == UTF16 StringLatin1.inflate(value, srcPos, dst, dstBegin, length); } }
When coder is `UTF16` (which it guaranteed to be without `CompactStrings`), then `srcPos` in `byte[]` array has to be adjusted by `coder` as well.
Additional testing: - [x] Linux ARM32, affected test now passes - [x] Linux x86_64, affected test now passes - [x] Linux x86_64 `tier1` default, passes - [x] Linux x86_64 `tier1`, `-XX:-CompactStrings`, passes modulo two testbugs ([JDK-8260933](https://bugs.openjdk.java.net/browse/JDK-8260933), [JDK-8260934](https://bugs.openjdk.java.net/browse/JDK-8260934)) - [x] Linux x86_64 `tier2` default, passes - [x] Linux x86_64 `tier2`, `-XX:-CompactStrings`, passes
This pull request has now been integrated. Changeset: 081fa3e7 Author: Aleksey Shipilev <shade@openjdk.org> URL: https://git.openjdk.java.net/jdk16/commit/081fa3e7 Stats: 4 lines in 2 files changed: 2 ins; 1 del; 1 mod 8260927: StringBuilder::insert is incorrect without Compact Strings Reviewed-by: redestad, alanb, rriggs, jlaskey ------------- PR: https://git.openjdk.java.net/jdk16/pull/143
participants (5)
-
Alan Bateman
-
Aleksey Shipilev
-
Claes Redestad
-
Jim Laskey
-
Roger Riggs