Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Serguei, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/ I refactored the debugInit_exit function. Now we have separate exit code for transport error and better readable code. -Dmitry On 2014-08-27 00:22, serguei.spit...@oracle.com wrote: Dmitry, It makes sense to consider a special exit code for transport init error. Other than that it looks good. No need to re-review. Thanks, Serguei On 8/26/14 10:46 AM, Dmitry Samersoff wrote: Serguei, exit_code set to 1 at ll. 1288 I thought of refactoring this code for better readability but finally decide to keep changes minimal. -Dmitry On 2014-08-26 21:35, serguei.spit...@oracle.com wrote: Dmitry, This looks good in general. But what error code will be returned in the case of AGENT_ERROR_TRANSPORT_INIT ? Is it Ok to return whatever exit_code we already have or we better return some special error code? Probably, it is Ok to keep the comment at the line 1315 as it is. Thanks, Serguei On 8/26/14 7:47 AM, Dmitry Samersoff wrote: Staffan, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/ After couple of experiments I end up with simple fix - don't coredump on AGENT_ERROR_TRANSPORT_INIT Current code don't propagate transport error to upper level, so if we need fine grained control I'll rewrite transport initialization. -Dmitry On 2014-08-25 15:53, Staffan Larsen wrote: On 25 aug 2014, at 13:33, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Staffan, On 2014-08-25 15:26, Staffan Larsen wrote: Dmitry, One problem with this fix is that debugInit_exit() is used from many places in the JDWP code. For some of these I think it still does make sense to receive a core dump for analysis. I agree that when parsing options, we do not need a core dump. jdwp has explicit option to request a coredump at debugInit_exit(). Is it enough or I should create a more sophisticated fix ? I don’t think that is enough. Often a problem will happen and will not be reproducible. Maybe this code: if ((arg.error != JDWP_ERROR(NONE)) (arg.startCount == 0) initOnStartup) { EXIT_ERROR(map2jvmtiError(arg.error), No transports initialized); } can use some variant of EXIT_ERROR that does not call fatalError() ? /Staffan Actually, I don't think that coredump on every call to jni_FatalError() (see jni.cpp:726) is necessary but this hotspot code is here for 6 years and changing it probably out of scope of this fix. -Dmitry /Staffan On 20 aug 2014, at 17:55, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Serguei, After some additional testing I changed the fix a bit. Please take a look at new version. http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ New version reports JVMTI error to stderr. Also jniFatalError is not referenced any more so I remove it. -Dmitry On 2014-08-20 12:08, serguei.spit...@oracle.com wrote: Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return non-zero exit code as expected but will not dump the core. -Dmitry On 2014-08-20 00:54, serguei.spit...@oracle.com wrote: Hi Dmitry, The fix seems to be Ok. Just want to make it clear... This fix just changes the bug pattern. It a case of incorrect transport parameters the test is still going to fail but without crash, right? Thanks, Serguei On 8/19/14 12:09 PM, Dmitry Samersoff wrote: Hi Everybody, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/ JDWP call jniFatalError if transport can't be initialized (e.g. wrong parameters) and jniFatalError call os::abort(). Therefor all transport initialization errors cause vm to coredump. I see no reason for debugInit_exit to call jniFatalError so remove this code. -Dmitry -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
This looks good. Could you, please, remove extra spaces in the following conditions ?: 1291 if (error != JVMTI_ERROR_NONE docoredump ) { 1302 if ( gdata-jvmti != NULL ) { 1309 if ( error == JVMTI_ERROR_NONE ) { Thanks, Serguei On 8/27/14 12:36 AM, Dmitry Samersoff wrote: Serguei, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/ I refactored the debugInit_exit function. Now we have separate exit code for transport error and better readable code. -Dmitry On 2014-08-27 00:22, serguei.spit...@oracle.com wrote: Dmitry, It makes sense to consider a special exit code for transport init error. Other than that it looks good. No need to re-review. Thanks, Serguei On 8/26/14 10:46 AM, Dmitry Samersoff wrote: Serguei, exit_code set to 1 at ll. 1288 I thought of refactoring this code for better readability but finally decide to keep changes minimal. -Dmitry On 2014-08-26 21:35, serguei.spit...@oracle.com wrote: Dmitry, This looks good in general. But what error code will be returned in the case of AGENT_ERROR_TRANSPORT_INIT ? Is it Ok to return whatever exit_code we already have or we better return some special error code? Probably, it is Ok to keep the comment at the line 1315 as it is. Thanks, Serguei On 8/26/14 7:47 AM, Dmitry Samersoff wrote: Staffan, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/ After couple of experiments I end up with simple fix - don't coredump on AGENT_ERROR_TRANSPORT_INIT Current code don't propagate transport error to upper level, so if we need fine grained control I'll rewrite transport initialization. -Dmitry On 2014-08-25 15:53, Staffan Larsen wrote: On 25 aug 2014, at 13:33, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Staffan, On 2014-08-25 15:26, Staffan Larsen wrote: Dmitry, One problem with this fix is that debugInit_exit() is used from many places in the JDWP code. For some of these I think it still does make sense to receive a core dump for analysis. I agree that when parsing options, we do not need a core dump. jdwp has explicit option to request a coredump at debugInit_exit(). Is it enough or I should create a more sophisticated fix ? I don’t think that is enough. Often a problem will happen and will not be reproducible. Maybe this code: if ((arg.error != JDWP_ERROR(NONE)) (arg.startCount == 0) initOnStartup) { EXIT_ERROR(map2jvmtiError(arg.error), No transports initialized); } can use some variant of EXIT_ERROR that does not call fatalError() ? /Staffan Actually, I don't think that coredump on every call to jni_FatalError() (see jni.cpp:726) is necessary but this hotspot code is here for 6 years and changing it probably out of scope of this fix. -Dmitry /Staffan On 20 aug 2014, at 17:55, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Serguei, After some additional testing I changed the fix a bit. Please take a look at new version. http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ New version reports JVMTI error to stderr. Also jniFatalError is not referenced any more so I remove it. -Dmitry On 2014-08-20 12:08, serguei.spit...@oracle.com wrote: Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return non-zero exit code as expected but will not dump the core. -Dmitry On 2014-08-20 00:54, serguei.spit...@oracle.com wrote: Hi Dmitry, The fix seems to be Ok. Just want to make it clear... This fix just changes the bug pattern. It a case of incorrect transport parameters the test is still going to fail but without crash, right? Thanks, Serguei On 8/19/14 12:09 PM, Dmitry Samersoff wrote: Hi Everybody, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/ JDWP call jniFatalError if transport can't be initialized (e.g. wrong parameters) and jniFatalError call os::abort(). Therefor all transport initialization errors cause vm to coredump. I see no reason for debugInit_exit to call jniFatalError so remove this code. -Dmitry -- Dmitry Samersoff Oracle Java
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
1284 enum exit_codes { EXIT_NO_ERRORS = 0, EXIT_TRANSPORT_ERROR, EXIT_JVMTI_ERROR }; I'd suggest to swap the positions of EXIT_TRANSPORT_ERROR and EXIT_JVMTI_ERROR in the enum to keep this as compatible to previous behavior as possible. Thanks, Serguei On 8/27/14 1:08 AM, serguei.spit...@oracle.com wrote: This looks good. Could you, please, remove extra spaces in the following conditions ?: 1291 if (error != JVMTI_ERROR_NONE docoredump ) { 1302 if ( gdata-jvmti != NULL ) { 1309 if ( error == JVMTI_ERROR_NONE ) { Thanks, Serguei On 8/27/14 12:36 AM, Dmitry Samersoff wrote: Serguei, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/ I refactored the debugInit_exit function. Now we have separate exit code for transport error and better readable code. -Dmitry On 2014-08-27 00:22,serguei.spit...@oracle.com wrote: Dmitry, It makes sense to consider a special exit code for transport init error. Other than that it looks good. No need to re-review. Thanks, Serguei On 8/26/14 10:46 AM, Dmitry Samersoff wrote: Serguei, exit_code set to 1 at ll. 1288 I thought of refactoring this code for better readability but finally decide to keep changes minimal. -Dmitry On 2014-08-26 21:35,serguei.spit...@oracle.com wrote: Dmitry, This looks good in general. But what error code will be returned in the case of AGENT_ERROR_TRANSPORT_INIT ? Is it Ok to return whatever exit_code we already have or we better return some special error code? Probably, it is Ok to keep the comment at the line 1315 as it is. Thanks, Serguei On 8/26/14 7:47 AM, Dmitry Samersoff wrote: Staffan, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/ After couple of experiments I end up with simple fix - don't coredump on AGENT_ERROR_TRANSPORT_INIT Current code don't propagate transport error to upper level, so if we need fine grained control I'll rewrite transport initialization. -Dmitry On 2014-08-25 15:53, Staffan Larsen wrote: On 25 aug 2014, at 13:33, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Staffan, On 2014-08-25 15:26, Staffan Larsen wrote: Dmitry, One problem with this fix is that debugInit_exit() is used from many places in the JDWP code. For some of these I think it still does make sense to receive a core dump for analysis. I agree that when parsing options, we do not need a core dump. jdwp has explicit option to request a coredump at debugInit_exit(). Is it enough or I should create a more sophisticated fix ? I don’t think that is enough. Often a problem will happen and will not be reproducible. Maybe this code: if ((arg.error != JDWP_ERROR(NONE)) (arg.startCount == 0) initOnStartup) { EXIT_ERROR(map2jvmtiError(arg.error), No transports initialized); } can use some variant of EXIT_ERROR that does not call fatalError() ? /Staffan Actually, I don't think that coredump on every call to jni_FatalError() (see jni.cpp:726) is necessary but this hotspot code is here for 6 years and changing it probably out of scope of this fix. -Dmitry /Staffan On 20 aug 2014, at 17:55, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Serguei, After some additional testing I changed the fix a bit. Please take a look at new version. http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ New version reports JVMTI error to stderr. Also jniFatalError is not referenced any more so I remove it. -Dmitry On 2014-08-20 12:08,serguei.spit...@oracle.com wrote: Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return non-zero exit code as expected but will not dump the core. -Dmitry On 2014-08-20 00:54,serguei.spit...@oracle.com wrote: Hi Dmitry, The fix seems to be Ok. Just want to make it clear... This fix just changes the bug pattern. It a case of incorrect transport parameters the test is still going to fail but without crash, right? Thanks, Serguei On 8/19/14 12:09 PM, Dmitry Samersoff wrote: Hi Everybody, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Serguei, Fixed (in-place, press shift reload) -Dmitry On 2014-08-27 12:12, serguei.spit...@oracle.com wrote: 1284 enum exit_codes { EXIT_NO_ERRORS = 0, EXIT_TRANSPORT_ERROR, EXIT_JVMTI_ERROR }; I'd suggest to swap the positions of EXIT_TRANSPORT_ERROR and EXIT_JVMTI_ERROR in the enum to keep this as compatible to previous behavior as possible. Thanks, Serguei On 8/27/14 1:08 AM, serguei.spit...@oracle.com wrote: This looks good. Could you, please, remove extra spaces in the following conditions ?: 1291 if (error != JVMTI_ERROR_NONE docoredump ) { 1302 if ( gdata-jvmti != NULL ) { 1309 if ( error == JVMTI_ERROR_NONE ) { Thanks, Serguei On 8/27/14 12:36 AM, Dmitry Samersoff wrote: Serguei, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/ I refactored the debugInit_exit function. Now we have separate exit code for transport error and better readable code. -Dmitry On 2014-08-27 00:22, serguei.spit...@oracle.com wrote: Dmitry, It makes sense to consider a special exit code for transport init error. Other than that it looks good. No need to re-review. Thanks, Serguei On 8/26/14 10:46 AM, Dmitry Samersoff wrote: Serguei, exit_code set to 1 at ll. 1288 I thought of refactoring this code for better readability but finally decide to keep changes minimal. -Dmitry On 2014-08-26 21:35, serguei.spit...@oracle.com wrote: Dmitry, This looks good in general. But what error code will be returned in the case of AGENT_ERROR_TRANSPORT_INIT ? Is it Ok to return whatever exit_code we already have or we better return some special error code? Probably, it is Ok to keep the comment at the line 1315 as it is. Thanks, Serguei On 8/26/14 7:47 AM, Dmitry Samersoff wrote: Staffan, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/ After couple of experiments I end up with simple fix - don't coredump on AGENT_ERROR_TRANSPORT_INIT Current code don't propagate transport error to upper level, so if we need fine grained control I'll rewrite transport initialization. -Dmitry On 2014-08-25 15:53, Staffan Larsen wrote: On 25 aug 2014, at 13:33, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Staffan, On 2014-08-25 15:26, Staffan Larsen wrote: Dmitry, One problem with this fix is that debugInit_exit() is used from many places in the JDWP code. For some of these I think it still does make sense to receive a core dump for analysis. I agree that when parsing options, we do not need a core dump. jdwp has explicit option to request a coredump at debugInit_exit(). Is it enough or I should create a more sophisticated fix ? I don’t think that is enough. Often a problem will happen and will not be reproducible. Maybe this code: if ((arg.error != JDWP_ERROR(NONE)) (arg.startCount == 0) initOnStartup) { EXIT_ERROR(map2jvmtiError(arg.error), No transports initialized); } can use some variant of EXIT_ERROR that does not call fatalError() ? /Staffan Actually, I don't think that coredump on every call to jni_FatalError() (see jni.cpp:726) is necessary but this hotspot code is here for 6 years and changing it probably out of scope of this fix. -Dmitry /Staffan On 20 aug 2014, at 17:55, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Serguei, After some additional testing I changed the fix a bit. Please take a look at new version. http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ New version reports JVMTI error to stderr. Also jniFatalError is not referenced any more so I remove it. -Dmitry On 2014-08-20 12:08, serguei.spit...@oracle.com wrote: Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return non-zero exit code as expected but will not dump the core. -Dmitry On 2014-08-20 00:54, serguei.spit...@oracle.com wrote: Hi Dmitry, The fix seems to be Ok. Just want to make it clear... This fix just changes the bug pattern. It a case of incorrect transport parameters the
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
One more minor comment. The indent in this file must be 4. Could you fix it? Thanks, Serguei On 8/27/14 2:03 AM, Dmitry Samersoff wrote: Serguei, Fixed (in-place, press shift reload) -Dmitry On 2014-08-27 12:12, serguei.spit...@oracle.com wrote: 1284 enum exit_codes { EXIT_NO_ERRORS = 0, EXIT_TRANSPORT_ERROR, EXIT_JVMTI_ERROR }; I'd suggest to swap the positions of EXIT_TRANSPORT_ERROR and EXIT_JVMTI_ERROR in the enum to keep this as compatible to previous behavior as possible. Thanks, Serguei On 8/27/14 1:08 AM, serguei.spit...@oracle.com wrote: This looks good. Could you, please, remove extra spaces in the following conditions ?: 1291 if (error != JVMTI_ERROR_NONE docoredump ) { 1302 if ( gdata-jvmti != NULL ) { 1309 if ( error == JVMTI_ERROR_NONE ) { Thanks, Serguei On 8/27/14 12:36 AM, Dmitry Samersoff wrote: Serguei, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/ I refactored the debugInit_exit function. Now we have separate exit code for transport error and better readable code. -Dmitry On 2014-08-27 00:22, serguei.spit...@oracle.com wrote: Dmitry, It makes sense to consider a special exit code for transport init error. Other than that it looks good. No need to re-review. Thanks, Serguei On 8/26/14 10:46 AM, Dmitry Samersoff wrote: Serguei, exit_code set to 1 at ll. 1288 I thought of refactoring this code for better readability but finally decide to keep changes minimal. -Dmitry On 2014-08-26 21:35, serguei.spit...@oracle.com wrote: Dmitry, This looks good in general. But what error code will be returned in the case of AGENT_ERROR_TRANSPORT_INIT ? Is it Ok to return whatever exit_code we already have or we better return some special error code? Probably, it is Ok to keep the comment at the line 1315 as it is. Thanks, Serguei On 8/26/14 7:47 AM, Dmitry Samersoff wrote: Staffan, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/ After couple of experiments I end up with simple fix - don't coredump on AGENT_ERROR_TRANSPORT_INIT Current code don't propagate transport error to upper level, so if we need fine grained control I'll rewrite transport initialization. -Dmitry On 2014-08-25 15:53, Staffan Larsen wrote: On 25 aug 2014, at 13:33, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Staffan, On 2014-08-25 15:26, Staffan Larsen wrote: Dmitry, One problem with this fix is that debugInit_exit() is used from many places in the JDWP code. For some of these I think it still does make sense to receive a core dump for analysis. I agree that when parsing options, we do not need a core dump. jdwp has explicit option to request a coredump at debugInit_exit(). Is it enough or I should create a more sophisticated fix ? I don’t think that is enough. Often a problem will happen and will not be reproducible. Maybe this code: if ((arg.error != JDWP_ERROR(NONE)) (arg.startCount == 0) initOnStartup) { EXIT_ERROR(map2jvmtiError(arg.error), No transports initialized); } can use some variant of EXIT_ERROR that does not call fatalError() ? /Staffan Actually, I don't think that coredump on every call to jni_FatalError() (see jni.cpp:726) is necessary but this hotspot code is here for 6 years and changing it probably out of scope of this fix. -Dmitry /Staffan On 20 aug 2014, at 17:55, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Serguei, After some additional testing I changed the fix a bit. Please take a look at new version. http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ New version reports JVMTI error to stderr. Also jniFatalError is not referenced any more so I remove it. -Dmitry On 2014-08-20 12:08, serguei.spit...@oracle.com wrote: Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return non-zero exit code as expected but will not dump the core. -Dmitry On 2014-08-20 00:54, serguei.spit...@oracle.com wrote: Hi Dmitry, The fix seems to be Ok. Just want to make it clear... This fix just changes the bug pattern. It a case of
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Serguei, Fixed (in-place, press shift reload) -Dmitry On 2014-08-27 14:25, serguei.spit...@oracle.com wrote: One more minor comment. The indent in this file must be 4. Could you fix it? Thanks, Serguei On 8/27/14 2:03 AM, Dmitry Samersoff wrote: Serguei, Fixed (in-place, press shift reload) -Dmitry On 2014-08-27 12:12, serguei.spit...@oracle.com wrote: 1284 enum exit_codes { EXIT_NO_ERRORS = 0, EXIT_TRANSPORT_ERROR, EXIT_JVMTI_ERROR }; I'd suggest to swap the positions of EXIT_TRANSPORT_ERROR and EXIT_JVMTI_ERROR in the enum to keep this as compatible to previous behavior as possible. Thanks, Serguei On 8/27/14 1:08 AM, serguei.spit...@oracle.com wrote: This looks good. Could you, please, remove extra spaces in the following conditions ?: 1291 if (error != JVMTI_ERROR_NONE docoredump ) { 1302 if ( gdata-jvmti != NULL ) { 1309 if ( error == JVMTI_ERROR_NONE ) { Thanks, Serguei On 8/27/14 12:36 AM, Dmitry Samersoff wrote: Serguei, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/ I refactored the debugInit_exit function. Now we have separate exit code for transport error and better readable code. -Dmitry On 2014-08-27 00:22, serguei.spit...@oracle.com wrote: Dmitry, It makes sense to consider a special exit code for transport init error. Other than that it looks good. No need to re-review. Thanks, Serguei On 8/26/14 10:46 AM, Dmitry Samersoff wrote: Serguei, exit_code set to 1 at ll. 1288 I thought of refactoring this code for better readability but finally decide to keep changes minimal. -Dmitry On 2014-08-26 21:35, serguei.spit...@oracle.com wrote: Dmitry, This looks good in general. But what error code will be returned in the case of AGENT_ERROR_TRANSPORT_INIT ? Is it Ok to return whatever exit_code we already have or we better return some special error code? Probably, it is Ok to keep the comment at the line 1315 as it is. Thanks, Serguei On 8/26/14 7:47 AM, Dmitry Samersoff wrote: Staffan, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/ After couple of experiments I end up with simple fix - don't coredump on AGENT_ERROR_TRANSPORT_INIT Current code don't propagate transport error to upper level, so if we need fine grained control I'll rewrite transport initialization. -Dmitry On 2014-08-25 15:53, Staffan Larsen wrote: On 25 aug 2014, at 13:33, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Staffan, On 2014-08-25 15:26, Staffan Larsen wrote: Dmitry, One problem with this fix is that debugInit_exit() is used from many places in the JDWP code. For some of these I think it still does make sense to receive a core dump for analysis. I agree that when parsing options, we do not need a core dump. jdwp has explicit option to request a coredump at debugInit_exit(). Is it enough or I should create a more sophisticated fix ? I don’t think that is enough. Often a problem will happen and will not be reproducible. Maybe this code: if ((arg.error != JDWP_ERROR(NONE)) (arg.startCount == 0) initOnStartup) { EXIT_ERROR(map2jvmtiError(arg.error), No transports initialized); } can use some variant of EXIT_ERROR that does not call fatalError() ? /Staffan Actually, I don't think that coredump on every call to jni_FatalError() (see jni.cpp:726) is necessary but this hotspot code is here for 6 years and changing it probably out of scope of this fix. -Dmitry /Staffan On 20 aug 2014, at 17:55, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Serguei, After some additional testing I changed the fix a bit. Please take a look at new version. http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ New version reports JVMTI error to stderr. Also jniFatalError is not referenced any more so I remove it. -Dmitry On 2014-08-20 12:08, serguei.spit...@oracle.com wrote: Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Thanks! Serguei On 8/27/14 3:51 AM, Dmitry Samersoff wrote: Serguei, Fixed (in-place, press shift reload) -Dmitry On 2014-08-27 14:25, serguei.spit...@oracle.com wrote: One more minor comment. The indent in this file must be 4. Could you fix it? Thanks, Serguei On 8/27/14 2:03 AM, Dmitry Samersoff wrote: Serguei, Fixed (in-place, press shift reload) -Dmitry On 2014-08-27 12:12, serguei.spit...@oracle.com wrote: 1284 enum exit_codes { EXIT_NO_ERRORS = 0, EXIT_TRANSPORT_ERROR, EXIT_JVMTI_ERROR }; I'd suggest to swap the positions of EXIT_TRANSPORT_ERROR and EXIT_JVMTI_ERROR in the enum to keep this as compatible to previous behavior as possible. Thanks, Serguei On 8/27/14 1:08 AM, serguei.spit...@oracle.com wrote: This looks good. Could you, please, remove extra spaces in the following conditions ?: 1291 if (error != JVMTI_ERROR_NONE docoredump ) { 1302 if ( gdata-jvmti != NULL ) { 1309 if ( error == JVMTI_ERROR_NONE ) { Thanks, Serguei On 8/27/14 12:36 AM, Dmitry Samersoff wrote: Serguei, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/ I refactored the debugInit_exit function. Now we have separate exit code for transport error and better readable code. -Dmitry On 2014-08-27 00:22, serguei.spit...@oracle.com wrote: Dmitry, It makes sense to consider a special exit code for transport init error. Other than that it looks good. No need to re-review. Thanks, Serguei On 8/26/14 10:46 AM, Dmitry Samersoff wrote: Serguei, exit_code set to 1 at ll. 1288 I thought of refactoring this code for better readability but finally decide to keep changes minimal. -Dmitry On 2014-08-26 21:35, serguei.spit...@oracle.com wrote: Dmitry, This looks good in general. But what error code will be returned in the case of AGENT_ERROR_TRANSPORT_INIT ? Is it Ok to return whatever exit_code we already have or we better return some special error code? Probably, it is Ok to keep the comment at the line 1315 as it is. Thanks, Serguei On 8/26/14 7:47 AM, Dmitry Samersoff wrote: Staffan, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/ After couple of experiments I end up with simple fix - don't coredump on AGENT_ERROR_TRANSPORT_INIT Current code don't propagate transport error to upper level, so if we need fine grained control I'll rewrite transport initialization. -Dmitry On 2014-08-25 15:53, Staffan Larsen wrote: On 25 aug 2014, at 13:33, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Staffan, On 2014-08-25 15:26, Staffan Larsen wrote: Dmitry, One problem with this fix is that debugInit_exit() is used from many places in the JDWP code. For some of these I think it still does make sense to receive a core dump for analysis. I agree that when parsing options, we do not need a core dump. jdwp has explicit option to request a coredump at debugInit_exit(). Is it enough or I should create a more sophisticated fix ? I don’t think that is enough. Often a problem will happen and will not be reproducible. Maybe this code: if ((arg.error != JDWP_ERROR(NONE)) (arg.startCount == 0) initOnStartup) { EXIT_ERROR(map2jvmtiError(arg.error), No transports initialized); } can use some variant of EXIT_ERROR that does not call fatalError() ? /Staffan Actually, I don't think that coredump on every call to jni_FatalError() (see jni.cpp:726) is necessary but this hotspot code is here for 6 years and changing it probably out of scope of this fix. -Dmitry /Staffan On 20 aug 2014, at 17:55, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Serguei, After some additional testing I changed the fix a bit. Please take a look at new version. http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ New version reports JVMTI error to stderr. Also jniFatalError is not referenced any more so I remove it. -Dmitry On 2014-08-20 12:08, serguei.spit...@oracle.com wrote: Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return non-zero exit code as expected but will not dump the core.
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
This version looks good. Reviewed. /Staffan On 27 aug 2014, at 12:51, Dmitry Samersoff dmitry.samers...@oracle.com wrote: Serguei, Fixed (in-place, press shift reload) -Dmitry On 2014-08-27 14:25, serguei.spit...@oracle.com wrote: One more minor comment. The indent in this file must be 4. Could you fix it? Thanks, Serguei On 8/27/14 2:03 AM, Dmitry Samersoff wrote: Serguei, Fixed (in-place, press shift reload) -Dmitry On 2014-08-27 12:12, serguei.spit...@oracle.com wrote: 1284 enum exit_codes { EXIT_NO_ERRORS = 0, EXIT_TRANSPORT_ERROR, EXIT_JVMTI_ERROR }; I'd suggest to swap the positions of EXIT_TRANSPORT_ERROR and EXIT_JVMTI_ERROR in the enum to keep this as compatible to previous behavior as possible. Thanks, Serguei On 8/27/14 1:08 AM, serguei.spit...@oracle.com wrote: This looks good. Could you, please, remove extra spaces in the following conditions ?: 1291 if (error != JVMTI_ERROR_NONE docoredump ) { 1302 if ( gdata-jvmti != NULL ) { 1309 if ( error == JVMTI_ERROR_NONE ) { Thanks, Serguei On 8/27/14 12:36 AM, Dmitry Samersoff wrote: Serguei, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/ I refactored the debugInit_exit function. Now we have separate exit code for transport error and better readable code. -Dmitry On 2014-08-27 00:22, serguei.spit...@oracle.com wrote: Dmitry, It makes sense to consider a special exit code for transport init error. Other than that it looks good. No need to re-review. Thanks, Serguei On 8/26/14 10:46 AM, Dmitry Samersoff wrote: Serguei, exit_code set to 1 at ll. 1288 I thought of refactoring this code for better readability but finally decide to keep changes minimal. -Dmitry On 2014-08-26 21:35, serguei.spit...@oracle.com wrote: Dmitry, This looks good in general. But what error code will be returned in the case of AGENT_ERROR_TRANSPORT_INIT ? Is it Ok to return whatever exit_code we already have or we better return some special error code? Probably, it is Ok to keep the comment at the line 1315 as it is. Thanks, Serguei On 8/26/14 7:47 AM, Dmitry Samersoff wrote: Staffan, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/ After couple of experiments I end up with simple fix - don't coredump on AGENT_ERROR_TRANSPORT_INIT Current code don't propagate transport error to upper level, so if we need fine grained control I'll rewrite transport initialization. -Dmitry On 2014-08-25 15:53, Staffan Larsen wrote: On 25 aug 2014, at 13:33, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Staffan, On 2014-08-25 15:26, Staffan Larsen wrote: Dmitry, One problem with this fix is that debugInit_exit() is used from many places in the JDWP code. For some of these I think it still does make sense to receive a core dump for analysis. I agree that when parsing options, we do not need a core dump. jdwp has explicit option to request a coredump at debugInit_exit(). Is it enough or I should create a more sophisticated fix ? I don’t think that is enough. Often a problem will happen and will not be reproducible. Maybe this code: if ((arg.error != JDWP_ERROR(NONE)) (arg.startCount == 0) initOnStartup) { EXIT_ERROR(map2jvmtiError(arg.error), No transports initialized); } can use some variant of EXIT_ERROR that does not call fatalError() ? /Staffan Actually, I don't think that coredump on every call to jni_FatalError() (see jni.cpp:726) is necessary but this hotspot code is here for 6 years and changing it probably out of scope of this fix. -Dmitry /Staffan On 20 aug 2014, at 17:55, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Serguei, After some additional testing I changed the fix a bit. Please take a look at new version. http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ New version reports JVMTI error to stderr. Also jniFatalError is not referenced any more so I remove it. -Dmitry On 2014-08-20 12:08, serguei.spit...@oracle.com wrote: Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Staffan, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/ After couple of experiments I end up with simple fix - don't coredump on AGENT_ERROR_TRANSPORT_INIT Current code don't propagate transport error to upper level, so if we need fine grained control I'll rewrite transport initialization. -Dmitry On 2014-08-25 15:53, Staffan Larsen wrote: On 25 aug 2014, at 13:33, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Staffan, On 2014-08-25 15:26, Staffan Larsen wrote: Dmitry, One problem with this fix is that debugInit_exit() is used from many places in the JDWP code. For some of these I think it still does make sense to receive a core dump for analysis. I agree that when parsing options, we do not need a core dump. jdwp has explicit option to request a coredump at debugInit_exit(). Is it enough or I should create a more sophisticated fix ? I don’t think that is enough. Often a problem will happen and will not be reproducible. Maybe this code: if ((arg.error != JDWP_ERROR(NONE)) (arg.startCount == 0) initOnStartup) { EXIT_ERROR(map2jvmtiError(arg.error), No transports initialized); } can use some variant of EXIT_ERROR that does not call fatalError() ? /Staffan Actually, I don't think that coredump on every call to jni_FatalError() (see jni.cpp:726) is necessary but this hotspot code is here for 6 years and changing it probably out of scope of this fix. -Dmitry /Staffan On 20 aug 2014, at 17:55, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Serguei, After some additional testing I changed the fix a bit. Please take a look at new version. http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ New version reports JVMTI error to stderr. Also jniFatalError is not referenced any more so I remove it. -Dmitry On 2014-08-20 12:08, serguei.spit...@oracle.com wrote: Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return non-zero exit code as expected but will not dump the core. -Dmitry On 2014-08-20 00:54, serguei.spit...@oracle.com wrote: Hi Dmitry, The fix seems to be Ok. Just want to make it clear... This fix just changes the bug pattern. It a case of incorrect transport parameters the test is still going to fail but without crash, right? Thanks, Serguei On 8/19/14 12:09 PM, Dmitry Samersoff wrote: Hi Everybody, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/ JDWP call jniFatalError if transport can't be initialized (e.g. wrong parameters) and jniFatalError call os::abort(). Therefor all transport initialization errors cause vm to coredump. I see no reason for debugInit_exit to call jniFatalError so remove this code. -Dmitry -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Dmitry, This looks good in general. But what error code will be returned in the case of AGENT_ERROR_TRANSPORT_INIT ? Is it Ok to return whatever exit_code we already have or we better return some special error code? Probably, it is Ok to keep the comment at the line 1315 as it is. Thanks, Serguei On 8/26/14 7:47 AM, Dmitry Samersoff wrote: Staffan, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/ After couple of experiments I end up with simple fix - don't coredump on AGENT_ERROR_TRANSPORT_INIT Current code don't propagate transport error to upper level, so if we need fine grained control I'll rewrite transport initialization. -Dmitry On 2014-08-25 15:53, Staffan Larsen wrote: On 25 aug 2014, at 13:33, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Staffan, On 2014-08-25 15:26, Staffan Larsen wrote: Dmitry, One problem with this fix is that debugInit_exit() is used from many places in the JDWP code. For some of these I think it still does make sense to receive a core dump for analysis. I agree that when parsing options, we do not need a core dump. jdwp has explicit option to request a coredump at debugInit_exit(). Is it enough or I should create a more sophisticated fix ? I don’t think that is enough. Often a problem will happen and will not be reproducible. Maybe this code: if ((arg.error != JDWP_ERROR(NONE)) (arg.startCount == 0) initOnStartup) { EXIT_ERROR(map2jvmtiError(arg.error), No transports initialized); } can use some variant of EXIT_ERROR that does not call fatalError() ? /Staffan Actually, I don't think that coredump on every call to jni_FatalError() (see jni.cpp:726) is necessary but this hotspot code is here for 6 years and changing it probably out of scope of this fix. -Dmitry /Staffan On 20 aug 2014, at 17:55, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Serguei, After some additional testing I changed the fix a bit. Please take a look at new version. http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ New version reports JVMTI error to stderr. Also jniFatalError is not referenced any more so I remove it. -Dmitry On 2014-08-20 12:08, serguei.spit...@oracle.com wrote: Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return non-zero exit code as expected but will not dump the core. -Dmitry On 2014-08-20 00:54, serguei.spit...@oracle.com wrote: Hi Dmitry, The fix seems to be Ok. Just want to make it clear... This fix just changes the bug pattern. It a case of incorrect transport parameters the test is still going to fail but without crash, right? Thanks, Serguei On 8/19/14 12:09 PM, Dmitry Samersoff wrote: Hi Everybody, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/ JDWP call jniFatalError if transport can't be initialized (e.g. wrong parameters) and jniFatalError call os::abort(). Therefor all transport initialization errors cause vm to coredump. I see no reason for debugInit_exit to call jniFatalError so remove this code. -Dmitry -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Serguei, exit_code set to 1 at ll. 1288 I thought of refactoring this code for better readability but finally decide to keep changes minimal. -Dmitry On 2014-08-26 21:35, serguei.spit...@oracle.com wrote: Dmitry, This looks good in general. But what error code will be returned in the case of AGENT_ERROR_TRANSPORT_INIT ? Is it Ok to return whatever exit_code we already have or we better return some special error code? Probably, it is Ok to keep the comment at the line 1315 as it is. Thanks, Serguei On 8/26/14 7:47 AM, Dmitry Samersoff wrote: Staffan, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/ After couple of experiments I end up with simple fix - don't coredump on AGENT_ERROR_TRANSPORT_INIT Current code don't propagate transport error to upper level, so if we need fine grained control I'll rewrite transport initialization. -Dmitry On 2014-08-25 15:53, Staffan Larsen wrote: On 25 aug 2014, at 13:33, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Staffan, On 2014-08-25 15:26, Staffan Larsen wrote: Dmitry, One problem with this fix is that debugInit_exit() is used from many places in the JDWP code. For some of these I think it still does make sense to receive a core dump for analysis. I agree that when parsing options, we do not need a core dump. jdwp has explicit option to request a coredump at debugInit_exit(). Is it enough or I should create a more sophisticated fix ? I don’t think that is enough. Often a problem will happen and will not be reproducible. Maybe this code: if ((arg.error != JDWP_ERROR(NONE)) (arg.startCount == 0) initOnStartup) { EXIT_ERROR(map2jvmtiError(arg.error), No transports initialized); } can use some variant of EXIT_ERROR that does not call fatalError() ? /Staffan Actually, I don't think that coredump on every call to jni_FatalError() (see jni.cpp:726) is necessary but this hotspot code is here for 6 years and changing it probably out of scope of this fix. -Dmitry /Staffan On 20 aug 2014, at 17:55, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Serguei, After some additional testing I changed the fix a bit. Please take a look at new version. http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ New version reports JVMTI error to stderr. Also jniFatalError is not referenced any more so I remove it. -Dmitry On 2014-08-20 12:08, serguei.spit...@oracle.com wrote: Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return non-zero exit code as expected but will not dump the core. -Dmitry On 2014-08-20 00:54, serguei.spit...@oracle.com wrote: Hi Dmitry, The fix seems to be Ok. Just want to make it clear... This fix just changes the bug pattern. It a case of incorrect transport parameters the test is still going to fail but without crash, right? Thanks, Serguei On 8/19/14 12:09 PM, Dmitry Samersoff wrote: Hi Everybody, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/ JDWP call jniFatalError if transport can't be initialized (e.g. wrong parameters) and jniFatalError call os::abort(). Therefor all transport initialization errors cause vm to coredump. I see no reason for debugInit_exit to call jniFatalError so remove this code. -Dmitry -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Dmitry, One problem with this fix is that debugInit_exit() is used from many places in the JDWP code. For some of these I think it still does make sense to receive a core dump for analysis. I agree that when parsing options, we do not need a core dump. /Staffan On 20 aug 2014, at 17:55, Dmitry Samersoff dmitry.samers...@oracle.com wrote: Serguei, After some additional testing I changed the fix a bit. Please take a look at new version. http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ New version reports JVMTI error to stderr. Also jniFatalError is not referenced any more so I remove it. -Dmitry On 2014-08-20 12:08, serguei.spit...@oracle.com wrote: Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return non-zero exit code as expected but will not dump the core. -Dmitry On 2014-08-20 00:54, serguei.spit...@oracle.com wrote: Hi Dmitry, The fix seems to be Ok. Just want to make it clear... This fix just changes the bug pattern. It a case of incorrect transport parameters the test is still going to fail but without crash, right? Thanks, Serguei On 8/19/14 12:09 PM, Dmitry Samersoff wrote: Hi Everybody, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/ JDWP call jniFatalError if transport can't be initialized (e.g. wrong parameters) and jniFatalError call os::abort(). Therefor all transport initialization errors cause vm to coredump. I see no reason for debugInit_exit to call jniFatalError so remove this code. -Dmitry -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
This is a valid concern. Sorry, I did not catch it. Thanks, Serguei On 8/25/14 4:26 AM, Staffan Larsen wrote: Dmitry, One problem with this fix is that debugInit_exit() is used from many places in the JDWP code. For some of these I think it still does make sense to receive a core dump for analysis. I agree that when parsing options, we do not need a core dump. /Staffan On 20 aug 2014, at 17:55, Dmitry Samersoff dmitry.samers...@oracle.com wrote: Serguei, After some additional testing I changed the fix a bit. Please take a look at new version. http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ New version reports JVMTI error to stderr. Also jniFatalError is not referenced any more so I remove it. -Dmitry On 2014-08-20 12:08, serguei.spit...@oracle.com wrote: Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return non-zero exit code as expected but will not dump the core. -Dmitry On 2014-08-20 00:54, serguei.spit...@oracle.com wrote: Hi Dmitry, The fix seems to be Ok. Just want to make it clear... This fix just changes the bug pattern. It a case of incorrect transport parameters the test is still going to fail but without crash, right? Thanks, Serguei On 8/19/14 12:09 PM, Dmitry Samersoff wrote: Hi Everybody, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/ JDWP call jniFatalError if transport can't be initialized (e.g. wrong parameters) and jniFatalError call os::abort(). Therefor all transport initialization errors cause vm to coredump. I see no reason for debugInit_exit to call jniFatalError so remove this code. -Dmitry -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
On 25 aug 2014, at 13:33, Dmitry Samersoff dmitry.samers...@oracle.com wrote: Staffan, On 2014-08-25 15:26, Staffan Larsen wrote: Dmitry, One problem with this fix is that debugInit_exit() is used from many places in the JDWP code. For some of these I think it still does make sense to receive a core dump for analysis. I agree that when parsing options, we do not need a core dump. jdwp has explicit option to request a coredump at debugInit_exit(). Is it enough or I should create a more sophisticated fix ? I don’t think that is enough. Often a problem will happen and will not be reproducible. Maybe this code: if ((arg.error != JDWP_ERROR(NONE)) (arg.startCount == 0) initOnStartup) { EXIT_ERROR(map2jvmtiError(arg.error), No transports initialized); } can use some variant of EXIT_ERROR that does not call fatalError() ? /Staffan Actually, I don't think that coredump on every call to jni_FatalError() (see jni.cpp:726) is necessary but this hotspot code is here for 6 years and changing it probably out of scope of this fix. -Dmitry /Staffan On 20 aug 2014, at 17:55, Dmitry Samersoff dmitry.samers...@oracle.com wrote: Serguei, After some additional testing I changed the fix a bit. Please take a look at new version. http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ New version reports JVMTI error to stderr. Also jniFatalError is not referenced any more so I remove it. -Dmitry On 2014-08-20 12:08, serguei.spit...@oracle.com wrote: Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return non-zero exit code as expected but will not dump the core. -Dmitry On 2014-08-20 00:54, serguei.spit...@oracle.com wrote: Hi Dmitry, The fix seems to be Ok. Just want to make it clear... This fix just changes the bug pattern. It a case of incorrect transport parameters the test is still going to fail but without crash, right? Thanks, Serguei On 8/19/14 12:09 PM, Dmitry Samersoff wrote: Hi Everybody, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/ JDWP call jniFatalError if transport can't be initialized (e.g. wrong parameters) and jniFatalError call os::abort(). Therefor all transport initialization errors cause vm to coredump. I see no reason for debugInit_exit to call jniFatalError so remove this code. -Dmitry -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Staffan, OK. Will create better fix. -Dmitry On 2014-08-25 15:53, Staffan Larsen wrote: On 25 aug 2014, at 13:33, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Staffan, On 2014-08-25 15:26, Staffan Larsen wrote: Dmitry, One problem with this fix is that debugInit_exit() is used from many places in the JDWP code. For some of these I think it still does make sense to receive a core dump for analysis. I agree that when parsing options, we do not need a core dump. jdwp has explicit option to request a coredump at debugInit_exit(). Is it enough or I should create a more sophisticated fix ? I don’t think that is enough. Often a problem will happen and will not be reproducible. Maybe this code: if ((arg.error != JDWP_ERROR(NONE)) (arg.startCount == 0) initOnStartup) { EXIT_ERROR(map2jvmtiError(arg.error), No transports initialized); } can use some variant of EXIT_ERROR that does not call fatalError() ? /Staffan Actually, I don't think that coredump on every call to jni_FatalError() (see jni.cpp:726) is necessary but this hotspot code is here for 6 years and changing it probably out of scope of this fix. -Dmitry /Staffan On 20 aug 2014, at 17:55, Dmitry Samersoff dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote: Serguei, After some additional testing I changed the fix a bit. Please take a look at new version. http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ New version reports JVMTI error to stderr. Also jniFatalError is not referenced any more so I remove it. -Dmitry On 2014-08-20 12:08, serguei.spit...@oracle.com wrote: Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return non-zero exit code as expected but will not dump the core. -Dmitry On 2014-08-20 00:54, serguei.spit...@oracle.com wrote: Hi Dmitry, The fix seems to be Ok. Just want to make it clear... This fix just changes the bug pattern. It a case of incorrect transport parameters the test is still going to fail but without crash, right? Thanks, Serguei On 8/19/14 12:09 PM, Dmitry Samersoff wrote: Hi Everybody, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/ JDWP call jniFatalError if transport can't be initialized (e.g. wrong parameters) and jniFatalError call os::abort(). Therefor all transport initialization errors cause vm to coredump. I see no reason for debugInit_exit to call jniFatalError so remove this code. -Dmitry -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return non-zero exit code as expected but will not dump the core. -Dmitry On 2014-08-20 00:54, serguei.spit...@oracle.com wrote: Hi Dmitry, The fix seems to be Ok. Just want to make it clear... This fix just changes the bug pattern. It a case of incorrect transport parameters the test is still going to fail but without crash, right? Thanks, Serguei On 8/19/14 12:09 PM, Dmitry Samersoff wrote: Hi Everybody, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/ JDWP call jniFatalError if transport can't be initialized (e.g. wrong parameters) and jniFatalError call os::abort(). Therefor all transport initialization errors cause vm to coredump. I see no reason for debugInit_exit to call jniFatalError so remove this code. -Dmitry -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return non-zero exit code as expected but will not dump the core. -Dmitry On 2014-08-20 00:54, serguei.spit...@oracle.com wrote: Hi Dmitry, The fix seems to be Ok. Just want to make it clear... This fix just changes the bug pattern. It a case of incorrect transport parameters the test is still going to fail but without crash, right? Thanks, Serguei On 8/19/14 12:09 PM, Dmitry Samersoff wrote: Hi Everybody, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/ JDWP call jniFatalError if transport can't be initialized (e.g. wrong parameters) and jniFatalError call os::abort(). Therefor all transport initialization errors cause vm to coredump. I see no reason for debugInit_exit to call jniFatalError so remove this code. -Dmitry
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Serguei, After some additional testing I changed the fix a bit. Please take a look at new version. http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ New version reports JVMTI error to stderr. Also jniFatalError is not referenced any more so I remove it. -Dmitry On 2014-08-20 12:08, serguei.spit...@oracle.com wrote: Ok. Thank you for the explanation! Serguei On 8/20/14 1:01 AM, Dmitry Samersoff wrote: Serguei, 1. Historically JDI test-suite had no tests for failed transport initialization behavior and invalid parameters handling. 2. As a part of JDWP hardening work I added couple of such tests to OptionTest.java - these tests pass invalid parameters to dt_socket transport to make sure that transport doesn't crash (one such crash was discovered and fixed) but just return non-zero exit code to upper level. 3. After fix for JDK-6694099 any non-zero exit code from transport cause VM to coredump. Dumping multiple cores on busy machine takes a time so harness kills the test by timeout. We can just increase timeout for this test but I don't think it's a good idea to dump core when invalid parameters passed to transport. So there is the fix. 4. After the fix tests for negative parameters will return non-zero exit code as expected but will not dump the core. -Dmitry On 2014-08-20 00:54, serguei.spit...@oracle.com wrote: Hi Dmitry, The fix seems to be Ok. Just want to make it clear... This fix just changes the bug pattern. It a case of incorrect transport parameters the test is still going to fail but without crash, right? Thanks, Serguei On 8/19/14 12:09 PM, Dmitry Samersoff wrote: Hi Everybody, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/ JDWP call jniFatalError if transport can't be initialized (e.g. wrong parameters) and jniFatalError call os::abort(). Therefor all transport initialization errors cause vm to coredump. I see no reason for debugInit_exit to call jniFatalError so remove this code. -Dmitry -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Hi Everybody, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/ JDWP call jniFatalError if transport can't be initialized (e.g. wrong parameters) and jniFatalError call os::abort(). Therefor all transport initialization errors cause vm to coredump. I see no reason for debugInit_exit to call jniFatalError so remove this code. -Dmitry -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Hi Dmitry, The fix seems to be Ok. Just want to make it clear... This fix just changes the bug pattern. It a case of incorrect transport parameters the test is still going to fail but without crash, right? Thanks, Serguei On 8/19/14 12:09 PM, Dmitry Samersoff wrote: Hi Everybody, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/ JDWP call jniFatalError if transport can't be initialized (e.g. wrong parameters) and jniFatalError call os::abort(). Therefor all transport initialization errors cause vm to coredump. I see no reason for debugInit_exit to call jniFatalError so remove this code. -Dmitry