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

[David Holmes]

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