Integrated: 8339285: Test fails with assert(depth < max_critical_stack_depth) failed: can't have more than 10 critical frames

[Maurizio Cimadamore]

On Wed, 4 Sep 2024 15:23:51 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> Scoped methods are critical methods in the FFM API where memory is accessed in a potentially unsafe way. When closing shared arenas, we look at threads in the middle of a scoped operation involving that arena, and if we find one, we make it fail (by installing an async handshake on that thread).
> 
> To find whether a thread is in a scoped method or not, we need a stack walk. For performance reasons, it is preferrable to have the stack walk to be bounded in size.
> 
> A test started picking up a JVM assertion where the stack of a scoped method (namely `ScopedMemoryAccess::isLoaded`) is too big. This is caused by the scoped method stack walk finding the thread using the scoped method in the middle of some JNI lookup (which is required as `isLoaded` eventually ends up in a native method). This condition seems to have been made easier by the fact that these [changes](https://git.openjdk.org/jdk/pull/19213).
> 
> This PR reverts the stack trace associated with JNI lookup to what it was before, by eliminating the extra frame with a bit of refactoring/cleanup. But this is not enough: the stress test introduced in this PR still fails, even when the stack associated with `ClassLoader::findNative` is restored.
> 
> To address this problem in full, I have resorted to `registerNatives` - that is, the native `isLoaded0`, `load0`, `unload0` and `force0` are pre-registered, when the static initializer of `MappedMemoryUtils` is ran. This means that we no longer need to run a JNI lookup in the middle of a scoped method call. This brings the stack back under control, and passes the stress test.
> 
> Of course there's more to do in this area - we should have a more direct test to check the stack associated with scoped methods (for instance, vector load/store operations are also potential suspects), in order to catch "suspicious refactoring" earlier in the process. For this reason I also filed a follow up i[ssue](https://bugs.openjdk.org/browse/JDK-8339551).

This pull request has now been integrated.

Changeset: 9e1af8cc
Author:    Maurizio Cimadamore <mcimadamore at openjdk.org>
URL:       https://git.openjdk.org/jdk/commit/9e1af8cc7cc9f63453097bd35eb3cf29f945d765
Stats:     253 lines in 9 files changed: 212 ins; 12 del; 29 mod

8339285: Test fails with assert(depth < max_critical_stack_depth) failed: can't have more than 10 critical frames

Reviewed-by: alanb

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

PR: https://git.openjdk.org/jdk/pull/20854