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

[Coleen Phillimore]

On 1/6/16 5:55 PM, Christian Thalinger wrote:
>
>> On Jan 6, 2016, at 11:11 AM, Coleen Phillimore 
>> <coleen.phillimore at oracle.com <mailto: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 
>>>>>>> <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?
>
> 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.

In the initialization case, you can only get there if 
java_lang_Throwable is already initialized (an exception has been 
created), so I think it's probably fine to call up to Java.  Love to get 
rid of more code that uses BacktraceBuilder and the weird arrays.

>
>>
>> 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.
>
No, I didn't.
Coleen
>>
>> 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> wrote:
>>>>>>>>>
>>>>>>>>>> On Dec 17, 2015, at 3:56 AM, Doug Simon 
>>>>>>>>>> <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> 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
>>>
>>
>