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

[Thomas Stüfe]

Tests ran through, no visible regressions so far.

Cheers, Thomas

On Tue, Feb 11, 2020 at 9:35 AM Thomas Stüfe <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>
> 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
>>
>