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

[Mike Frysinger via Gdb-patches <gdb-patches@sourceware.org>]

On 05 Apr 2021 21:36, Simon Marchi via Gdb-patches wrote:
> 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:
> >>> 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?

while i'm sure there is some automatic heap freeing, C++ stdlib can do
smarter memory management, whether it be caches, or stack based.  

> Although I'm always in favor of using C++ for just not having to think
> about free-ing stuff.

right, and we don't have to debate this :).  i can let it go for the sake
of the gains with the better language.

NB: this would only apply to build-time tools like opc2c.

to be clear, if you want to take on changing these tools to C++, i'd be
more than happy to help review.  but if you feel that's too much to take
on, we can do the aforementioned conditional frees.
-mike

--- a/sim/Makefile.am
+++ b/sim/Makefile.am
@@ -36,6 +36,7 @@ DISTCLEANFILES =
 MOSTLYCLEANFILES = core
 
 COMPILE_FOR_BUILD = $(CC_FOR_BUILD) $(AM_CPPFLAGS) $(CFLAGS_FOR_BUILD)
+CXXCOMPILE_FOR_BUILD = $(CXX_FOR_BUILD) $(AM_CPPFLAGS) $(CXXFLAGS_FOR_BUILD)
 LINK_FOR_BUILD = $(CC_FOR_BUILD) $(CFLAGS_FOR_BUILD) $(LDFLAGS_FOR_BUILD) -o $@
 
 # Generate nltvals.def for newlib/libgloss using devo and build tree.
--- a/sim/m4/sim_ac_toolchain.m4
+++ b/sim/m4/sim_ac_toolchain.m4
@@ -27,15 +27,21 @@ AC_PROG_INSTALL
 dnl Setup toolchain settings for build-time tools..
 if test "x$cross_compiling" = "xno"; then
   : "${CC_FOR_BUILD:=\$(CC)}"
+  : "${CXX_FOR_BUILD:=\$(CXX)}"
   : "${CFLAGS_FOR_BUILD:=\$(CFLAGS)}"
+  : "${CXXFLAGS_FOR_BUILD:=\$(CXXFLAGS)}"
   : "${LDFLAGS_FOR_BUILD:=\$(LDFLAGS)}"
 else
   : "${CC_FOR_BUILD:=gcc}"
+  : "${CXX_FOR_BUILD:=g++}"
   : "${CFLAGS_FOR_BUILD:=-g -O}"
+  : "${CXXFLAGS_FOR_BUILD:=-g -O}"
   : "${LDLFAGS_FOR_BUILD:=}"
 fi
 AC_SUBST(CC_FOR_BUILD)
+AC_SUBST(CXX_FOR_BUILD)
 AC_SUBST(CFLAGS_FOR_BUILD)
+AC_SUBST(CXXFLAGS_FOR_BUILD)
 AC_SUBST(LDFLAGS_FOR_BUILD)
 
 AC_SUBST(CFLAGS)