Please review: 8066185: VM crashed with SIGSEGV VirtualMemoryTracker::add_reserved_region

[David Holmes]

Thumbs up!

Thanks,
David

On 25/02/2015 2:17 AM, Kumar Srinivasan wrote:
> Hi David,
>
> comments inlined.
>
>> Hi Kumar,
>>
>> On 24/02/2015 8:14 AM, Kumar Srinivasan wrote:
>>> Hello,
>>>
>>> Please review the fix for the above issue.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~ksrini/8066185/webrev.00/
>>>
>>> The fix is self explanatory, as for the test I have done the following:
>>
>> I found the comment:
>>
>>           /*
>> +          * Since this is a VM flag, we need to ensure it is not an
>> +          * application argument, meaning the argument must precede
>> +          * the main class or those flags that invoke the VM directly.
>> +          */
>>
>> a bit confusing - specifically the "or those flags that invoke the VM
>> directly". Took me while to realize that all the args you treat
>> specially (-version, -h, -jar etc) are all "terminal" arguments -
>> either the launcher stops looking at args after the given arg, or all
>> following args must be for the application, not the launcher or VM. I
>> would have expressed this as:
>>
>>   /*
>>    * Since this must be a VM flag we stop processing once we see
>>    * an argument the launcher would not have processed beyond (such
>>    * as -version or -h), or an argument that indicates the following
>>    * arguments are for the application (i.e. the main class name, or
>>    * the -jar argument).
>>    */
>>
>>
> +1,  I have updated the webrev with your comments, also had an AHA moment,
> I realized, had missed -X, which prints the XUsage, added this to the logic
> and test.
>
>>> a. refactored it from a single longish test to sub-tests for
>>> readability.
>>> b. added new sub-test for NMT Argument Processing checks.
>>
>> Can you please update the @summary for that test. It seems the OSX
>> specific test was hijacked for NMT arg testing and the summary was
>> never updated to reflect that.
>
> This test was "commandeered" :-) for other purposes, updated the summary to
> describe what is being performed.
>
> Webrev to last reviewed:
> http://cr.openjdk.java.net/~ksrini/8066185/webrev.01/webrev.delta/index.html
>
>
> Full webrev:
> http://cr.openjdk.java.net/~ksrini/8066185/webrev.01/index.html
>
> Thanks.
> Kumar
>
>>
>> Thanks,
>> David
>>
>>> Thanks
>>> Kumar
>>>
>