Re: [PATCH] sim/m32c: fix memory leaks in opc2c

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-04-05 5:51 p.m., Mike Frysinger wrote:
> On 05 Apr 2021 14:46, Simon Marchi via Gdb-patches wrote:
>> On 2021-04-05 12:23 p.m., Mike Frysinger wrote:> On 05 Apr 2021 10:58, Simon Marchi via Gdb-patches wrote:
>>>> Fix the leak in dump_lines by free-ing the elements of varnames.
>>>
>>> i dislike stuffing a bunch of free's at the end of a program's lifetime just
>>> to satisfy a tool that is not normally used.  which isn't to say LSAN isn't
>>> useful, just that i think we should do better.
>>
>> Why though?  Because it adds execution time where not necessary?
> 
> when the process exits, the kernel releases all the memory at once.  there's
> no point to calling free() on all your allocated buffers before exiting.  it
> only wastes time with the C library heap accounting & syscalls.
> 
> i get that we're talking about opc2c here which is used only twice to generate
> two other files, so in the larger scheme of things, it's barely noise.  i'm
> trying to define standard patterns we can apply in general for "the next one".

I understand.  I'm not that worried about that kind of performance hit
though.

>>> in other codebases, i've done things like:
>>> #ifdef __SANITIZE_ADDRESS__
>>> # define ENABLE_CLEANUP_ONEXIT 1
>>> #else
>>> # define ENABLE_CLEANUP_ONEXIT 0
>>> #endif
>>>
>>> then this could be written:
>>>
>>> if (ENABLE_CLEANUP_ONEXIT) {
>>>   for (i = 0; i < vn; i++)
>>>     free (varnames[i]);
>>> }
>>>
>>> wdyt ?  feel free to bikeshed the name.  not sure if there's prior art in
>>> the tree currently.
>>
>> I find this complexity a bit unnecessary, versus just free-ing the
>> stuff.  But I don't really mind, we can do as you like, I just want to
>> my build to be fixed!
>>
>> Note that the igen tool doesn't free anything, so it's next on the list
>> of things that break the -fsanizite=address build.  I started to update
>> it to free things, it's a bit tedious but it should be do-able.
>>
>> Another option would be to change the Makefile to call igen with the
>> ASAN_OPTIONS=detect_leaks=0 environment variable.
> 
> ah yes, this is exactly what i mean wrt "the tip of the iceberg" and it being
> "a slippery slope" ;).  first it's the small build tools, then the larger
> build tools, then the programs we actually install, ...
> 
> maybe an alternative would be to convert these to C++.  then it would handle
> stack/heap resources for us.

If you convert to C++ and the memory is managed automatically, isn't it
exactly the same (runtime-wise) as free-ing everything by hand in C?
Although I'm always in favor of using C++ for just not having to think
about free-ing stuff.

Simon