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

[David Holmes]

Hi Dan,

On 12/02/2020 3:12 am, Daniel D. Daugherty wrote:
> Hi David,
> 
> Very nice cleanup of some crufty/old code.

Thanks for the review!

Commenbts inlined ...

> 
> On 2/11/20 12:40 AM, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8234372
>> webrev: http://cr.openjdk.java.net/~dholmes/8234372/webrev.v2/
> 
> src/hotspot/share/runtime/thread.hpp
>      L691:   // Warning: the method can only be used on the running thread.
>          Not fond of this existing comment wording. Perhaps:
> 
>              // Warning: Can only be called by the calling thread on 
> itself.
> 
> src/hotspot/share/runtime/thread.cpp
>      L1822: // Check for adr in the usable portion of our stack.
>          Not fond of "our stack" here. To me it implies the caller's
>          stack.  Perhaps:
> 
>             // Check for adr in the usable portion of the JavaThread's 
> stack.
> 
>          This is the comment from the thread.hpp:
> 
>          L1733:   // Check if address is in the usable part of the stack 
> (excludes protected
>          L1734:   // guard pages). Can be applied to any thread and is 
> an approximation for
>          L1735:   // using is_in_stack when the query has to happen from 
> another thread.
> 
>          It is much more clear, but also very long.

I'll tweak the comments to further improve clarity.

> 
> src/hotspot/os/linux/os_linux.cpp
>      Note: Old code is this:
> 
>      L722:   if (addr <  t->stack_base() && addr >= 
> t->stack_reserved_zone_base()) {
> 
>      so it is different than the Win* version. The new call is better.
> 
> src/hotspot/os/windows/os_windows.cpp
>      Note: Old code is this:
> 
>      L2545:           if (addr > thread->stack_reserved_zone_base() && 
> addr < thread->stack_base()) {
> 
>      so it is different than the Linux version. The new call is better.
> 
> 
> src/hotspot/cpu/sparc/frame_sparc.cpp
>      No comments.

I missed a change here:

254     bool sender_fp_safe = (sender_fp <= thread->stack_base()) &&
255                    (sender_fp > _FP);

Should be < not <=. Now fixed.

> src/hotspot/cpu/x86/frame_x86.cpp
>      No comments.
> 
> 
> src/hotspot/cpu/aarch64/frame_aarch64.cpp
>      No comments.
> 
> src/hotspot/cpu/arm/frame_arm.cpp
>      L144:       bool saved_fp_safe = ((address)saved_fp < 
> thread->stack_base()) && (saved_fp > sender_sp);
>      L174:       bool saved_fp_safe = ((address)saved_fp < 
> thread->stack_base()) && (saved_fp >= sender_sp);
>          L144 uses '>' and L174 uses '>=' with the same operands. Why?

Well spotted. I can't answer why. Looking at the equivalent code in 
frame_aarch64.cpp both statements use >, so I'm going to assume that the 
 >= at L174 is a bug. The x86 seems to confirm that too, so I'm changing 
L174.

> src/hotspot/os_cpu/linux_arm/os_linux_arm.cpp
>      No comments.
> 
> 
> src/hotspot/cpu/ppc/frame_ppc.cpp
>      L72:   bool fp_safe = (fp < thread->stack_base()) &&  (fp > sp);
>          existing nit: extra space before second (...) expression.
> 
> 
> src/hotspot/cpu/s390/frame_s390.cpp
>      L76:   bool fp_safe = (fp < thread->stack_base()) &&  (fp > sp);
>          existing nit: extra space before second (...) expression.

Fixed.

> src/hotspot/os_cpu/linux_s390/thread_linux_s390.cpp
>      No comments.
> 
> 
> Other than one possible typo in src/hotspot/cpu/arm/frame_arm.cpp
> everything else is a nit. I don't need to see another webrev.

I have produced an updated version for other folk to test the ARM change:

http://cr.openjdk.java.net/~dholmes/8234372/webrev.v3/

Thanks,
David
-----

> Thumbs up!
> 
> As you like to say: The proof will be in the testing.
> And I'll change that last word to "testing over time" since some
> of these code paths might be rarely used...
> 
> Dan
> 
> 
> 
>>
>> 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
>