8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled #17057

voitylov · 2023-12-11T13:48:18Z

Since JDK-8311906, if CompactStrings is not enabled, index is not considered when calling extractCodepoints from StringUTF16.toBytes(). Because of that the last elements of the source codepoints array are stripped from the resulting UTF16 string, which fires in other places (e.g. during RegEx processing).

The fix replaces len in extractCodepoints parameters with end that is index + len.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled (Bug - P2)

Reviewers

Roger Riggs (@RogerRiggs - Reviewer)

Contributors

Roger Riggs <rriggs@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17057/head:pull/17057
$ git checkout pull/17057

Update a local copy of the PR:
$ git checkout pull/17057
$ git pull https://git.openjdk.org/jdk.git pull/17057/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17057

View PR using the GUI difftool:
$ git pr show -t 17057

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17057.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-12-11T13:49:08Z

👋 Welcome back avoitylov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-12-11T13:51:41Z

@voitylov The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-12-11T15:08:27Z

Webrevs

RogerRiggs

Thanks for tracking down this issue, it exposes a gap in the testing.
The fix looks fine.
The test is very specific to a particular use case.
I suggest adding a new test in PR#17066.
If it is suitable, pull it into the PR.

voitylov · 2023-12-12T10:50:06Z

/contributor add rriggs

openjdk · 2023-12-12T10:50:23Z

@voitylov RogerRiggs was not found in the census.

Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

/contributor add @openjdk-bot
/contributor add duke
/contributor add J. Duke <duke@openjdk.org>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

voitylov · 2023-12-12T10:57:20Z

Thanks Roger. I agree with your point and pulled your suggested test in the PR.

RogerRiggs · 2023-12-12T18:56:04Z

test/jdk/java/lang/String/Chars.java

+                        errOffset, actual[errOffset]);
+                System.err.println("expected: " + Arrays.toString(expected));
+                System.err.println("actual: " + Arrays.toString(actual));
+                throw new RuntimeException("testCharsSubrange failed!");


"testCharsSubrange" -> "testIntsSubrange" as commented in the dependent PR#17066.

RogerRiggs · 2023-12-12T19:11:43Z

test/jdk/java/lang/String/Chars.java

@@ -45,6 +47,7 @@ public static void main(String[] args) {
            }
            testChars(cc, ccExp);
            testCharsSubrange(cc, ccExp);
+            testIntsSubrange(ccExp);


Please also add the same call after line 74 to apply the test to the surrogates case.
(As suggested by @rgiulietti in #17066)

Just pulled the update from that branch.

RogerRiggs · 2023-12-12T21:03:13Z

@voitylov Are you aware of -XX:-CompactStrings being used in production or was this discovered as part of release testing?
Thanks

voitylov · 2023-12-13T11:39:29Z

Yes, I'm aware of -XX:-CompactStrings being used in production in server deployments (no, I didn't collect the reasons for that).
Yes, it was discovered as part of release testing. Also related to our support efforts for ARM32, but not just that.

If you think it's worth it, I can go through some tests and add some more -XX:-CompactStrings mode to java.lang.String tests here and there, lightly. It's not the first time we hit this issue.

voitylov · 2023-12-13T11:39:45Z

/contributor add rriggs

openjdk · 2023-12-13T11:40:22Z

@voitylov
Contributor Roger Riggs <rriggs@openjdk.org> successfully added.

RogerRiggs

Looks good, thanks for the followup.

openjdk · 2023-12-13T14:39:06Z

@voitylov This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled

Co-authored-by: Roger Riggs <rriggs@openjdk.org>
Reviewed-by: rriggs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 36 new commits pushed to the master branch:

