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

[Daniel D. Daugherty]

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