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

[David Holmes]

Could I get a second reviewer please.

Thanks,
David

On 20/02/2020 12:39 pm, David Holmes wrote:
> 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
>>>> -----
>>