Re: [PATCH] Fix producer string memory leaks
On 2/18/21 2:22 AM, Richard Biener wrote:
On Tue, Feb 16, 2021 at 5:09 PM Martin Sebor wrote:
On 2/15/21 7:39 AM, Richard Biener wrote:
On Mon, Feb 15, 2021 at 2:46 PM Martin Liška 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 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
Re: [PATCH] Fix producer string memory leaks
On Tue, Feb 16, 2021 at 5:09 PM Martin Sebor wrote:
>
> On 2/15/21 7:39 AM, Richard Biener wrote:
> > On Mon, Feb 15, 2021 at 2:46 PM Martin Liška 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 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 auto_free
> > {
> > auto_free (T *ptr) : m_ptr (ptr) {};
> >~auto_free () { m_ptr->~T
Re: [PATCH] Fix producer string memory leaks
> "Martin" == Martin Sebor via Gcc-patches writes: Martin> FWIW, I have prototyped Martin> a simple string class over the weekend (based on auto_vec) that I'm Martin> willing to contribute if std::string turns out to be out of favor. I wonder whether GDB and GCC can or should collaborate in this area. GDB has been using C++ for a while now, and we've encountered many of these same transitional problems. My sense is that, unlike GCC, GDB has been a bit more aggressive about adopting C++11 style though. For this particular area, GDB uses std::string pretty freely -- but not universally, both due to its history as a C code base, but also because std::string can be too heavy for some uses. Not all code needs to carry around the length, etc. Pedro adapted the C++17 string_view from libstdc++, and this is used in some spots in GDB. See gdbsupport/gdb_string_view.h. GDB also uses a unique_ptr specialization that wraps xmalloc/xfree. See gdbsupport/gdb_unique_ptr.h. So in GDB, instead of writing a new string-ish class, I suppose we'd simply hook string_view up to unique_xmalloc_ptr or something along those lines. Martin> There is std::unique_ptr that we could use rather than rolling our Martin> own. That said, I don't think using std::unique_ptr over a string Martin> class would be appropriate for things like local (string) variables Martin> or return types of functions returning strings. In GDB we do this kind of thing all the time. The main idea is to indicate ownership transfer via the type system. This helps eliminate comments like "the caller must free the result" -- the return of a unique_ptr explains this directly. >> But then there's the issue of introducing lifetime bugs because >> you definitely need to have the pointer escape at points like >> the printf ... This is a valid concern in C++, but hasn't been a big practical issue in GDB. Tom
Re: [PATCH] Fix producer string memory leaks
On 2/15/21 7:39 AM, Richard Biener wrote:
On Mon, Feb 15, 2021 at 2:46 PM Martin Liška 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 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 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 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
Re: [PATCH] Fix producer string memory leaks
On Mon, Feb 15, 2021 at 2:46 PM Martin Liška 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 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 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 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
> >>>
> >
>
Re: [PATCH] Fix producer string memory leaks
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 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.
I'm sending a patch that fixed the remaining leaks.
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?
Martin
Martin
#if CHECKING_P
--
2.30.0
>From 8c3c6812cb094888696302001c114dc11cfa2694 Mon Sep 17 00:00:00 2001
From: Martin Liska
Date: Mon, 15 Feb 2021 11:28:19 +0100
Subject: [PATCH] Fix 2 more leaks related to gen_command_line_string.
gcc/ChangeLog:
* toplev.c (init_asm_output): Free output of
gen_command_line_string function.
(process_options): Likewise.
---
gcc/toplev.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 05bd449eafc..d8cc254adef 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -748,9 +748,10 @@ init_asm_output (const char *name)
print_version (asm_out_file, ASM_COMMENT_START, true);
fputs (ASM_COMMENT_START, asm_out_file);
fputs (" options passed: ", asm_out_file);
- fputs (gen_command_line_string (save_decoded_options,
- save_decoded_options_count),
- asm_out_file);
+ char *cmdline = gen_command_line_string (save_decoded_options,
+ save_decoded_options_count);
+ fputs (cmdline, asm_out_file);
+ free (cmdline);
fputc ('\n', asm_out_file);
}
}
@@ -1384,8 +1385,11 @@ process_options (void)
if (!quiet_flag)
{
fputs ("options passed: ", stderr);
- fputs (gen_command_line_string (save_decoded_options,
- save_decoded_options_count), stderr);
+ char *cmdline = gen_command_line_string (save_decoded_options,
+ save_decoded_options_count);
+
+ fputs (cmdline, stderr);
+ free (cmdline);
fputc ('\n', stderr);
}
}
--
2.30.0
Re: [PATCH] Fix producer string memory leaks
On 2/12/21 9:56 AM, 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 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.
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.
It occurs to me that until the APIs are improved (to use RAII), or
where changing them is not feasible, an alternative is to let our
own analyzer detect the leaks by annotating gen_command_line_string
and other functions like it with the new attribute malloc. With
that, this leak is diagnosed like so:
/src/gcc/master/gcc/opts.c: In function ‘char*
gen_command_line_string(cl_decoded_option*, unsigned int)’:
/src/gcc/master/gcc/opts.c:3395:10: warning: leak of ‘cmdline’ [CWE-401]
[-Wanalyzer-malloc-leak]
3395 | return options_string;
| ^~
‘char* gen_producer_string(const char*, cl_decoded_option*, unsigned
int)’: events 1-3
|
| 3401 | gen_producer_string (const char *language_string,
cl_decoded_option *options,
| | ^~~
| | |
| | (1) entry to ‘gen_producer_string’
|..
| 3404 | char *cmdline = gen_command_line_string (options,
options_count);
...
The attribute malloc annotation would look like this:
__attribute__ ((__malloc__, __malloc__ (free)))
extern char *gen_command_line_string (cl_decoded_option *options,
unsigned int options_count);
It would be worthwhile to do a review of GCC's own APIs and add
the attribute wherever appropriate, not just to detect leaks but
other memory management bugs (e.g., calling free on memory
allocated by operator new). Something to consider for GCC 12.
Martin
Re: [PATCH] Fix producer string memory leaks
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 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.
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.
Martin
#if CHECKING_P
--
2.30.0
Re: [PATCH] Fix producer string memory leaks
On Thu, Feb 11, 2021 at 6:41 PM Martin Liška 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);
> 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;
> }
>
> #if CHECKING_P
> --
> 2.30.0
>
[PATCH] Fix producer string memory leaks
Hello.
This fixes 2 memory leaks I noticed.
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
Ready to be installed?
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);
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;
}
#if CHECKING_P
--
2.30.0
