RFR(S): 8031475: Missing oopmap in patching stubs

[Bertrand Delsart]

On 30/04/14 15:06, Nils Eliasson wrote:
> Hi,
>
> I would like some feedback on this change from the c1 experts. It's made
> in platform dependent code and will be added to the other plattforms as
> well before submit.
>
> This change fixes a bug that has been observed in testing, and dug up
> from a core file, but haven't reproduced standalone yet. When patching
> for checkcast the oop we are casting is not kept in an oopmap during the
> runtime patching call, a one time chance per run.
>
> The current change is for all the patching stub cases (access_field_id,
> load_klass_id, load_mirror_id, load_appendix_id) - is that needed? Do
> you see any potential for breaking anything? It is difficult to trigger
> a GC at exact this point during a test.

It is not needed for all patching operations and might be dangerous. You 
should add _obj to the oopMap only if you are sure _obj is an oop.

In most cases, these oops are input arguments of the C1 instruction and 
should have been added to the patching info before calling 
PatchingStub::emit_code.

Problems mostly occur when loading such an oop in a temporary register 
(like here to check for NULL) before potentially calling some patching 
code. In that case, you can either:
- fix 'info' in the code that calls the patching stub generator (e.g. 
the caller is responsible for ensuring the value in the temporary 
register is protected)
- or ignore the value in the temporary register when patching occurred 
(e.g. reload the oop after the runtime patching call).

This would be cleaner than hard coding in the patching stub that for 
some patching_ids, _obj is an oop that is not yet covered by an oopmap.

>
> http://cr.openjdk.java.net/~neliasso/8031475/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-8031475
>
> Thanks,
> Nils Eliasson
>
>
> Code example:
>
>    0x00007f20c943590c: mov    $0x718d65d38,%rdx  ;   {oop(a
> 'BeanImpl2')}   // oops to be casted in rdx
>    0x00007f20c9435916: cmp    $0x0,%rdx
>    0x00007f20c943591a: je     0x00007f20c9435967   // jump to patching stub
>    // patch location
>    0x00007f20c9435920: jmpq   0x00007f20c94359c5  ;   {no_reloc}
>    0x00007f20c9435925: add    %al,(%rax)
>    0x00007f20c9435927: add    %al,(%rax)
>    0x00007f20c9435929: add    %cl,-0x3eb7f786(%rbx)
>    0x00007f20c943592f: out    %eax,$0x3
>    // end of patch location
>    0x00007f20c9435931: cmp    %rbx,%rdi
>    0x00007f20c9435934: je     0x00007f20c9435967 // A dereference of rdx
> somewhere here may crash if the oop has moved during a gc
>    0x00007f20c943593a: mov    0x10(%rbx),%esi
>    0x00007f20c943593d: cmp    (%rdi,%rsi,1),%rbx
>    0x00007f20c9435941: je     0x00007f20c9435967
>
>    ...
>
>    ;; PatchingStub slow case
>    ;;  patch template
>    0x00007f20c94359b6: mov    $0x0,%rbx          ;   {metadata(NULL)}
>    ;; patch data encoded as movl
>    0x00007f20c94359c0: mov    $0xa050f00,%eax
>    ;; patch entry point
>    0x00007f20c94359c5: callq  0x00007f20c942e3e0  ; OopMap{[32]=Oop
> off=266}   // rdx not live here, may safepoint on return from runtime call
>                                                  ;*checkcast
>                                                  ; -
> TestCheckCast::main at 25 (line 24)
>                                                  ;   {runtime_call}
>    0x00007f20c94359ca: jmpq   0x00007f20c9435920   // back to normal
> control flow after patching
>


-- 
Bertrand Delsart,                     Grenoble Engineering Center
Oracle,         180 av. de l'Europe,          ZIRST de Montbonnot
38334 Saint Ismier,                                        FRANCE
bertrand.delsart at oracle.com             Phone : +33 4 76 18 81 23

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged
information. Any unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended recipient,
please contact the sender by reply email and destroy all copies of
the original message.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~