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

Reply via email to