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

[David Holmes]

On 17/12/2015 3:45 AM, Christian Thalinger wrote:
>
>> On Dec 15, 2015, at 2:54 PM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> 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.
>
> I’ll let you guys take care of that.
>
>>
>> --
>>
>> 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.
>
> It’s a cleanup and it’s already done but I’m not arguing for this.
>
>>
>> ---
>>
>> 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.
>
> I disagree.  We are shutting down the VM and while doing that we are running Java code.  If the Java code throws an exception the user wants to see that exception.

But it is not the user's Java code it is an internal implementation 
artifact. I have no doubt this is helping you with debugging JVMCI but I 
don't think it is the correct way to handle an internal JVMCI error 
during termination. Ditto for the use of the sleep mechanism.

David
------

>
>>
>> ---
>>
>> 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?
>
> As the comment says:
>
> +  // This can be very useful when debugging class initialization
> +  // failures.
>
> Since multiple compiler threads can initialize the JVMCI subsystem we need to print all of the exceptions to see the root cause.  Doug would know more about this but that’s the gist.
>
>>
>> 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.
>>>
>