Patch 8.2.1819

[Bram Moolenaar]

Patch 8.2.1819
Problem: Vim9: Memory leak when using a closure.
Solution: Compute the mininal refcount in the funcstack. Reenable disabled
tests.
Files: src/vim9execute.c, src/proto/vim9execute.pro, src/structs.h,
src/eval.c, src/testdir/test_vim9_disassemble.vim,
src/testdir/test_vim9_func.vim


*** ../vim-8.2.1818/src/vim9execute.c 2020-10-07 19:08:00.009793481 +0200
--- src/vim9execute.c 2020-10-10 14:12:23.944818556 +0200
***************
*** 349,356 ****
// Move them to the called function.
if (funcstack == NULL)
return FAIL;
! funcstack->fs_ga.ga_len = argcount + STACK_FRAME_SIZE
! + dfunc->df_varcount;
stack = ALLOC_CLEAR_MULT(typval_T, funcstack->fs_ga.ga_len);
funcstack->fs_ga.ga_data = stack;
if (stack == NULL)
--- 349,356 ----
// Move them to the called function.
if (funcstack == NULL)
return FAIL;
! funcstack->fs_var_offset = argcount + STACK_FRAME_SIZE;
! funcstack->fs_ga.ga_len = funcstack->fs_var_offset + dfunc->df_varcount;
stack = ALLOC_CLEAR_MULT(typval_T, funcstack->fs_ga.ga_len);
funcstack->fs_ga.ga_data = stack;
if (stack == NULL)
***************
*** 376,404 ****
{
tv = STACK_TV(ectx->ec_frame_idx + STACK_FRAME_SIZE + idx);

! // Do not copy a partial created for a local function.
! // TODO: This won't work if the closure actually uses it. But when
! // keeping it it gets complicated: it will create a reference cycle
! // inside the partial, thus needs special handling for garbage
! // collection.
! // For now, decide on the reference count.
if (tv->v_type == VAR_PARTIAL && tv->vval.v_partial != NULL)
{
! int i;

for (i = 0; i < closure_count; ++i)
! {
! partial_T *pt = ((partial_T **)gap->ga_data)[gap->ga_len
! - closure_count + i];
!
! if (tv->vval.v_partial == pt && pt->pt_refcount < 2)
! break;
! }
! if (i < closure_count)
! continue;
}

! *(stack + argcount + STACK_FRAME_SIZE + idx) = *tv;
tv->v_type = VAR_UNKNOWN;
}

--- 376,397 ----
{
tv = STACK_TV(ectx->ec_frame_idx + STACK_FRAME_SIZE + idx);

! // A partial created for a local function, that is also used as a
! // local variable, has a reference count for the variable, thus
! // will never go down to zero. When all these refcounts are one
! // then the funcstack is unused. We need to count how many we have
! // so we need when to check.
if (tv->v_type == VAR_PARTIAL && tv->vval.v_partial != NULL)
{
! int i;

for (i = 0; i < closure_count; ++i)
! if (tv->vval.v_partial == ((partial_T **)gap->ga_data)[
! gap->ga_len - closure_count + i])
! ++funcstack->fs_min_refcount;
}

! *(stack + funcstack->fs_var_offset + idx) = *tv;
tv->v_type = VAR_UNKNOWN;
}

***************
*** 427,432 ****
--- 420,462 ----
}

