3 remarks about undo.c

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

3 remarks about undo.c

Dominique Pellé
Hi

3 remarks about vim/src/undo.c (at changeset: 271a5907f944):

(1) Using persistent-undo, I notice once in a while the following memory
    leak, but I have not found the way to reproduce it all the time:

==3371== 10 bytes in 2 blocks are definitely lost in loss record 23 of 116
==3371==    at 0x4024F70: malloc (vg_replace_malloc.c:236)
==3371==    by 0x8114F98: lalloc (misc2.c:919)
==3371==    by 0x81BCC06: u_read_undo (undo.c:983)
==3371==    by 0x80C5359: readfile (fileio.c:2591)
==3371==    by 0x8053976: open_buffer (buffer.c:132)
==3371==    by 0x8098ABE: do_ecmd (ex_cmds.c:3658)
==3371==    by 0x80AEF7B: do_exedit (ex_docmd.c:7620)
==3371==    by 0x80AEC38: ex_edit (ex_docmd.c:7516)
==3371==    by 0x80A782C: do_one_cmd (ex_docmd.c:2639)
==3371==    by 0x80A5105: do_cmdline (ex_docmd.c:1108)
==3371==    by 0x812AD39: nv_colon (normal.c:5226)
==3371==    by 0x81245C3: normal_cmd (normal.c:1188)
==3371==    by 0x80E7938: main_loop (main.c:1216)
==3371==    by 0x80E742F: main (main.c:960)

It's this alloc in undo.c:

 983        array = (char_u **)U_ALLOC_LINE(
 984                             (unsigned)(sizeof(char_u *) * uep->ue_size));

Looking at the undo.c code, memory seems to be properly freed
but I might be missing something since Valgrind does not generally
give spurious leak messages.


