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

[Christian Thalinger]

> On Jan 6, 2016, at 11:11 AM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
> 
> 
> 
> 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 < <mailto:christian.thalinger at oracle.com>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 <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?

I think it’s needed for bootstrapping in:

void vm_exit_during_initialization(Handle exception) {

Although that’s the only place that seems to really need it.  The other usages in ciReplay should be able to handle calls out to Java.

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

Did you see my other review for 8146246?  I sent it to the wrong list.

> 
> 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 < <mailto:christian.thalinger at oracle.com>christian.thalinger at oracle.com <mailto:christian.thalinger at oracle.com>> wrote:
>>>>>>>> 
>>>>>>>>> On Dec 17, 2015, at 3:56 AM, Doug Simon < <mailto:doug.simon at oracle.com>doug.simon at oracle.com <mailto: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 < <mailto:tom.rodriguez at oracle.com>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
>> 
>