Re: Review Request (S) 8017230: Internal Error (jvmtiRedefineClasses.cpp:1662): guarantee(false) failed: insert_space_at() failed

2013-09-13 Thread serguei.spit...@oracle.com
Thank you for the review! Serguei On 9/13/13 4:21 AM, David Holmes wrote: On 13/09/2013 5:21 AM, serguei.spit...@oracle.com wrote: On 9/11/13 8:54 PM, David Holmes wrote: Hi Dmitry, It seems odd that you install the new_method even if there was an exception. What if the new_method is not vali

Re: Review Request (S) 8017230: Internal Error (jvmtiRedefineClasses.cpp:1662): guarantee(false) failed: insert_space_at() failed

2013-09-13 Thread David Holmes
On 13/09/2013 5:21 AM, serguei.spit...@oracle.com wrote: On 9/11/13 8:54 PM, David Holmes wrote: Hi Dmitry, It seems odd that you install the new_method even if there was an exception. What if the new_method is not valid because of the exception ? Coleen suggested this fragment. New methods w

Re: Review Request (S) 8017230: Internal Error (jvmtiRedefineClasses.cpp:1662): guarantee(false) failed: insert_space_at() failed

2013-09-12 Thread serguei.spit...@oracle.com
I believe, you found the answer to this in your next email. Thanks, Serguei On 9/12/13 6:10 PM, Coleen Phillmore wrote: It took me a while to remember why this change is the way it is. Why do you clear the pending exception at line 1601? thanks, Coleen On 9/12/2013 3:21 PM, serguei.spit...

Re: Review Request (S) 8017230: Internal Error (jvmtiRedefineClasses.cpp:1662): guarantee(false) failed: insert_space_at() failed

2013-09-12 Thread serguei.spit...@oracle.com
On 9/12/13 6:22 PM, Coleen Phillmore wrote: I see the answer to my question in the mail that hasn't come out yet. The error handling in this code relies on boolean returns in order to throw a JVMTI_ERROR code. Maybe a comment to this effect would be good above CLEAR_PENDING_EXCEPTION. I

Re: Review Request (S) 8017230: Internal Error (jvmtiRedefineClasses.cpp:1662): guarantee(false) failed: insert_space_at() failed

2013-09-12 Thread Coleen Phillmore
It took me a while to remember why this change is the way it is. Why do you clear the pending exception at line 1601? thanks, Coleen On 9/12/2013 3:21 PM, serguei.spit...@oracle.com wrote: On 9/11/13 8:54 PM, David Holmes wrote: Hi Dmitry, It seems odd that you install the new_method even i

Re: Review Request (S) 8017230: Internal Error (jvmtiRedefineClasses.cpp:1662): guarantee(false) failed: insert_space_at() failed

2013-09-12 Thread Coleen Phillmore
I see the answer to my question in the mail that hasn't come out yet. The error handling in this code relies on boolean returns in order to throw a JVMTI_ERROR code. Maybe a comment to this effect would be good above CLEAR_PENDING_EXCEPTION. I don't need to rereview this.understand that

Re: Review Request (S) 8017230: Internal Error (jvmtiRedefineClasses.cpp:1662): guarantee(false) failed: insert_space_at() failed

2013-09-12 Thread Stefan Karlsson
On 9/12/13 9:21 PM, serguei.spit...@oracle.com wrote: On 9/11/13 8:54 PM, David Holmes wrote: Hi Dmitry, It seems odd that you install the new_method even if there was an exception. What if the new_method is not valid because of the exception ? Coleen suggested this fragment. New methods wi

Re: Review Request (S) 8017230: Internal Error (jvmtiRedefineClasses.cpp:1662): guarantee(false) failed: insert_space_at() failed

2013-09-12 Thread serguei.spit...@oracle.com
On 9/12/13 12:58 PM, Stefan Karlsson wrote: On 9/12/13 9:21 PM, serguei.spit...@oracle.com wrote: On 9/11/13 8:54 PM, David Holmes wrote: Hi Dmitry, It seems odd that you install the new_method even if there was an exception. What if the new_method is not valid because of the exception ? C

Re: Review Request (S) 8017230: Internal Error (jvmtiRedefineClasses.cpp:1662): guarantee(false) failed: insert_space_at() failed

2013-09-12 Thread serguei.spit...@oracle.com
On 9/11/13 8:54 PM, David Holmes wrote: Hi Dmitry, It seems odd that you install the new_method even if there was an exception. What if the new_method is not valid because of the exception ? Coleen suggested this fragment. New methods will be deallocated with the scratch class in a case of e

Re: Review Request (S) 8017230: Internal Error (jvmtiRedefineClasses.cpp:1662): guarantee(false) failed: insert_space_at() failed

2013-09-12 Thread serguei.spit...@oracle.com
On 9/11/13 8:58 PM, David Holmes wrote: On 12/09/2013 1:54 PM, David Holmes wrote: Hi Dmitry, Sorry Serguei. I'm Ok. :) Thank you for review! Thanks, Serguei David It seems odd that you install the new_method even if there was an exception. What if the new_method is not valid because o

Re: Review Request (S) 8017230: Internal Error (jvmtiRedefineClasses.cpp:1662): guarantee(false) failed: insert_space_at() failed

2013-09-11 Thread David Holmes
On 12/09/2013 1:54 PM, David Holmes wrote: Hi Dmitry, Sorry Serguei. David It seems odd that you install the new_method even if there was an exception. What if the new_method is not valid because of the exception ? Also once you've cleared the exception and returned false, the user has no i

Re: Review Request (S) 8017230: Internal Error (jvmtiRedefineClasses.cpp:1662): guarantee(false) failed: insert_space_at() failed

2013-09-11 Thread David Holmes
Hi Dmitry, It seems odd that you install the new_method even if there was an exception. What if the new_method is not valid because of the exception ? Also once you've cleared the exception and returned false, the user has no information as to why this failed. I understand we don't want to hi

Review Request (S) 8017230: Internal Error (jvmtiRedefineClasses.cpp:1662): guarantee(false) failed: insert_space_at() failed

2013-09-11 Thread serguei.spit...@oracle.com
Please, review the fix for: bug: http://bugs.sun.com/view_bug.do?bug_id=8017230 jbs: https://bugs.openjdk.java.net/browse/JDK-8017230 Open webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/8017230-JVMTI-MEM.1 Summary: Handle pending exceptions instead of firing a guarantee