Loading ...
Sorry, an error occurred while loading the content.
 

Re: 3 remarks about undo.c

Expand Messages
  • Dominique Pellé
    ... I now understand why vim-7.3a crashes when undefining line #define U_USE_MALLOC 1 in undo.c . u_alloc_line() and u_free_line(), which are used when
    Message 1 of 9 , May 29 7:01 AM
      Dominique Pellé wrote:

      > Bram Moolenaar wrote:
      >
      >> Dominique Pelle wrote:
      ....
      >>> (2) In vim/src/undo.c  I see at line 92:
      >>>
      >>>  91  /* See below: use malloc()/free() for memory management. */
      >>>  92  #define U_USE_MALLOC 1
      >>>
      >>> Whether U_USE_MALLOC is defined or not selects different
      >>> implementations of U_FREE_LINE and U_ALLOC_LINE.
      >>>
      >>> Is the implementation when U_USE_MALLOC is undefined
      >>> still meant to be used? Or can it be removed?
      >>>
      >>> I'm asking because if I comment out the line #define U_USE_MALLOC 1
      >>> at line undo.c:92, then Vim quickly crashes when using persistent-undo.
      >>> In other words, persistent undo only works when U_USE_MALLOC is defined.
      >>
      >> Can you see why it crashes?  Perhaps it allocates a block that is too
      >> big?
      >
      > If I just recompile Vim with "#define U_USE_MALLOC 1" commented out
      > (without any patch) as follows:
      >
      >  /* See below: use malloc()/free() for memory management. */
      > /*#define U_USE_MALLOC 1*/
      >
      > Then the following command is enough to make Vim crash:
      >
      > $ vim -u NONE --noplugin -c 'wundo! foo' -c 'rundo foo'
      > Vim: Caught deadly signal SEGV
      > Vim: Finished.
      > Segmentation fault
      >
      > Program received signal SIGSEGV, Segmentation fault.
      > 0x081bf48c in u_free_line (ptr=0x8264f90 "", keep=0) at undo.c:2846
      > 2846        if ((nextb = curbuf->b_mb_current->mb_next) != NULL
      > (gdb) bt
      > #0  0x081bf48c in u_free_line (ptr=0x8264f90 "", keep=0) at undo.c:2846
      > #1  0x081bc5f0 in u_read_undo (name=0x8264e06 "foo",
      >    hash=0xbfffed9c
      > "\343\260\304B\230\374\034\024\232\373\364\310\231o\271$'\256A\344d\233\223L\244\225\231\033xR\270U")
      > at undo.c:1094
      > #2  0x080afc34 in ex_rundo (eap=0xbfffedfc) at ex_docmd.c:8481
      > #3  0x080a70dd in do_one_cmd (cmdlinep=0xbfffefb0, sourcing=1,
      > cstack=0xbfffefb8, fgetline=0,
      >    cookie=0x0) at ex_docmd.c:2639
      > #4  0x080a49b6 in do_cmdline (cmdline=0xbffff6a3 "rundo foo",
      > getline=0, cookie=0x0, flags=11)
      >    at ex_docmd.c:1108
      > #5  0x080a4070 in do_cmdline_cmd (cmd=0xbffff6a3 "rundo foo") at ex_docmd.c:714
      > #6  0x080e935d in exe_commands (parmp=0xbffff364) at main.c:2750
      > #7  0x080e6b3a in main (argc=8, argv=0xbffff4e4) at main.c:880
      > (gdb) list
      > 2841        if (curbuf->b_mb_current == NULL || mp < (minfo_T
      > *)curbuf->b_mb_current)
      > 2842        {
      > 2843            curbuf->b_mb_current = curbuf->b_block_head.mb_next;
      > 2844            curbuf->b_m_search = NULL;
      > 2845        }
      > 2846        if ((nextb = curbuf->b_mb_current->mb_next) != NULL
      > 2847                                                         && (minfo_T *)nextb < mp)
      > 2848        {
      > 2849            curbuf->b_mb_current = nextb;
      > 2850            curbuf->b_m_search = NULL;
      >
      > (gdb) p curbuf
      > $1 = (buf_T *) 0x8234dd0
      > (gdb) p curbuf->b_mb_current
      > $2 = (mblock_T *) 0x0
      >
      > So accessing curbuf->b_mb_current->mb_next dereferences NULL.
      >
      > I have not had time to look at how u_alloc_line() and u_free_line()
      > work.  Using vim_malloc() and vim_free() is certainly simpler. malloc()
      > and free() are also quite optimized.  So I don't see the need
      > for a special implementation.


      I now understand why vim-7.3a crashes when undefining line
      #define U_USE_MALLOC 1 in "undo.c".

      u_alloc_line() and u_free_line(), which are used when
      U_USE_MALLOC is undefined, use buffers inside 'curbuf'.
      All allocations make during in u_read_undo() gets freed
      when calling "u_blockfree(curbuf);".

      Attached patch makes it work I think (I did not test in details)
      by calling u_blockfree(curbuf) earlier (before any call to U_ALLOC_LINE())
      in u_read_undo().

      However, I DON'T THINK IT'S A GOOD IDEA TO APPLY THIS PATCH
      anyway since if an error happens while loading the persistent undo file,
      the current undo information would be lost!

      It's best and a lot simpler to just remove all code in
      #define U_USE_MALLOC which is not currently used anyway
      and no longer valid.

      Regards
      -- Dominique

      --
      You received this message from the "vim_dev" maillist.
      Do not top-post! Type your reply below the text you are replying to.
      For more information, visit http://www.vim.org/maillist.php
    • Dominique Pellé
      ... Sorry, I forgot to attach the patch in my previous email. Here it is. -- Dominique -- You received this message from the vim_dev maillist. Do not
      Message 2 of 9 , May 29 7:05 AM
        Dominique Pellé wrote:

        > Dominique Pellé wrote:
        >
        >> Bram Moolenaar wrote:
        >>
        >>> Dominique Pelle wrote:
        > ....
        >>>> (2) In vim/src/undo.c  I see at line 92:
        >>>>
        >>>>  91  /* See below: use malloc()/free() for memory management. */
        >>>>  92  #define U_USE_MALLOC 1
        >>>>
        >>>> Whether U_USE_MALLOC is defined or not selects different
        >>>> implementations of U_FREE_LINE and U_ALLOC_LINE.
        >>>>
        >>>> Is the implementation when U_USE_MALLOC is undefined
        >>>> still meant to be used? Or can it be removed?
        >>>>
        >>>> I'm asking because if I comment out the line #define U_USE_MALLOC 1
        >>>> at line undo.c:92, then Vim quickly crashes when using persistent-undo.
        >>>> In other words, persistent undo only works when U_USE_MALLOC is defined.
        >>>
        >>> Can you see why it crashes?  Perhaps it allocates a block that is too
        >>> big?
        >>
        >> If I just recompile Vim with "#define U_USE_MALLOC 1" commented out
        >> (without any patch) as follows:
        >>
        >>  /* See below: use malloc()/free() for memory management. */
        >> /*#define U_USE_MALLOC 1*/
        >>
        >> Then the following command is enough to make Vim crash:
        >>
        >> $ vim -u NONE --noplugin -c 'wundo! foo' -c 'rundo foo'
        >> Vim: Caught deadly signal SEGV
        >> Vim: Finished.
        >> Segmentation fault
        >>
        >> Program received signal SIGSEGV, Segmentation fault.
        >> 0x081bf48c in u_free_line (ptr=0x8264f90 "", keep=0) at undo.c:2846
        >> 2846        if ((nextb = curbuf->b_mb_current->mb_next) != NULL
        >> (gdb) bt
        >> #0  0x081bf48c in u_free_line (ptr=0x8264f90 "", keep=0) at undo.c:2846
        >> #1  0x081bc5f0 in u_read_undo (name=0x8264e06 "foo",
        >>    hash=0xbfffed9c
        >> "\343\260\304B\230\374\034\024\232\373\364\310\231o\271$'\256A\344d\233\223L\244\225\231\033xR\270U")
        >> at undo.c:1094
        >> #2  0x080afc34 in ex_rundo (eap=0xbfffedfc) at ex_docmd.c:8481
        >> #3  0x080a70dd in do_one_cmd (cmdlinep=0xbfffefb0, sourcing=1,
        >> cstack=0xbfffefb8, fgetline=0,
        >>    cookie=0x0) at ex_docmd.c:2639
        >> #4  0x080a49b6 in do_cmdline (cmdline=0xbffff6a3 "rundo foo",
        >> getline=0, cookie=0x0, flags=11)
        >>    at ex_docmd.c:1108
        >> #5  0x080a4070 in do_cmdline_cmd (cmd=0xbffff6a3 "rundo foo") at ex_docmd.c:714
        >> #6  0x080e935d in exe_commands (parmp=0xbffff364) at main.c:2750
        >> #7  0x080e6b3a in main (argc=8, argv=0xbffff4e4) at main.c:880
        >> (gdb) list
        >> 2841        if (curbuf->b_mb_current == NULL || mp < (minfo_T
        >> *)curbuf->b_mb_current)
        >> 2842        {
        >> 2843            curbuf->b_mb_current = curbuf->b_block_head.mb_next;
        >> 2844            curbuf->b_m_search = NULL;
        >> 2845        }
        >> 2846        if ((nextb = curbuf->b_mb_current->mb_next) != NULL
        >> 2847                                                         && (minfo_T *)nextb < mp)
        >> 2848        {
        >> 2849            curbuf->b_mb_current = nextb;
        >> 2850            curbuf->b_m_search = NULL;
        >>
        >> (gdb) p curbuf
        >> $1 = (buf_T *) 0x8234dd0
        >> (gdb) p curbuf->b_mb_current
        >> $2 = (mblock_T *) 0x0
        >>
        >> So accessing curbuf->b_mb_current->mb_next dereferences NULL.
        >>
        >> I have not had time to look at how u_alloc_line() and u_free_line()
        >> work.  Using vim_malloc() and vim_free() is certainly simpler. malloc()
        >> and free() are also quite optimized.  So I don't see the need
        >> for a special implementation.
        >
        >
        > I now understand why vim-7.3a crashes when undefining line
        > #define U_USE_MALLOC 1  in  "undo.c".
        >
        > u_alloc_line() and u_free_line(), which are used when
        > U_USE_MALLOC is undefined, use buffers inside 'curbuf'.
        > All allocations make during in u_read_undo() gets freed
        > when calling "u_blockfree(curbuf);".
        >
        > Attached patch makes it work I think (I did not test in details)
        > by calling u_blockfree(curbuf) earlier (before any call to U_ALLOC_LINE())
        > in u_read_undo().
        >
        > However, I DON'T THINK IT'S A GOOD IDEA TO APPLY THIS PATCH
        > anyway since if an error happens while loading the persistent undo file,
        > the current undo information would be lost!
        >
        > It's best and a lot simpler to just remove all code in
        > #define U_USE_MALLOC which is not currently used anyway
        > and no longer valid.

        Sorry, I forgot to attach the patch in my previous email. Here it is.

        -- Dominique

        --
        You received this message from the "vim_dev" maillist.
        Do not top-post! Type your reply below the text you are replying to.
        For more information, visit http://www.vim.org/maillist.php
      • Bram Moolenaar
        ... I have deleted the !U_USE_MALLOC code. I don t see a good reason to keep it around. So the crash you reported won t need to be fixed. -- hundred-and-one
        Message 3 of 9 , May 29 8:24 AM
          Dominique Pellé wrote:

          > I now understand why vim-7.3a crashes when undefining line
          > #define U_USE_MALLOC 1 in "undo.c".
          >
          > u_alloc_line() and u_free_line(), which are used when
          > U_USE_MALLOC is undefined, use buffers inside 'curbuf'.
          > All allocations make during in u_read_undo() gets freed
          > when calling "u_blockfree(curbuf);".
          >
          > Attached patch makes it work I think (I did not test in details)
          > by calling u_blockfree(curbuf) earlier (before any call to U_ALLOC_LINE())
          > in u_read_undo().
          >
          > However, I DON'T THINK IT'S A GOOD IDEA TO APPLY THIS PATCH
          > anyway since if an error happens while loading the persistent undo file,
          > the current undo information would be lost!
          >
          > It's best and a lot simpler to just remove all code in
          > #define U_USE_MALLOC which is not currently used anyway
          > and no longer valid.

          I have deleted the !U_USE_MALLOC code. I don't see a good reason to
          keep it around. So the crash you reported won't need to be fixed.

          --
          hundred-and-one symptoms of being an internet addict:
          119. You are reading a book and look for the scroll bar to get to
          the next page.

          /// Bram Moolenaar -- Bram@... -- http://www.Moolenaar.net \\\
          /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
          \\\ download, build and distribute -- http://www.A-A-P.org ///
          \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

          --
          You received this message from the "vim_dev" maillist.
          Do not top-post! Type your reply below the text you are replying to.
          For more information, visit http://www.vim.org/maillist.php
        • Bram Moolenaar
          ... It turns out this is not a problem with the undo file, but the undo structures becoming invalid when using :earlier . I have fixed this in patch 7.4.441.
          Message 4 of 9 , May 30 8:25 AM
            Dominique Pelle wrote:

            > Regarding the leak in undo.c, I found a way to reproduce
            > it all the times with Vim-7.3a (2216:dd5c1983e355).
            > It also give an error E438 (which is an internal error):
            >
            > Using attached file leak.vim, run:
            >
            > $ rm -f foo* ; valgrind --leak-check=yes 2> vg.log \
            > vim -u NONE --noplugin -c 'set undofile' \
            > -c 'so! leak.vim' \
            > -c 'so! leak.vim' foo
            >
            > Then quit with :q!
            >
            > Log file vg.log shows 2 leaks both in u_read_undo():
            >
            > ==2962== 2 bytes in 2 blocks are definitely lost in loss record 2 of 106
            > ==2962== at 0x4024F70: malloc (vg_replace_malloc.c:236)
            > ==2962== by 0x81144F6: lalloc (misc2.c:919)
            > ==2962== by 0x81BC14C: u_read_undo (undo.c:1001)
            > ==2962== by 0x80C4C09: readfile (fileio.c:2591)
            > ==2962== by 0x8053226: open_buffer (buffer.c:132)
            > ==2962== by 0x809836E: do_ecmd (ex_cmds.c:3658)
            > ==2962== by 0x80AE82B: do_exedit (ex_docmd.c:7620)
            > ==2962== by 0x80AE4E8: ex_edit (ex_docmd.c:7516)
            > ==2962== by 0x80A70DC: do_one_cmd (ex_docmd.c:2639)
            > ==2962== by 0x80A49B5: do_cmdline (ex_docmd.c:1108)
            > ==2962== by 0x812A22D: nv_colon (normal.c:5226)
            > ==2962== by 0x8123AB7: normal_cmd (normal.c:1188)
            >
            > ==2962== 1,705 (802 direct, 903 indirect) bytes in 2 blocks are
            > definitely lost in loss record 101 of 106
            > ==2962== at 0x4024F70: malloc (vg_replace_malloc.c:236)
            > ==2962== by 0x81144F6: lalloc (misc2.c:919)
            > ==2962== by 0x81BBE9E: u_read_undo (undo.c:938)
            > ==2962== by 0x80C4C09: readfile (fileio.c:2591)
            > ==2962== by 0x8053226: open_buffer (buffer.c:132)
            > ==2962== by 0x809836E: do_ecmd (ex_cmds.c:3658)
            > ==2962== by 0x80AE82B: do_exedit (ex_docmd.c:7620)
            > ==2962== by 0x80AE4E8: ex_edit (ex_docmd.c:7516)
            > ==2962== by 0x80A70DC: do_one_cmd (ex_docmd.c:2639)
            > ==2962== by 0x80A49B5: do_cmdline (ex_docmd.c:1108)
            > ==2962== by 0x812A22D: nv_colon (normal.c:5226)
            > ==2962== by 0x8123AB7: normal_cmd (normal.c:1188)
            >
            > undo.c:
            > 938 uhp = (u_header_T *)U_ALLOC_LINE((unsigned)sizeof(u_header_T));
            > ...
            > 1001 array = (char_u **)U_ALLOC_LINE(
            > 1002 (unsigned)(sizeof(char_u *) *
            > uep->ue_size));
            >
            > If you source 'so! leak.vim' n times (n == 2 in my example)
            > then 2*n blocks blocks are leaked.
            >
            > Now if you add -N as follows, you also get error
            > "E438: u_undo: line numbers wrong" (which is an
            > internal error:
            >
            > $ rm -f foo* ; valgrind --leak-check=yes 2> vg.log \
            > vim -N -u NONE --noplugin -c 'set undofile' \
            > -c 'so! leak.vim' \
            > -c 'so! leak.vim' foo
            >
            > The patch I sent earlier (which avoids wasting 1 byte
            > in some alloc) actually fixes one of the leaks but not both.
            > E438 happens with or without patch.
            >
            > I have not found how to fix this yet. Any idea?

            It turns out this is not a problem with the undo file, but the undo
            structures becoming invalid when using ":earlier". I have fixed this in
            patch 7.4.441. I no longer see the leak now.

            Along the way I added a few more checks for the undo information that is
            read back from the file to be valid.

            --
            hundred-and-one symptoms of being an internet addict:
            128. You can access the Net -- via your portable and cellular phone.

            /// Bram Moolenaar -- Bram@... -- http://www.Moolenaar.net \\\
            /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
            \\\ download, build and distribute -- http://www.A-A-P.org ///
            \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

            --
            You received this message from the "vim_dev" maillist.
            Do not top-post! Type your reply below the text you are replying to.
            For more information, visit http://www.vim.org/maillist.php
          • Dominique Pellé
            ... Just to confirm that I no longer see the leak either after your latest changes (2228:3b241fd8d7c0). Internal error E438 described in this thread no longer
            Message 5 of 9 , May 30 8:34 AM
              Bram Moolenaar:

              > Dominique Pelle wrote:
              >
              >> Regarding the leak in undo.c, I found a way to reproduce
              >> it all the times with Vim-7.3a (2216:dd5c1983e355).
              >> It also give an error E438 (which is an internal error):
              >>
              >> Using attached file leak.vim, run:
              >>
              >> $ rm -f foo* ; valgrind --leak-check=yes 2> vg.log \
              >>         vim -u NONE --noplugin -c 'set undofile' \
              >>         -c 'so! leak.vim' \
              >>         -c 'so! leak.vim' foo
              >>
              >> Then quit with :q!
              >>
              >> Log file vg.log shows 2 leaks both in u_read_undo():
              >>
              >> ==2962== 2 bytes in 2 blocks are definitely lost in loss record 2 of 106
              >> ==2962==    at 0x4024F70: malloc (vg_replace_malloc.c:236)
              >> ==2962==    by 0x81144F6: lalloc (misc2.c:919)
              >> ==2962==    by 0x81BC14C: u_read_undo (undo.c:1001)
              >> ==2962==    by 0x80C4C09: readfile (fileio.c:2591)
              >> ==2962==    by 0x8053226: open_buffer (buffer.c:132)
              >> ==2962==    by 0x809836E: do_ecmd (ex_cmds.c:3658)
              >> ==2962==    by 0x80AE82B: do_exedit (ex_docmd.c:7620)
              >> ==2962==    by 0x80AE4E8: ex_edit (ex_docmd.c:7516)
              >> ==2962==    by 0x80A70DC: do_one_cmd (ex_docmd.c:2639)
              >> ==2962==    by 0x80A49B5: do_cmdline (ex_docmd.c:1108)
              >> ==2962==    by 0x812A22D: nv_colon (normal.c:5226)
              >> ==2962==    by 0x8123AB7: normal_cmd (normal.c:1188)
              >>
              >> ==2962== 1,705 (802 direct, 903 indirect) bytes in 2 blocks are
              >> definitely lost in loss record 101 of 106
              >> ==2962==    at 0x4024F70: malloc (vg_replace_malloc.c:236)
              >> ==2962==    by 0x81144F6: lalloc (misc2.c:919)
              >> ==2962==    by 0x81BBE9E: u_read_undo (undo.c:938)
              >> ==2962==    by 0x80C4C09: readfile (fileio.c:2591)
              >> ==2962==    by 0x8053226: open_buffer (buffer.c:132)
              >> ==2962==    by 0x809836E: do_ecmd (ex_cmds.c:3658)
              >> ==2962==    by 0x80AE82B: do_exedit (ex_docmd.c:7620)
              >> ==2962==    by 0x80AE4E8: ex_edit (ex_docmd.c:7516)
              >> ==2962==    by 0x80A70DC: do_one_cmd (ex_docmd.c:2639)
              >> ==2962==    by 0x80A49B5: do_cmdline (ex_docmd.c:1108)
              >> ==2962==    by 0x812A22D: nv_colon (normal.c:5226)
              >> ==2962==    by 0x8123AB7: normal_cmd (normal.c:1188)
              >>
              >> undo.c:
              >>  938         uhp = (u_header_T *)U_ALLOC_LINE((unsigned)sizeof(u_header_T));
              >> ...
              >> 1001             array = (char_u **)U_ALLOC_LINE(
              >> 1002                                  (unsigned)(sizeof(char_u *) *
              >> uep->ue_size));
              >>
              >> If you source  'so! leak.vim'  n  times (n == 2 in my example)
              >> then 2*n blocks blocks are leaked.
              >>
              >> Now if you add -N as follows, you also get error
              >> "E438: u_undo: line numbers wrong" (which is an
              >> internal error:
              >>
              >> $ rm -f foo* ; valgrind --leak-check=yes 2> vg.log \
              >>         vim -N -u NONE --noplugin -c 'set undofile' \
              >>         -c 'so! leak.vim' \
              >>         -c 'so! leak.vim' foo
              >>
              >> The patch I sent earlier (which avoids wasting 1 byte
              >> in some alloc) actually fixes one of the leaks but not both.
              >> E438 happens with or without patch.
              >>
              >> I have not found how to fix this yet.  Any idea?
              >
              > It turns out this is not a problem with the undo file, but the undo
              > structures becoming invalid when using ":earlier".  I have fixed this in
              > patch 7.4.441.  I no longer see the leak now.
              >
              > Along the way I added a few more checks for the undo information that is
              > read back from the file to be valid.

              Just to confirm that I no longer see the leak either after your
              latest changes (2228:3b241fd8d7c0). Internal error E438
              described in this thread no longer happens either.

              Thanks!
              -- Dominique

              --
              You received this message from the "vim_dev" maillist.
              Do not top-post! Type your reply below the text you are replying to.
              For more information, visit http://www.vim.org/maillist.php
            Your message has been successfully submitted and would be delivered to recipients shortly.