RFR: 8238988: Rename thread "in stack" methods and add in_stack_range

[coleen.phillimore at oracle.com]

http://cr.openjdk.java.net/~dholmes/8238988/webrev.v2/src/hotspot/cpu/sparc/frame_sparc.cpp.udiff.html 


    // an fp must be within the stack and above (but not equal) sp
- bool fp_safe = (_FP < thread->stack_base()) &&
- (_FP > _SP);
+ bool fp_safe = thread->is_in_stack_range_incl(_FP, _SP);

Shouldn't this be exclusive?  Or did I read it incorrectly?

I'm trying to figure out why each call site makes the choice whether to 
call inclusive vs exclusive.  It seems that when the limit is FP or SP, 
the code chooses inclusive.  Having the functions make this explicit 
might prevent subtle bugs like JDK-8215355. 
<https://bugs.openjdk.java.net/browse/JDK-8215355>

The functions are also nice in that stack_base() seems like it's the 
lower address, whereas it's really the higher address.  Some code 
compares > to stack_base and other code compares >=. Which I think 
doesn't matter.  The functions help preventing cut/paste errors.

http://cr.openjdk.java.net/~dholmes/8238988/webrev.v2/src/hotspot/share/runtime/thread.hpp.udiff.html

+ // stack: stack_base() > addr > limit
+ bool is_in_stack_range_excl(address adr, address limit) const {

and two other places.

nit, the comment says "adr" and the formal parameter is "addr", can you 
make them the same?

This change looks like a good cleanup.

thanks,
Coleen

On 2/17/20 1:19 AM, David Holmes wrote:
> PS. I fixed the bug synopsis to read is_in_stack_range.
>
> David
>
> On 17/02/2020 3:18 pm, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8238988
>>
>> Following on from JDK-8234372 this change introduces some renaming:
>>
>> Thread::is_in_stack -> Thread::is_in_live_stack
>> Thread::on_local_stack -> Thread::is_in_full_stack
>>
>> and then introduces private:
>>
>>   bool is_in_stack_range(address adr, address limit, bool inclusive)
>>
>> with public wrappers:
>>
>> bool is_in_stack_range_incl(address adr, address limit)
>> bool is_in_stack_range_excl(address adr, address limit)
>>
>> and reimplements the existing "in stack" methods using 
>> is_in_stack_range().
>>
>> This can be seen in the phase 1 webrev:
>>
>> http://cr.openjdk.java.net/~dholmes/8238988/webrev.v1/
>>
>> (There are some copyright updates missing in phase 1 but they are in 
>> the phase 2 full webrev.)
>>
>> Phase 2 then replaces all the explicit stack range checks (x < 
>> stack_base() && x >= limit) with use of the new public methods.
>>
>> Incremental webrev from phase 1:
>>
>> http://cr.openjdk.java.net/~dholmes/8238988/webrev.v2-incr/
>>
>> Full webrev for phases 1 & 2:
>>
>> http://cr.openjdk.java.net/~dholmes/8238988/webrev.v2/
>>
>> A few notes:
>>
>> - some checks for x < stack_base() are now is_in_full_stack(x), 
>> adding the missing/implied check against stack_end() even though it 
>> may not strictly be needed in some cases
>> - some checks for x != NULL are removed as it is implicit in the 
>> range check
>> - removed some redundant assertions that stack_base() != NULL as that 
>> assertion is already executed in the "in stack" functions.
>> - removed redundant os::Solaris::valid_stack_address ands replaced 
>> with thread method usage
>>
>> Testing:
>>   - tier 1-3 on x86 and sparc platforms
>>   - build test on ARM 32-bit, Aarch64, PPC64le and S390X.
>>
>> Non-x86/sparc platforms need testing by folk who have access to those 
>> platforms - thanks.
>>
>> Thanks,
>> David
>> -----