RFR: 8238988: Rename thread "in stack" methods and add in_stack_range
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Feb 21 17:30:21 UTC 2020
> http://cr.openjdk.java.net/~dholmes/8238988/webrev.v3/
src/hotspot/share/runtime/thread.hpp
L687: private:
L688:
nit - why add a blank line after private?
src/hotspot/share/runtime/thread.cpp
old L1823: bool JavaThread::is_in_usable_stack(address adr) const {
old L1824: size_t stack_guard_size = os::uses_stack_guard_pages()
? JavaThread::stack_guard_zone_size() : 0;
old L1825: size_t usable_stack_size = _stack_size - stack_guard_size;
old L1826:
old L1827: return ((stack_base() > adr) && (adr >= (stack_base()
- usable_stack_size)));
old L1828: }
New version is in thread.hpp:
new L1762: bool is_in_usable_stack(address adr) const {
new L1763: return is_in_stack_range_incl(adr,
stack_reserved_zone_base());
new L1764: }
Okay. Now that I see them side-by-side, it looks good.
Nice use of stack_reserved_zone_base()!
L1026: bool Thread::is_lock_owned(address adr) const {
L1027: return is_in_full_stack(adr);
L1028: }
I don't _think_ we can have a stack lock down in the red pages
and maybe not in the yellow pages, BUT this code has used
stack_end() for that boundary check for a very long time so
leave it.
src/hotspot/share/runtime/frame.cpp
No comments.
src/hotspot/share/runtime/handles.inline.hpp
L62: assert (_thread->is_in_live_stack((address)this), "not on
stack?");\
nit - not your issue, but please delete space before '(' and
add a space after ';' (since you'll have room now).
src/hotspot/share/runtime/handles.cpp
L54: assert (_thread->is_in_live_stack((address)this), "not on
stack?");
L71: assert (_thread->is_in_live_stack((address)this), "not on
stack?");
nit - not your issue, but please delete space before '(' and
add a space after ';' (since you'll have room now).
src/hotspot/share/runtime/jniHandles.cpp
No comments.
src/hotspot/share/runtime/os.cpp
No comments.
src/hotspot/share/runtime/unhandledOops.cpp
L61: if (!_thread->is_in_live_stack((address)op))
L62: return;
nit - not your issue, but since you changed L61 do you mind adding
the missing '{' on L61 and add an L63 with '}'?
Or join L61 and L62 which seems to be acceptable HotSpot style.
L120: if(!_thread->is_in_live_stack((address)entry._oop_ptr)) {
nit - not your issue, but please add a space before '('.
src/hotspot/share/utilities/vmError.cpp
No comments.
src/hotspot/cpu/sparc/frame_sparc.cpp
No comments.
L676: // We'd have to be pretty unlucky to be mislead at this point
But I always liked that comment... :-)
src/hotspot/cpu/x86/frame_x86.cpp
No comments.
src/hotspot/os/linux/os_linux.cpp
No comment.
src/hotspot/os/solaris/os_solaris.hpp
No comments.
src/hotspot/os/solaris/os_solaris.cpp
No comments.
src/hotspot/os_cpu/linux_sparc/os_linux_sparc.cpp
No comments.
src/hotspot/os_cpu/linux_x86/os_linux_x86.cpp
No comments.
src/hotspot/os_cpu/solaris_sparc/os_solaris_sparc.cpp
No comments.
src/hotspot/os_cpu/solaris_x86/os_solaris_x86.cpp
No comments.
src/hotspot/os_cpu/solaris_x86/thread_solaris_x86.cpp
No comments.
src/hotspot/cpu/aarch64/frame_aarch64.cpp
old L80: bool unextended_sp_safe = (unextended_sp <
thread->stack_base());
new L79: if (!thread->is_in_full_stack(unextended_sp)) {
is_in_full_stack() has the stack_base() check and a stack_end()
check. This change in behavior might catch some existing bugs.
old L150: if ((address)sender_sp >= thread->stack_base()) {
new L148: if (!thread->is_in_full_stack((address)sender_sp)) {
is_in_full_stack() has the stack_base() check and a stack_end()
check. This change in behavior might catch some existing bugs.
src/hotspot/cpu/arm/frame_arm.cpp
old L75: bool fp_safe = (fp != NULL &&
old L76: (fp < thread->stack_base()) &&
old L77: fp >= sp);
new L72: bool fp_safe = thread->is_in_stack_range_excl(fp, sp);
The 'fp >= sp' makes me think this should be
is_in_stack_range_incl(),
but I think other 'fp' checks are exclusive so perhaps this one
was just wrong.
old L121: if ((address)sender_sp >= thread->stack_base()) {
new L116: if (!thread->is_in_full_stack((address)sender_sp)) {
is_in_full_stack() has the stack_base() check and a stack_end()
check. This change in behavior might catch some existing bugs.
src/hotspot/cpu/ppc/frame_ppc.cpp
old L65: bool unextended_sp_safe = (unextended_sp <
thread->stack_base());
new L65: if (!thread->is_in_full_stack(unextended_sp)) {
is_in_full_stack() has the stack_base() check and a stack_end()
check. This change in behavior might catch some existing bugs.
src/hotspot/cpu/s390/frame_s390.cpp
old L69: bool unextended_sp_safe = (unextended_sp <
thread->stack_base());
new L69: if (!thread->is_in_full_stack(unextended_sp)) {
is_in_full_stack() has the stack_base() check and a stack_end()
check. This change in behavior might catch some existing bugs.
src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp
No comments.
src/hotspot/os_cpu/bsd_x86/os_bsd_x86.cpp
No comments.
src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp
No comments.
src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.cpp
No comments.
src/hotspot/os_cpu/linux_arm/os_linux_arm.cpp
No comments.
src/hotspot/os_cpu/linux_ppc/os_linux_ppc.cpp
No comments.
src/hotspot/os_cpu/linux_s390/os_linux_s390.cpp
No comments.
src/hotspot/os_cpu/linux_s390/thread_linux_s390.cpp
No comments.
src/hotspot/os_cpu/linux_zero/os_linux_zero.cpp
No comments.
Thumbs up! I only have nits so I don't need to see a new webrev.
So I have some comments above where is_in_full_stack() adds an
additional check to what was originally there. I think this is a
good idea and now that I re-read the code review invite I see you
had this bullet:
> - 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
I like the addition and I think it might catch some latent bugs.
Great cleanup here!
Dan
On 2/19/20 9: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
>>>> -----
>>
More information about the hotspot-runtime-dev
mailing list