RFR (M): 8145435: [JVMCI] some tests on Windows fail with: assert(!thread->is_Java_thread()) failed: must not be java thread

[David Holmes]

Hi Christian,

os_posix.cpp:

tl;dr: the new assert is wrong, don't add it.

os::sleep has two paths:

1. interruptible sleeps

This is the implementation of java.lang.Thread.sleep and should only be 
called by JavaThreads using that API (but see below)

2. non-interruptible sleep

Used for 'incidental' sleeps within the VM, primarily by 
non-JavaThreads. If a JavaThread uses this it will delay safepoints, so 
the sleep should be very small.

As you have discovered the Windows implementation of os::sleep actually 
asserts that JavaThreads don't take the second path. That's not correct. 
There are two existing cases where this will trigger today.

The first is a historical usage of ConvertYieldToSleep:

JVM_ENTRY(void, JVM_Yield(JNIEnv *env, jclass threadClass))
   JVMWrapper("JVM_Yield");
   if (os::dont_yield()) return;
   HOTSPOT_THREAD_YIELD();

   // When ConvertYieldToSleep is off (default), this matches the 
classic VM use of yield.
   // Critical for similar threading behaviour
   if (ConvertYieldToSleep) {
     os::sleep(thread, MinSleepInterval, false);
   } else {
     os::naked_yield();
   }
JVM_END

Then in JVM_Sleep itself:

  if (millis == 0) {
     if (ConvertSleepToYield) {
       os::naked_yield();
     } else {
       ThreadState old_state = thread->osthread()->get_state();
       thread->osthread()->set_state(SLEEPING);
       os::sleep(thread, MinSleepInterval, false);
       thread->osthread()->set_state(old_state);
     }

This would also blow up on Windows - if ConvertSleepToYield is turned 
off (it is on by default on x86 - in fact all platforms except sparc - 
which really should be a Solaris setting as it will now affect the 
Linux/sparc port (which would then crash with your proposed additional 
assert in os_posix.cpp!).

So adding the assert to os::sleep on Posix is not correct, and arguably 
the assertion should be removed from the windows code.

--

os_windows.cpp:
runtime/os.hpp"

If you grep you will find about a 50-50 split between interruptible and 
interruptable through the codebase, so unless you want to file a cleanup 
bug to make it consistent I would just drop this change.

---

java.cpp

It is not at all clear to me will be in a position to print the 
exception information correctly at this stage of the termination logic. 
I would argue that any errors during JVMCIRuntime::shutdown should be 
handled internally and logged using the logging mechanism, not via 
exceptions and exception printing.

---

src/share/vm/jvmci/jvmciCompiler.cpp

typo:

+   // Give other aborting threads to also print their stack traces.

should be:

+   // Give other aborting threads a chance to also print their stack 
traces.

though I am dubious about the need for this and the use of the os::sleep 
that seemed to trigger this whole issue. Why aren't you using a normal 
vmError-based termination path for such fatal errors?

Cheers,
David

On 16/12/2015 6:54 AM, Christian Thalinger wrote:
> https://bugs.openjdk.java.net/browse/JDK-8145435
> http://cr.openjdk.java.net/~twisti/8145435/webrev/
>
> The actual fix for the problem is passing true to os::sleep:
>
> +  const bool interruptible = true;
> +  os::sleep(THREAD, 200, interruptible);
>
> since compiler threads are Java threads but I fixed a couple more things...
>
> First, I’ve added the same assert to os_posix.cpp:
>
> http://cr.openjdk.java.net/~twisti/8145435/webrev/src/os/posix/vm/os_posix.cpp.udiff.html
>
> because it’s odd to only hit this assert on Windows:
>
> Additionally I’ve changed the way we handle an exception in before_exit:
>
> http://cr.openjdk.java.net/~twisti/8145435/webrev/src/share/vm/runtime/java.cpp.udiff.html
>
> and moved abort_on_pending_exception into JVMCICompiler and made it private.  It’s used for one thing only now.
>
> Finally I replaced JVMCIRuntime::call_printStackTrace with calls to java_lang_Throwable::print_stack_trace.
>