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

Fri Dec 18 07:33:18 UTC 2015

> On Dec 17, 2015, at 6:37 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> On 18/12/2015 3:29 AM, Christian Thalinger wrote:
>> 
>>> On Dec 16, 2015, at 10:12 PM, David Holmes <david.holmes at oracle.com <mailto:david.holmes at oracle.com>
>>> <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com>>> wrote:
>>> 
>>> 
>>> 
>>> 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 <mailto:david.holmes at oracle.com>
>>>>> <mailto:david.holmes at oracle.com <mailto: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.
>> 
>> I’m not sure I understand.  Are you saying that users should not see
>> exceptions that are thrown from system code when the VM starts up and
>> shuts down?  That doesn’t make any sense.
> 
> I think logging plus use of fatal error handler is preferable. I'm genuinely concerned that the VM may not be in any position to print exception stacktraces at late stages of VM termination.

It is.  Perhaps I should have mentioned that I tried to throw an exception and it works perfectly fine.  All this is very theoretical because in practice this will never happen.

> 
> David
> -----
> 
>> Also, remember that JVMCI is an experimental feature in 9.  It is not
>> turned on by default.
>> 
>>> 
>>> 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.