[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR (S): 8239895: assert(_stack_base != 0LL) failed: Sanity check
From:       David Holmes <david.holmes () oracle ! com>
Date:       2020-03-31 21:34:20
Message-ID: 2423bf9a-ee1c-4eac-3fc2-2ac5efb9afa9 () oracle ! com
[Download RAW message or body]

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. I will submit a tier 5/6 run while waiting 
for a second review.

Thanks,
David

> Dan
> 
> 
>>
>> Thanks,
>> David
>>
> 
[prev in list] [next in list] [prev in thread] [next in thread]