RFR (S): 8239895: assert(_stack_base != 0LL) failed: Sanity check

[Thomas Stüfe]

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?

--
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 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.

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)

But these are idle nitpickings, feel free to ignore them :)


Cheers, Thomas

On Wed, Apr 1, 2020 at 4:56 AM David Holmes <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
> >>>>
> >>>
> >
>