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

Wed Aug 27 10:25:19 UTC 2014

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.spitsyn at 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.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.
>