shada: Also save numbered marks in a ShaDa file

[ZyX-I]

After #5079, unfinished. Only commits starting from “shada: Save numbered marks” are relevant, others are part of #5079.

[justinmk]

ARRAY_LAST_ENTRY may help with "discoverability".

[draffensperger]

Any update on this?

I'm experiencing the same issue @wreed4 had with numbered marks not getting set correctly even when you run NeoVim with nvim -u NONE, see #4360 (comment)

[mcepl]

Works for me, :marks command works. Could we get this rebased on the current master, please?

[jamessan]

Indeed, rebasing this on master does seem to work nicely with one small change to avoid a saving invalid marks to the shada file:

[  FAILED  ] test/functional/shada/marks_spec.lua @ 224: ShaDa support code does not create incorrect file for non-existent buffers when writing from -c
test/functional/shada/marks_spec.lua:229: Expected objects to be the same.
Passed in:
(string) 'Vim(rshada):E575: Error while reading ShaDa file: mark entry at position 94 has invalid line number'
Expected:
(number) 0

stack traceback:
        test/functional/shada/marks_spec.lua:229: in function <test/functional/shada/marks_spec.lua:225>

diff --git i/src/nvim/shada.c w/src/nvim/shada.c
index 893a41b95..8ce8e1720 100644
--- i/src/nvim/shada.c
+++ w/src/nvim/shada.c
@@ -2796,7 +2796,8 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer,
 
   // Update numbered marks: '0' should be replaced with the current position,
   // '9' should be removed and all other marks shifted.
