Re: [PATCH 2/2] x86: fix bug in emit_jni_trampoline()
2009/7/6 Pekka Enberg :
> On Mon, 2009-07-06 at 01:16 +0200, Tomek Grabiec wrote:
>> We cannot overwrite a register which is not saved (ESI) because this
>> will lead to a corruption in JIT code. The proper solution is
>> to copy the call arguments.
>>
>> Signed-off-by: Tomek Grabiec
>
> I'm confused. What exactly is the problem here and why do we need to
> copy arguments?
>
We need to push struct vm_jni_env * before calling JNI target. When
trampoline is called, it contains a return address on top
of the stack. Therefore we need to pop the return address and then
push the JNI env pointer, then call the target, and on return
cleanup JNI env argument and restore return address.
The problem is that we cannot scratch any register in jni_trampoline
that liveness analysis is not aware of being scratched.
Therefore we cannot scratch any caller saved register. Linveness
analysis assumes that all callee saved registers are scratched
but we cannot use any of those either because they can be scratched in
the JNI target function.
The approach in the patch simply copies call arguments in trampoline
and pushes JNI env pointer on top of
them and calls the target.
One alternative solution:
For each native method call we could reserve the space on stack for
one word above call arguments to which
the return address can be saved and restored later
>> ---
>> arch/x86/emit-code.c | 37 +++--
>> include/jit/compiler.h | 3 ++-
>> include/jit/emit-code.h | 3 ++-
>> jit/trampoline.c | 7 ---
>> 4 files changed, 39 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/x86/emit-code.c b/arch/x86/emit-code.c
>> index 236ba0e..c4ba179 100644
>> --- a/arch/x86/emit-code.c
>> +++ b/arch/x86/emit-code.c
>> @@ -1389,21 +1389,46 @@ void emit_trampoline(struct compilation_unit *cu,
>> }
>>
>> void emit_jni_trampoline(struct buffer *buf, struct vm_jni_env *jni_env,
>> - void *target)
>> + void *target, struct vm_method *method)
>> {
>> + unsigned long args_size;
>> +
>> jit_text_lock();
>>
>> buf->buf = jit_text_ptr();
>>
>> - /* save return address into caller-saved register */
>> - __emit_pop_reg(buf, REG_ESI);
>> + __emit_push_reg(buf, REG_ESI);
>> + __emit_push_reg(buf, REG_EDI);
>> + __emit_push_reg(buf, REG_ECX);
>> +
>> + /*
>> + * Make a copy of call arguments
>> + */
>> + __emit_mov_reg_reg(buf, REG_ESP, REG_ESI);
>> + __emit_add_imm_reg(buf, (3 + 1) * sizeof(unsigned long), REG_ESI);
>> +
>> + args_size = sizeof(unsigned long) + (method->args_count + 1);
>> +
>> + __emit_add_imm_reg(buf, -args_size, REG_ESP);
>> + __emit_mov_reg_reg(buf, REG_ESP, REG_EDI);
>> +
>> + __emit_mov_imm_reg(buf, method->args_count + 1, REG_ECX);
>> +
>> + /* CLD */
>> + emit(buf, 0xfc);
>> +
>> + /* REP MOVSD */
>> + emit(buf, 0xf3);
>> + emit(buf, 0xa5);
>>
>> __emit_push_imm(buf, (unsigned long) jni_env);
>> __emit_call(buf, target);
>> - __emit_add_imm_reg(buf, 4, REG_ESP);
>> + __emit_add_imm_reg(buf, args_size + sizeof(unsigned long), REG_ESP);
>> +
>> + __emit_pop_reg(buf, REG_ECX);
>> + __emit_pop_reg(buf, REG_EDI);
>> + __emit_pop_reg(buf, REG_ESI);
>>
>> - /* return to caller*/
>> - __emit_push_reg(buf, REG_ESI);
>> emit_ret(buf);
>>
>> jit_text_reserve(buffer_offset(buf));
>> diff --git a/include/jit/compiler.h b/include/jit/compiler.h
>> index 8584954..e2609ef 100644
>> --- a/include/jit/compiler.h
>> +++ b/include/jit/compiler.h
>> @@ -14,6 +14,7 @@ struct compilation_unit;
>> struct expression;
>> struct statement;
>> struct buffer;
>> +struct vm_method;
>>
>> struct fixup_site {
>> /* Compilation unit to which relcall_insn belongs */
>> @@ -62,7 +63,7 @@ void *jit_magic_trampoline(struct compilation_unit *);
>> struct jit_trampoline *alloc_jit_trampoline(void);
>> struct jni_trampoline *alloc_jni_trampoline(void);
>> struct jit_trampoline *build_jit_trampoline(struct compilation_unit *);
>> -struct jni_trampoline *build_jni_trampoline(void *);
>> +struct jni_trampoline *build_jni_trampoline(struct vm_method *, void *);
>> void free_jit_trampoline(struct jit_trampoline *);
>> void free_jni_trampoline(struct jni_trampoline *);
>>
>> diff --git a/include/jit/emit-code.h b/include/jit/emit-code.h
>> index 07c41bf..291955f 100644
>> --- a/include/jit/emit-code.h
>> +++ b/include/jit/emit-code.h
>> @@ -8,6 +8,7 @@ struct buffer;
>> struct insn;
>> struct vm_object;
>> struct vm_jni_env;
>> +struct vm_method;
>>
>> enum emitter_type {
>> NO_OPERANDS = 1,
>> @@ -39,6 +40,6 @@ extern void emit_body(struct basic_block *, struct buffer
>> *);
>> extern void backpatch_branch_target(struct buffer *buf, struct insn *insn,
>> unsigned long target_offset);
>> extern void emit_jni_trampoline(struct buffer *buf, struct vm_jni_env
>> *jni_en
Re: [PATCH 2/2] x86: fix bug in emit_jni_trampoline()
On Mon, 2009-07-06 at 01:16 +0200, Tomek Grabiec wrote:
> We cannot overwrite a register which is not saved (ESI) because this
> will lead to a corruption in JIT code. The proper solution is
> to copy the call arguments.
>
> Signed-off-by: Tomek Grabiec
I'm confused. What exactly is the problem here and why do we need to
copy arguments?
> ---
> arch/x86/emit-code.c| 37 +++--
> include/jit/compiler.h |3 ++-
> include/jit/emit-code.h |3 ++-
> jit/trampoline.c|7 ---
> 4 files changed, 39 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/emit-code.c b/arch/x86/emit-code.c
> index 236ba0e..c4ba179 100644
> --- a/arch/x86/emit-code.c
> +++ b/arch/x86/emit-code.c
> @@ -1389,21 +1389,46 @@ void emit_trampoline(struct compilation_unit *cu,
> }
>
> void emit_jni_trampoline(struct buffer *buf, struct vm_jni_env *jni_env,
> - void *target)
> + void *target, struct vm_method *method)
> {
> + unsigned long args_size;
> +
> jit_text_lock();
>
> buf->buf = jit_text_ptr();
>
> - /* save return address into caller-saved register */
> - __emit_pop_reg(buf, REG_ESI);
> + __emit_push_reg(buf, REG_ESI);
> + __emit_push_reg(buf, REG_EDI);
> + __emit_push_reg(buf, REG_ECX);
> +
> + /*
> + * Make a copy of call arguments
> + */
> + __emit_mov_reg_reg(buf, REG_ESP, REG_ESI);
> + __emit_add_imm_reg(buf, (3 + 1) * sizeof(unsigned long), REG_ESI);
> +
> + args_size = sizeof(unsigned long) + (method->args_count + 1);
> +
> + __emit_add_imm_reg(buf, -args_size, REG_ESP);
> + __emit_mov_reg_reg(buf, REG_ESP, REG_EDI);
> +
> + __emit_mov_imm_reg(buf, method->args_count + 1, REG_ECX);
> +
> + /* CLD */
> + emit(buf, 0xfc);
> +
> + /* REP MOVSD */
> + emit(buf, 0xf3);
> + emit(buf, 0xa5);
>
> __emit_push_imm(buf, (unsigned long) jni_env);
> __emit_call(buf, target);
> - __emit_add_imm_reg(buf, 4, REG_ESP);
> + __emit_add_imm_reg(buf, args_size + sizeof(unsigned long), REG_ESP);
> +
> + __emit_pop_reg(buf, REG_ECX);
> + __emit_pop_reg(buf, REG_EDI);
> + __emit_pop_reg(buf, REG_ESI);
>
> - /* return to caller*/
> - __emit_push_reg(buf, REG_ESI);
> emit_ret(buf);
>
> jit_text_reserve(buffer_offset(buf));
> diff --git a/include/jit/compiler.h b/include/jit/compiler.h
> index 8584954..e2609ef 100644
> --- a/include/jit/compiler.h
> +++ b/include/jit/compiler.h
> @@ -14,6 +14,7 @@ struct compilation_unit;
> struct expression;
> struct statement;
> struct buffer;
> +struct vm_method;
>
> struct fixup_site {
> /* Compilation unit to which relcall_insn belongs */
> @@ -62,7 +63,7 @@ void *jit_magic_trampoline(struct compilation_unit *);
> struct jit_trampoline *alloc_jit_trampoline(void);
> struct jni_trampoline *alloc_jni_trampoline(void);
> struct jit_trampoline *build_jit_trampoline(struct compilation_unit *);
> -struct jni_trampoline *build_jni_trampoline(void *);
> +struct jni_trampoline *build_jni_trampoline(struct vm_method *, void *);
> void free_jit_trampoline(struct jit_trampoline *);
> void free_jni_trampoline(struct jni_trampoline *);
>
> diff --git a/include/jit/emit-code.h b/include/jit/emit-code.h
> index 07c41bf..291955f 100644
> --- a/include/jit/emit-code.h
> +++ b/include/jit/emit-code.h
> @@ -8,6 +8,7 @@ struct buffer;
> struct insn;
> struct vm_object;
> struct vm_jni_env;
> +struct vm_method;
>
> enum emitter_type {
> NO_OPERANDS = 1,
> @@ -39,6 +40,6 @@ extern void emit_body(struct basic_block *, struct buffer
> *);
> extern void backpatch_branch_target(struct buffer *buf, struct insn *insn,
> unsigned long target_offset);
> extern void emit_jni_trampoline(struct buffer *buf, struct vm_jni_env
> *jni_env,
> - void *target);
> + void *target, struct vm_method *method);
>
> #endif /* JATO_EMIT_CODE_H */
> diff --git a/jit/trampoline.c b/jit/trampoline.c
> index 0de2db5..42f3afa 100644
> --- a/jit/trampoline.c
> +++ b/jit/trampoline.c
> @@ -65,7 +65,8 @@ static void *jit_native_trampoline(struct compilation_unit
> *cu)
> add_cu_mapping((unsigned long)ret, cu);
>
> if (!method->jni_trampoline)
> - method->jni_trampoline = build_jni_trampoline(ret);
> + method->jni_trampoline =
> + build_jni_trampoline(method, ret);
>
> return buffer_ptr(method->jni_trampoline->objcode);
> }
> @@ -151,13 +152,13 @@ struct jit_trampoline *build_jit_trampoline(struct
> compilation_unit *cu)
> return trampoline;
> }
>
> -struct jni_trampoline *build_jni_trampoline(void *target)
> +struct jni_trampoline *build_jni_trampoline(struct vm_method *vmm, void
> *target)
> {
> struct jni_trampoline *trampoline;
>
>
