RFR: 8292674: ReportJNIFatalError should print all java frames

[Patricio Chilano Mateo]

On Mon, 5 Dec 2022 06:58:32 GMT, David Holmes <dholmes at openjdk.org> wrote:

> When JNI reports errors and warnings and produces a stack trace, we need to see the exact Java code that was being executed. The existing `JavaThread::print_stack` that is used is only for platform/carrier threads and doesn't show any mounted virtual thread. So we introduce a new `JavaThread::print_virtual_stack_on` method to print the mounted virtual thread's stack, and we add `JavaThread::print_jni_stack` as a utility for JNI to call that dispatches to the appropriate function based on the thread type.
> 
> The existing test `hotspot/jtreg/runtime/jni/checked/TestPrimitiveArrayCriticalWithBadParam.java` was modified to test regular and virtual threads, and to expect to see the specific stack frame of the Java native method.
> 
> Additional manual testing of the JNI `FatalError` method was done by direct fault injection in modified JDK code.
> 
> Sanity testing: Oracle tiers 1-4 (to include our -Xcheck:jni run in tier4)
> 
> Thanks,
> David

Looks good to me. Some comments below.

Thanks,
Patricio

src/hotspot/share/prims/jniCheck.cpp line 145:

> 143: static void ReportJNIWarning(JavaThread* thr, const char *msg) {
> 144:   tty->print_cr("WARNING in native method: %s", msg);
> 145:   thr->print_jni_stack();

There is another print_stack() call in check_pending_exception() that wasn't changed. Was that intended?

src/hotspot/share/runtime/javaThread.cpp line 1758:

> 1756:       // Print out lock information
> 1757:       if (JavaMonitorsInStackTrace) {
> 1758:         jvf->print_lock_info_on(st, count);

I see this method looks for objects that are locked in this frame and prints their klass. At least for interpretedVFrame we get the oops from the BasicObjectLocks in the stack so that would already require to set _process_frames. Don't understand completely how the compiled case works but would certainly need that too (and should probably also set _update_map). I see that's actually how JavaThread::print_stack_on() defines them.

test/hotspot/jtreg/runtime/jni/checked/TestPrimitiveArrayCriticalWithBadParam.java line 32:

> 30:  * @comment Tests reporting with regular thread and virtual thread.
> 31:  * @library /test/lib
> 32:  * @enablePreview

Don't we need @requires vm.continuations also? I remember having failures on the linux-x86 tests because of this, but don't see those runs in your Github actions to double check.

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

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