/*
+ * Called when a partial is freed or its reference count goes down to one. The
+ * funcstack may be the only reference to the partials in the local variables.
+ * Go over all of them, the funcref and can be freed if all partials
+ * referencing the funcstack have a reference count of one.
+ */
+ void
+ funcstack_check_refcount(funcstack_T *funcstack)
+ {
+ int i;
+ garray_T *gap = &funcstack->fs_ga;
+ int done = 0;
+
+ if (funcstack->fs_refcount > funcstack->fs_min_refcount)
+ return;
+ for (i = funcstack->fs_var_offset; i < gap->ga_len; ++i)
+ {
+ typval_T *tv = ((typval_T *)gap->ga_data) + i;
+
+ if (tv->v_type == VAR_PARTIAL && tv->vval.v_partial != NULL
+ && tv->vval.v_partial->pt_funcstack == funcstack
+ && tv->vval.v_partial->pt_refcount == 1)
+ ++done;
+ }
+ if (done == funcstack->fs_min_refcount)
+ {
+ typval_T *stack = gap->ga_data;
+
+ // All partials referencing the funcstack have a reference count of
+ // one, thus the funcstack is no longer of use.
+ for (i = 0; i < gap->ga_len; ++i)
+ clear_tv(stack + i);
+ vim_free(stack);
+ vim_free(funcstack);
+ }
+ }
+
+ /*
* Return from the current function.
*/
static int
*** ../vim-8.2.1818/src/proto/vim9execute.pro 2020-08-12 21:34:43.266489468 +0200
--- src/proto/vim9execute.pro 2020-10-10 13:53:07.324695567 +0200
***************
*** 1,5 ****
--- 1,6 ----
/* vim9execute.c */
void to_string_error(vartype_T vartype);
+ void funcstack_check_refcount(funcstack_T *funcstack);
int call_def_function(ufunc_T *ufunc, int argc_arg, typval_T *argv, partial_T *partial, typval_T *rettv);
void ex_disassemble(exarg_T *eap);
int tv2bool(typval_T *tv);
*** ../vim-8.2.1818/src/structs.h 2020-10-08 21:16:38.643643838 +0200
--- src/structs.h 2020-10-10 13:33:35.539450332 +0200
***************
*** 1869,1876 ****
--- 1869,1879 ----
// - arguments
// - frame
// - local variables
+ int fs_var_offset; // count of arguments + frame size == offset to
+ // local variables

int fs_refcount; // nr of closures referencing this funcstack
+ int fs_min_refcount; // nr of closures on this funcstack
int fs_copyID; // for garray_T collection
} funcstack_T;

*** ../vim-8.2.1818/src/eval.c 2020-10-08 21:16:38.643643838 +0200
--- src/eval.c 2020-10-10 14:02:55.015120200 +0200
***************
*** 3984,4004 ****
else
func_ptr_unref(pt->pt_func);

if (pt->pt_funcstack != NULL)
{
! // Decrease the reference count for the context of a closure. If down
! // to zero free it and clear the variables on the stack.
! if (--pt->pt_funcstack->fs_refcount == 0)
! {
! garray_T *gap = &pt->pt_funcstack->fs_ga;
! typval_T *stack = gap->ga_data;
!
! for (i = 0; i < gap->ga_len; ++i)
! clear_tv(stack + i);
! ga_clear(gap);
! vim_free(pt->pt_funcstack);
! }
! pt->pt_funcstack = NULL;
}

vim_free(pt);
--- 3984,3995 ----
else
func_ptr_unref(pt->pt_func);

+ // Decrease the reference count for the context of a closure. If down
+ // to the minimum it may be time to free it.
if (pt->pt_funcstack != NULL)
{
! --pt->pt_funcstack->fs_refcount;
! funcstack_check_refcount(pt->pt_funcstack);
}

vim_free(pt);
***************
*** 4011,4018 ****
void
partial_unref(partial_T *pt)
{
! if (pt != NULL && --pt->pt_refcount <= 0)
! partial_free(pt);
}

