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

[Paul Hohensee]

  Whoops, my bad.  I was thinking of the old version where 
StackXXXPagesCheck
was declared uintx.

Paul

On 8/19/10 12:01 PM, Pavel Tisnovsky wrote:
> 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
>>>>>>>>>