RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again

Wed Aug 27 08:12:04 UTC 2014

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.spitsyn at 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.spitsyn at 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.spitsyn at 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.samersoff at oracle.com
>>>>>>> <mailto:dmitry.samersoff at 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.samersoff at oracle.com
>>>>>>>>> <mailto:dmitry.samersoff at 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.spitsyn at 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.spitsyn at 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.
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20140827/6d9948a8/attachment.html>