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

[David Holmes]

Ping! Second review please.

Thanks,
David

On 1/04/2020 12:56 pm, David Holmes 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
>>>>>
>>>>
>>