[PATCH] Fix producer string memory leaks
Martin Sebor
msebor@gmail.com
Thu Feb 18 23:53:34 GMT 2021
On 2/18/21 2:22 AM, Richard Biener wrote:
> On Tue, Feb 16, 2021 at 5:09 PM Martin Sebor <msebor@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 <mliska@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 <mliska@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.
Let's try to fix that then.
>
> 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.
Very little C++ code has the luxury of being pure C++, without
having to interface with C code at some level. Most projects that
started out as C and transitioned to C++ also are a mix of C and
C++ APIs and idioms as the transition is usually slow, piecemeal,
and commonly never fully complete. That doesn't mean they can't
or shouldn't use std::string, or other components from the C++
standard library, or powerful C++ idioms.
But since we're making no progress here let me start a broader
discussion about this topic. Maybe closer to when we're done with
the release. If it turns out that there is consensus against using
std::string in GCC I volunteer to update the coding conventions and
contribute the class I prototyped. Sharing experiences and even
code with other projects like GDB might be worth considering.
Martin
>> 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
>>>>>>>
>>>>>
>>>>
>>
More information about the Gcc-patches
mailing list