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

Wed Aug 27 08:08:04 UTC 2014

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/c6c9d7e6/attachment-0001.html>