8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 #15906

voitylov · 2023-09-25T15:52:12Z

test java.lang.String.RegionMatches1Tests fails on all platforms with -XX:-CompactStrings option and on ARM32 where Compact Strings is disabled by default. The fix is to return true immediately if len is negative, since for negative length this condition will never be satisfied.

Testing: JCK, JTREG passed with the fix with -XX:-CompactStrings on x86_64 and on ARM32.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 (Bug - P2)

Reviewers

Volker Simonis (@simonis - Reviewer) ⚠️ Review applies to b95bf8f0
@M4ximumPizza (no known openjdk.org user name / role) ⚠️ Review applies to b95bf8f0
Raffaello Giulietti (@rgiulietti - Committer)
Roger Riggs (@RogerRiggs - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15906

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-09-25T15:53:48Z

👋 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-09-25T15:55:54Z

@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-09-25T16:00:48Z

Webrevs

simonis

Nice catch. I don't think a negative length should have ever been allowed in the first place, but this is Java 1.0 legacy ...

simonis

Looks good.

PS: I specifically like the "Looks simple and harmless" comment on the PR of the original change :)

simonis · 2023-09-25T18:34:58Z

Might be a good idea to add a simple regressions test.

openjdk · 2023-09-25T18:36:26Z

@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:

8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163

Reviewed-by: simonis, rgiulietti, 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 61 new commits pushed to the master branch:

ca5eee2: 8316445: Mark com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java as vm.flagless
060db1b: 8315031: YoungPLABSize and OldPLABSize not aligned by ObjectAlignmentInBytes
3481a48: 8316974: ListFormat creation is unsuccessful for some of the supported Locales
29348b3: 8316894: make test TEST="jtreg:test/jdk/..." fails on AIX
3f19df6: 8315936: Parallelize gc/stress/TestStressG1Humongous.java test
fc98998: 8316580: HttpClient with StructuredTaskScope does not close when a task fails
065203d: 8313631: SA: stack trace printed using "where" command does not show class name
0c55887: 8309599: WeakHandle and OopHandle release should clear obj pointer
1230aed: 8316885: jcmd: Compiler.CodeHeap_Analytics cmd does not inform about missing aggregate
d0c1444: 8316957: Serial: Change GenCollectedHeap to SerialHeap inside gc/serial folder
... and 51 more: https://git.openjdk.org/jdk/compare/0f0c5b2d71e6dec442a5105ba305043cb59b99fc...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 (@simonis, @rgiulietti, @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).

AlanBateman · 2023-09-26T05:41:49Z

/reviewers 2 reviewer

AlanBateman · 2023-09-26T05:42:24Z

As Volker said, a test should be added. Also please have a maintainer working in this area review it too.

openjdk · 2023-09-26T05:42:34Z

@AlanBateman
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

src/java.base/share/classes/java/lang/String.java

Co-authored-by: Raffaello Giulietti <raffaello.giulietti@oracle.com>

voitylov · 2023-09-26T14:06:40Z

@rgiulietti @simonis @AlanBateman thank you for the suggestions!

test/jdk/java/lang/String/RegionMatches.java was converted to testng and split into two @tests, the latter now covers 8316879.

RogerRiggs · 2023-09-26T14:29:05Z

src/java.base/share/classes/java/lang/String.java

+        if (len < 0) {
+           return true;
+        }


Isn't it also true that the regions trivial match if len == 0?

Yes, you are right. The statement "there is some nonnegative integer k less than len " covers the case len == 0, and the rest of the cases are checked before this line.

RogerRiggs · 2023-09-26T16:23:58Z

test/jdk/java/lang/String/RegionMatches.java was converted to testng and split into two @tests, the latter now covers 8316879.

Can the test use JUnit 5? JUnit is being maintained compared to TestNG.

mlbridge · 2023-09-26T17:36:55Z

Mailing list message from Raffaello Giulietti on core-libs-dev:

Since the tests are now TestNG, I think it would make more sense to use
TestNG `assertTrue` rather than using explicit `if`s and `throw`s.

On 2023-09-26 16:09, Aleksei Voitylov wrote:

rgiulietti · 2023-09-26T18:05:42Z

I second @RogerRiggs' note that JUnit is better maintained than TestNG.

rgiulietti · 2023-09-27T10:35:22Z

