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

[David Holmes]

Hi Coleen,

Thanks for taking a look at this!

On 20/02/2020 5:20 am, coleen.phillimore at oracle.com wrote:
> 
> 
> 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?

Good catch! copy'n'paste error. I've re-checked all the changes.

> 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>

I made no attempt to try and reason about whether any of the uses of > 
versus >= were incorrect. :)

> 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?

Fixed.

http://cr.openjdk.java.net/~dholmes/8238988/webrev.v3-incr/
http://cr.openjdk.java.net/~dholmes/8238988/webrev.v3/

> This change looks like a good cleanup.

Thanks for the review.

David
-----

> 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
>>> -----
>