-  if (!ignore_buf(curbuf, &removable_bufs)) {
+  if (!ignore_buf(curbuf, &removable_bufs)
+      && curwin->w_cursor.lnum > 0 && curwin->w_cursor.col >= 0) {
     replace_numbered_mark(wms, 0, (PossiblyFreedShadaEntry) {
       .can_free_entry = false,
       .data = {

@ZyX-I Do you have time to fininsh this or do you want me to add this change and update the PR?

[ZyX-I]

Screwed up a test code during merging, should be fine now.

[justinmk]

Tried this out with existing shada file (using default flags: shada=!,'100,<50,s10,h), abort() on exit:

src/nvim/shada.c:2435: void replace_numbered_mark(WriteMergerState *const, const size_t, const PossiblyFreedShadaEntry): 
Assertion `ascii_isdigit(wms->numbered_marks[i].data.data.filemark.name)' failed.

Adding a log statement before the assertion:

ELOG("XXX=%d / %c / %c", wms->numbered_marks[i].data.data.filemark.name, ..., ...)

reveals that wms->numbered_marks[i].data.data.filemark.name is char : (integer 58).
Log looks like this:

2018/03/27 01:22:44 ERROR 26165 replace_numbered_mark:2435: XXX=54 / 6 / 6
2018/03/27 01:22:44 ERROR 26165 replace_numbered_mark:2435: XXX=55 / 7 / 7
2018/03/27 01:22:44 ERROR 26165 replace_numbered_mark:2435: XXX=54 / 6 / 6
2018/03/27 01:22:44 ERROR 26165 replace_numbered_mark:2435: XXX=50 / 2 / 2
2018/03/27 01:22:44 ERROR 26165 replace_numbered_mark:2435: XXX=51 / 3 / 3
2018/03/27 01:22:44 ERROR 26165 replace_numbered_mark:2435: XXX=52 / 4 / 4
2018/03/27 01:22:44 ERROR 26165 replace_numbered_mark:2435: XXX=50 / 2 / 2
2018/03/27 01:22:44 ERROR 26165 replace_numbered_mark:2435: XXX=55 / 7 / 7
2018/03/27 01:22:44 ERROR 26165 replace_numbered_mark:2435: XXX=56 / 8 / 8
2018/03/27 01:22:44 ERROR 26165 replace_numbered_mark:2435: XXX=58 / : / :

[jamessan]

Based on #5908 (comment), but with adjusted condition as line number or column less then zero should not appear at all based on what I know.

There are various places in the code that currently check for lnum <= 0. lnum is only valid if it's > 0 and col is only valid if it's >= 0. Since both are signed types, I figured it was better to be consistent while we're fixing this than to need to return to the issue later.

shada: In place of ignoring cursor position with lnum 0 save with 1

Why rotate away a valid value just to store one that was invalid?

[ZyX-I]

Why rotate away a valid value just to store one that was invalid?

Primary because I incorrectly assumed where is the error with tests. But in any case I am acting under assumption that window always has a cursor and always has a buffer at the time ShaDa file is being saved, meaning that no such thing as “invalid value” may exist. So if lnum==0 cannot stand for invalid value in this case it ought to stand for the very first position instead.

[ZyX-I]

Oh, trying to reproduce @justinmk’s issue got a leak on

  it('can merge with file with mark 9 as the only numeric mark', function()
    wshada('\007\001\018\131\162mX\195\161f\196\006' .. mock_file_path .. '-\161n9')
    eq(0, exc_exec(sdrcmd()))
    nvim_command('normal! `9oabc')
    eq('-', funcs.fnamemodify(curbufmeths.get_name(), ':t'))
    eq(0, exc_exec('wshada ' .. shada_fname))
    local found = {}
    for _, v in ipairs(read_shada_file(shada_fname)) do
      if v.type == 7 and v.value.f == mock_file_path .. '-' then
        local name = ('%c'):format(v.value.n)
        found[name] = (found[name] or 0) + 1
      end
    end
    eq({['0']=1, ['1']=1}, found)
  end)

(test is testing against proposed fix which is currently

diff --git a/src/nvim/shada.c b/src/nvim/shada.c
index f726f09fa..81261a183 100644
--- a/src/nvim/shada.c
+++ b/src/nvim/shada.c
@@ -2431,14 +2431,14 @@ static inline void replace_numbered_mark(WriteMergerState *const wms,
   }
   for (size_t i = idx; i < ARRAY_SIZE(wms->numbered_marks) - 1; i++) {
     if (wms->numbered_marks[i].data.type == kSDItemGlobalMark) {
-      wms->numbered_marks[i].data.data.filemark.name++;
-      assert(ascii_isdigit(wms->numbered_marks[i].data.data.filemark.name));
+      wms->numbered_marks[i].data.data.filemark.name = '0' + (char)i;
     }
   }
   memmove(wms->numbered_marks + idx + 1, wms->numbered_marks + idx,
           sizeof(wms->numbered_marks[0])
           * (ARRAY_SIZE(wms->numbered_marks) - 1 - idx));
   wms->numbered_marks[idx] = entry;
+  wms->numbered_marks[idx].data.data.filemark.name = '0' + (char)idx;
 }
 
 /// Write ShaDa file
diff --git a/test/functional/shada/merging_spec.lua b/test/functional/shada/merging_spec.lua
index 1a289a2de..5d486bbd9 100644
--- a/test/functional/shada/merging_spec.lua
+++ b/test/functional/shada/merging_spec.lua
@@ -525,6 +525,22 @@ describe('ShaDa marks support code', function()
     eq('-', funcs.fnamemodify(curbufmeths.get_name(), ':t'))
   end)
 
+  it('can merge with file with mark 9 as the only numeric mark', function()
+    wshada('\007\001\018\131\162mX\195\161f\196\006' .. mock_file_path .. '-\161n9')
+    eq(0, exc_exec(sdrcmd()))
+    nvim_command('normal! `9oabc')
+    eq('-', funcs.fnamemodify(curbufmeths.get_name(), ':t'))
+    eq(0, exc_exec('wshada ' .. shada_fname))
+    local found = {}
+    for _, v in ipairs(read_shada_file(shada_fname)) do
+      if v.type == 7 and v.value.f == mock_file_path .. '-' then
+        local name = ('%c'):format(v.value.n)
+        found[name] = (found[name] or 0) + 1
+      end
+    end
+    eq({['0']=1, ['1']=1}, found)
+  end)
+
   it('uses last A mark with gt timestamp from file when reading with !',
   function()
     wshada('\007\001\018\131\162mX\195\161f\196\006' .. mock_file_path .. '-\161nA')

)

But leak like

==================== File /home/zyx/a.a/Proj/c/neovim-master/asan.31561 ====================
=
= =================================================================
= ==31561==ERROR: LeakSanitizer: detected memory leaks
=
= Direct leak of 7 byte(s) in 1 object(s) allocated from:
=     #0 0x507980 in malloc /var/tmp/portage/sys-libs/compiler-rt-sanitizers-5.0.1/work/compiler-rt-5.0.1.src/lib/asan/asan_malloc_linux.cc:67
=     #1 0xf81761 in try_malloc /home/zyx/a.a/Proj/c/neovim-master/src/nvim/memory.c:87:15
=     #2 0xf81989 in xmalloc /home/zyx/a.a/Proj/c/neovim-master/src/nvim/memory.c:121:15
=     #3 0xf81f5d in xmallocz /home/zyx/a.a/Proj/c/neovim-master/src/nvim/memory.c:196:15
=     #4 0xf820de in xmemdupz /home/zyx/a.a/Proj/c/neovim-master/src/nvim/memory.c:214:17
=     #5 0x15e6f89 in shada_read_next_item /home/zyx/a.a/Proj/c/neovim-master/src/nvim/shada.c:3673:9
=     #6 0x15af9dc in shada_read_when_writing /home/zyx/a.a/Proj/c/neovim-master/src/nvim/shada.c:2042:22
=     #7 0x1586f72 in shada_write /home/zyx/a.a/Proj/c/neovim-master/src/nvim/shada.c:2790:39
=     #8 0x157b40d in shada_write_file /home/zyx/a.a/Proj/c/neovim-master/src/nvim/shada.c:3049:35
=     #9 0xbbbfba in ex_shada /home/zyx/a.a/Proj/c/neovim-master/src/nvim/ex_docmd.c:9586:5
=     #10 0xb4e5f3 in do_one_cmd /home/zyx/a.a/Proj/c/neovim-master/src/nvim/ex_docmd.c:2232:5
=     #11 0xb30c09 in do_cmdline /home/zyx/a.a/Proj/c/neovim-master/src/nvim/ex_docmd.c:601:20
=     #12 0x867a61 in ex_execute /home/zyx/a.a/Proj/c/neovim-master/src/nvim/eval.c:19479:7
=     #13 0xb4e5f3 in do_one_cmd /home/zyx/a.a/Proj/c/neovim-master/src/nvim/ex_docmd.c:2232:5
=     #14 0xb30c09 in do_cmdline /home/zyx/a.a/Proj/c/neovim-master/src/nvim/ex_docmd.c:601:20
=     #15 0xb36d45 in do_cmdline_cmd /home/zyx/a.a/Proj/c/neovim-master/src/nvim/ex_docmd.c:277:10
=     #16 0x63b6a1 in nvim_command /home/zyx/a.a/Proj/c/neovim-master/src/nvim/api/vim.c:58:3
=     #17 0x599ac8 in handle_nvim_command /home/zyx/a.a/Proj/c/neovim-master/build/src/nvim/auto/api/private/dispatch_wrappers.generated.h:1616:3
=     #18 0x105b457 in on_request_event /home/zyx/a.a/Proj/c/neovim-master/src/nvim/msgpack_rpc/channel.c:361:19
=     #19 0xa519ea in multiqueue_process_events /home/zyx/a.a/Proj/c/neovim-master/src/nvim/event/multiqueue.c:150:7
=     #20 0x111666b in nv_event /home/zyx/a.a/Proj/c/neovim-master/src/nvim/normal.c:7973:3
=     #21 0x10c4c9a in normal_execute /home/zyx/a.a/Proj/c/neovim-master/src/nvim/normal.c:1137:3
=     #22 0x17447f9 in state_enter /home/zyx/a.a/Proj/c/neovim-master/src/nvim/state.c:67:26
=     #23 0x1081a84 in normal_enter /home/zyx/a.a/Proj/c/neovim-master/src/nvim/normal.c:467:3
=     #24 0xe74cbd in main /home/zyx/a.a/Proj/c/neovim-master/src/nvim/main.c:567:3
=     #25 0x7f99d85c0530 in __libc_start_main (/lib64/libc.so.6+0x20530)
=
= SUMMARY: AddressSanitizer: 7 byte(s) leaked in 1 allocation(s).
============================================================================================

is not the intended assert() crash. Can I ask what are the global marks in that crashy file?

[jamessan]

I commonly see leaks in the shada code when I'm running ASAN and have for the past year or so. I had never looked into it much because it's always when I'm testing something else and I wasn't quite sure how to reproduce it. Regardless, I'm pretty sure the stack you hit isn't new, @ZyX-I.

[justinmk]

As @jamessan said I've seen a shada leak usually when exiting, but thought it wasn't important since it's related to exiting.

@ZyX-I global marks: https://gist.github.com/justinmk/1c1db4fa802eb92548dc001e33610c3a

[ZyX-I]

Should be ready if AV succeeds.

[mcepl]

Should be ready if AV succeeds.

Except it requires another rebase, already first commit fails to rebase without conflicts. Sigh.

[ZyX-I]

@mcepl GH reports that it can merge PR.

[mcepl]

@mcepl GH reports that it can merge PR.

Good for you! It's more able than me then.

[ZyX-I]

One (second in log) of the AV failures looks unrelated, but I do not see how it may be a flaky test. Second (first in log) looks like an actual error, except that I have no idea how that may have passed in other AV builds:

[  FAILED  ] 1 test, listed below:
  [  FAILED  ] C:/projects/neovim/test/functional\shada\merging_spec.lua @ 673: ShaDa marks support code uses last A mark with gt timestamp from file when writing
  C:/projects/neovim/test/functional\shada\merging_spec.lua:690: Expected objects to be the same.
  Passed in:
  (table) {
    [0] = {
      [C:/a/-] = 1 }
   *[A] = {
      [C:/a/?] = 1 } }
  Expected:
  (table) {
    [0] = {
      [/a/b/-] = 1 }
   *[A] = {
     *[/a/b/?] = 1 } }
  
  stack traceback:
  	C:/projects/neovim/test/functional\shada\merging_spec.lua:690: in function <C:/projects/neovim/test/functional\shada\merging_spec.lua:674>
  
  [  ERROR   ] 1 error, listed below:
  [  ERROR   ] test\unit\viml\expressions\parser_tests.lua @ 6329: api nvim_parse_expression works with &opt
  C:/projects/neovim/test/functional\api\vim_spec.lua:969: Error while processing test ('(1+&)', E):
CUSTOMBUILD : table error :  [C:\projects\neovim\build\functionaltest.vcxproj]
  C:/projects/neovim/test/functional\api\vim_spec.lua:953: Expected objects to be the same.
  Passed in:
  (table) {
    [1] = 'NvimNestingParenthesis:0:0:('
    [2] = 'NvimNumber:0:1:1'
    [3] = 'NvimBinaryPlus:0:2:+'
    [4] = 'NvimInvalidOptionSigil:0:3:&'
   *[5] = 'NvimInvalidOptionScope:0:4:)'
    [6] = 'NvimInvalidOptionScopeDelimiter:0:5:'
    [7] = 'NvimInvalidOptionName:0:6:'
    [8] = 'NvimNestingParenthesis:0:4:)' }
  Expected:
  (table) {
    [1] = 'NvimNestingParenthesis:0:0:('
    [2] = 'NvimNumber:0:1:1'
    [3] = 'NvimBinaryPlus:0:2:+'
    [4] = 'NvimInvalidOptionSigil:0:3:&'
   *[5] = 'NvimNestingParenthesis:0:4:)' }
  
  ({message = 'C:/projects/neovim/test/functional\\api\\vim_spec.lua:953: Expected objects to be the same.\nPassed in:\n(table) {\n  [1] = \'NvimNestingParenthesis:0:0:(\'\n  [2] = \'NvimNumber:0:1:1\'\n  [3] = \'NvimBinaryPlus:0:2:+\'\n  [4] = \'NvimInvalidOptionSigil:0:3:&\'\n *[5] = \'NvimInvalidOptionScope:0:4:)\'\n  [6] = \'NvimInvalidOptionScopeDelimiter:0:5:\'\n  [7] = \'NvimInvalidOptionName:0:6:\'\n  [8] = \'NvimNestingParenthesis:0:4:)\' }\nExpected:\n(table) {\n  [1] = \'NvimNestingParenthesis:0:0:(\'\n  [2] = \'NvimNumber:0:1:1\'\n  [3] = \'NvimBinaryPlus:0:2:+\'\n  [4] = \'NvimInvalidOptionSigil:0:3:&\'\n *[5] = \'NvimNestingParenthesis:0:4:)\' }'})
  
  stack traceback:
  	C:/projects/neovim/test/functional\api\vim_spec.lua:969: in function 'check_parsing'
  	test\unit\viml\expressions\parser_tests.lua:6475: in function <test\unit\viml\expressions\parser_tests.lua:6329>
  
  
   69 SKIPPED TESTS
   1 FAILED TEST
   1 ERROR
  -- Output to stderr:
  The directory is not empty.
  
  CMake Error at C:/projects/neovim/cmake/RunTests.cmake:53 (message):
CUSTOMBUILD : Running functional tests failed with error : 1. [C:\projects\neovim\build\functionaltest.vcxproj]

[justinmk]

Tested locally, works for me now.

spell_spec.lua failure on travis is unrelated. #8102

fix #3472
fix #5875