2838a91: 8288989: Make tests not depend on the source code
d2ba3b1: 8312150: Remove -Xnoagent option
d632d74: 8321820: TestLoadNIdeal fails on 32-bit because -XX:+UseCompressedOops is not recognized
ddbbd36: 8320279: Link issues in java.xml module-info.java
c8ad7b7: 8321974: Crash in ciKlass::is_subtype_of because TypeAryPtr::_klass is not initialized
cf94854: 8321565: [REDO] Heap dump does not contain virtual Thread stack references
7ece9e9: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
9320ef9: 8321973: Parallel: Remove unused methods in AdaptiveSizePolicy
2a565ff: 8321808: G1: Use unsigned type for non-negative G1 flags
493b5bd: 8293622: Cleanup use of G1ConcRefinementThreads
... and 26 more: https://git.openjdk.org/jdk/compare/3c6459e1de9e75898a1b32a95acf684050fbe1af...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RogerRiggs) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

RogerRiggs · 2023-12-13T14:42:38Z

If you think it's worth it, I can go through some tests and add some more -XX:-CompactStrings mode to java.lang.String tests here and there, lightly. It's not the first time we hit this issue.

Verifying the coverage of the tests would be useful. I noticed in many tests of the API signatures that include offset and length, the offset is frequently zero.

As for -XX:-CompactStrings, it was originally a precaution about backward compatibility. From time to time it comes up to consider simplifying the code to remove it. It would be good to know the use cases where it is needed and the rationale.

RogerRiggs · 2023-12-13T14:45:31Z

I'll keep an eye out for the backport to jdk 22 if a review is needed for that. (I meant JDK22)

voitylov · 2023-12-14T12:03:11Z

Thank you, Roger. Yes, I'll work on the 22 backport.

voitylov · 2023-12-14T12:03:50Z

/integrate

openjdk · 2023-12-14T12:05:42Z

@voitylov
Your change (at version cf622fa) is now ready to be sponsored by a Committer.

RogerRiggs · 2023-12-14T14:37:53Z

/sponsor

RogerRiggs · 2023-12-14T14:38:21Z

Thank you, Roger. Yes, I'll work on the 22 backport.

See https://wiki.openjdk.org/display/SKARA/Backports

Once the mainline is integrated, find the commit and add a "/backport jdk22" comment.
Skara should do the rest (but read the directions also)

Thanks

openjdk · 2023-12-14T14:39:06Z

Going to push as commit fde5b16.
Since your change was applied there have been 40 commits pushed to the master branch:

45a9ade: 8202598: keytool -certreq output contains inconsistent line separators
62b7c5e: 8319647: Few java/lang/System/LoggerFinder/modules tests ignore vm flags
69014cd: 8320682: [AArch64] C1 compilation fails with "Field too big for insn"
5a97dbf: 8322034: Parallel: Remove unused methods in PSAdaptiveSizePolicy
2838a91: 8288989: Make tests not depend on the source code
d2ba3b1: 8312150: Remove -Xnoagent option
d632d74: 8321820: TestLoadNIdeal fails on 32-bit because -XX:+UseCompressedOops is not recognized
ddbbd36: 8320279: Link issues in java.xml module-info.java
c8ad7b7: 8321974: Crash in ciKlass::is_subtype_of because TypeAryPtr::_klass is not initialized
cf94854: 8321565: [REDO] Heap dump does not contain virtual Thread stack references
... and 30 more: https://git.openjdk.org/jdk/compare/3c6459e1de9e75898a1b32a95acf684050fbe1af...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-12-14T14:39:13Z

@RogerRiggs @voitylov Pushed as commit fde5b16.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

voitylov · 2023-12-18T11:55:08Z

/backport jdk22

openjdk · 2023-12-18T11:56:09Z

@voitylov the backport was successfully created on the branch backport-voitylov-fde5b168 in my personal fork of openjdk/jdk22. To create a pull request with this backport targeting openjdk/jdk22:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit fde5b168 from the openjdk/jdk repository.

The commit being backported was authored by Aleksei Voitylov on 14 Dec 2023 and was reviewed by Roger Riggs.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk22:

$ git fetch https://github.com/openjdk-bots/jdk22.git backport-voitylov-fde5b168:backport-voitylov-fde5b168
$ git checkout backport-voitylov-fde5b168
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk22.git backport-voitylov-fde5b168

JDK-8321514 implementation

62b5310

openjdk bot added the core-libs label Dec 11, 2023

voitylov marked this pull request as draft December 11, 2023 13:54