(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.


(3) When U_USE_MALLOC is defined (default behavior), I see that 1 byte
    is added to every allocations at line 117:

 115 #ifdef U_USE_MALLOC
 116 # define U_FREE_LINE(ptr) vim_free(ptr)
 117 # define U_ALLOC_LINE(size) lalloc((long_u)((size) + 1), FALSE)
 118 #else

This extra 1 byte is odd since it's not needed everywhere U_ALLOC_LINE(...)
is used.  I think it's better if the caller is responsible for adding +1
when needed to avoid wasting memory.  Attached patch does that.

Maybe this patch also fixes the leak (1) but I'm not sure since I don't
know how to reproduce it all the time.  At least, I have not seen the
leak  so far after applying the attached patch.

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

dont_waste_mem-undo.c-7.3a.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: 3 remarks about undo.c

Bram Moolenaar

Dominique Pelle wrote:

> 3 remarks about vim/src/undo.c (at changeset: 271a5907f944):
>
> (1) Using persistent-undo, I notice once in a while the following memory
>     leak, but I have not found the way to reproduce it all the time:
>
> ==3371== 10 bytes in 2 blocks are definitely lost in loss record 23 of 116
> ==3371==    at 0x4024F70: malloc (vg_replace_malloc.c:236)
> ==3371==    by 0x8114F98: lalloc (misc2.c:919)
> ==3371==    by 0x81BCC06: u_read_undo (undo.c:983)
> ==3371==    by 0x80C5359: readfile (fileio.c:2591)
> ==3371==    by 0x8053976: open_buffer (buffer.c:132)
> ==3371==    by 0x8098ABE: do_ecmd (ex_cmds.c:3658)
> ==3371==    by 0x80AEF7B: do_exedit (ex_docmd.c:7620)
> ==3371==    by 0x80AEC38: ex_edit (ex_docmd.c:7516)
> ==3371==    by 0x80A782C: do_one_cmd (ex_docmd.c:2639)
> ==3371==    by 0x80A5105: do_cmdline (ex_docmd.c:1108)
> ==3371==    by 0x812AD39: nv_colon (normal.c:5226)
> ==3371==    by 0x81245C3: normal_cmd (normal.c:1188)
> ==3371==    by 0x80E7938: main_loop (main.c:1216)
> ==3371==    by 0x80E742F: main (main.c:960)
>
> It's this alloc in undo.c:
>
>  983        array = (char_u **)U_ALLOC_LINE(
>  984                             (unsigned)(sizeof(char_u *) * uep->ue_size));
>
> Looking at the undo.c code, memory seems to be properly freed
> but I might be missing something since Valgrind does not generally
> give spurious leak messages.

The logic is not easy to follow.  Also, there are hardly any checks for
the undo file to contain invalid information.  I think it's good to
assume that every value read from the file might be wrong.

I think we need to add more checks that what is read back from the undo
file is correct.  E.g., when "num_head" is too small the code would
gladly store pointers beyond the end of uhp_table[].


> (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?

I don't think that anybody compiles Vim with U_USE_MALLOC undefined.
It's probably safe to remove this old code now.

> (3) When U_USE_MALLOC is defined (default behavior), I see that 1 byte
>     is added to every allocations at line 117:
>
>  115 #ifdef U_USE_MALLOC
>  116 # define U_FREE_LINE(ptr) vim_free(ptr)
>  117 # define U_ALLOC_LINE(size) lalloc((long_u)((size) + 1), FALSE)
>  118 #else
>
> This extra 1 byte is odd since it's not needed everywhere U_ALLOC_LINE(...)
> is used.  I think it's better if the caller is responsible for adding +1
> when needed to avoid wasting memory.  Attached patch does that.
>
> Maybe this patch also fixes the leak (1) but I'm not sure since I don't
> know how to reproduce it all the time.  At least, I have not seen the
> leak  so far after applying the attached patch.

I don't think it fixes the leak.  It does make it a little bit more
efficient.

Please test the code with this patch for a while.  I'll include it
later.

--
Common sense is what tells you that the world is flat.

 /// Bram Moolenaar -- [hidden email] -- 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
Reply | Threaded
Open this post in threaded view
|

Re: 3 remarks about undo.c

Dominique Pellé
Bram Moolenaar wrote:

> Dominique Pelle wrote:
>
>> 3 remarks about vim/src/undo.c (at changeset: 271a5907f944):
>>
>> (1) Using persistent-undo, I notice once in a while the following memory
>>     leak, but I have not found the way to reproduce it all the time:
>>
>> ==3371== 10 bytes in 2 blocks are definitely lost in loss record 23 of 116
>> ==3371==    at 0x4024F70: malloc (vg_replace_malloc.c:236)
>> ==3371==    by 0x8114F98: lalloc (misc2.c:919)
>> ==3371==    by 0x81BCC06: u_read_undo (undo.c:983)
>> ==3371==    by 0x80C5359: readfile (fileio.c:2591)
>> ==3371==    by 0x8053976: open_buffer (buffer.c:132)
>> ==3371==    by 0x8098ABE: do_ecmd (ex_cmds.c:3658)
>> ==3371==    by 0x80AEF7B: do_exedit (ex_docmd.c:7620)
>> ==3371==    by 0x80AEC38: ex_edit (ex_docmd.c:7516)
>> ==3371==    by 0x80A782C: do_one_cmd (ex_docmd.c:2639)
>> ==3371==    by 0x80A5105: do_cmdline (ex_docmd.c:1108)
>> ==3371==    by 0x812AD39: nv_colon (normal.c:5226)
>> ==3371==    by 0x81245C3: normal_cmd (normal.c:1188)
>> ==3371==    by 0x80E7938: main_loop (main.c:1216)
>> ==3371==    by 0x80E742F: main (main.c:960)
>>
>> It's this alloc in undo.c:
>>
>>  983        array = (char_u **)U_ALLOC_LINE(
>>  984                             (unsigned)(sizeof(char_u *) * uep->ue_size));
>>
>> Looking at the undo.c code, memory seems to be properly freed
>> but I might be missing something since Valgrind does not generally
>> give spurious leak messages.
>
> The logic is not easy to follow.  Also, there are hardly any checks for
> the undo file to contain invalid information.  I think it's good to
> assume that every value read from the file might be wrong.
>
> I think we need to add more checks that what is read back from the undo
> file is correct.  E.g., when "num_head" is too small the code would
> gladly store pointers beyond the end of uhp_table[].
>
>
>> (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 don't think that anybody compiles Vim with U_USE_MALLOC undefined.
> It's probably safe to remove this old code now.
>
>> (3) When U_USE_MALLOC is defined (default behavior), I see that 1 byte
>>     is added to every allocations at line 117:
>>
>>  115 #ifdef U_USE_MALLOC
>>  116 # define U_FREE_LINE(ptr) vim_free(ptr)
>>  117 # define U_ALLOC_LINE(size) lalloc((long_u)((size) + 1), FALSE)
>>  118 #else
>>
>> This extra 1 byte is odd since it's not needed everywhere U_ALLOC_LINE(...)
>> is used.  I think it's better if the caller is responsible for adding +1
>> when needed to avoid wasting memory.  Attached patch does that.
>>
>> Maybe this patch also fixes the leak (1) but I'm not sure since I don't
>> know how to reproduce it all the time.  At least, I have not seen the
>> leak  so far after applying the attached patch.
>
> I don't think it fixes the leak.  It does make it a little bit more
> efficient.
>
> Please test the code with this patch for a while.  I'll include it
> later.

I'll keep using the patch.  So far no problem to report with it.
I'll have more time during the weekend to stress test persistent undo
with & without the patch. It would be nice to find a way to reproduce
the leak.

-- 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
Reply | Threaded
Open this post in threaded view
|

Re: 3 remarks about undo.c

Dominique Pellé
In reply to this post by Bram Moolenaar
Bram Moolenaar wrote:

> Dominique Pelle wrote:
>
>> 3 remarks about vim/src/undo.c (at changeset: 271a5907f944):
>>
>> (1) Using persistent-undo, I notice once in a while the following memory
>>     leak, but I have not found the way to reproduce it all the time:
>>
>> ==3371== 10 bytes in 2 blocks are definitely lost in loss record 23 of 116
>> ==3371==    at 0x4024F70: malloc (vg_replace_malloc.c:236)
>> ==3371==    by 0x8114F98: lalloc (misc2.c:919)
>> ==3371==    by 0x81BCC06: u_read_undo (undo.c:983)
>> ==3371==    by 0x80C5359: readfile (fileio.c:2591)
>> ==3371==    by 0x8053976: open_buffer (buffer.c:132)
>> ==3371==    by 0x8098ABE: do_ecmd (ex_cmds.c:3658)
>> ==3371==    by 0x80AEF7B: do_exedit (ex_docmd.c:7620)
>> ==3371==    by 0x80AEC38: ex_edit (ex_docmd.c:7516)
>> ==3371==    by 0x80A782C: do_one_cmd (ex_docmd.c:2639)
>> ==3371==    by 0x80A5105: do_cmdline (ex_docmd.c:1108)
>> ==3371==    by 0x812AD39: nv_colon (normal.c:5226)
>> ==3371==    by 0x81245C3: normal_cmd (normal.c:1188)
>> ==3371==    by 0x80E7938: main_loop (main.c:1216)
>> ==3371==    by 0x80E742F: main (main.c:960)
>>
>> It's this alloc in undo.c:
>>
>>  983        array = (char_u **)U_ALLOC_LINE(
>>  984                             (unsigned)(sizeof(char_u *) * uep->ue_size));
>>
>> Looking at the undo.c code, memory seems to be properly freed
>> but I might be missing something since Valgrind does not generally
>> give spurious leak messages.
>
> The logic is not easy to follow.  Also, there are hardly any checks for
> the undo file to contain invalid information.  I think it's good to
> assume that every value read from the file might be wrong.
>
> I think we need to add more checks that what is read back from the undo
> file is correct.  E.g., when "num_head" is too small the code would
> gladly store pointers beyond the end of uhp_table[].

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?

-- 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

leak.vim (48 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: 3 remarks about undo.c

Dominique Pellé
In reply to this post by Dominique Pellé
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
Reply | Threaded
Open this post in threaded view
|

Re: 3 remarks about undo.c

Dominique Pellé
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

fixed-U_USE_MALLOC-undo.c-7.3a.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: 3 remarks about undo.c

Bram Moolenaar
In reply to this post by Dominique Pellé

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 -- [hidden email] -- 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
Reply | Threaded
Open this post in threaded view
|

Re: 3 remarks about undo.c

Bram Moolenaar
In reply to this post by Dominique Pellé

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 -- [hidden email] -- 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
Reply | Threaded
Open this post in threaded view
|

Re: 3 remarks about undo.c

Dominique Pellé
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