RFR: 8238988: Rename thread "in stack" methods and add in_stack_range
David Holmes
david.holmes at oracle.com
Mon Feb 24 01:26:57 UTC 2020
Hi Dan,
Thanks for taking a look at this.
I have applied all your nits.
Good catch on the frame_arm.cpp change! I still managed to misread that
even after Coleen found the same issue sparc on sparc. No change of
behaviour intended so changed to incl version.
I'll push once I've sanity checked the changes through a build.
Thanks for the review.
David
-----
On 22/02/2020 3:30 am, Daniel D. Daugherty wrote:
> > 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