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

[John Coomes]

David Holmes (David.Holmes at oracle.com) wrote:
> John Coomes said the following on 08/24/10 07:10:
> > 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(...);
> 
> Thanks John, I've implemented your suggestion - webrev updated.

Looks good.

-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.
> >>>>   
> >