RFR(m): 8220351: Cross-modifying code

[Robbin Ehn]

Hi Richard,

On 3/14/19 11:12 AM, Reingruber, Richard wrote:
> Hi Robbin,
> 
>    > the reason I heard for not doing invalidate_range/word in nmethod::oops_do
>    > is that it was deemed that a safepoint is 'long enough'.
> 
> This doesn't seem to be a very good reason. I did a little modification to your
> initial figure:

That's why we are adding the instruction barrier!

> 
> JavaThread:             | VMThread
> StoreLoad               |
>                          | update <immediate oop>
>                          | ? Global ICache Invalidate ?
>                          | disarm
> load thread_poll        |
> 
> Question would be: does an ICache flush happen after the immediate oop update?
> I would consider it problematic if not. Even if the safepoint was very long.

The icache flush assumes it can stop speculation and flush instruction cache in 
patchee thread.
Correct me if anything is wrong:
- sparc needs flush <adr> in all patchee threads.
- arm/aarch64 needs isb + ___clear_cache(adr,..) in all patchee threads.
- ppc needs isync in all patchee threads?
- s390 needs zarch_sync all in patchee threads?
- x86 needs cpuid in all in patchee threads.

So a "Global ICache Invalidate" done by patcher thread (VMThread in this case) 
have no effect in patchee threads!?

Adding a few words on x86 ICache::Invalidate/word.
We have TSO, so if the patchee thread can see poll is disarmed all other stores 
are visible. I have seen no case ICache::Invalidate/word would make a difference 
from a StoreStore/compiler barrier (still talking about platform code for x86) ?
The problem is if we prefect an instruction over the load of the poll.
If poll is disarmed this perfected instruction can be fully executed.
If that happens to contain an immediate oop, we don't know if it's the patched
oop or not.

/Robbin

> 
> Cheers Richard.
> 
> -----Original Message-----
> From: Robbin Ehn <robbin.ehn at oracle.com>
> Sent: Donnerstag, 14. März 2019 10:34
> To: Doerr, Martin <martin.doerr at sap.com>; David Holmes <david.holmes at oracle.com>; hotspot-dev at openjdk.java.net
> Cc: Reingruber, Richard <richard.reingruber at sap.com>
> Subject: Re: RFR(m): 8220351: Cross-modifying code
> 
> Hi Martin,
> 
> On 3/14/19 10:25 AM, Doerr, Martin wrote:
>> Hi Robbin,
>>
>>> But that is whole new subject and let's keep that separate from this :)
>> Sure, we can keep the modification side discussion separate. This change is already difficult enough ��
>>
>>
>> Btw. I haven't sent the ppc/s390 instruction cache barriers. Here they are:
>>
>> orderAccess_aix/linux_ppc.hpp:
>> inline void OrderAccess::instruction_pipeline() { inlasm_isync(); }
>>
>> orderAccess_linux_s390.hpp:
>> inline void OrderAccess::instruction_pipeline() { inlasm_zarch_sync(); }
>>
> 
> Thanks!
> 
>>> nmethod::oops_do calls the closure f->do_oop(r->oop_addr()) for oops encoded into the instruction stream.
>>> I couldn't find a call to AbstractICache::invalidate_range.
> 
> I think I miss-understood you here, the reason I heard for not doing
> invalidate_range/word in nmethod::oops_do is that it was deemed that a
> safepoint is 'long enough'.
> 
> /Robbin
> 
>>
>> Look for ICache::invalidate_range, I see 11 calls on x64.
>> And one call to AbstractICache/ICache::invalidate_word.
>> Btw I have spent some time looking at x86/icache_x86.cpp...
>> But that is whole new subject and let's keep that separate from this :)
>>
>> I'll create issues for some of these topics, which if nothing else should be
>> discussed.
>>
>> /Robbin
>>
>>>
>>> Does anybody know where it is or if it's missing?
>>>
>>> Best regards,
>>> Martin
>>>