RFR: 8234372: Investigate use of Thread::stack_base() and queries for "in stack"

[David Holmes]

Bug: https://bugs.openjdk.java.net/browse/JDK-8234372
webrev: http://cr.openjdk.java.net/~dholmes/8234372/webrev.v2/

Following on from JDK-8215355 I checked all uses of Thread::stack_base() 
to watch for range tests that should be exclusive but are inclusive, and 
vice-versa. And in addition clarified and streamlined the various "in 
stack" checks that are made.

Summary of changes:

src/hotspot/cpu/aarch64/frame_aarch64.cpp
src/hotspot/cpu/arm/frame_arm.cpp
src/hotspot/cpu/ppc/frame_ppc.cpp
src/hotspot/cpu/s390/frame_s390.cpp
src/hotspot/cpu/sparc/frame_sparc.cpp
src/hotspot/cpu/x86/frame_x86.cpp

In terms of actual bugs the implementation of frame::safe_for_sender on 
all platforms except x86 and aarch64 was using the wrong range test in a 
number of cases, so these are all now correct and consistent.

All platforms had an incorrect range check in relation to the "locals".

All platforms now use is_in_usable_stack to check for a valid sp, rather 
than duplicating (sometimes incorrectly) that logic.

--

src/hotspot/os/linux/os_linux.cpp
src/hotspot/os/windows/os_windows.cpp

Replaced explicit range check with is_in_usable_stack

src/hotspot/os_cpu/linux_arm/os_linux_arm.cpp
src/hotspot/os_cpu/linux_s390/thread_linux_s390.cpp

Replaced explicit range check with on_local_stack.

---

src/hotspot/share/runtime/thread.?pp

Moved is_in_usable_stack from Thread to JavaThread (guard regions are 
only relevant for JavaThreads).

Clarified functionality and use of the three "in stack" variants.

Removed redundant check from is_in_stack:

!   // Allow non Java threads to call this without stack_base
!   if (_stack_base == NULL) return true;

As this is executed by the current thread, and the very first thing a 
thread does is set its stack base and size, it is impossible to find a 
NULL stack_base (which is already asserted inside stack_base()). [I 
tested this extensively just as a sanity check: tiers 1-5 plus hotspot 
runtime/serviceability/gc.]

Misc cleanup to use stack_end() rather than recalulate it.

---

There are some further possible cleanups here but I didn't want to go 
too far with things that would obscure the functional changes too much. 
As mentioned in the bug report the three "in stack" functions would 
benefit from some minor renamings so that their relationship is clearer. 
But I can leave that to a follow on RFE. Further, it may be possible to 
replace a lot of the remaining uses of stack_base() with a more 
constrained "in stack" function that takes a limit. For example, rather 
than something like:

if (thread->stack_base() > fp && fp >= sp)

have:

if (thread->is_in_stack_range(fp /* addr*/, sp /*limit*/))

which checks the given addr against stack_base and the limit, and checks 
the limit against stack_end(). The difficultly may lie in determining 
whether checking against the limit should be a > or >= test, as it will 
be dependent on the context. Again this seems like something for a 
second RFE.

---

Testing:

Thanks to Andrew Haley for taking the frame changes for a spin on 
ARM/PPC/Aarch64/S390(?).

I also ran our tier 1 to 3 testing on x86 and sparc.

Thanks,
David