vim-patch:9.1.1943: Memory leak with :breakadd expr

Problem:  Memory leak with :breakadd expr
Solution: Free debug_oldval and debug_newval before assigning to them.
          Verify the existing (though confusing) :breakadd expr behavior
          (zeertzjq).

It seems that :breakadd expr doesn't work as documented at all. This PR
only fixes the memory leak. The tests are for the existing behavior.

closes: vim/vim#18844

a474de64df

src/nvim/debugger.c

@@ -828,17 +828,21 @@ static linenr_T debuggy_find(bool file, char *fname, linenr_T after, garray_T *g
       typval_T *const tv = eval_expr_no_emsg(bp);
       if (tv != NULL) {
         if (bp->dbg_val == NULL) {
+          xfree(debug_oldval);
           debug_oldval = typval_tostring(NULL, true);
           bp->dbg_val = tv;
+          xfree(debug_newval);
           debug_newval = typval_tostring(bp->dbg_val, true);
           line = true;
         } else {
           if (typval_compare(tv, bp->dbg_val, EXPR_IS, false) == OK
               && tv->vval.v_number == false) {
             line = true;
+            xfree(debug_oldval);
             debug_oldval = typval_tostring(bp->dbg_val, true);
             // Need to evaluate again, typval_compare() overwrites "tv".
             typval_T *const v = eval_expr_no_emsg(bp);
+            xfree(debug_newval);
             debug_newval = typval_tostring(v, true);
             tv_free(bp->dbg_val);
             bp->dbg_val = v;
@@ -846,7 +850,9 @@ static linenr_T debuggy_find(bool file, char *fname, linenr_T after, garray_T *g
           tv_free(tv);
         }
       } else if (bp->dbg_val != NULL) {
+        xfree(debug_oldval);
         debug_oldval = typval_tostring(bp->dbg_val, true);
+        xfree(debug_newval);
         debug_newval = typval_tostring(NULL, true);
         tv_free(bp->dbg_val);
         bp->dbg_val = NULL;

test/functional/legacy/debugger_spec.lua

@@ -13,64 +13,112 @@ describe('debugger', function()
   local screen
 
   before_each(function()
-    screen = Screen.new(999, 10)
+    screen = Screen.new(999, 7)
   end)
 
   -- oldtest: Test_Debugger_breakadd_expr()
+  -- This doesn't seem to work as documented. The breakpoint is not
+  -- triggered until the next function call.
   it(':breakadd expr', function()
-    write_file('XdebugBreakExpr.vim', 'let g:Xtest_var += 1')
+    write_file(
+      'XbreakExpr.vim',
+      [[
+      func Foo()
+        eval 1
+        eval 2
+      endfunc
+
+      let g:Xtest_var += 1
+      call Foo()
+      let g:Xtest_var += 1
+      call Foo()]]
+    )
     finally(function()
-      os.remove('XdebugBreakExpr.vim')
+      os.remove('XbreakExpr.vim')
     end)
 
-    command('edit XdebugBreakExpr.vim')
+    command('edit XbreakExpr.vim')
     command(':let g:Xtest_var = 10')
     command(':breakadd expr g:Xtest_var')
-    feed(':source %<CR>')
-    screen:expect {
-      grid = [[
-      ^let g:Xtest_var += 1{MATCH: *}|
-      {1:~{MATCH: *}}|*8
-      :source %{MATCH: *}|
-    ]],
-    }
-    feed(':source %<CR>')
-    screen:expect {
-      grid = [[
+    local initial_screen = [[
+      ^func Foo(){MATCH: *}|
+        eval 1{MATCH: *}|
+        eval 2{MATCH: *}|
+      endfunc{MATCH: *}|
+      {MATCH: *}|
       let g:Xtest_var += 1{MATCH: *}|
-      {1:~{MATCH: *}}|
-      {3:{MATCH: *}}|
-      Breakpoint in "{MATCH:.*}XdebugBreakExpr.vim" line 1{MATCH: *}|
+      {MATCH: *}|
+    ]]
+    screen:expect(initial_screen)
+
+    feed(':source %<CR>')
+    screen:expect([[
+      Breakpoint in "Foo" line 1{MATCH: *}|
       Entering Debug mode.  Type "cont" to continue.{MATCH: *}|
       Oldval = "10"{MATCH: *}|
       Newval = "11"{MATCH: *}|
-      {MATCH:.*}XdebugBreakExpr.vim{MATCH: *}|
-      line 1: let g:Xtest_var += 1{MATCH: *}|
+      {MATCH:.*}XbreakExpr.vim[7]..function Foo{MATCH: *}|
+      line 1: eval 1{MATCH: *}|
       >^{MATCH: *}|
-    ]],
-    }
+    ]])
     feed('cont<CR>')
-    screen:expect {
-      grid = [[
-      ^let g:Xtest_var += 1{MATCH: *}|
-      {1:~{MATCH: *}}|*8
-      {MATCH: *}|
-    ]],
-    }
-    feed(':source %<CR>')
-    screen:expect {
-      grid = [[
-      let g:Xtest_var += 1{MATCH: *}|
-      {1:~{MATCH: *}}|
-      {3:{MATCH: *}}|
-      Breakpoint in "{MATCH:.*}XdebugBreakExpr.vim" line 1{MATCH: *}|
-      Entering Debug mode.  Type "cont" to continue.{MATCH: *}|
+    screen:expect([[
+      >cont{MATCH: *}|
+      Breakpoint in "Foo" line 1{MATCH: *}|
       Oldval = "11"{MATCH: *}|
       Newval = "12"{MATCH: *}|
-      {MATCH:.*}XdebugBreakExpr.vim{MATCH: *}|
-      line 1: let g:Xtest_var += 1{MATCH: *}|
+      {MATCH:.*}XbreakExpr.vim[9]..function Foo{MATCH: *}|
+      line 1: eval 1{MATCH: *}|
       >^{MATCH: *}|
-    ]],
-    }
+    ]])
+    feed('cont<CR>')
+    screen:expect(initial_screen)
+
+    -- Check the behavior without the g: prefix.
+    -- The Oldval and Newval don't look right here.
+    command(':breakdel *')
+    command(':breakadd expr Xtest_var')
+    feed(':source %<CR>')
+    screen:expect([[
+      Breakpoint in "Foo" line 1{MATCH: *}|
+      Entering Debug mode.  Type "cont" to continue.{MATCH: *}|
+      Oldval = "13"{MATCH: *}|
+      Newval = "(does not exist)"{MATCH: *}|
+      {MATCH:.*}XbreakExpr.vim[7]..function Foo{MATCH: *}|
+      line 1: eval 1{MATCH: *}|
+      >^{MATCH: *}|
+    ]])
+    feed('cont<CR>')
+    screen:expect([[
+      {MATCH:.*}XbreakExpr.vim[7]..function Foo{MATCH: *}|
+      line 1: eval 1{MATCH: *}|
+      >cont{MATCH: *}|
+      Breakpoint in "Foo" line 2{MATCH: *}|
+      {MATCH:.*}XbreakExpr.vim[7]..function Foo{MATCH: *}|
+      line 2: eval 2{MATCH: *}|
+      >^{MATCH: *}|
+    ]])
+    feed('cont<CR>')
+    screen:expect([[
+      >cont{MATCH: *}|
+      Breakpoint in "Foo" line 1{MATCH: *}|
+      Oldval = "14"{MATCH: *}|
+      Newval = "(does not exist)"{MATCH: *}|
+      {MATCH:.*}XbreakExpr.vim[9]..function Foo{MATCH: *}|
+      line 1: eval 1{MATCH: *}|
+      >^{MATCH: *}|
+    ]])
+    feed('cont<CR>')
+    screen:expect([[
+      {MATCH:.*}XbreakExpr.vim[9]..function Foo{MATCH: *}|
+      line 1: eval 1{MATCH: *}|
+      >cont{MATCH: *}|
+      Breakpoint in "Foo" line 2{MATCH: *}|
+      {MATCH:.*}XbreakExpr.vim[9]..function Foo{MATCH: *}|
+      line 2: eval 2{MATCH: *}|
+      >^{MATCH: *}|
+    ]])
+    feed('cont<CR>')
+    screen:expect(initial_screen)
   end)
 end)

test/old/testdir/test_debugger.vim

@@ -364,35 +364,74 @@ func Test_Debugger_breakadd()
 endfunc
 
 " Test for expression breakpoint set using ":breakadd expr <expr>"
+" FIXME: This doesn't seem to work as documented. The breakpoint is not
+" triggered until the next function call.
 func Test_Debugger_breakadd_expr()
   CheckRunVimInTerminal
   CheckCWD
 
   let lines =<< trim END
+    func Foo()
+      eval 1
+      eval 2
+    endfunc
+
     let g:Xtest_var += 1
+    call Foo()
+    let g:Xtest_var += 1
+    call Foo()
   END
-  call writefile(lines, 'XdebugBreakExpr.vim', 'D')
+  call writefile(lines, 'XbreakExpr.vim', 'D')
 
   " Start Vim in a terminal
-  let buf = RunVimInTerminal('XdebugBreakExpr.vim', {})
+  let buf = RunVimInTerminal('XbreakExpr.vim', {})
   call s:RunDbgCmd(buf, ':let g:Xtest_var = 10')
   call s:RunDbgCmd(buf, ':breakadd expr g:Xtest_var')
-  call s:RunDbgCmd(buf, ':source %')
   let expected =<< trim eval END
     Oldval = "10"
     Newval = "11"
-    {fnamemodify('XdebugBreakExpr.vim', ':p')}
-    line 1: let g:Xtest_var += 1
+    {fnamemodify('XbreakExpr.vim', ':p')}[7]..function Foo
+    line 1: eval 1
   END
   call s:RunDbgCmd(buf, ':source %', expected)
-  call s:RunDbgCmd(buf, 'cont')
   let expected =<< trim eval END
     Oldval = "11"
     Newval = "12"
-    {fnamemodify('XdebugBreakExpr.vim', ':p')}
-    line 1: let g:Xtest_var += 1
+    {fnamemodify('XbreakExpr.vim', ':p')}[9]..function Foo
+    line 1: eval 1
+  END
+  call s:RunDbgCmd(buf, 'cont', expected)
+  call s:RunDbgCmd(buf, 'cont')
+
+  " Check the behavior without the g: prefix.
+  " FIXME: The Oldval and Newval don't look right here.
+  call s:RunDbgCmd(buf, ':breakdel *')
+  call s:RunDbgCmd(buf, ':breakadd expr Xtest_var')
+  let expected =<< trim eval END
+    Oldval = "13"
+    Newval = "(does not exist)"
+    {fnamemodify('XbreakExpr.vim', ':p')}[7]..function Foo
+    line 1: eval 1
   END
   call s:RunDbgCmd(buf, ':source %', expected)
+  let expected =<< trim eval END
+    {fnamemodify('XbreakExpr.vim', ':p')}[7]..function Foo
+    line 2: eval 2
+  END
+  call s:RunDbgCmd(buf, 'cont', expected)
+  let expected =<< trim eval END
+    Oldval = "14"
+    Newval = "(does not exist)"
+    {fnamemodify('XbreakExpr.vim', ':p')}[9]..function Foo
+    line 1: eval 1
+  END
+  call s:RunDbgCmd(buf, 'cont', expected)
+  let expected =<< trim eval END
+    {fnamemodify('XbreakExpr.vim', ':p')}[9]..function Foo
+    line 2: eval 2
+  END
+  call s:RunDbgCmd(buf, 'cont', expected)
+  call s:RunDbgCmd(buf, 'cont')
 
   call StopVimInTerminal(buf)
 endfunc