RFR: 8311906: Improve robustness of String constructors with mutable array inputs [v4]

[Roger Riggs]

On Wed, 15 Nov 2023 15:23:48 GMT, Raffaello Giulietti <rgiulietti at openjdk.org> wrote:

>> Roger Riggs has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Update PPC implementation of string_compress to return the index of the non-latin1 char
>>   Patch supplied by TheRealMDoerr
>
> test/jdk/java/lang/String/StringRacyConstructor.java line 110:
> 
>> 108:         for (int i = 0; i < orig.length(); i++)
>> 109:             accum |= orig.charAt(i);
>> 110:         byte expectedCoder = (accum < 256) ? LATIN1 : UTF16;
> 
> I think this assumes that compact strings are enabled during this test.

Correct, the test should be enabled iff COMPACT_STRINGS is true.

> test/jdk/java/lang/String/StringRacyConstructor.java line 190:
> 
>> 188:                 if (printWarningCount == 0) {
>> 189:                     printWarningCount = 1;
>> 190:                     System.out.println("StringUTF16.compress returned 0, may not be intrinsic");
> 
> It seems to me that the Java code for `StringUTF16.compress` also returns the index of the non-latin1 char, so I'm not sure I understand this. Just caution?

There are separate implementations of the intrinsic for each platform.
This test would help identify the platforms on which the intrinsic had not been updated to return the index instead of zero.

> test/jdk/java/lang/String/StringRacyConstructor.java line 288:
> 
>> 286:                 }
>> 287:                 if (i >= 1_000_000) {
>> 288:                     System.err.printf("Unable to produce a UTF16 string in %d iterations: %s%n", i, original);
> 
> AFAIU, this writes to `System.err` on "success". Is this the intent?

System.out is more appropriate for an informational message that a great many attempts had been made to produce an unexpected coder without success.

> test/jdk/java/lang/String/StringRacyConstructor.java line 400:
> 
>> 398:         @Override
>> 399:         public int length() {
>> 400:             return aString.length() + 1;
> 
> Not sure why ` + 1`.

Removed; it was a leftover from a prior way to throw an exception during `CharSequence.charAt(n)`.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16425#discussion_r1394889724
PR Review Comment: https://git.openjdk.org/jdk/pull/16425#discussion_r1394885799
PR Review Comment: https://git.openjdk.org/jdk/pull/16425#discussion_r1394892328
PR Review Comment: https://git.openjdk.org/jdk/pull/16425#discussion_r1394883379