RFR 4505697: nsk/jdi/ExceptionEvent/_itself_/exevent006 and exevent008 tests fail with InvocationTargetException

[serguei.spitsyn at oracle.com]

On 2/20/14 12:49 AM, Jaroslav Bachorik wrote:
> On 19.2.2014 18:01, Daniel D. Daugherty wrote:
>>  > Webrev: http://cr.openjdk.java.net/~jbachorik/4505697/webrev.02
>>
>> src/share/vm/prims/jvmtiExport.hpp
>>      No comments.
>>
>> src/share/vm/prims/jvmtiExport.cpp
>>      No comments.
>>
>> src/share/vm/oops/instanceKlass.cpp
>>      No comments.
>>
>> src/share/vm/prims/jvm.cpp
>>      No comments.
>>
>> src/share/vm/runtime/reflection.cpp
>>      lines 948, 1085: HotSpot indent is two spaces
>>
>> Thumbs up.
>>
>
> Thanks Dan!
>
> Could I have a second HS reviewer to take a look at this, please?

You need just one review with the reviewer status and at least two 
reviews total.
You already got reviews from Dan and me which is enough.
But it still might be worth to wait for David to review this version as 
he already reviewed prev. one.


Thanks,
Serguei

>
> -JB-
>
>> Dan
>>
>>
>> On 2/19/14 8:59 AM, Jaroslav Bachorik wrote:
>>> On 18.2.2014 11:18, serguei.spitsyn at oracle.com wrote:
>>>> On 2/17/14 12:04 AM, Jaroslav Bachorik wrote:
>>>>> On 14.2.2014 23:13, serguei.spitsyn at oracle.com wrote:
>>>>>> On 2/14/14 12:33 PM, Daniel D. Daugherty wrote:
>>>>>>> On 2/14/14 11:46 AM, serguei.spitsyn at oracle.com wrote:
>>>>>>>> Jaroslav,
>>>>>>>>
>>>>>>>> It looks good in general modulo indent comments from Dan.
>>>>>>>>
>>>>>>>> But I have a doubt that acquiring the JvmtiThreadState_lock is
>>>>>>>> needed
>>>>>>>> or right thing to do in the 
>>>>>>>> JvmtiExport::clear_detected_exception().
>>>>>>>> It seems, both clear_exception_detected() and
>>>>>>>> set_exception_detected() are always
>>>>>>>> called on current thread and so, it has to be safe to do without
>>>>>>>> acquiring any locks.
>>>>>>>
>>>>>>> My JVM/TI-foo is rusty, but I believe that JvmtiThreadState stuff
>>>>>>> can also be queried/modified by other threads so grabbing the
>>>>>>> associated lock is a good idea.
>>>>>>
>>>>>> The lock synchronization is cooperative.
>>>>>> It does not help much if the lock is not acquired in other places.
>>>>>> I can be wrong, but I've not found yet any place in the code 
>>>>>> where the
>>>>>> clear_exception_detected() and set_exception_detected() are called
>>>>>> under protection of the JvmtiThreadState_lock.
>>>>>
>>>>> I copied the locking over from
>>>>> "JvmtiExport::cleanup_thread(JavaThread* thread)". That method is 
>>>>> also
>>>>> supposed to work only with the current thread but acquires the lock
>>>>> nonetheless. But if you are sure that the lock is not required I have
>>>>> no objections removing it.
>>>>
>>>> I'm suggesting to remove it, as it is not used in other places in the
>>>> code.
>>>> It is going to be confusing if it is used in one place and missed in
>>>> others.
>>>
>>> I've removed the lock and applied the same cleanup logic to other
>>> places where exceptions are rewrapped.
>>>
>>> Webrev: http://cr.openjdk.java.net/~jbachorik/4505697/webrev.02
>>> JPRT run:
>>> http://prt-web.us.oracle.com//archive/2014/02/2014-02-19-114618.jbachorik.hotspot/ 
>>>
>>>
>>> Aurora Adhoc:
>>> http://aurora.ru.oracle.com//faces/Batch.xhtml?batchName=418853.VMSQE.adhoc.JPRT.full 
>>>
>>> (still running at the moment; no failures so far)
>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>>
>>>>> -JB-
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>>
>>>>>>> Dan
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> And I'm repeating my question about pre-integration testing 
>>>>>>>> (Dan is
>>>>>>>> asking about the same).
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/14/14 3:07 AM, Jaroslav Bachorik wrote:
>>>>>>>>> This is a round-0 review request.
>>>>>>>>>
>>>>>>>>> The reflection code intercepting the exceptions thrown in the
>>>>>>>>> invoked methods does not play nicely with JVMTI (which, in this
>>>>>>>>> case, propagates to JDI).
>>>>>>>>>
>>>>>>>>> The reflection code lacks the traditional error handler -
>>>>>>>>> therefore,
>>>>>>>>> upon throwing the NumberFormatException, the stack is searched 
>>>>>>>>> for
>>>>>>>>> appropriate handlers and none are found. This leaves the
>>>>>>>>> "exception_detected" flag set to true while normally it would be
>>>>>>>>> reset to false once the exception is handled. The reflection code
>>>>>>>>> then goes on and wraps the NumberFormatException into
>>>>>>>>> InvocationTargetException and throws it. But, alas, the
>>>>>>>>> "exception_detected" flag is still set to true and no JVMTI
>>>>>>>>> exception event will be sent out.
>>>>>>>>>
>>>>>>>>> The proposed solution is to call
>>>>>>>>> thread->jvmti_thread_state()->clear_exception_detected() at the
>>>>>>>>> appropriate places in the reflection code to reset the
>>>>>>>>> "exception_detected" flag and enable the 
>>>>>>>>> InvocationTargetException
>>>>>>>>> be properly reported over JVMTI.
>>>>>>>>>
>>>>>>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-4505697
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/4505697/webrev.00
>>>>>>>>>
>>>>>>>>> Thanks!
>>>>>>>>>
>>>>>>>>> -JB-
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>>
>>
>