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

[Thomas Stüfe]

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.

Or, something like this:
static bool is_primordial_thread(void) WINDOWS_ONLY({return false;})
BSD_ONLY({return false;})

-----

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 {} ?

----

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.

-----

src/hotspot/share/runtime/os.hpp

s/Some platforms have to special-case handling/Some platforms need
special-case handling

----

Kind Regards, Thomas


On Tue, Nov 14, 2017 at 11:39 AM, David Holmes <david.holmes at oracle.com>
wrote:

> Bug: https://bugs.openjdk.java.net/browse/JDK-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
>