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

[Daniel D. Daugherty]

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/

src/hotspot/share/runtime/thread.hpp
     No comments.

Thumbs up!

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