On 2/18/21 2:22 AM, Richard Biener wrote:
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.
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
