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

[dean.long at oracle.com]

On 4/4/19 3:29 AM, Robbin Ehn wrote:
> 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?
>

I would say yes, unless you want to add the Atomic::read now.

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

I don't think I took into account concurrent access when I added those 
asserts :-)

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

dl

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