Request for review: 6761744 Hotspot crashes if process size limit is exceeded

[Tao Mao]

1. check overflow routine is wrapped up.

2. The reduction of code duplication is adopted.

3. Develop a way to intake 0~multiple external vm options in jtreg 
main(). Hope it helps others implement similar functionality.

webrev:
http://cr.openjdk.java.net/~tamao/6761744/webrev.02/

test:
JTREG: passed.
jtreg -jdk:/home/tamao/jdk1.7.0_04-i586 -vmoptions:"-tamao <GC_OPTION>" 
/home/tamao/src/6761744CrashIfProcessSizeLimitExceeded_hsx24/test/gc/init/TestHandleExceedingProcessSizeLimitIn32BitBuilds.java

where GC_OPTION rotates in -XX:+UseParallelGC -XX:+UseG1GC 
-XX:+UseSerialGC -XX:+UseParNewGC -XX:+UseConcMarkSweepGC


Thanks.
Tao

On 4/15/13 1:37 AM, Bengt Rutisson wrote:
>
> Hi Tao,
>
> I agree with John. The changes look fine and I'm ok with them. But it 
> would look nicer to add the 
> CollectedHeap::add_and_verify_no_overflow() method.
>
> Also, regarding the test, a couple of minor things.
>
> First, with the new naming convention I think the test should be 
> called something like:
>
> test/gc/init/TestHandleExceedingProcessSizeLimitOn32BitSystems.java
>
> Then I think you can reduce the code duplication a bit. You create the 
> same ProcessBuilder and OutputAnalyzer in both 32 and 64 bit cases. So 
> maybe it could look like this instead:
>
> public class TestHandleExceedingProcessSizeLimitOn32BitSystems {
>   public static void main(String args[]) throws Exception {
>     ProcessBuilder pb =
>       
> ProcessTools.createJavaProcessBuilder(System.getProperty("test.vm.opts"),
>                                               "-Xmx3072m",
>                                               "-XX:MaxPermSize=1024m",
>                                               "-version");
>     OutputAnalyzer output = new OutputAnalyzer(pb.start());
>
>     String dataModel = System.getProperty("sun.arch.data.model");
>     if (dataModel.equals("32")) {
>       output.shouldContain("The size of the object heap + perm gen 
> exceeds the maximum representable size");
>       if (output.getExitValue() == 0) {
>         throw new RuntimeException("Not expected to get exit value 0");
>       }
>     } else if (dataModel.equals("64")) {
>       output.shouldHaveExitValue(0);
>     }
>   }
> }
>
> There is also a small bug in the test. If you run JTreg without 
> passing any vmoptions the test will fail. The reason is that 
> System.getProperty("test.vm.opts") will evaluate to the empty string. 
> This will be passed as the first argument to the Java process, which 
> will assume that this is the class name. So, it fails to start because 
> it can't load the class <empty string>.
>
> To reproduce the problem use this command line:
>
> java  -jar <path_to_jtreg>/lib/jtreg.jar 
> test/gc/6761744/TestHandleExceedingProcessSizeLimitOn32BitSystems.java
>
> I guess the fix is to check that System.getProperty("test.vm.opts") is 
> not empty before passing it on to createJavaProcessBuilder().
>
> Thanks,
> Bengt
>
> On 4/13/13 3:15 AM, John Cuthbertson wrote:
>> Hi Tao,
>>
>> This looks OK to me.
>>
>> I can see what Thomas is saying though. All of the checks involve 
>> something like:
>>
>> total += some_value;
>> if (total < some_value) {
>>   // We must have overflowed
>>   vm_exit(...);
>> }
>>
>> So a function in CollectedHeap (the base class of SharedHeap and 
>> ParallelScavengeHeap) might make sense. For example:
>>
>> total_reserved = 0;
>> total_reserved = add_and_verify_no_overflow(total_reserved, 
>> max_heap_size);
>> total_reserved = add_and_verify_no_overflow(total_reserved, 
>> max_perm_size);
>>
>> Where the function is:
>>
>> size_t add_and_verify_no_overflow(size_t x, size_t y) {
>>   const char* msg = "...";
>>   x += y;
>>   if (x < y) {
>>     vm_exit(msg);
>>   }
>>   return x;
>> }
>>
>> But I can live with your current changes.
>>
>> JohnC
>>
>> On 4/10/2013 9:52 AM, Tao Mao wrote:
>>> Hi Bengt,
>>>
>>> Thank you for reviewing. A new webrev is updated.
>>> http://cr.openjdk.java.net/~tamao/6761744/webrev.01/
>>>
>>> Cheers,
>>> Tao
>>>
>>> On 4/10/13 1:54 AM, Bengt Rutisson wrote:
>>>>
>>>> Hi Tao,
>>>>
>>>> This change looks good. Thanks for adding the JTReg test, it looks 
>>>> good!
>>>>
>>>> One minor nit:
>>>>
>>>> In src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp I would 
>>>> suggest to store "(size_t) align_size_up(pgs->max_size(), 
>>>> HeapRegion::GrainBytes)" in a local variable rather than 
>>>> duplicating the calculation.
>>>>
>>>> Thanks,
>>>> Bengt
>>>>
>>>> On 4/8/13 7:22 AM, Tao Mao wrote:
>>>>> 6761744 Hotspot crashes if process size limit is exceeded
>>>>> https://jbs.oracle.com/bugs/browse/JDK-6761744
>>>>>
>>>>> webrev:
>>>>> http://cr.openjdk.java.net/~tamao/6761744/webrev.00/
>>>>>
>>>>> changeset:
>>>>> The fix only needs to go to hsx24 since there's no perm gen in 
>>>>> hotspot-25. Thus, the webrev is based on hsx24 repo.
>>>>>
>>>>> It provides for 32-bit builds a preventive check of the size of 
>>>>> "the object heap + perm gen" before reserving VM memory. The total 
>>>>> size should not exceed 4096MB (i.e. 4GB) for 32-bit builds; 
>>>>> otherwise, the total doesn't make sense and, what's worse, 
>>>>> overflow occurs. It will consequentially trigger anther error of 
>>>>> memory access violation, which was not protected.
>>>>>
>>>>> jtreg testing java code is also written, checking both 32-bit and 
>>>>> (trivially) 64-bit builds.
>>>>>
>>>>> testing:
>>>>> check jtreg tests with flags -XX:+UseParallelGC, -XX:+UseG1GC, 
>>>>> -XX:+UseParNewGC, -XX:+UseConcMarkSweepGC, -XX:+UseSerialGC and 
>>>>> builds of 32-bit and 64-bit. All passed.
>>>>>
>>>>> Needs JPRT test when pushing.
>>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130419/e13561a7/attachment.htm>