Code Review fix for 6799919 Recursive calls to report_vm_out_of_memory are handled incorrectly

[Daniel D. Daugherty]

On 2/20/13 10:37 AM, Coleen Phillimore wrote:
> On 2/20/2013 12:05 PM, Daniel D. Daugherty wrote:
>> On 2/20/13 9:57 AM, Daniel D. Daugherty wrote:
>>> On 2/20/13 9:34 AM, Coleen Phillimore wrote:
>>>>
>>>> This looks good.
>>>
>>> Thanks for the review! Don't know if Ron is having the same connectivity
>>> problems I'm having this morning, but my access into OWAN is going up
>>> and down...
>>>
>>>
>>>> It looks like the webrev was updated to get rid of the unused 
>>>> variable, so that is good.
>>>
>>> The webrev was not updated.
>
> Yes, I see that now.  Mikael has a much better eye than I do.
>>>
>>>
>>>> Is there a test for ErrorHandlerTest in our repository already?
>>>
>>> ErrorHandlerTest? Is there yet another possible test that we don't
>>> know about?
>>
>> OK. I know that test by a different name:
>>
>> $ rgrep ErrorHandlerTest agent make src test
>> src/share/vm/runtime/globals.hpp:  notproduct(uintx, 
>> ErrorHandlerTest, 0,                                    \
>> src/share/vm/prims/jni.cpp: 
>> NOT_PRODUCT(test_error_handler(ErrorHandlerTest));
>> test/runtime/6888954/vmerrors.sh: -XX:ErrorHandlerTest=${i} -version 
>> > ${i2}.out 2>&1
>> test/runtime/6888954/vmerrors.sh:    # If ErrorHandlerTest is ignored 
>> (product build), stop.
>> test/runtime/6888954/vmerrors.sh:            echo 
>> "ErrorHandlerTest=$i failed ($f)"
>>
>> Ron had previously explored using vmerror.sh to exercise the
>> vm_exit_out_of_memory() code path as one of his early experiments.
>> He also did some testing using the ErrorHandlerTest command line
>> option. In neither case did it seem possible to get multi-threaded
>> test cases up and running. Perhaps both he and I missed something.
>>
>> It also looks like Ron didn't record the specifics of his testing
>> with either vmerrors.sh or the ErrorHandlerTest command line option
>> in the bug report. I'll have to rattle his cage about that.
>>
>
> My question was mostly if we had a jtreg test in hotspot/test 
> repository that exercised this ErrorHandlerTest option.   The second 
> part of the question is whether we should have a test because it'll 
> look like it failed.   Maybe this is more of a question for Christian 
> Tornqvist and SQE and is not tied to this specific change.
>
> I talked to Ron about trying to test this also and we couldn't come up 
> with a good strategy either.   We need a good way to test out of C 
> heap memory without actually allocating lots of memory and running out 
> of C heap.  Such tests don't run well. Maybe we can do something in 
> the future with this ErrorHandlerTest option to have the VM return 
> NULL or assert for various malloc calls triggered through some 
> heuristic.   Having the experiments to date recorded somewhere would 
> be great.

See the READ_ME file attached to the bug report for the craziness that
Ron and I had to go through to properly test this. Some of what we came
up with should be useful as a diagnostic option, but that should be done
as a separate bug fix.

Dan


>
> Thanks,
> Coleen
>
>> Dan
>>
>>>
>>> Dan
>>>
>>>
>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>> On 2/19/2013 6:48 PM, Daniel D. Daugherty wrote:
>>>>> Greetings,
>>>>>
>>>>> I'm sponsoring this code review request from Ron Durbin. This change
>>>>> is targeted at JDK8/HSX-25 in the RT_Baseline repo.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>> I have a proposed fix for the following bug:
>>>>>
>>>>>     6799919 Recursive calls to report_vm_out_of_memory are handled 
>>>>> incorrectly
>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6799919
>>>>> https://jbs.oracle.com/bugs/browse/JDK-6799919
>>>>>
>>>>> This is one of those bug fixes where the commit message nicely 
>>>>> describes
>>>>> the change:
>>>>>
>>>>> 6799919: Recursive calls to report_vm_out_of_memory are handled 
>>>>> incorrectly
>>>>> Summary: report_vm_out_of_memory() should allow 
>>>>> VMError.report_and_die() to handle multiple out of native memory 
>>>>> errors.
>>>>> Reviewed-by: dcubed, <other-reviewers>
>>>>> Contributed-by ron.durbin at oracle.com
>>>>>
>>>>> Here is the webrev URL:
>>>>>
>>>>> http://cr.openjdk.java.net/~dcubed/for_rdurbin/6799919-webrev/0-hsx25
>>>>>
>>>>> Testing:
>>>>>    - See the READ_ME file attached to the JDK-6799919 for the gory 
>>>>> details
>>>>>      of the testing needed to reproduce this failure and verify 
>>>>> the fix
>>>>>    - regular JPRT test job is in process
>>>>>
>>>>> Comments, questions and suggestions are welcome.
>>>>>
>>>>> Ron
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20130220/587cdaeb/attachment-0001.html