Skip to content

Commit

Permalink
fix inline method cache sync bug
Browse files Browse the repository at this point in the history 
`cd` is passed to method call functions to method invocation
functions, but `cd` can be manipulated by other ractors simultaneously
so it contains thread-safety issue.

To solve this issue, this patch stores `ci` and found `cc` to `calling`
and stops to pass `cd`.
  • Loading branch information
@ko1
ko1 committed Dec 15, 2020
1 parent 40b7358 commit aa6287c
Show file tree
Hide file tree
Showing 8 changed files with 260 additions and 281 deletions.
5 changes: 0 additions & 5 deletions insns.def
View file
Expand Up @@ -891,11 +891,6 @@ invokeblock
// attr rb_snum_t sp_inc = sp_inc_of_invokeblock(cd->ci);
// attr rb_snum_t comptime_sp_inc = sp_inc_of_invokeblock(ci);
{
if (UNLIKELY(vm_cc_call(cd->cc) != vm_invokeblock_i)) {
const struct rb_callcache *cc = vm_cc_new(0, NULL, vm_invokeblock_i);
RB_OBJ_WRITE(GET_ISEQ(), &cd->cc, cc);
}

VALUE bh = VM_BLOCK_HANDLER_NONE;
val = vm_sendish(ec, GET_CFP(), cd, bh, vm_search_invokeblock);

Expand Down
5 changes: 2 additions & 3 deletions internal/vm.h
View file
Expand Up @@ -27,8 +27,7 @@ struct rb_callable_method_entry_struct; /* in method.h */
struct rb_method_definition_struct; /* in method.h */
struct rb_execution_context_struct; /* in vm_core.h */
struct rb_control_frame_struct; /* in vm_core.h */
struct rb_calling_info; /* in vm_core.h */
struct rb_call_data;
struct rb_callinfo; /* in vm_core.h */

enum method_missing_reason {
MISSING_NOENTRY = 0x00,
Expand Down Expand Up @@ -93,7 +92,7 @@ VALUE rb_eql_opt(VALUE obj1, VALUE obj2);

struct rb_iseq_struct;
MJIT_SYMBOL_EXPORT_BEGIN
void rb_vm_search_method_slowpath(VALUE cd_owner, struct rb_call_data *cd, VALUE klass);
const struct rb_callcache *rb_vm_search_method_slowpath(const struct rb_callinfo *ci, VALUE klass);
MJIT_SYMBOL_EXPORT_END

/* vm_method.c */
Expand Down
4 changes: 2 additions & 2 deletions template/call_iseq_optimized.inc.tmpl
View file
Expand Up @@ -16,10 +16,10 @@
% P.each{|param|
% L.each{|local|
static VALUE
<%= fname(param, local) %>(rb_execution_context_t *ec, rb_control_frame_t *cfp, struct rb_calling_info *calling, struct rb_call_data *cd)
<%= fname(param, local) %>(rb_execution_context_t *ec, rb_control_frame_t *cfp, struct rb_calling_info *calling)
{
RB_DEBUG_COUNTER_INC(ccf_iseq_fix);
return vm_call_iseq_setup_normal(ec, cfp, calling, vm_cc_cme(cd->cc), 0, <%= param %>, <%= local %>);
return vm_call_iseq_setup_normal(ec, cfp, calling, vm_cc_cme(calling->cc), 0, <%= param %>, <%= local %>);
}

% }
Expand Down
5 changes: 3 additions & 2 deletions tool/ruby_vm/views/_mjit_compile_send.erb
View file
Expand Up @@ -76,8 +76,9 @@

if (vm_cc_cme(captured_cc)->def->type == VM_METHOD_TYPE_CFUNC) {
% # TODO: optimize this more
fprintf(f, " struct rb_call_data cc_cd = { .ci = (CALL_INFO)0x%"PRIxVALUE", .cc = cc };\n", (VALUE)ci); // creating local cd here because operand's cd->cc may not be the same as inlined cc.
fprintf(f, " val = vm_call_cfunc_with_frame(ec, reg_cfp, &calling, &cc_cd);\n");
fprintf(f, " calling.ci = (CALL_INFO)0x%"PRIxVALUE";\n", (VALUE)ci); // creating local cd here because operand's cd->cc may not be the same as inlined cc.
fprintf(f, " calling.cc = cc;");
fprintf(f, " val = vm_call_cfunc_with_frame(ec, reg_cfp, &calling);\n");
}
else { // VM_METHOD_TYPE_ISEQ
% # fastpath_applied_iseq_p checks rb_simple_iseq_p, which ensures has_opt == FALSE
Expand Down
3 changes: 1 addition & 2 deletions vm_callinfo.h
View file
Expand Up @@ -267,8 +267,7 @@ vm_ci_markable(const struct rb_callinfo *ci)
typedef VALUE (*vm_call_handler)(
struct rb_execution_context_struct *ec,
struct rb_control_frame_struct *cfp,
struct rb_calling_info *calling,
struct rb_call_data *cd);
struct rb_calling_info *calling);

// imemo_callcache

Expand Down
2 changes: 2 additions & 0 deletions vm_core.h
View file
Expand Up @@ -238,6 +238,8 @@ union iseq_inline_storage_entry {
};

struct rb_calling_info {
const struct rb_callinfo *ci;
const struct rb_callcache *cc;
VALUE block_handler;
VALUE recv;
int argc;
Expand Down
34 changes: 16 additions & 18 deletions vm_eval.c
View file
Expand Up @@ -38,32 +38,30 @@ typedef enum call_type {
} call_type;

static VALUE send_internal(int argc, const VALUE *argv, VALUE recv, call_type scope);
static VALUE vm_call0_body(rb_execution_context_t* ec, struct rb_calling_info *calling, struct rb_call_data *cd, const VALUE *argv);
static VALUE vm_call0_body(rb_execution_context_t* ec, struct rb_calling_info *calling, const VALUE *argv);

#ifndef MJIT_HEADER

MJIT_FUNC_EXPORTED VALUE
rb_vm_call0(rb_execution_context_t *ec, VALUE recv, ID id, int argc, const VALUE *argv, const rb_callable_method_entry_t *me, int kw_splat)
rb_vm_call0(rb_execution_context_t *ec, VALUE recv, ID id, int argc, const VALUE *argv, const rb_callable_method_entry_t *cme, int kw_splat)
{
struct rb_calling_info calling = {
.ci = &VM_CI_ON_STACK(id, kw_splat ? VM_CALL_KW_SPLAT : 0, argc, NULL),
.cc = &VM_CC_ON_STACK(Qfalse, vm_call_general, { 0 }, cme),
.block_handler = VM_BLOCK_HANDLER_NONE,
.recv = recv,
.argc = argc,
.kw_splat = kw_splat,
};
struct rb_call_data cd = {
.ci = &VM_CI_ON_STACK(id, kw_splat ? VM_CALL_KW_SPLAT : 0, argc, NULL),
.cc = &VM_CC_ON_STACK(Qfalse, vm_call_general, { 0 }, me),
};

return vm_call0_body(ec, &calling, &cd, argv);
return vm_call0_body(ec, &calling, argv);
}

static VALUE
vm_call0_cfunc_with_frame(rb_execution_context_t* ec, struct rb_calling_info *calling, struct rb_call_data *cd, const VALUE *argv)
vm_call0_cfunc_with_frame(rb_execution_context_t* ec, struct rb_calling_info *calling, const VALUE *argv)
{
const struct rb_callinfo *ci = cd->ci;
const struct rb_callcache *cc = cd->cc;
const struct rb_callinfo *ci = calling->ci;
const struct rb_callcache *cc = calling->cc;
VALUE val;
const rb_callable_method_entry_t *me = vm_cc_cme(cc);
const rb_method_cfunc_t *cfunc = UNALIGNED_MEMBER_PTR(me->def, body.cfunc);
Expand Down Expand Up @@ -106,17 +104,17 @@ vm_call0_cfunc_with_frame(rb_execution_context_t* ec, struct rb_calling_info *ca
}

static VALUE
vm_call0_cfunc(rb_execution_context_t *ec, struct rb_calling_info *calling, struct rb_call_data *cd, const VALUE *argv)
vm_call0_cfunc(rb_execution_context_t *ec, struct rb_calling_info *calling, const VALUE *argv)
{
return vm_call0_cfunc_with_frame(ec, calling, cd, argv);
return vm_call0_cfunc_with_frame(ec, calling, argv);
}

/* `ci' should point temporal value (on stack value) */
static VALUE
vm_call0_body(rb_execution_context_t *ec, struct rb_calling_info *calling, struct rb_call_data *cd, const VALUE *argv)
vm_call0_body(rb_execution_context_t *ec, struct rb_calling_info *calling, const VALUE *argv)
{
const struct rb_callinfo *ci = cd->ci;
const struct rb_callcache *cc = cd->cc;
const struct rb_callinfo *ci = calling->ci;
const struct rb_callcache *cc = calling->cc;

VALUE ret;

Expand All @@ -137,13 +135,13 @@ vm_call0_body(rb_execution_context_t *ec, struct rb_calling_info *calling, struc
*reg_cfp->sp++ = argv[i];
}

vm_call_iseq_setup(ec, reg_cfp, calling, cd);
vm_call_iseq_setup(ec, reg_cfp, calling);
VM_ENV_FLAGS_SET(ec->cfp->ep, VM_FRAME_FLAG_FINISH);
return vm_exec(ec, TRUE); /* CHECK_INTS in this function */
}
case VM_METHOD_TYPE_NOTIMPLEMENTED:
case VM_METHOD_TYPE_CFUNC:
ret = vm_call0_cfunc(ec, calling, cd, argv);
ret = vm_call0_cfunc(ec, calling, argv);
goto success;
case VM_METHOD_TYPE_ATTRSET:
if (calling->kw_splat &&
Expand All @@ -168,7 +166,7 @@ vm_call0_body(rb_execution_context_t *ec, struct rb_calling_info *calling, struc
ret = rb_attr_get(calling->recv, vm_cc_cme(cc)->def->body.attr.id);
goto success;
case VM_METHOD_TYPE_BMETHOD:
ret = vm_call_bmethod_body(ec, calling, cd, argv);
ret = vm_call_bmethod_body(ec, calling, argv);
goto success;
case VM_METHOD_TYPE_ZSUPER:
case VM_METHOD_TYPE_REFINED:
Expand Down

0 comments on commit aa6287c

Please sign in to comment.