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

David Holmes david.holmes at oracle.com
Tue Nov 14 12:10:02 UTC 2017


Hi Thomas,

Thanks for taking a look so quickly.

On 14/11/2017 9:07 PM, Thomas Stüfe wrote:
> Hi David,
> 
> Looks mostly fine, nice cleanup.
> 
> Minor nits:
> 
> os_bsd.cpp, os_windows.cpp:
> 
> I do not like the fact that we offer a generic function but do not 
> implement it. The comment
> 
> +// Unlike Linux we don't try to handle the initial process
> +// thread in any special way - so just report 'false'
> 
> assumes a given usage of this function, but the function looks general 
> purpose and may be used in different contexts in the future. I'd either 
> invest the work to implement it on bsd/windows or at least add a comment 
> that the function should not be relied upon on these platforms.

I went hunting for a way to implement it and could not find anything. I 
hadn't realized initially that we didn't already have something on each 
platform. Given the narrow problem this is trying to solve it's already 
exceeded the time budget I can allocate to it. I was tempted to drop 
back to having a Linux only solution ... but three platforms can support 
it, while three platforms can't.

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

> -----
> 
> src/hotspot/os/linux/os_linux.cpp
> 
> +  //   But ensure we don't underflow the stack size - allow 1 page spare
> +  if (stack_size >= (size_t)(3 * page_size()))
>     stack_size -= 2 * page_size();
> 
> Could you please add {} ?

Fixed.

> ----
> 
> Sidenote (not touched by your thread):
> 
> 3045     uintptr_t stack_extent = (uintptr_t) 
> os::Linux::initial_thread_stack_bottom();
> 3046     unsigned char vec[1];
> 3047
> 3048     if (mincore((address)stack_extent, os::vm_page_size(), vec) == 
> -1) {
> 
> I feel a tiny bit queasy about this coding. mincore(2) is defined to 
> work with pages as returned by sysconf(_SC_PAGESIZE). Should the 
> implementation of os::vm_page_size() ever be something different than 
> sysconf(_SC_PAGESIZE), this may overflow vec.

Is there any reason to think it would ever be something different? On 
Linux os::vm_page_size() returns os::Linux::page_size() which is set by:

   Linux::set_page_size(sysconf(_SC_PAGESIZE));

> -----
> 
> src/hotspot/share/runtime/os.hpp
> 
> s/Some platforms have to special-case handling/Some platforms need 
> special-case handling

Fixed.

Thanks,
David

> ----
> 
> Kind Regards, Thomas
> 
> 
> On Tue, Nov 14, 2017 at 11:39 AM, David Holmes <david.holmes at oracle.com 
> <mailto:david.holmes at oracle.com>> wrote:
> 
>     Bug: https://bugs.openjdk.java.net/browse/JDK-8189170
>     <https://bugs.openjdk.java.net/browse/JDK-8189170>
> 
>     webrev: http://cr.openjdk.java.net/~dholmes/8189170/webrev/
>     <http://cr.openjdk.java.net/~dholmes/8189170/webrev/>
> 
> 
>     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
> 
> 


More information about the hotspot-runtime-dev mailing list