RFR: 8234372: Investigate use of Thread::stack_base() and queries for "in stack"
David Holmes
david.holmes at oracle.com
Wed Feb 12 22:07:33 UTC 2020
On 13/02/2020 6:34 am, Thomas Stüfe wrote:
> Hi David,
>
> still looks good (same nits apply, but if you want to push this version
> its fine by me).
Thanks Thomas!
David
-----
> Cheers, Thomas
>
> On Wed, Feb 12, 2020 at 2:15 PM David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
> On 12/02/2020 5:23 pm, Thomas Stüfe wrote:
> > Tests ran through, no visible regressions so far.
>
> Thanks Thomas!
>
> As per my email response to Dan I made a further correction to the
> ARM code:
>
> http://cr.openjdk.java.net/~dholmes/8234372/webrev.v3/
>
> Thanks,
> David
>
>
> > Cheers, Thomas
> >
> > On Tue, Feb 11, 2020 at 9:35 AM Thomas Stüfe
> <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>
> > <mailto:thomas.stuefe at gmail.com
> <mailto:thomas.stuefe at gmail.com>>> wrote:
> >
> > Hi David,
> >
> > I did not find anything wrong in your patch. Nice cleanup,
> and great
> > archaeological work :)
> >
> > Only small nits and some bike shedding:
> >
> > ---
> >
> > So, if I get this right:
> >
> > is_in_stack -> is in live stack (base ... sp]
> > on_local_stack -> is in (base...start] includes guard pages
> > is_in_usable_stack -> is in (base...start - guard] excludes
> guard pages
> >
> > The naming is confusing but I saw you recommended renaming the
> > functions in the JBS comments, and I like all your suggestions
> > better than what we have now.
> >
> > -----
> >
> >
> http://cr.openjdk.java.net/~dholmes/8234372/webrev.v2/src/hotspot/cpu/aarch64/frame_aarch64.cpp.udiff.html
> >
> > - if (locals > thread->stack_base() || locals < (address) fp())
> > return false;
> > + if (locals >= thread->stack_base() || locals < (address) fp())
> > return false;
> >
> > This would be easier to read as a negated positive (also
> applies to
> > all other frame_xxx.cpp).
> >
> > Just an idea, maybe we could add a function
> > Thread::is_in_stack_limited_by(ptr, arbitrary_end_ptr) which
> could
> > compare that ptr is between (base .. arbitrary_end_ptr] and
> based on
> > that we could implement the other three stack functions.
> >
> > For cases like this we could then write:
> > if (!thread->is_in_stack_limited_by(locals, fp())
> >
> > But I am unsure, maybe I overthink things.
> >
> > -------
> >
> >
> http://cr.openjdk.java.net/~dholmes/8234372/webrev.v2/src/hotspot/os/linux/os_linux.cpp.udiff.html
> >
> > - if (addr < t->stack_base() && addr >=
> > t->stack_reserved_zone_base()) {
> > + if (t->is_in_usable_stack(addr)) {
> >
> > First confused me but then I read Fredericks comment in JBS so I
> > think it is okay.
> >
> > But it would be nice to be able to remove this
> > manually-expand-stack-coding altogether :)
> >
> > -------
> >
> > Cheers, Thomas
> >
> > On Tue, Feb 11, 2020 at 6:40 AM David Holmes
> > <david.holmes at oracle.com <mailto:david.holmes at oracle.com>
> <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com>>>
> wrote:
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8234372
> > webrev:
> http://cr.openjdk.java.net/~dholmes/8234372/webrev.v2/
> >
> > Following on from JDK-8215355 I checked all uses of
> > Thread::stack_base()
> > to watch for range tests that should be exclusive but are
> > inclusive, and
> > vice-versa. And in addition clarified and streamlined the
> > various "in
> > stack" checks that are made.
> >
> > Summary of changes:
> >
> > src/hotspot/cpu/aarch64/frame_aarch64.cpp
> > src/hotspot/cpu/arm/frame_arm.cpp
> > src/hotspot/cpu/ppc/frame_ppc.cpp
> > src/hotspot/cpu/s390/frame_s390.cpp
> > src/hotspot/cpu/sparc/frame_sparc.cpp
> > src/hotspot/cpu/x86/frame_x86.cpp
> >
> > In terms of actual bugs the implementation of
> > frame::safe_for_sender on
> > all platforms except x86 and aarch64 was using the wrong
> range
> > test in a
> > number of cases, so these are all now correct and consistent.
> >
> > All platforms had an incorrect range check in relation to the
> > "locals".
> >
> > All platforms now use is_in_usable_stack to check for a valid
> > sp, rather
> > than duplicating (sometimes incorrectly) that logic.
> >
> > --
> >
> > src/hotspot/os/linux/os_linux.cpp
> > src/hotspot/os/windows/os_windows.cpp
> >
> > Replaced explicit range check with is_in_usable_stack
> >
> > src/hotspot/os_cpu/linux_arm/os_linux_arm.cpp
> > src/hotspot/os_cpu/linux_s390/thread_linux_s390.cpp
> >
> > Replaced explicit range check with on_local_stack.
> >
> > ---
> >
> > src/hotspot/share/runtime/thread.?pp
> >
> > Moved is_in_usable_stack from Thread to JavaThread (guard
> > regions are
> > only relevant for JavaThreads).
> >
> > Clarified functionality and use of the three "in stack"
> variants.
> >
> > Removed redundant check from is_in_stack:
> >
> > ! // Allow non Java threads to call this without stack_base
> > ! if (_stack_base == NULL) return true;
> >
> > As this is executed by the current thread, and the very first
> > thing a
> > thread does is set its stack base and size, it is
> impossible to
> > find a
> > NULL stack_base (which is already asserted inside
> stack_base()). [I
> > tested this extensively just as a sanity check: tiers 1-5
> plus
> > hotspot
> > runtime/serviceability/gc.]
> >
> > Misc cleanup to use stack_end() rather than recalulate it.
> >
> > ---
> >
> > There are some further possible cleanups here but I
> didn't want
> > to go
> > too far with things that would obscure the functional changes
> > too much.
> > As mentioned in the bug report the three "in stack"
> functions would
> > benefit from some minor renamings so that their
> relationship is
> > clearer.
> > But I can leave that to a follow on RFE. Further, it may be
> > possible to
> > replace a lot of the remaining uses of stack_base() with
> a more
> > constrained "in stack" function that takes a limit. For
> example,
> > rather
> > than something like:
> >
> > if (thread->stack_base() > fp && fp >= sp)
> >
> > have:
> >
> > if (thread->is_in_stack_range(fp /* addr*/, sp /*limit*/))
> >
> > which checks the given addr against stack_base and the limit,
> > and checks
> > the limit against stack_end(). The difficultly may lie in
> > determining
> > whether checking against the limit should be a > or >=
> test, as
> > it will
> > be dependent on the context. Again this seems like
> something for a
> > second RFE.
> >
> > ---
> >
> > Testing:
> >
> > Thanks to Andrew Haley for taking the frame changes for a
> spin on
> > ARM/PPC/Aarch64/S390(?).
> >
> > I also ran our tier 1 to 3 testing on x86 and sparc.
> >
> > Thanks,
> > David
> >
>
More information about the hotspot-dev
mailing list