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

[Jaroslav Bachorik]

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?

-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-
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>>
>>
>