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

[Robbin Ehn]

Hi,

On 4/3/19 7:20 PM, dean.long at oracle.com wrote:
> Does the store need to be atomic?

Some people have started to use it for concurrently stored/read values.
In this case VMThread and JFR sampler thread can execute this code at the same 
time when a JavaThread is in native.
Since _last_Java_pc is volatile aligned word-size there is no issue, just a 
gesture. (to make the gesture better it should also be read with Atomic::read)

Shall I remove it?

> 
> Any idea what problem the original assert was trying to catch?

No... you push it as part of your fix for 8161598 :)
I do not see it related, several assert which made sense was added.

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

Thanks, Robbin

> 
> dl
> 
> On 4/3/19 3:36 AM, Robbin Ehn wrote:
>> Hi all, please review.
>>
>> If a JavaThread in native both gets selected for java-stack sampling and a
>> handshake both VMThread and JFR sampler will call make_walkable. There is an
>> assert making sure we do not do this twice. Since we only store _last_Java_pc
>> from sp, we can allow it be executed multiple times for both aarch64/x64 which 
>> have the assert.
>>
>> The asserts comes from:
>> 8161598: Kitchensink fails: assert(nm->insts_contains(original_pc)) failed: 
>> original PC must be in nmethod/CompiledMethod
>>
>> They seems not to be directly connected to the bug.
>>
>> Issue:
>> https://bugs.openjdk.java.net/browse/JDK-8218147
>>
>> Webrev:
>> http://cr.openjdk.java.net/~rehn/8218147/webrev/
>>
>> Compiled aarch64, x64 passes t1-3.
>>
>> Thanks, Robbin
>