[jdk17] RFR: 8269661: JNI_GetStringCritical does not lock char array [v2]

[Kim Barrett]

> Please review this fix to jni_GetStringCritical.  When object pinning is
> being used it pins/unpins the String argument, but that's the wrong object.
> It should be pinning the associated byte array (when !latin1), as it is a
> reference into that array that will be returned and must be imobilized.
> 
> Presently only Shenandoah is affected by this bug, as it is the only GC that
> uses object pinning (at the region level) rather than the GC locker.  But
> both G1 and ZGC plan to use object pinning in the future.  But with region
> pinning the problem is still going to be rare because the String and its
> value array are likely to be allocated in the same region.
> 
> In addition, if pinning the String's value array, we also need to prevent
> string deduplication from changing the array while in the critical section.
> Otherwise, the unpin when the critical section is released will use the
> wrong object for the unpin operation.  We accomplish this by marking the
> String as no longer subject to string deduplication.  As that state is
> sticky, when pinning is used a String can't be deduplicated after being used
> in a critical section.  We could add a critical section counter to String,
> but for now we're going to guess that isn't worth the effort.  (String has
> at least 2 bytes available for the two string deduplication flags plus such
> a counter (more in some configurations, but always at least 2 at present)).
> 
> As part of the restructuring to accomplish these changes, it proved to be
> easy to avoid using the GC locker (or pinning) for the latin1 case, where a
> copy of the value array will be used.  Hence this change also addresses
> JDK-8269650.
> 
> Testing:
> Existing tests for GetStringCritical are in various vmTestbase/gc/lock
> tests and vmTestbase/nsk/stress/jni/jnistress004.java.
> 
> Ran mach5 tier1, tier5.  Tier5 is where the vmTestbase/gc/lock and
> vmTestbase/nsk/stress/jni tests are run.
> 
> Locally (linux-x64) ran those tests with Shenandoah, with and without
> string deduplication enabled.

Kim Barrett has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:

 - Merge branch 'master' into getstringcritical
 - conditionally lock the string value array

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

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/209/files
  - new: https://git.openjdk.java.net/jdk17/pull/209/files/ea8bd386..32bc98fc

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk17&pr=209&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk17&pr=209&range=00-01

  Stats: 254 lines in 7 files changed: 233 ins; 1 del; 20 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/209.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/209/head:pull/209

PR: https://git.openjdk.java.net/jdk17/pull/209