# HG changeset patch # User Bram Moolenaar # Date 1670532303 -3600 # Node ID 60b1d266548da5603e613ce4fc5be2fdf56292d3 # Parent bb11538fdad28160d2b52d187d5314dbe883021f patch 9.0.1035: object members are not being marked as used Commit: https://github.com/vim/vim/commit/d28d7b94f5a76a817585c115dbf6fecac9b0b4cd Author: Bram Moolenaar Date: Thu Dec 8 20:42:00 2022 +0000 patch 9.0.1035: object members are not being marked as used Problem: Object members are not being marked as used, garbage collection may free them. Solution: Mark object members as used. Fix reference counting. diff --git a/src/eval.c b/src/eval.c --- a/src/eval.c +++ b/src/eval.c @@ -5233,12 +5233,15 @@ free_unref_items(int copyID) * themselves yet, so that it is possible to decrement refcount counters */ - // Go through the list of dicts and free items without the copyID. + // Go through the list of dicts and free items without this copyID. did_free |= dict_free_nonref(copyID); - // Go through the list of lists and free items without the copyID. + // Go through the list of lists and free items without this copyID. did_free |= list_free_nonref(copyID); + // Go through the list of objects and free items without this copyID. + did_free |= object_free_nonref(copyID); + #ifdef FEAT_JOB_CHANNEL // Go through the list of jobs and free items without the copyID. This // must happen before doing channels, because jobs refer to channels, but @@ -5405,7 +5408,8 @@ set_ref_in_callback(callback_T *cb, int } /* - * Mark all lists and dicts referenced through typval "tv" with "copyID". + * Mark all lists, dicts and other container types referenced through typval + * "tv" with "copyID". * "list_stack" is used to add lists to be marked. Can be NULL. * "ht_stack" is used to add hashtabs to be marked. Can be NULL. * @@ -5420,162 +5424,214 @@ set_ref_in_item( { int abort = FALSE; - if (tv->v_type == VAR_DICT) - { - dict_T *dd = tv->vval.v_dict; - - if (dd != NULL && dd->dv_copyID != copyID) - { - // Didn't see this dict yet. - dd->dv_copyID = copyID; - if (ht_stack == NULL) + switch (tv->v_type) + { + case VAR_DICT: + { + dict_T *dd = tv->vval.v_dict; + + if (dd != NULL && dd->dv_copyID != copyID) { - abort = set_ref_in_ht(&dd->dv_hashtab, copyID, list_stack); - } - else - { - ht_stack_T *newitem = ALLOC_ONE(ht_stack_T); - - if (newitem == NULL) - abort = TRUE; + // Didn't see this dict yet. + dd->dv_copyID = copyID; + if (ht_stack == NULL) + { + abort = set_ref_in_ht(&dd->dv_hashtab, copyID, list_stack); + } else { - newitem->ht = &dd->dv_hashtab; - newitem->prev = *ht_stack; - *ht_stack = newitem; + ht_stack_T *newitem = ALLOC_ONE(ht_stack_T); + + if (newitem == NULL) + abort = TRUE; + else + { + newitem->ht = &dd->dv_hashtab; + newitem->prev = *ht_stack; + *ht_stack = newitem; + } } } - } - } - else if (tv->v_type == VAR_LIST) - { - list_T *ll = tv->vval.v_list; - - if (ll != NULL && ll->lv_copyID != copyID) - { - // Didn't see this list yet. - ll->lv_copyID = copyID; - if (list_stack == NULL) + break; + } + + case VAR_LIST: + { + list_T *ll = tv->vval.v_list; + + if (ll != NULL && ll->lv_copyID != copyID) { - abort = set_ref_in_list_items(ll, copyID, ht_stack); - } - else - { - list_stack_T *newitem = ALLOC_ONE(list_stack_T); - - if (newitem == NULL) - abort = TRUE; + // Didn't see this list yet. + ll->lv_copyID = copyID; + if (list_stack == NULL) + { + abort = set_ref_in_list_items(ll, copyID, ht_stack); + } else { - newitem->list = ll; - newitem->prev = *list_stack; - *list_stack = newitem; + list_stack_T *newitem = ALLOC_ONE(list_stack_T); + + if (newitem == NULL) + abort = TRUE; + else + { + newitem->list = ll; + newitem->prev = *list_stack; + *list_stack = newitem; + } } } - } - } - else if (tv->v_type == VAR_FUNC) - { - abort = set_ref_in_func(tv->vval.v_string, NULL, copyID); - } - else if (tv->v_type == VAR_PARTIAL) - { - partial_T *pt = tv->vval.v_partial; - int i; - - if (pt != NULL && pt->pt_copyID != copyID) - { - // Didn't see this partial yet. - pt->pt_copyID = copyID; - - abort = set_ref_in_func(pt->pt_name, pt->pt_func, copyID); - - if (pt->pt_dict != NULL) + break; + } + + case VAR_FUNC: + { + abort = set_ref_in_func(tv->vval.v_string, NULL, copyID); + break; + } + + case VAR_PARTIAL: + { + partial_T *pt = tv->vval.v_partial; + int i; + + if (pt != NULL && pt->pt_copyID != copyID) { - typval_T dtv; - - dtv.v_type = VAR_DICT; - dtv.vval.v_dict = pt->pt_dict; - set_ref_in_item(&dtv, copyID, ht_stack, list_stack); + // Didn't see this partial yet. + pt->pt_copyID = copyID; + + abort = set_ref_in_func(pt->pt_name, pt->pt_func, copyID); + + if (pt->pt_dict != NULL) + { + typval_T dtv; + + dtv.v_type = VAR_DICT; + dtv.vval.v_dict = pt->pt_dict; + set_ref_in_item(&dtv, copyID, ht_stack, list_stack); + } + + for (i = 0; i < pt->pt_argc; ++i) + abort = abort || set_ref_in_item(&pt->pt_argv[i], copyID, + ht_stack, list_stack); + // pt_funcstack is handled in set_ref_in_funcstacks() + // pt_loopvars is handled in set_ref_in_loopvars() } - - for (i = 0; i < pt->pt_argc; ++i) - abort = abort || set_ref_in_item(&pt->pt_argv[i], copyID, - ht_stack, list_stack); - // pt_funcstack is handled in set_ref_in_funcstacks() - // pt_loopvars is handled in set_ref_in_loopvars() - } - } + break; + } + + case VAR_JOB: + { #ifdef FEAT_JOB_CHANNEL - else if (tv->v_type == VAR_JOB) - { - job_T *job = tv->vval.v_job; - typval_T dtv; - - if (job != NULL && job->jv_copyID != copyID) - { - job->jv_copyID = copyID; - if (job->jv_channel != NULL) - { - dtv.v_type = VAR_CHANNEL; - dtv.vval.v_channel = job->jv_channel; - set_ref_in_item(&dtv, copyID, ht_stack, list_stack); - } - if (job->jv_exit_cb.cb_partial != NULL) + job_T *job = tv->vval.v_job; + typval_T dtv; + + if (job != NULL && job->jv_copyID != copyID) { - dtv.v_type = VAR_PARTIAL; - dtv.vval.v_partial = job->jv_exit_cb.cb_partial; - set_ref_in_item(&dtv, copyID, ht_stack, list_stack); - } - } - } - else if (tv->v_type == VAR_CHANNEL) - { - channel_T *ch =tv->vval.v_channel; - ch_part_T part; - typval_T dtv; - jsonq_T *jq; - cbq_T *cq; - - if (ch != NULL && ch->ch_copyID != copyID) - { - ch->ch_copyID = copyID; - for (part = PART_SOCK; part < PART_COUNT; ++part) - { - for (jq = ch->ch_part[part].ch_json_head.jq_next; jq != NULL; - jq = jq->jq_next) - set_ref_in_item(jq->jq_value, copyID, ht_stack, list_stack); - for (cq = ch->ch_part[part].ch_cb_head.cq_next; cq != NULL; - cq = cq->cq_next) - if (cq->cq_callback.cb_partial != NULL) - { - dtv.v_type = VAR_PARTIAL; - dtv.vval.v_partial = cq->cq_callback.cb_partial; - set_ref_in_item(&dtv, copyID, ht_stack, list_stack); - } - if (ch->ch_part[part].ch_callback.cb_partial != NULL) + job->jv_copyID = copyID; + if (job->jv_channel != NULL) + { + dtv.v_type = VAR_CHANNEL; + dtv.vval.v_channel = job->jv_channel; + set_ref_in_item(&dtv, copyID, ht_stack, list_stack); + } + if (job->jv_exit_cb.cb_partial != NULL) { dtv.v_type = VAR_PARTIAL; - dtv.vval.v_partial = - ch->ch_part[part].ch_callback.cb_partial; + dtv.vval.v_partial = job->jv_exit_cb.cb_partial; set_ref_in_item(&dtv, copyID, ht_stack, list_stack); } } - if (ch->ch_callback.cb_partial != NULL) - { - dtv.v_type = VAR_PARTIAL; - dtv.vval.v_partial = ch->ch_callback.cb_partial; - set_ref_in_item(&dtv, copyID, ht_stack, list_stack); - } - if (ch->ch_close_cb.cb_partial != NULL) +#endif + break; + } + + case VAR_CHANNEL: + { +#ifdef FEAT_JOB_CHANNEL + channel_T *ch = tv->vval.v_channel; + ch_part_T part; + typval_T dtv; + jsonq_T *jq; + cbq_T *cq; + + if (ch != NULL && ch->ch_copyID != copyID) { - dtv.v_type = VAR_PARTIAL; - dtv.vval.v_partial = ch->ch_close_cb.cb_partial; - set_ref_in_item(&dtv, copyID, ht_stack, list_stack); + ch->ch_copyID = copyID; + for (part = PART_SOCK; part < PART_COUNT; ++part) + { + for (jq = ch->ch_part[part].ch_json_head.jq_next; + jq != NULL; jq = jq->jq_next) + set_ref_in_item(jq->jq_value, copyID, + ht_stack, list_stack); + for (cq = ch->ch_part[part].ch_cb_head.cq_next; cq != NULL; + cq = cq->cq_next) + if (cq->cq_callback.cb_partial != NULL) + { + dtv.v_type = VAR_PARTIAL; + dtv.vval.v_partial = cq->cq_callback.cb_partial; + set_ref_in_item(&dtv, copyID, ht_stack, list_stack); + } + if (ch->ch_part[part].ch_callback.cb_partial != NULL) + { + dtv.v_type = VAR_PARTIAL; + dtv.vval.v_partial = + ch->ch_part[part].ch_callback.cb_partial; + set_ref_in_item(&dtv, copyID, ht_stack, list_stack); + } + } + if (ch->ch_callback.cb_partial != NULL) + { + dtv.v_type = VAR_PARTIAL; + dtv.vval.v_partial = ch->ch_callback.cb_partial; + set_ref_in_item(&dtv, copyID, ht_stack, list_stack); + } + if (ch->ch_close_cb.cb_partial != NULL) + { + dtv.v_type = VAR_PARTIAL; + dtv.vval.v_partial = ch->ch_close_cb.cb_partial; + set_ref_in_item(&dtv, copyID, ht_stack, list_stack); + } } - } - } #endif + break; + } + + case VAR_CLASS: + // TODO: mark methods in class_obj_methods ? + break; + + case VAR_OBJECT: + { + object_T *obj = tv->vval.v_object; + if (obj != NULL && obj->obj_copyID != copyID) + { + obj->obj_copyID = copyID; + + // The typval_T array is right after the object_T. + typval_T *mtv = (typval_T *)(obj + 1); + for (int i = 0; !abort + && i < obj->obj_class->class_obj_member_count; ++i) + abort = abort || set_ref_in_item(mtv + i, copyID, + ht_stack, list_stack); + } + break; + } + + case VAR_UNKNOWN: + case VAR_ANY: + case VAR_VOID: + case VAR_BOOL: + case VAR_SPECIAL: + case VAR_NUMBER: + case VAR_FLOAT: + case VAR_STRING: + case VAR_BLOB: + case VAR_INSTR: + // Types that do not contain any other item + break; + } + return abort; } diff --git a/src/proto/vim9class.pro b/src/proto/vim9class.pro --- a/src/proto/vim9class.pro +++ b/src/proto/vim9class.pro @@ -8,5 +8,8 @@ int class_object_index(char_u **arg, typ void copy_object(typval_T *from, typval_T *to); void object_unref(object_T *obj); void copy_class(typval_T *from, typval_T *to); -void class_unref(typval_T *tv); +void class_unref(class_T *cl); +void object_created(object_T *obj); +void object_cleared(object_T *obj); +int object_free_nonref(int copyID); /* vim: set ft=c : */ diff --git a/src/structs.h b/src/structs.h --- a/src/structs.h +++ b/src/structs.h @@ -1487,8 +1487,13 @@ struct class_S // Used for v_object of typval of VAR_OBJECT. // The member variables follow in an array of typval_T. struct object_S { - class_T *obj_class; // class this object is created for + class_T *obj_class; // class this object is created for; + // pointer adds to class_refcount int obj_refcount; + + object_T *obj_next_used; // for list headed by "first_object" + object_T *obj_prev_used; // for list headed by "first_object" + int obj_copyID; // used by garbage collection }; #define TTFLAG_VARARGS 0x01 // func args ends with "..." diff --git a/src/testdir/test_vim9_class.vim b/src/testdir/test_vim9_class.vim --- a/src/testdir/test_vim9_class.vim +++ b/src/testdir/test_vim9_class.vim @@ -132,11 +132,10 @@ def Test_class_basic() this.col: number endclass - # # FIXME: this works but leaks memory - # # use the automatically generated new() method - # var pos = TextPosition.new(2, 12) - # assert_equal(2, pos.lnum) - # assert_equal(12, pos.col) + # use the automatically generated new() method + var pos = TextPosition.new(2, 12) + assert_equal(2, pos.lnum) + assert_equal(12, pos.col) END v9.CheckScriptSuccess(lines) enddef diff --git a/src/typval.c b/src/typval.c --- a/src/typval.c +++ b/src/typval.c @@ -85,7 +85,7 @@ free_tv(typval_T *varp) break; #endif case VAR_CLASS: - class_unref(varp); + class_unref(varp->vval.v_class); break; case VAR_OBJECT: object_unref(varp->vval.v_object); @@ -161,7 +161,7 @@ clear_tv(typval_T *varp) VIM_CLEAR(varp->vval.v_instr); break; case VAR_CLASS: - class_unref(varp); + class_unref(varp->vval.v_class); break; case VAR_OBJECT: object_unref(varp->vval.v_object); diff --git a/src/version.c b/src/version.c --- a/src/version.c +++ b/src/version.c @@ -696,6 +696,8 @@ static char *(features[]) = static int included_patches[] = { /* Add new patch number below this line */ /**/ + 1035, +/**/ 1034, /**/ 1033, diff --git a/src/vim9class.c b/src/vim9class.c --- a/src/vim9class.c +++ b/src/vim9class.c @@ -419,13 +419,18 @@ class_object_index( CLEAR_FIELD(funcexe); funcexe.fe_evaluate = TRUE; + // Clear the class or object after calling the function, in + // case the refcount is one. + typval_T tv_tofree = *rettv; + rettv->v_type = VAR_UNKNOWN; + // Call the user function. Result goes into rettv; // TODO: pass the object - rettv->v_type = VAR_UNKNOWN; int error = call_user_func_check(fp, argcount, argvars, rettv, &funcexe, NULL); - // Clear the arguments. + // Clear the previous rettv and the arguments. + clear_tv(&tv_tofree); for (int idx = 0; idx < argcount; ++idx) clear_tv(&argvars[idx]); @@ -494,7 +499,11 @@ object_clear(object_T *obj) for (int i = 0; i < cl->class_obj_member_count; ++i) clear_tv(tv + i); + // Remove from the list headed by "first_object". + object_cleared(obj); + vim_free(obj); + class_unref(cl); } /* @@ -522,9 +531,8 @@ copy_class(typval_T *from, typval_T *to) * Unreference a class. Free it when the reference count goes down to zero. */ void -class_unref(typval_T *tv) +class_unref(class_T *cl) { - class_T *cl = tv->vval.v_class; if (cl != NULL && --cl->class_refcount <= 0) { vim_free(cl->class_name); @@ -547,5 +555,60 @@ class_unref(typval_T *tv) } } +static object_T *first_object = NULL; + +/* + * Call this function when an object has been created. It will be added to the + * list headed by "first_object". + */ + void +object_created(object_T *obj) +{ + if (first_object != NULL) + { + obj->obj_next_used = first_object; + first_object->obj_prev_used = obj; + } + first_object = obj; +} + +/* + * Call this function when an object has been cleared and is about to be freed. + * It is removed from the list headed by "first_object". + */ + void +object_cleared(object_T *obj) +{ + if (obj->obj_next_used != NULL) + obj->obj_next_used->obj_prev_used = obj->obj_prev_used; + if (obj->obj_prev_used != NULL) + obj->obj_prev_used->obj_next_used = obj->obj_next_used; + else if (first_object == obj) + first_object = obj->obj_next_used; +} + +/* + * Go through the list of all objects and free items without "copyID". + */ + int +object_free_nonref(int copyID) +{ + int did_free = FALSE; + object_T *next_obj; + + for (object_T *obj = first_object; obj != NULL; obj = next_obj) + { + next_obj = obj->obj_next_used; + if ((obj->obj_copyID & COPYID_MASK) != (copyID & COPYID_MASK)) + { + // Free the object and items it contains. + object_clear(obj); + did_free = TRUE; + } + } + + return did_free; +} + #endif // FEAT_EVAL diff --git a/src/vim9execute.c b/src/vim9execute.c --- a/src/vim9execute.c +++ b/src/vim9execute.c @@ -3018,7 +3018,9 @@ exec_instructions(ectx_T *ectx) iptr->isn_arg.construct.construct_size); tv->vval.v_object->obj_class = iptr->isn_arg.construct.construct_class; + ++tv->vval.v_object->obj_class->class_refcount; tv->vval.v_object->obj_refcount = 1; + object_created(tv->vval.v_object); break; // execute Ex command line