Re: RFR 4505697: nsk/jdi/ExceptionEvent/_itself_/exevent006 and exevent008 tests fail with InvocationTargetException
Hi Dan, On 14.2.2014 17:38, Daniel D. Daugherty wrote: > Webrev: http://cr.openjdk.java.net/~jbachorik/4505697/webrev.00 Nice job on such an old bug. src/share/vm/prims/jvmtiExport.hpp No comments. src/share/vm/prims/jvmtiExport.cpp line 2170: state->clear_exception_detected(); HotSpot indent is two spaces. src/share/vm/runtime/reflection.cpp line 948: JvmtiExport::clear_detected_exception((JavaThread*) THREAD); line 1085: JvmtiExport::clear_detected_exception((JavaThread*) THREAD); HotSpot indent is two spaces. Indentation will be fixed. So there are two existing tests for this failure mode: nsk/jdi/ExceptionEvent/_itself_/exevent006 nsk/jdi/ExceptionEvent/_itself_/exevent008 so I'm presuming that you've run the above tests and they are now happy, but you haven't really stated that. What other pre-integration testing do you have planned? Yes, those two tests are not failing any more. The full testing would include running both JTREG (testsets 'core' and 'svc') and aurora testbase at least for 'runtime', 'nsk/jvmti' and 'nsk/jdi'. -JB- Dan On 2/14/14 4: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-
Re: RFR(XS): JDK-8034176 Update mapfile for libjfr
On 17 feb 2014, at 02:30, David Holmes wrote: > On 11/02/2014 10:01 PM, Staffan Larsen wrote: >> >> On 11 feb 2014, at 12:14, Erik Joelsson wrote: >> >>> Looks good, but I can't help but wonder why the mapfile for libjfr is in >>> the open. >> >> Yes, that is unfortunate. It used to be in closed source in jdk7, but moved >> into open with the new build system. > > That should be fixed. Looks like we need a make/closed/lib/Serviceability.gmk > that contains the BUILD_LIBJFR definition. Yes, I’ve filed a bug about it. Thanks, /Staffan > > David > >> /Staffan >> >>> >>> /Erik >>> >>> On 2014-02-11 12:00, Staffan Larsen wrote: Resending (email to build-dev bounced since I used the wrong sender address). On 11 feb 2014, at 11:18, staf...@larsen.se wrote: > Please review this small fix for libjfr/mapfile-vers. > > Thanks, > /Staffan > > diff --git a/make/mapfiles/libjfr/mapfile-vers > b/make/mapfiles/libjfr/mapfile-vers > --- a/make/mapfiles/libjfr/mapfile-vers > +++ b/make/mapfiles/libjfr/mapfile-vers > @@ -34,6 +34,7 @@ > Java_oracle_jrockit_jfr_VMJFR_getPeriod; > Java_oracle_jrockit_jfr_VMJFR_descriptors; > Java_oracle_jrockit_jfr_VMJFR_redefineClass0; > + Java_oracle_jrockit_jfr_VMJFR_retransformClasses0; > JNI_OnLoad; > local: > *; >>> >>
Re: 8034856/8034857: More gcc warnings
I'm inclined to agree with this. Since the code depends on a specific behavior of isspace which does not match what the system provided function does I too think it would be more robust to implement our own version of it. Cheers, Mikael > On Feb 16, 2014, at 14:20, Martin Buchholz wrote: > > Those locale-dependent APIs - more trouble than they're worth. More often > than not, you want a locale-independent version. > > So just define your own is_ASCII_space etc. like everybody else has done and > move on. > > >> On Sun, Feb 16, 2014 at 9:20 AM, Alan Bateman >> wrote: >>> On 13/02/2014 21:14, Mikael Vidstedt wrote: >>> : >>> >>> The change in question appears to come from >>> https://bugs.openjdk.java.net/browse/JDK-6679866, but I'm not sure the bug >>> gives enough additional information. My speculation (and it's really just a >>> speculation) is that it's not related to isspace per-se, but to something >>> else which gets defined/redefined/undefined by including ctype.h. I guess >>> it would be good to know if we have tests which cover the thing the comment >>> is alluding to (non-ascii in Premain-Class). >> Thanks for pointing this out. I looked at it again and the issue is that >> isspace is a macro and depends on the locale. By not including ctype.h then >> it means we get linked to the libc function instead. One approach is to >> include ctype.h and then #undef isspace, another is to define function >> prototype ourselves. I think the latter is a little bit better because it >> would avoid accidental usage of other local sensitive char classifiers. >> Attached is the patch that I propose. I have deliberate moved to to after >> other includes so we get a chance to #undef in the event that it gets >> included by something else. >> >> On tests then PremainClassTest.java is good enough to find this on Solaris. >> >> -Alan >> >> >> diff --git a/src/share/instrument/JarFacade.c >> b/src/share/instrument/JarFacade.c >> --- a/src/share/instrument/JarFacade.c >> +++ b/src/share/instrument/JarFacade.c >> @@ -23,17 +23,20 @@ >> * questions. >> */ >> >> -#ifdef _WIN32 >> -/* >> - * Win* needs this include. However, Linux and Solaris do not. >> - * Having this include on Solaris SPARC breaks having non US-ASCII >> - * characters in the value of the Premain-Class attribute. >> - */ >> -#include >> -#endif /* _WIN32 */ >> #include >> #include >> -#include >> + >> +/** >> + * ctype.h is required on Windows. For other platforms we use a function >> + * prototype to ensure that we use the libc isspace function rather than >> + * the isspace macro (due to isspace being locale sensitive) >> + */ >> +#ifdef _WIN32 >> + #include >> +#else >> + #undef isspace >> + extern int isspace(int c); >> +#endif /* _WIN32 */ >> >> #include "jni.h" >> #include "manifest_info.h" >
Re: RFR(XS): JDK-8034176 Update mapfile for libjfr
On 11/02/2014 10:01 PM, Staffan Larsen wrote: On 11 feb 2014, at 12:14, Erik Joelsson wrote: Looks good, but I can't help but wonder why the mapfile for libjfr is in the open. Yes, that is unfortunate. It used to be in closed source in jdk7, but moved into open with the new build system. That should be fixed. Looks like we need a make/closed/lib/Serviceability.gmk that contains the BUILD_LIBJFR definition. David /Staffan /Erik On 2014-02-11 12:00, Staffan Larsen wrote: Resending (email to build-dev bounced since I used the wrong sender address). On 11 feb 2014, at 11:18, staf...@larsen.se wrote: Please review this small fix for libjfr/mapfile-vers. Thanks, /Staffan diff --git a/make/mapfiles/libjfr/mapfile-vers b/make/mapfiles/libjfr/mapfile-vers --- a/make/mapfiles/libjfr/mapfile-vers +++ b/make/mapfiles/libjfr/mapfile-vers @@ -34,6 +34,7 @@ Java_oracle_jrockit_jfr_VMJFR_getPeriod; Java_oracle_jrockit_jfr_VMJFR_descriptors; Java_oracle_jrockit_jfr_VMJFR_redefineClass0; + Java_oracle_jrockit_jfr_VMJFR_retransformClasses0; JNI_OnLoad; local: *;
Re: 8034856/8034857: More gcc warnings
On 13/02/2014 21:14, Mikael Vidstedt wrote: : The change in question appears to come from https://bugs.openjdk.java.net/browse/JDK-6679866, but I'm not sure the bug gives enough additional information. My speculation (and it's really just a speculation) is that it's not related to isspace per-se, but to something else which gets defined/redefined/undefined by including ctype.h. I guess it would be good to know if we have tests which cover the thing the comment is alluding to (non-ascii in Premain-Class). Thanks for pointing this out. I looked at it again and the issue is that isspace is a macro and depends on the locale. By not including ctype.h then it means we get linked to the libc function instead. One approach is to include ctype.h and then #undef isspace, another is to define function prototype ourselves. I think the latter is a little bit better because it would avoid accidental usage of other local sensitive char classifiers. Attached is the patch that I propose. I have deliberate moved to to after other includes so we get a chance to #undef in the event that it gets included by something else. On tests then PremainClassTest.java is good enough to find this on Solaris. -Alan diff --git a/src/share/instrument/JarFacade.c b/src/share/instrument/JarFacade.c --- a/src/share/instrument/JarFacade.c +++ b/src/share/instrument/JarFacade.c @@ -23,17 +23,20 @@ * questions. */ -#ifdef _WIN32 -/* - * Win* needs this include. However, Linux and Solaris do not. - * Having this include on Solaris SPARC breaks having non US-ASCII - * characters in the value of the Premain-Class attribute. - */ -#include -#endif /* _WIN32 */ #include #include -#include + +/** + * ctype.h is required on Windows. For other platforms we use a function + * prototype to ensure that we use the libc isspace function rather than + * the isspace macro (due to isspace being locale sensitive) + */ +#ifdef _WIN32 + #include +#else + #undef isspace + extern int isspace(int c); +#endif /* _WIN32 */ #include "jni.h" #include "manifest_info.h"
