Request for review: 6978641 ( was Re: Request for approval: 6929067: Stack guard pages should be removed when thread is detached)

[David Holmes]

Indeed this was pushed a long long time ago.

In case the issue is the "unverified" part it simply means that there 
was no verification procedure for actually checking that the bug was 
fixed. Once bugs are fixed they move to a "Fix delivered" state and from 
there they move to "Closed - verified" or "Closed - unverified" 
depending on whether QA had a practical means of verifying the fix (eg a 
testcase).

David

On 20/10/2011 3:31 AM, Daniel D. Daugherty wrote:
> Andrew,
>
> My e-mail archive shows the fix for this bug as pushed a long time ago.
> Please see the attached changeset notification.
>
> Dan
>
>
>
> On 10/19/11 11:18 AM, Andrew Haley wrote:
>> What is the status of this? 6978641 is marked as
>>
>> 11-Closed, Unverified, request for enhancement
>>
>> Andrew.
>>
>>
>>
>> On 08/23/2010 10:10 PM, John Coomes wrote:
>>> David Holmes (David.Holmes at oracle.com) wrote:
>>>> Here's the official OpenJKDK webrev for this:
>>>>
>>>> http://cr.openjdk.java.net/~dholmes/6978641/
>>> Hi David,
>>>
>>> Looks good to me, aside from one syntax nit: I find it hard to read
>>> when control flow is embedded in macros like DEBUG_ONLY(),
>>> PRODUCT_ONLY(), etc. Using a separate variable is easier to read and
>>> also can be useful in the debugger, e.g.,
>>>
>>> bool chk_bounds = NOT_DEBUG(os::Linux::is_initial_thread())
>>> DEBUG_ONLY(true);
>>> if (chk_bounds&& get_stack_bounds(&stack_extent,&stack_base)) {
>>> assert(...);
>>>
>>> -John
>>>
>>>> If I can add you both as reviewers that would be great.
>>>>
>>>> Coleen can you send me details on which release/build I should use in
>>>> the CR and also if there is any special "magic" I need to submit via
>>>> JPRT.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> Coleen Phillimore said the following on 08/20/10 23:15:
>>>>> This change looks fine to me and yes, thanks for pointing out that
>>>>> I did
>>>>> add the test.
>>>>>
>>>>> Coleen
>>>>>
>>>>> Andrew Haley wrote:
>>>>>> On 08/19/2010 04:49 AM, David Holmes wrote:
>>>>>>
>>>>>>
>>>>>>> diff -r f707b4c0c36f src/os/linux/vm/os_linux.cpp
>>>>>>> --- a/src/os/linux/vm/os_linux.cpp
>>>>>>> +++ b/src/os/linux/vm/os_linux.cpp
>>>>>>> @@ -2628,10 +2628,12 @@ get_stack_bounds(uintptr_t *bottom, uint
>>>>>>> // where we're going to put our guard pages, truncate the mapping at
>>>>>>> // that point by munmap()ping it. This ensures that when we later
>>>>>>> // munmap() the guard pages we don't leave a hole in the stack
>>>>>>> -// mapping.
>>>>>>> +// mapping. This only affects the main/initial thread, but guard
>>>>>>> +// against future OS changes
>>>>>>> bool os::create_stack_guard_pages(char* addr, size_t size) {
>>>>>>> uintptr_t stack_extent, stack_base;
>>>>>>> - if (get_stack_bounds(&stack_extent,&stack_base)) {
>>>>>>> + if (NOT_DEBUG(os::Linux::is_initial_thread()&&)
>>>>>>> get_stack_bounds(&stack_extent,&stack_base)) {
>>>>>>> + assert(os::Linux::is_initial_thread(), "growable stack in
>>>>>>> non-initial thread");
>>>>>>> if (stack_extent< (uintptr_t)addr)
>>>>>>> ::munmap((void*)stack_extent, (uintptr_t)addr - stack_extent);
>>>>>>> }
>>>>>>>
>>>>>> Is stack_extent initialized here in NOT_DEBUG mode?
>>>>>>
>>>>>>
>>>>>>> @@ -2640,10 +2642,13 @@ bool os::create_stack_guard_pages(char*
>>>>>>> }
>>>>>>>
>>>>>>> // If this is a growable mapping, remove the guard pages entirely by
>>>>>>> -// munmap()ping them. If not, just call uncommit_memory().
>>>>>>> +// munmap()ping them. If not, just call uncommit_memory(). This
>>>>>>> only
>>>>>>> +// affects the main/initial thread, but guard against future OS
>>>>>>> changes
>>>>>>> bool os::remove_stack_guard_pages(char* addr, size_t size) {
>>>>>>> uintptr_t stack_extent, stack_base;
>>>>>>> - if (get_stack_bounds(&stack_extent,&stack_base)) {
>>>>>>> + if (NOT_DEBUG(os::Linux::is_initial_thread()&&)
>>>>>>> get_stack_bounds(&stack_extent,&stack_base)) {
>>>>>>> + assert(os::Linux::is_initial_thread(), "growable stack in
>>>>>>> non-initial thread");
>>>>>>> +
>>>>>>> return ::munmap(addr, size) == 0;
>>>>>>> }
>>>>>> This is basically a good compromise, I think.
>>>>>>
>>>>>> Andrew.
>>>>>>
>>