> 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.
Dan
On 2/19/14 8:59 AM, Jaroslav Bachorik wrote:
On 18.2.2014 11:18, [email protected] wrote:
On 2/17/14 12:04 AM, Jaroslav Bachorik wrote:
On 14.2.2014 23:13, [email protected] wrote:
On 2/14/14 12:33 PM, Daniel D. Daugherty wrote:
On 2/14/14 11:46 AM, [email protected] 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-