LGTM (but I'm not an official Reviewer)

rgiulietti · 2023-09-27T12:32:55Z

Just out of curiosity: why not write, e.g.,

private final String s1_UTF16 = "\u043d\u0430\u0436\u0434";
private final String s2_UTF16 = "\u0432\u0020\u0441\u0442";

voitylov · 2023-09-27T12:49:39Z

\u0XXX\u0XXX\u0XXX just seems harder to read, but maybe it's just me. I don't have a strong opinion on this, let me know if you believe what you suggest somehow improves readability or is more standard.

rgiulietti · 2023-09-27T13:06:48Z

It avoids the two String constructor invocations, otherwise it is as (badly) readable as with the byte[].
My question was just curiosity.

The best thing would be to use "нажд" and "в ст", but AFAIK that's not allowed in OpenJDK.

RogerRiggs · 2023-09-27T13:27:10Z

test/jdk/java/lang/String/RegionMatches.java

+  private final byte[] b1_UTF16 = new byte[]{0x04, 0x3d, 0x04, 0x30, 0x04, 0x36, 0x04, 0x34};
+  private final byte[] b2_UTF16 = new byte[]{0x04, 0x32, 0x00, 0x20, 0x04, 0x41, 0x04, 0x42};


For strings, the \uxxxx version would be preferred; it is clearer that what the character is and there is less of a chance that the UTF encoding has a mistake.

RogerRiggs

Looks good. Thanks for the junit and unicode encoding changes.

rgiulietti

(not an official Reviewer)

voitylov · 2023-09-28T17:27:34Z

/integrate

voitylov · 2023-09-28T17:28:41Z

Thanks everyone for prompt reviews! Could someone sponsor this? I also intend to backport to 21u soon.

openjdk · 2023-09-28T17:54:50Z

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

RogerRiggs · 2023-09-28T18:09:10Z

/sponsor

openjdk · 2023-09-28T18:11:42Z

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

ca5eee2: 8316445: Mark com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java as vm.flagless
060db1b: 8315031: YoungPLABSize and OldPLABSize not aligned by ObjectAlignmentInBytes
3481a48: 8316974: ListFormat creation is unsuccessful for some of the supported Locales
29348b3: 8316894: make test TEST="jtreg:test/jdk/..." fails on AIX
3f19df6: 8315936: Parallelize gc/stress/TestStressG1Humongous.java test
fc98998: 8316580: HttpClient with StructuredTaskScope does not close when a task fails
065203d: 8313631: SA: stack trace printed using "where" command does not show class name
0c55887: 8309599: WeakHandle and OopHandle release should clear obj pointer
1230aed: 8316885: jcmd: Compiler.CodeHeap_Analytics cmd does not inform about missing aggregate
d0c1444: 8316957: Serial: Change GenCollectedHeap to SerialHeap inside gc/serial folder
... and 51 more: https://git.openjdk.org/jdk/compare/0f0c5b2d71e6dec442a5105ba305043cb59b99fc...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-09-28T18:11:49Z

@RogerRiggs @voitylov Pushed as commit cfcbfc6.

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

voitylov · 2023-09-29T15:14:26Z

/backport jdk21u

openjdk · 2023-09-29T15:18:37Z

@voitylov the backport was successfully created on the branch voitylov-backport-cfcbfc6c in my personal fork of openjdk/jdk21u. To create a pull request with this backport targeting openjdk/jdk21u: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 cfcbfc6c from the openjdk/jdk repository.

The commit being backported was authored by Aleksei Voitylov on 28 Sep 2023 and was reviewed by Volker Simonis, Raffaello Giulietti and 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/jdk21u:

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

JDK-8316879 implementation

b95bf8f

openjdk bot added the rfr Pull request is ready for review label Sep 25, 2023

openjdk bot added the core-libs core-libs-dev@openjdk.org label Sep 25, 2023

simonis suggested changes Sep 25, 2023

View reviewed changes

simonis approved these changes Sep 25, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 25, 2023

M4ximumPizza approved these changes Sep 26, 2023

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Sep 26, 2023

add regression test

003c142

rgiulietti reviewed Sep 26, 2023

View reviewed changes

src/java.base/share/classes/java/lang/String.java Outdated Show resolved Hide resolved

Update src/java.base/share/classes/java/lang/String.java

7a505e0

Co-authored-by: Raffaello Giulietti <raffaello.giulietti@oracle.com>

RogerRiggs reviewed Sep 26, 2023

View reviewed changes

address review comments

84965be

address review comments

caaa99d

rgiulietti approved these changes Sep 27, 2023

View reviewed changes

RogerRiggs suggested changes Sep 27, 2023

View reviewed changes

address review comments

aa5faea

RogerRiggs approved these changes Sep 27, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 27, 2023

rgiulietti approved these changes Sep 27, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 28, 2023

openjdk bot added the integrated Pull request has been integrated label Sep 28, 2023

openjdk bot closed this Sep 28, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Sep 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 #15906

8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 #15906

voitylov commented Sep 25, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 25, 2023

openjdk bot commented Sep 25, 2023

mlbridge bot commented Sep 25, 2023 •

edited

Loading

simonis left a comment •

edited

Loading

simonis left a comment

simonis commented Sep 25, 2023

openjdk bot commented Sep 25, 2023 •

edited

Loading

AlanBateman commented Sep 26, 2023

AlanBateman commented Sep 26, 2023

openjdk bot commented Sep 26, 2023

voitylov commented Sep 26, 2023

RogerRiggs Sep 26, 2023

voitylov Sep 26, 2023

RogerRiggs commented Sep 26, 2023

mlbridge bot commented Sep 26, 2023

rgiulietti commented Sep 26, 2023

rgiulietti commented Sep 27, 2023

rgiulietti commented Sep 27, 2023

voitylov commented Sep 27, 2023

rgiulietti commented Sep 27, 2023

RogerRiggs Sep 27, 2023

voitylov Sep 27, 2023

RogerRiggs left a comment

rgiulietti left a comment

voitylov commented Sep 28, 2023 •

edited

Loading

voitylov commented Sep 28, 2023

openjdk bot commented Sep 28, 2023

RogerRiggs commented Sep 28, 2023

openjdk bot commented Sep 28, 2023

openjdk bot commented Sep 28, 2023

voitylov commented Sep 29, 2023

openjdk bot commented Sep 29, 2023

		private final byte[] b1_UTF16 = new byte[]{0x04, 0x3d, 0x04, 0x30, 0x04, 0x36, 0x04, 0x34};
		private final byte[] b2_UTF16 = new byte[]{0x04, 0x32, 0x00, 0x20, 0x04, 0x41, 0x04, 0x42};

8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 #15906

8316879: RegionMatches1Tests fails if CompactStrings are disabled after JDK-8302163 #15906

Conversation

voitylov commented Sep 25, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 25, 2023

openjdk bot commented Sep 25, 2023

mlbridge bot commented Sep 25, 2023 • edited Loading

Webrevs

simonis left a comment • edited Loading

Choose a reason for hiding this comment

simonis left a comment

Choose a reason for hiding this comment

simonis commented Sep 25, 2023

openjdk bot commented Sep 25, 2023 • edited Loading

AlanBateman commented Sep 26, 2023

AlanBateman commented Sep 26, 2023

openjdk bot commented Sep 26, 2023

voitylov commented Sep 26, 2023

RogerRiggs Sep 26, 2023

Choose a reason for hiding this comment

voitylov Sep 26, 2023

Choose a reason for hiding this comment

RogerRiggs commented Sep 26, 2023

mlbridge bot commented Sep 26, 2023

rgiulietti commented Sep 26, 2023

rgiulietti commented Sep 27, 2023

rgiulietti commented Sep 27, 2023

voitylov commented Sep 27, 2023

rgiulietti commented Sep 27, 2023

RogerRiggs Sep 27, 2023

Choose a reason for hiding this comment

voitylov Sep 27, 2023

Choose a reason for hiding this comment

RogerRiggs left a comment

Choose a reason for hiding this comment

rgiulietti left a comment

Choose a reason for hiding this comment

voitylov commented Sep 28, 2023 • edited Loading

voitylov commented Sep 28, 2023

openjdk bot commented Sep 28, 2023

RogerRiggs commented Sep 28, 2023

openjdk bot commented Sep 28, 2023

openjdk bot commented Sep 28, 2023

voitylov commented Sep 29, 2023

openjdk bot commented Sep 29, 2023

voitylov commented Sep 25, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 25, 2023 •

edited

Loading

simonis left a comment •

edited

Loading

openjdk bot commented Sep 25, 2023 •

edited

Loading

voitylov commented Sep 28, 2023 •

edited

Loading