RFR (S): 8239895: assert(_stack_base != 0LL) failed: Sanity check
David Holmes
david.holmes at oracle.com
Thu Apr 2 13:54:51 UTC 2020
Hi Thomas,
Thanks for looking at this.
On 2/04/2020 8:49 pm, Thomas Stüfe wrote:
> Hi David,
>
> src/hotspot/share/runtime/thread.cpp
>
> I do not understand why you removed the assert in
> Thread::record_stack_base_and_size() ? Is there any case where this
> could be NULL?
I added that assert under JDK-8241043 (which this change backs out
again) as a way to try and track down where the failure was happening.
There's really no way it could be NULL.
> --
> src/hotspot/os_cpu/solaris_x86/os_solaris_x86.cpp
> src/hotspot/os_cpu/solaris_sparc/os_solaris_sparc.cpp
>
>
> Isn't os::Solaris::valid_ucontext() called from signal handling, so the
> Thread may have any state? In that case, it may be safer to ignore base
> NULL.
I restored things to the way they were before JDK-8238988. I thought it
odd that the Solaris version of this code used the stack_base() call
that checked for NULL, but it did so ...
A new thread would have to take a signal before setting the stack-base
for this to be an issue, which is an extremely short window so there is
really little chance of SEGV.
> --
>
> I wonder whether it would make more sense to do it the other way around:
> "check-if-adress-is-in-stack-but-its-okay-if-there-is-none" seems more
> of a special requirement.
But that is exactly what on_local_stack() previously did.
> In that case, Alternative naming proposal could follow our "_or_null"
> scheme:
>
> is_in_full_stack_checked -> is_in_full_stack (asserts for base==0)
> is_in_full_stack -> is_in_full_stack_or_null (returns false for base==0)
Yes I initially thought about doing it from that side, but after some to
and fro with Dan I decided to do it from the current side. Bit of a coin
toss really. There were fewer changes needed this way.
> But these are idle nitpickings, feel free to ignore them :)
I hope I have given some explanation to address your comments :)
Thanks,
David
-----
>
>
> Cheers, Thomas
>
> On Wed, Apr 1, 2020 at 4:56 AM David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
> Hi Dan,
>
> Tiers 5 and 6 passed.
>
> Can I get a second review please.
>
> Thanks,
> David
>
> On 1/04/2020 10:35 am, Daniel D. Daugherty wrote:
> > On 3/31/20 5:34 PM, David Holmes wrote:
> >> Hi Dan,
> >>
> >> On 1/04/2020 12:26 am, Daniel D. Daugherty wrote:
> >>> On 3/31/20 1:18 AM, David Holmes wrote:
> >>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8239895
> >>>> webrev: http://cr.openjdk.java.net/~dholmes/8239895/webrev/
> >>>
> >>> src/hotspot/cpu/aarch64/frame_aarch64.cpp
> >>> No comments.
> >>>
> >>> src/hotspot/cpu/arm/frame_arm.cpp
> >>> No comments.
> >>>
> >>> src/hotspot/cpu/ppc/frame_ppc.cpp
> >>> No comments.
> >>>
> >>> src/hotspot/cpu/s390/frame_s390.cpp
> >>> No comments.
> >>>
> >>> src/hotspot/cpu/x86/frame_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/share/runtime/thread.cpp
> >>> No comments (8241043 backout).
> >>>
> >>> src/hotspot/share/runtime/thread.hpp
> >>> No comments (new func and 8241043 backout).
> >>>
> >>>
> >>> Thumbs up.
> >>
> >> Thanks for the review!
> >>
> >>> More below...
> >>>
> >>>
> >>>> Prior to JDK-8238988 there were uses of stack_base() which
> checked
> >>>> it was initialized, and there was a raw use of _stack_base in
> >>>> on_local_stack() that did not need it to be initialized
> (because it
> >>>> may not be). After JDK-8238988 both cases call
> is_in_stack_range()
> >>>> which uses stack_base() and so asserts that the stack base is
> >>>> initialized in all cases. This leads to the assertion failures
> when
> >>>> the _stack_base is not initialised. The fix has three parts:
> >>>>
> >>>> 1. Rename is_in_full_stack to is_in_full_stack_checked - as it
> >>>> checks _stack_base is initialized via an assertion.
> >>>>
> >>>> 2. Add a new is_in_full_stack which doesn't use any assertions.
> >>>>
> >>>> 3. Update all the uses of stack_base() prior to JDK-8238988 that
> >>>> were changed to call is_in_full_stack, to now call
> >>>> is_in_full_stack_checked. There are not many of them. (The
> corollary
> >>>> to that is that all old calls to on_local_stack() call the new
> >>>> unchecked is_in_full_stack.)
> >>>>
> >>>> Here's the webrev for JDK-8238988 for comparison if desired:
> >>>>
> >>>> http://cr.openjdk.java.net/~dholmes/8238988/webrev/
> >>>
> >>> The above link doesn't work. I used this one:
> >>>
> >>> http://cr.openjdk.java.net/~dholmes/8238988/webrev.v3/
> >>
> >> Oops sorry about that.
> >>
> >>>>
> >>>> I also backed out the assertion changes that I made under:
> >>>>
> >>>> https://bugs.openjdk.java.net/browse/JDK-8241043
> >>>>
> >>>> as they were failing due to the use of get_thread_name(). I've
> filed
> >>>> a separate RFE for that issue:
> >>>>
> >>>> https://bugs.openjdk.java.net/browse/JDK-8241403
> >>>>
> >>>> Testing: tiers 1 - 3
> >>>
> >>> The test failures that we've been seeing are happening in Tier5
> and
> >>> Tier6.
> >>
> >> My testing was to check nothing new was unexpectedly broken. The
> test
> >> failures we occasionally see in tiers 5 and 6 do not reproduce
> easily
> >> and I've never had a failure occur in numerous such runs when
> trying
> >> to reproduce this originally.
> >
> > Sorry. I forgot that the bug hasn't been reproducing for you.
> >
> >
> >> I will submit a tier 5/6 run while waiting for a second review.
> >
> > That sounds good to me.
> >
> > Dan
> >
> >
> >>
> >> Thanks,
> >> David
> >>
> >>> Dan
> >>>
> >>>
> >>>>
> >>>> Thanks,
> >>>> David
> >>>>
> >>>
> >
>
More information about the hotspot-runtime-dev
mailing list