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