On 21.2.2014 08:24, David Holmes wrote:
On 20/02/2014 11:41 PM, Jaroslav Bachorik wrote:
On 20.2.2014 11:40, David Holmes wrote:
Hi Jaroslav,
instanceKlass.cpp:
Comment is wrong:
913 // JVMTI internal flag reset is needed in order to report
InvocationTargetException
It will be ExceptionInInitializerError
Will fix. Copypaste ...
You added this:
917
this_oop->set_initialization_state_and_notify(initialization_error,
THREAD);
918 CLEAR_PENDING_EXCEPTION; // ignore any exception thrown,
class initialization error is thrown below
+ 919 // JVMTI has already reported the pending exception
+ 920 // JVMTI internal flag reset is needed in order to report
InvocationTargetException
+ 921 JvmtiExport::clear_detected_exception((JavaThread*)THREAD);
but there are a number of places where
set_initialization_state_and_notify is called when a pending exception
has been cleared, and then CLEAR_PENDING_EXCEPTION is called again, but
you didn't modify those other locations. They will rethrow the original
exception so I suppose that is okay from JVMTI's perspective. But the
flip-side of this is that if set_initialization_state_and_notify does
throw an exception, JVMTI will never see it.
I don't know if it supposed to see it. It seems that any exception
thrown from set_initialization_state_and_notify is thoroughly ignored
and hidden from the outer world. Perhaps someone more experienced in
JVMTI than me would like to chime in here? Serguei?
set_initialization_state_and_notify is often called when an exception
has already occurred during the class loading/initialization process. It
is that original exception that we want to propagate but meanwhile we
have to perform this action to update the state and wakeup any waiters.
So we cache the original exception, clear it, do the state update and
then clear any pending exception (I think the only exception possible
here is OOME!), then rethrow the original. If this action did indeed
throw OOME then we might not be able to wake up the waiter(s) and that
might lead to a hang. While a debug VM could use TraceExceptions to
(hopefully) spot the OOME, in a product VM it would be invisible, even
if a JVMTI agent was tracking exceptions. So I think it should be
visible to JVMTI. I would like to hear other opinions though.
However this is going beyond the scope of fixing these particular tests
so I'm fine if this is simply recorded in another bug for future clean up.
Created https://bugs.openjdk.java.net/browse/JDK-8035646
Thanks for the review, everyone.
-JB-
Thanks,
David
-JB-
---
jvm.cpp
Comment is wrong again - not InvocationTargetException.
---
David
------
On 20/02/2014 1: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-