/*
--- 4002,4017 ----
void
partial_unref(partial_T *pt)
{
! if (pt != NULL)
! {
! if (--pt->pt_refcount <= 0)
! partial_free(pt);
!
! // If the reference count goes down to one, the funcstack may be the
! // only reference and can be freed if no other partials reference it.
! else if (pt->pt_refcount == 1 && pt->pt_funcstack != NULL)
! funcstack_check_refcount(pt->pt_funcstack);
! }
}

/*
*** ../vim-8.2.1818/src/testdir/test_vim9_disassemble.vim 2020-10-08 23:21:17.935678280 +0200
--- src/testdir/test_vim9_disassemble.vim 2020-10-10 14:07:55.922302264 +0200
***************
*** 436,477 ****
res)
enddef

! " TODO: fix memory leak and enable again
! "def s:CreateRefs()
! " var local = 'a'
! " def Append(arg: string)
! " local ..= arg
! " enddef
! " g:Append = Append
! " def Get(): string
! " return local
! " enddef
! " g:Get = Get
! "enddef
! "
! "def Test_disassemble_closure()
! " CreateRefs()
! " var res = execute('disass g:Append')
! " assert_match('<lambda>\d\_s*' ..
! " 'local ..= arg\_s*' ..
! " '\d LOADOUTER $0\_s*' ..
! " '\d LOAD arg\[-1\]\_s*' ..
! " '\d CONCAT\_s*' ..
! " '\d STOREOUTER $0\_s*' ..
! " '\d PUSHNR 0\_s*' ..
! " '\d RETURN',
! " res)
! "
! " res = execute('disass g:Get')
! " assert_match('<lambda>\d\_s*' ..
! " 'return local\_s*' ..
! " '\d LOADOUTER $0\_s*' ..
! " '\d RETURN',
! " res)
! "
! " unlet g:Append
! " unlet g:Get
! "enddef


def EchoArg(arg: string): string
--- 436,477 ----
res)
enddef

!
! def s:CreateRefs()
! var local = 'a'
! def Append(arg: string)
! local ..= arg
! enddef
! g:Append = Append
! def Get(): string
! return local
! enddef
! g:Get = Get
! enddef
!
! def Test_disassemble_closure()
! CreateRefs()
! var res = execute('disass g:Append')
! assert_match('<lambda>\d\_s*' ..
! 'local ..= arg\_s*' ..
! '\d LOADOUTER $0\_s*' ..
! '\d LOAD arg\[-1\]\_s*' ..
! '\d CONCAT\_s*' ..
! '\d STOREOUTER $0\_s*' ..
! '\d PUSHNR 0\_s*' ..
! '\d RETURN',
! res)
!
! res = execute('disass g:Get')
! assert_match('<lambda>\d\_s*' ..
! 'return local\_s*' ..
! '\d LOADOUTER $0\_s*' ..
! '\d RETURN',
! res)
!
! unlet g:Append
! unlet g:Get
! enddef


def EchoArg(arg: string): string
*** ../vim-8.2.1818/src/testdir/test_vim9_func.vim 2020-10-09 22:04:25.210842991 +0200
--- src/testdir/test_vim9_func.vim 2020-10-10 14:08:38.306088907 +0200
***************
*** 1330,1361 ****
unlet g:UseVararg
enddef

! " TODO: reenable after fixing memory leak
! "def MakeGetAndAppendRefs()
! " var local = 'a'
! "
! " def Append(arg: string)
! " local ..= arg
! " enddef
! " g:Append = Append
! "
! " def Get(): string
! " return local
! " enddef
! " g:Get = Get
! "enddef
! "
! "def Test_closure_append_get()
! " MakeGetAndAppendRefs()
! " g:Get()->assert_equal('a')
! " g:Append('-b')
! " g:Get()->assert_equal('a-b')
! " g:Append('-c')
! " g:Get()->assert_equal('a-b-c')
! "
! " unlet g:Append
! " unlet g:Get
! "enddef

def Test_nested_closure()
var local = 'text'
--- 1330,1360 ----
unlet g:UseVararg
enddef

! def MakeGetAndAppendRefs()
! var local = 'a'
!
! def Append(arg: string)
! local ..= arg
! enddef
! g:Append = Append
!
! def Get(): string
! return local
! enddef
! g:Get = Get
! enddef
!
! def Test_closure_append_get()
! MakeGetAndAppendRefs()
! g:Get()->assert_equal('a')
! g:Append('-b')
! g:Get()->assert_equal('a-b')
! g:Append('-c')
! g:Get()->assert_equal('a-b-c')
!
! unlet g:Append
! unlet g:Get
! enddef

def Test_nested_closure()
var local = 'text'
***************
*** 1389,1408 ****
CheckScriptSuccess(lines)
enddef

! " TODO: reenable after fixing memory leak
! "def Test_nested_closure_used()
! " var lines =<< trim END
! " vim9script
! " def Func()
! " var x = 'hello'
! " var Closure = {-> x}
! " g:Myclosure = {-> Closure()}
! " enddef
! " Func()
! " assert_equal('hello', g:Myclosure())
! " END
! " CheckScriptSuccess(lines)
! "enddef

def Test_nested_closure_fails()
var lines =<< trim END
--- 1388,1406 ----
CheckScriptSuccess(lines)
enddef

! def Test_nested_closure_used()
! var lines =<< trim END
! vim9script
! def Func()
! var x = 'hello'
! var Closure = {-> x}
! g:Myclosure = {-> Closure()}
! enddef
! Func()
! assert_equal('hello', g:Myclosure())
! END
! CheckScriptSuccess(lines)
! enddef

def Test_nested_closure_fails()
var lines =<< trim END
*** ../vim-8.2.1818/src/version.c 2020-10-09 23:04:43.676144228 +0200
--- src/version.c 2020-10-10 14:09:37.481731088 +0200
***************
*** 752,753 ****
--- 752,755 ----
{ /* Add new patch number below this line */
+ /**/
+ 1819,
/**/

--
"I can't complain, but sometimes I still do." (Joe Walsh)

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///