whitespace

50c0984

voitylov marked this pull request as ready for review December 11, 2023 15:04

openjdk bot added the rfr label Dec 11, 2023

RogerRiggs mentioned this pull request Dec 11, 2023

Extend testing of new String of int [] to cover subranges #17066

Closed

3 tasks

RogerRiggs reviewed Dec 11, 2023

View reviewed changes

review comments

759068e

RogerRiggs reviewed Dec 12, 2023

View reviewed changes

review comments

cf622fa

RogerRiggs approved these changes Dec 13, 2023

View reviewed changes

openjdk bot added the ready label Dec 13, 2023

openjdk bot added the sponsor label Dec 14, 2023

openjdk bot added the integrated label Dec 14, 2023

openjdk bot closed this Dec 14, 2023

openjdk bot removed ready rfr sponsor labels Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled #17057

8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled #17057

voitylov commented Dec 11, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 11, 2023

openjdk bot commented Dec 11, 2023

mlbridge bot commented Dec 11, 2023 •

edited

Loading

RogerRiggs left a comment

voitylov commented Dec 12, 2023 •

edited

Loading

openjdk bot commented Dec 12, 2023

voitylov commented Dec 12, 2023

RogerRiggs Dec 12, 2023

RogerRiggs Dec 12, 2023

voitylov Dec 13, 2023

RogerRiggs commented Dec 12, 2023

voitylov commented Dec 13, 2023

voitylov commented Dec 13, 2023

openjdk bot commented Dec 13, 2023

RogerRiggs left a comment

openjdk bot commented Dec 13, 2023 •

edited

Loading

RogerRiggs commented Dec 13, 2023

RogerRiggs commented Dec 13, 2023 •

edited

Loading

voitylov commented Dec 14, 2023 •

edited

Loading

voitylov commented Dec 14, 2023

openjdk bot commented Dec 14, 2023

RogerRiggs commented Dec 14, 2023

RogerRiggs commented Dec 14, 2023 •

edited

Loading

openjdk bot commented Dec 14, 2023

openjdk bot commented Dec 14, 2023

voitylov commented Dec 18, 2023

openjdk bot commented Dec 18, 2023

8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled #17057

8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled #17057

Conversation

voitylov commented Dec 11, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

Webrev

bridgekeeper bot commented Dec 11, 2023

openjdk bot commented Dec 11, 2023

mlbridge bot commented Dec 11, 2023 • edited Loading

Webrevs

RogerRiggs left a comment

Choose a reason for hiding this comment

voitylov commented Dec 12, 2023 • edited Loading

openjdk bot commented Dec 12, 2023

voitylov commented Dec 12, 2023

RogerRiggs Dec 12, 2023

Choose a reason for hiding this comment

RogerRiggs Dec 12, 2023

Choose a reason for hiding this comment

voitylov Dec 13, 2023

Choose a reason for hiding this comment

RogerRiggs commented Dec 12, 2023

voitylov commented Dec 13, 2023

voitylov commented Dec 13, 2023

openjdk bot commented Dec 13, 2023

RogerRiggs left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 13, 2023 • edited Loading

RogerRiggs commented Dec 13, 2023

RogerRiggs commented Dec 13, 2023 • edited Loading

voitylov commented Dec 14, 2023 • edited Loading

voitylov commented Dec 14, 2023

openjdk bot commented Dec 14, 2023

RogerRiggs commented Dec 14, 2023

RogerRiggs commented Dec 14, 2023 • edited Loading

openjdk bot commented Dec 14, 2023

openjdk bot commented Dec 14, 2023

voitylov commented Dec 18, 2023

openjdk bot commented Dec 18, 2023

voitylov commented Dec 11, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 11, 2023 •

edited

Loading

voitylov commented Dec 12, 2023 •

edited

Loading

openjdk bot commented Dec 13, 2023 •

edited

Loading

RogerRiggs commented Dec 13, 2023 •

edited

Loading

voitylov commented Dec 14, 2023 •

edited

Loading

RogerRiggs commented Dec 14, 2023 •

edited

Loading