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

Wed Aug 27 09:03:29 UTC 2014

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


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.