RFR(s): 8218147: make_walkable asserts on multiple calls

[Robbin Ehn]

Hi Dan,

On 4/16/19 3:22 PM, Daniel D. Daugherty wrote:
> On 4/16/19 8:06 AM, Robbin Ehn wrote:
>> Hi, here is v4.
>>
>> http://cr.openjdk.java.net/~rehn/8218147/v4/webrev/index.html
> 
> src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp
>      L362:   thread->set_trace_flag();  // Provides StoreLoad, needed to keep 
> read of thread state not floating up.
>          Typo: s/not floating/from floating/

Fixed!

> 
> src/hotspot/share/runtime/thread.hpp
>      No comments.
> 
> Thumbs up!
> 

Thanks Dan!

/Robbin

> Dan
> 
> 
>>
>> Re-prod test and t1-t2.
>>
>> Thanks, Robbin
>>
>> On 4/15/19 10:58 AM, Robbin Ehn wrote:
>>> Hi, please review.
>>>
>>> After reexamine this issue:
>>> Threads in native must always have their stack walkable.
>>> JFR sampler should never need to make a stack walkable (for native sample).
>>>
>>> I manage to locally reproduce reliable with changes to JFR sampler and having
>>> hundreds of threads running similar code as the in the bug.
>>> (Looping creating an array with negative size.)
>>>
>>> I found a place where we don't proper look at the suspend flags.
>>> The java thread can thus escape native and make it's stack unwalkable and later
>>> it tries to make it walkable at the same time as the JFR sampler.
>>>
>>> By removing some kind of fast check and instead always call the 
>>> check_safepoint_and_suspend_for_native_trans I can no longer reproduce.
>>> (which have the JFR native trans suspend check)
>>> And it passes t1-5.
>>>
>>> Code:
>>> http://cr.openjdk.java.net/~rehn/8218147/v3/webrev/
>>> Issue:
>>> https://bugs.openjdk.java.net/browse/JDK-8218147
>>>
>>> Thanks, Robbin
>>>
>>> On 4/5/19 5:43 PM, Robbin Ehn wrote:
>>>> Hi Dean,
>>>>
>>>> Sorry, I missed this mail.
>>>> Yes we can do that.
>>>> Ignore my other mail, I'll update.
>>>>
>>>> Thanks, Robbin
>>>>
>>>>
>>>> dean.long at oracle.com skrev: (5 april 2019 09:22:24 CEST)
>>>>> On 4/4/19 5:16 PM, dean.long at oracle.com wrote:
>>>>>>
>>>>>>>>
>>>>>>>> If it's already set, should we check that _last_Java_pc matches the
>>>>>
>>>>>>>> new value?
>>>>>>>
>>>>>>> We manually set the pc in several places, so if it's set, it's not
>>>>>>> certain that
>>>>>>> it should be the same as in last sp.
>>>>>>> I can't distinguish between the cases.
>>>>>>>
>>>>>>
>>>>>> If we get pc from sp[-1] then it should match, but you're right, we
>>>>>> sometimes get pc from somewhere else.
>>>>>
>>>>> How about if we combine the !walkable check and the
>>>>> capture_last_Java_pc() logic into a single method?
>>>>> Then we can do something like:
>>>>>
>>>>>      if (!walkable()) {
>>>>>          address pc = (address)_last_Java_sp[-1];
>>>>>          address a = Atomic::cmpxchg(pc, &_last_Java_pc, NULL);
>>>>>          assert(a == NULL || a == pc, "unexpected PC %p", a);
>>>>>      }
>>>>>
>>>>> dl
>