On Tue, Feb 16, 2021 at 5:09 PM Martin Sebor <mse...@gmail.com> wrote:
>
> On 2/15/21 7:39 AM, Richard Biener wrote:
> > 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
>
> It's not so much what I like as about using established C++ idioms
> to help us avoid common memory management mistakes (leaks, dangling
> pointers, etc.)
>
> With respect to the C++ standard library, the GCC Coding Conventions
> say:
>
> Use of the standard library is permitted. Note, however, that it
> is currently not usable with garbage collected data.
>
> So as a return value of a function, or as a local variable, using
> std::string seems entirely appropriate, and I have been using it
> that way.
>
> Richard, if that's not in line with your understanding of
> the intent of the text in the conventions or with your expectations,
The conventions were written / changed arbitrarily without real consent.
Using std::string just because it implements the RAII part you like
but then still needing to interface with C string APIs on _both_ sides
makes std::string a bad fit.
GCCs code-base is a mess of C/C++ mix already, throwing std::string
inbetween a C string API sandwitch isn't going to improve things.
> please propose a change for everyone's consideration. If a consensus
> emerges not to use std::string in GCC (or any other part of the C++
> library) let's update the coding conventions. FWIW, I have prototyped
> a simple string class over the weekend (based on auto_vec) that I'm
> willing to contribute if std::string turns out to be out of favor.
>
> > 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.
>
> There is std::unique_ptr that we could use rather than rolling our
> own. That said, I don't think using std::unique_ptr over a string
> class would be appropriate for things like local (string) variables
> or return types of functions returning strings. (GCC garbage
> collection means std::string might not be suitable as a member of
> persistent data structures).
>
> Martin
>
> >
> > 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
> >>>>>
> >>>
> >>
>
