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