On Mon, Feb 15, 2021 at 2:46 PM Martin Liška <mli...@suse.cz> wrote: > > On 2/12/21 5:56 PM, Martin Sebor wrote: > > On 2/12/21 2:09 AM, Richard Biener via Gcc-patches wrote: > >> On Thu, Feb 11, 2021 at 6:41 PM Martin Liška <mli...@suse.cz> wrote: > >>> > >>> Hello. > >>> > >>> This fixes 2 memory leaks I noticed. > >>> > >>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > >>> > >>> Ready to be installed? > >> > >> OK. > >> > >>> Thanks, > >>> Martin > >>> > >>> gcc/ChangeLog: > >>> > >>> * opts-common.c (decode_cmdline_option): Release werror_arg. > >>> * opts.c (gen_producer_string): Release output of > >>> gen_command_line_string. > >>> --- > >>> gcc/opts-common.c | 1 + > >>> gcc/opts.c | 7 +++++-- > >>> 2 files changed, 6 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/gcc/opts-common.c b/gcc/opts-common.c > >>> index 6cb5602896d..5e10edeb4cf 100644 > >>> --- a/gcc/opts-common.c > >>> +++ b/gcc/opts-common.c > >>> @@ -766,6 +766,7 @@ decode_cmdline_option (const char *const *argv, > >>> unsigned int lang_mask, > >>> werror_arg[0] = 'W'; > >>> > >>> size_t warning_index = find_opt (werror_arg, lang_mask); > >>> + free (werror_arg); > > > > Sorry to butt in here, but since we're having a discussion on this > > same subject in another review of a fix for a memory leak, I thought > > I'd pipe up: I would suggest a better (more in line with C++ and more > > future-proof) solution is to replace the call to xstrdup (and the need > > to subsequently call free) with std::string. > > Hello. > > To be honest, I like the suggested approach using std::string. On the other > hand, > I don't want to mix existing C API (char *) with std::string.
That's one of the main problems. > I'm sending a patch that fixed the remaining leaks. OK. > > > >>> if (warning_index != OPT_SPECIAL_unknown) > >>> { > >>> const struct cl_option *warning_option > >>> diff --git a/gcc/opts.c b/gcc/opts.c > >>> index fc5f61e13cc..24bb64198c8 100644 > >>> --- a/gcc/opts.c > >>> +++ b/gcc/opts.c > >>> @@ -3401,8 +3401,11 @@ char * > >>> gen_producer_string (const char *language_string, cl_decoded_option > >>> *options, > >>> unsigned int options_count) > >>> { > >>> - return concat (language_string, " ", version_string, " ", > >>> - gen_command_line_string (options, options_count), NULL); > >>> + char *cmdline = gen_command_line_string (options, options_count); > >>> + char *combined = concat (language_string, " ", version_string, " ", > >>> + cmdline, NULL); > >>> + free (cmdline); > >>> + return combined; > >>> } > > > > Here, changing gen_command_line_string() to return std::string instead > > of a raw pointer would similarly avoid having to remember to free > > the pointer (and forgetting). The function has two other callers, > > both in gcc/toplev.c, and both also leak the memory for the same > > reason as here. > > Btw. have we made a general consensus that using std::string is fine in the > GCC internals? No, we didn't. But if Martin likes RAII adding sth like template <class T> class auto_free { auto_free (T *ptr) : m_ptr (ptr) {}; ~auto_free () { m_ptr->~T (); free (m_ptr); } T *m_ptr; }; with appropriate move CTORs to support returning this should be straight-forward. You should then be able to change the return type from char * to auto_free<char> or so. But then there's the issue of introducing lifetime bugs because you definitely need to have the pointer escape at points like the printf ... Richard. > Martin > > > > > Martin > > > >>> > >>> #if CHECKING_P > >>> -- > >>> 2.30.0 > >>> > > >