Need reviewers for changes in JVM initialization code (added check for -XX:StackShadowPages option)

[Pavel Tisnovsky]

Hi Paul and Coleen,

there are another webrevs for both OpenJDKs:
http://cr.openjdk.java.net/~ptisnovs/StackXXXPagesCheck_OpenJDK6_v2/
http://cr.openjdk.java.net/~ptisnovs/StackXXXPagesCheck_OpenJDK7_v2/

To tell the truth I don't understand why the cast is necessary because
StackYellowPages etc. are declared as intx type in globals.hpp. Maybe I
have missed something important.

Paul Hohensee wrote:
>  You still should cast the first argument to verify_min_value in
> check_stack_pages
> to intx, vis
> 
> // Check stack pages settings
> bool Arguments::check_stack_pages()
> {
>   bool status = true;
>   status = status && verify_min_value((intx)StackYellowPages, 1, "StackYellowPages");
>   status = status && verify_min_value((intx)StackRedPages, 1, "StackRedPages");
>   status = status && verify_min_value((intx)StackShadowPages, 1, "StackShadowPages");
>   return status;
> }
> 
> lest some versions of gcc reject the code because of the implicit
> unsigned -> signed
> conversion.
> 
> Paul
> 
> On 8/19/10 8:43 AM, Pavel Tisnovsky wrote:
>> Hi Coleen and Paul,
>>
>> here are new versions of the patch:
>>
>> for OpenJDK6:
>> http://cr.openjdk.java.net/~ptisnovs/StackXXXPagesCheck_OpenJDK6/
>>
>> and for OpenJDK7 too:
>> http://cr.openjdk.java.net/~ptisnovs/StackXXXPagesCheck_OpenJDK7/
>>
>> (the change itself is the same for both OpenJDKs, but line numbers are
>> different)
>>
>>> I will push it through JPRT
>> does it mean that I need not do the push myself?
>>
>> Thank you for your help
>> Pavel
>>
>> Coleen Phillimore wrote:
>>> Yes, I agree they should stay intx.  Thank you for the change.  It looks
>>> fine to me.  If you update the patch, I will push it through JPRT.  Let
>>> me know when it is ready.
>>> thanks,
>>> Coleen
>>>
>>> Paul Hohensee wrote:
>>>>  Leave them as intx, imo.  Their high bound value isn't anywhere near
>>>> maxint,
>>>> and if you change them to uintx you'll have to cast them to intx for the
>>>> calls to verify_min_value.  Plus there's some places where their
>>>> values are
>>>> assigned to ints that would have to be changed.
>>>>
>>>> Also, pls use INTX_FORMAT instead of UINTX_FORMAT in verify_min_value.
>>>>
>>>> Paul
>>>>
>>>> On 8/17/10 3:50 PM, Tom Rodriguez wrote:
>>>>> On Aug 6, 2010, at 7:05 AM, Pavel Tisnovsky wrote:
>>>>>
>>>>>> Tom Rodriguez wrote:
>>>>>>> I think you could put this check into arguments.cpp since I all
>>>>>>> platforms would require a positive number for the
>>>>>>> StackShadowPages.  The same should be true of StackRedPages and
>>>>>>> StackYellowPages.  Actually they all should be required to greater
>>>>>>> than 0 I think.  Other than that I don't see other obvious
>>>>>>> constraints on the values.  That bug report doesn't really show
>>>>>>> other problematic values, though I'm not sure I follow the point of
>>>>>>> the guarantee that's failing either.  What does the min stack size
>>>>>>> have to do with the number of guard pages?  I would expect it to be
>>>>>>> checking against ThreadStackSize and returning an error if it was
>>>>>>> too small like the other places that check against TheadStackSize.
>>>>>>>
>>>>>>> tom
>>>>>> Hi Tom,
>>>>>>
>>>>>> I've added check for all Stack*Pages parameters to arguments.cpp. Can
>>>>>> you please review the changes?
>>>>>>
>>>>>> http://cr.openjdk.java.net/~ptisnovs/StackXXXPagesCheck/
>>>>>>
>>>>>> Is there any reason why Stack*Pages parameters are of type int and not
>>>>>> unsigned int? I'm able to change its types (these variables are
>>>>>> generated by macro which accepts type as one of its parameter) but I'm
>>>>>> not sure if it would be correct in all cases.
>>>>> Would someone from runtime want to comment on this?  It seems ok to
>>>>> me.  Changing to uintx would also be ok but it might require other
>>>>> uses to be converted to unsigned as well.  I'd probably just leave it
>>>>> alone myself.
>>>>>
>>>>> tom
>>>>>
>>>>>
>>>>>> Cheers
>>>>>> Pavel
>>>>>>
>>>>>>> On Jul 27, 2010, at 8:15 AM, Pavel Tisnovsky wrote:
>>>>>>>
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> can anybody please review two quite simple changes in JVM
>>>>>>>> initialization code? Webrev is available at:
>>>>>>>> http://cr.openjdk.java.net/~ptisnovs/StackShadowPagesCheck/
>>>>>>>>
>>>>>>>> When -XX:StackShadowPages is set to negative integer or zero
>>>>>>>> value, JVM segfaulted on Linux and, according to
>>>>>>>> http://bugs.sun.com/view_bug.do?bug_id=6885308, hangs up on
>>>>>>>> Solaris (although I only check this issue on Linux)
>>>>>>>>
>>>>>>>> I also would like to add more check for -XX:StackRedPages,
>>>>>>>> -XX:StackShadowPages and -XX:StackYellowPages options to avoid
>>>>>>>> issue described in the bug report mentioned above
>>>>>>>> (http://bugs.sun.com/view_bug.do?bug_id=6885308) but I'm unable to
>>>>>>>> find relevant information about proper conditions (it may depends
>>>>>>>> on page sizes, VM stack size etc.). Any ideas?
>>>>>>>>
>>>>>>>> Cheers
>>>>>>>> Pavel Tisnovsky
>>>>>>>>