RFR: 8189170: Add option to disable stack overflow checking in primordial thread for use with JNI_CreateJavaJVM

[Daniel D. Daugherty]

On 11/14/17 5:39 AM, David Holmes wrote:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8189170
>
> webrev: http://cr.openjdk.java.net/~dholmes/8189170/webrev/

src/hotspot/os/aix/os_aix.cpp
     No comments.

src/hotspot/os/aix/os_aix.hpp
     No comments.

src/hotspot/os/bsd/os_bsd.cpp
     L880: // thread in any special way - so just report 'false'
         Nit - needs a period after 'false' to make it a sentence.

     L882:     return false;
         Nit - indent should be 2 spaces.

src/hotspot/os/bsd/os_bsd.hpp
     No comments.

src/hotspot/os/linux/os_linux.cpp
     L947:   if (stack_size >= (size_t)(3 * page_size()))
         Nit - needs '{' and '}' on the if-statement body.

     L3070: // always place it right after end of the mapped region
         Nit - needs a period after 'region' to make it a sentence.
         (Not your problem, but you did update the sentence...)

src/hotspot/os/linux/os_linux.hpp
     No comments.

src/hotspot/os/solaris/os_solaris.cpp
     No comments other than I learned something new about thr_main() :-)
     Thanks for cleaning up that mess.

src/hotspot/os/windows/os_windows.cpp
     L452: // thread in any special way - so just report 'false'
         Nit - needs a period after 'false' to make it a sentence.

     L454:     return false;
         Nit - indent should be 2 spaces.

src/hotspot/share/gc/shared/threadLocalAllocBuffer.cpp
     No comments.

src/hotspot/share/runtime/globals.hpp
     L1181:   experimental(bool, DisablePrimordialThreadGuardPages, false,
         Experimental or diagnostic?

src/hotspot/share/runtime/os.hpp
     L450:   // that loads/creates the JVM via JNI_CreateJavaVM
         Nit - needs a period after 'JNI_CreateJavaVM'.

     L453:   // launcher nevers uses the primordial thread as the main 
thread, but
         Typo - 'nevers' -> 'never'

     L455:   // have to special-case handling of the primordial thread 
if it attaches
         Typo - 'have to' -> 'have'

src/hotspot/share/runtime/thread.cpp
     I like the new log message.

src/hotspot/share/runtime/thread.inline.hpp
     L159:   if (os::uses_stack_guard_pages() &&
     L160:       !(DisablePrimordialThreadGuardPages && 
os::is_primordial_thread())) {
         I'm not sure why, but I had to read these two lines several
         times before I convinced myself the logic is right.

I'm good with the code changes. Your call on whether to fix the
bits or not.

Regarding this part of Thomas' review:

>> Or, something like this:
>> static bool is_primordial_thread(void) WINDOWS_ONLY({return false;}) 
>> BSD_ONLY({return false;})
>
> I can do that. I'll see if anyone else has any comments on this.

I'm assuming that Thomas means for the above to be at the
platform independent layer (runtime/os.hpp?) so it is more
obvious to the reader that the function doesn't do anything
on those two platforms... I know we dislike such things at
the platform independent layer, but it does feel appropriate
to make this limitation more obvious.

I would be okay if you made the change that Thomas is suggesting.

Thumbs up!

Dan


>
>
> There are three parts to this which made it somewhat more complicated 
> than I had envisaged:
>
> 1. Add the flag and disable the guards.
>
> This is relatively straight-forward:
>
>  void JavaThread::create_stack_guard_pages() {
>    if (!os::uses_stack_guard_pages() ||
>        _stack_guard_state != stack_guard_unused ||
>        (DisablePrimordialThreadGuardPages && 
> os::is_primordial_thread())) {
>        log_info(os, thread)("Stack guard page creation for thread "
>                             UINTX_FORMAT " disabled", 
> os::current_thread_id());
>      return;
>
> with a tweak in JavaThread::stack_guards_enabled as well.
>
> 2. Promote os::XXX::is_initial_thread to os::is_primordial_thread()
>
> We have three platforms that already implement this functionality: 
> Linux, Solaris and AIX. For BSD/OSX and Windows we don't attempt to do 
> anything different for the primordial thread, and we don't have a way 
> to detect the primordial thread - so is_primordial_thread() just 
> returns false.
>
> I also tidied up some comments regarding terminology.
>
> 3. Thomas noticed that we unconditionally subtract 2*page_size() from 
> the rlimit stack size, without checking it was bigger than 
> 2*page_size() to start with. I was unsure how to tackle this. It's no 
> good leaving stack_size at zero so I opted to ensure we would have at 
> least one page left. Of course in such cases we would hit the bug in 
> libc (if it still exists, which seems unlikely but time consuming to 
> establish).
>
> Testing:
>   - Tier 1 (JPRT) testing with a modified launcher that uses the 
> primordial thread
>     - with guard pages enabled
>     - with guard pages disabled
>   - Tier 1 (JPRT) normal JDK build (which should be unaffected by this 
> change)
>
> The testing was primarily to ensure that disabling the stack guards on 
> the primordial thread wasn't totally broken. A number of tests fail:
> - setting -Xss32m fails for the primordial thread when guards are 
> enabled and the rlimit is 8m
> - tests that would hit StackOverflowError crash the VM when guards are 
> disabled (as you would expect).
> - runtime/execstack/Testexecstack.java crashes when the guards are 
> disabled
>
> Thanks,
> David
>