Re: [PATCH] Shut up valgrind

On 24 janv. 2014, at 03:53, François Revol <[email protected]> wrote:
> On 24/01/2014 02:03, Charles Gordon wrote:
>> On 23 janv. 2014, at 14:40, François Revol <[email protected]> wrote:
>>> On 23/01/2014 14:02, Charles Gordon wrote:
>>>> Hi Francois,
>>>> 
>>>> I just committed a large patch to fix this issue among other malloc related inconsistencies.
>>>> valgrind was right, it was a bug!
>>>> qe_mallocz would have cleared the allocated structure, but you used malloc in haiku.cpp to allocate it :(
>>>> valgrind rules!
>>> 
>>> Actually I ran it on GNU/Linux since valgrind isn't available yet on
>>> Haiku... (I just started the port).
>>> The X11 code did use qe_mallocz, but I didn't check the tty code though.
>> 
>> Then this issue is unresolved.
>> I fixed most memory leaks using valgrind, but I cannot test x11 on Linux for the moment.
> 
> It seems to be fixed here.
> 
> I believe it's the qe_malloc in tty.c at font creation that you changed
> to qe_mallocz.
> On Linux both X11 and tty support are built, so it probably initialized
> the tty display regardless.

Thank you for testing.  Your explanation seems correct.  qe_malloc was a mistake anyway.
As a matter of fact, I’m going to rename the “default” allocators to a more explicit name (qe_malloc_raw for instance)
to make them less palatable and therefore make the safe ones the obvious choice.

> As for the --free-all option, I don't see the need, if you want to free
> at exit then do it, but there is no need for an option :

I agree, the only reason I made this an option was to not do it by default, but allow me to test complete deletion
and therefore consistency under valgrind.  If there is a way to test at runtime if we are running under valgrind, I would only
do the deletions under valgrind and exit directly otherwise, and get rid of the option too.

> It makes a difference:
> 
> ==450==     in use at exit: 934,064 bytes in 894 blocks
> vs.
> ==451==     in use at exit: 71,025 bytes in 497 blocks
> 
> But it frees a little too much maybe:
> 
> ==451== Invalid read of size 8
> ==451==    at 0x41B104: main (qe.c:8111)
> ==451==  Address 0x647b1f0 is 0 bytes inside a block of size 24 free'd
> ==451==    at 0x4C2A70C: free (vg_replace_malloc.c:468)
> ==451==    by 0x41B12B: main (qe.c:8112)

Interesting.  is this the line where I free the key bindings ?
double free here smells fishy.

> So I'd say just don't bother.

Well, I don’t exactly agree.  If we want qemacs to become a “better” implementation of emacs, faster, more versatile and
extendable in a language spoken by mere mortals (lisp vs. C and C-like scripting), it needs to be very reliable.
Any rogue pointer or memory chunk unaccounted for is a liability.  Editor crashes are very very bad.
I am willing to sacrifice some performance and some extra code and time for this, and any help in tracking these bugs is a blessing.
valgrind is a great tool, let’s use it completely.

If you have a reproducible test case for the above, I’m interested.

Thanks

Chqrlie.