[8u] RFR: nativeInstruction atomicity and correctness of patching

[Andrew Haley]

On 05/07/16 18:51, Anton Kozlov wrote:
> 
> Hi, All!
> 
> Andrew, let me clarify, the concern is in cores cache coherence after executing
> NativeTrampolineCall::set_destination_mt_safe?
> 
> From my point of view, since
> 
>>>> looking at linux/arch/arm/include/asm/cacheflush.h and I can't see
>>>> anything like that.  All I see is that do_cache_op calls
>>>> __flush_cache_user_range, and all that does is ensure that the I and
>>>> D
>>>> caches are coherent.
>>
> 
> after any case from
> 1) instruction modification and ICache::flush
> 2) NativeTrampolineCall::set_destination_mt_safe and no data flush
> we came to the same state, when new data is in the dcache of single core.

We don't know that: the data is at least in the local dcache.  Some
of the data may be in other caches too.

> NativeTrampolineCall::set_destination_mt_safe is not different from any other
> instruction modification methods.

It's not wrong, but it does a pointless ICache::invalidate_word().
It's pointless because the word at addr() + 8 isn't an instruction,
as far as I can see.

> And we both agree that it's OK to call old destination even after modification.

Yes.

> And eventually data change will land to other core dchache, because there is
> fence on executing site, which you've mentioned.  
> set_destination_mt_safe is called when PatchingLock is taken, so it's followed
> by release.

> Executing thread could see only 2 options, since patch is word-aligned single
> word data chage:
> 1) new data. Then everything is OK, just call new destination
> 2) old data. Then execution stream should go to same patching code, c1_Runtime,
> which takes same PatchingLock, 
> and therefore executes acquire. Then it should see new data, perform no
> patching, leave patching routine and call new destination.

There is never any problem patching a single word.  That can be done
MT-safe if it's a word of data or it is a single patchable
instruction.  Both of these are guaranteed: nothing else is.

This is the code that smells bad:

 void NativeGeneralJump::replace_mt_safe(address instr_addr, address code_buffer) {
-  ShouldNotCallThis();
+  // FIXME NativeCall from patching_epilog nops filling
+  const int bytes_to_copy = NativeCall::instruction_size;
+  const address patching_switch_addr = code_buffer + bytes_to_copy;
+  NativeImmJump* patching_switch = NativeImmJump::from(patching_switch_addr);
+  assert(patching_switch->destination() == patching_switch_addr + NativeInstruction::arm_insn_sz,
+         "switch should be branch to next instr at this point");
+  patching_switch->set_destination(instr_addr + bytes_to_copy);
+  ICache::invalidate_word(patching_switch_addr);
+
+  NativeImmJump* nj = NativeImmJump::from(instr_addr); // checking that it is a jump
+  nj->set_destination(code_buffer);
+  ICache::invalidate_word(instr_addr);
+
 }

I don't think this is MT-safe.  I don't know why you call
ICache::invalidate_word twice.

As far as I can see the only place where
NativeGeneralJump::replace_mt_safe is used is in the C1 patching code,
but that multiple-instruction patching relies on ordering guarantees
we don't have on ARM.  It's hoping that if you patch things in a
certain order they will be seen by other cores in the same order.

Andrew.