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

[Thomas Stüfe]

Hi David,

still looks good (same nits apply, but if you want to push this version its
fine by me).

Cheers, Thomas

On Wed, Feb 12, 2020 at 2:15 PM David Holmes <david.holmes at oracle.com>
wrote:

> On 12/02/2020 5:23 pm, Thomas Stüfe wrote:
> > Tests ran through, no visible regressions so far.
>
> Thanks Thomas!
>
> As per my email response to Dan I made a further correction to the ARM
> code:
>
> http://cr.openjdk.java.net/~dholmes/8234372/webrev.v3/
>
> Thanks,
> David
>
>
> > Cheers, Thomas
> >
> > On Tue, Feb 11, 2020 at 9:35 AM Thomas Stüfe <thomas.stuefe at gmail.com
> > <mailto:thomas.stuefe at gmail.com>> wrote:
> >
> >     Hi David,
> >
> >     I did not find anything wrong in your patch. Nice cleanup, and great
> >     archaeological work :)
> >
> >     Only small nits and some bike shedding:
> >
> >     ---
> >
> >     So, if I get this right:
> >
> >     is_in_stack -> is in live stack (base ... sp]
> >     on_local_stack  -> is in (base...start] includes guard pages
> >     is_in_usable_stack -> is in (base...start - guard] excludes guard
> pages
> >
> >     The naming is confusing but I saw you recommended renaming the
> >     functions in the JBS comments, and I like all your suggestions
> >     better than what we have now.
> >
> >     -----
> >
> >
> http://cr.openjdk.java.net/~dholmes/8234372/webrev.v2/src/hotspot/cpu/aarch64/frame_aarch64.cpp.udiff.html
> >
> >     -  if (locals > thread->stack_base() || locals < (address) fp())
> >     return false;
> >     +  if (locals >= thread->stack_base() || locals < (address) fp())
> >     return false;
> >
> >     This would be easier to read as a negated positive (also applies to
> >     all other frame_xxx.cpp).
> >
> >     Just an idea, maybe we could add a function
> >     Thread::is_in_stack_limited_by(ptr, arbitrary_end_ptr) which could
> >     compare that ptr is between (base .. arbitrary_end_ptr] and based on
> >     that we could implement the other three stack functions.
> >
> >     For cases like this we could then write:
> >     if (!thread->is_in_stack_limited_by(locals, fp())
> >
> >     But I am unsure, maybe I overthink things.
> >
> >     -------
> >
> >
> http://cr.openjdk.java.net/~dholmes/8234372/webrev.v2/src/hotspot/os/linux/os_linux.cpp.udiff.html
> >
> >     -  if (addr <  t->stack_base() && addr >=
> >     t->stack_reserved_zone_base()) {
> >     +  if (t->is_in_usable_stack(addr)) {
> >
> >     First confused me but then I read Fredericks comment in JBS so I
> >     think it is okay.
> >
> >     But it would be nice to be able to remove this
> >     manually-expand-stack-coding altogether :)
> >
> >     -------
> >
> >     Cheers, Thomas
> >
> >     On Tue, Feb 11, 2020 at 6:40 AM David Holmes
> >     <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
> >
> >         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
> >
>