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

[Coleen Phillimore]

On 12/29/15 1:58 PM, Christian Thalinger wrote:
>
>> On Dec 17, 2015, at 9:17 AM, Coleen Phillimore 
>> <coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com>> 
>> wrote:
>>
>>
>>
>> On 12/17/15 2:08 PM, Christian Thalinger wrote:
>>>> On Dec 17, 2015, at 9:03 AM, Tom Rodriguez 
>>>> <tom.rodriguez at oracle.com <mailto:tom.rodriguez at oracle.com>> wrote:
>>>>
>>>>
>>>>> On Dec 17, 2015, at 10:53 AM, Christian Thalinger 
>>>>> <christian.thalinger at oracle.com 
>>>>> <mailto:christian.thalinger at oracle.com>> wrote:
>>>>>
>>>>>
>>>>>> On Dec 17, 2015, at 8:28 AM, Tom Rodriguez 
>>>>>> <tom.rodriguez at oracle.com <mailto:tom.rodriguez at oracle.com>> wrote:
>>>>>>
>>>>>> I think the ttyLocker should be in print_stack_trace.
>>>>> Well, the problem is that ttyLocker only locks 
>>>>> defaultStream::instance (whatever that is) but print_stack_trace 
>>>>> can take any outputStream.
>>>> Well maybe it shouldn’t.  No one uses that flexibility but having a 
>>>> ttyLocker would be practically useful.
>>> True.  I’m tempted to make that change but I see more bike-shedding 
>>> coming up. Maybe if Coleen is okay with that… ;-)
>>
>> No, just leave it where it was....   With logging the ttyLocker 
>> doesn't work anyway.
>
> Well, turns out that we can’t hold the tty lock when calling out to Java:
>
> https://bugs.openjdk.java.net/browse/JDK-8146246
>
> Either we give up the lock completely or we call out to Java code to 
> do the printing for us (which was there in the first place).  Since we 
> are already calling out to Java (to Throwable.getCause) calling Java 
> code at that point must work.

I finally had some time to look at this.   I was surprised that I never 
noticed that we call out to Java to get the cause. If you call out to 
Java to print the stack trace, I see a lock on the stream so at least 
the stack and cause are together.   Just not the exception line.   Can 
you add a function to java_lang_Throwable to print the exception name 
and stack trace or is that not allowed?

I don't know.  I wonder if we can remove the jvm's 
java_lang_Throwable::print_stack_trace altogether.  It doesn't have to 
be fast at all so could call java (which calls back into the jvm for 
each stack trace element).   Maybe it's needed for bootstrapping (but 
there it's already created an exception object) or for OutOfMemoryError?

Or remove the ttyLocker in your calls.  I don't have the entire context 
for the motivation for this change.

Coleen

>
>>
>> Coleen
>>>
>>>>  Anyway, it’s ok as is too.
>>>>
>>>> tom
>>>>
>>>>>> A short comment on print_stack_trace explaining that it prints 
>>>>>> the exception message along with the exception chain wouldn’t 
>>>>>> hurt.  Otherwise looks good.
>>>>> Sure.
>>>>>
>>>>>> tom
>>>>>>
>>>>>>> On Dec 17, 2015, at 9:46 AM, Christian Thalinger 
>>>>>>> <christian.thalinger at oracle.com 
>>>>>>> <mailto:christian.thalinger at oracle.com>> wrote:
>>>>>>>
>>>>>>>> On Dec 17, 2015, at 3:56 AM, Doug Simon <doug.simon at oracle.com 
>>>>>>>> <mailto:doug.simon at oracle.com> <mailto:doug.simon at oracle.com>> 
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> On 17 Dec 2015, at 08:20, Tom Rodriguez 
>>>>>>>>> <tom.rodriguez at oracle.com <mailto:tom.rodriguez at oracle.com>> 
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>>>> I feel like there was a reason we weren’t using 
>>>>>>>>>>>> java_lang_Throwable::print_stack_trace but it looks like it 
>>>>>>>>>>>> handles the causes properly which is the only reason I 
>>>>>>>>>>>> could think of. Maybe Doug knows?
>>>>>>>>>>>
>>>>>>>>>>> There are two issues with 
>>>>>>>>>>> java_lang_Throwable::print_stack_trace:
>>>>>>>>>>>
>>>>>>>>>>> 1. It doesn’t print the exception message, just the stack.
>>>>>>>>>> I’m printing the message separately:
>>>>>>>>>>
>>>>>>>>>> java_lang_Throwable::print(exception, tty);
>>>>>>>>>> tty->cr();
>>>>>>>>>> java_lang_Throwable::print_stack_trace(exception(), tty);
>>>>>>>>> There are already 3 other copies of this idiom.  Throwable 
>>>>>>>>> should probably have a single method for this.  Or maybe 
>>>>>>>>> print_stack_trace itself should do this?
>>>>>>>> +1
>>>>>>>>
>>>>>>>>> That would better parallel Throwable.printStackTrace().
>>>>>>>> Makes sense to me!
>>>>>>> It does.  Here it goes:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~twisti/8145435/webrev.02/ 
>>>>>>> <http://cr.openjdk.java.net/%7Etwisti/8145435/webrev.02/> 
>>>>>>> <http://cr.openjdk.java.net/~twisti/8145435/webrev.02/ 
>>>>>>> <http://cr.openjdk.java.net/%7Etwisti/8145435/webrev.02/>>
>>>>>>>
>>>>>>> I’ve made java_lang_Throwable::print_stack_trace to take a 
>>>>>>> Handle instead of an oop and removed the unneeded copy of 
>>>>>>> java_lang_Throwable::print.
>>>>>>>
>>>>>>>> -Doug
>