[PATCH] driver: Fix memory leak.

[PATCH] driver: Fix memory leak.

[Costas Argyris]

[-- Attachment #1.1: Type: text/plain, Size: 224 bytes --]

Use std::vector instead of malloc'd pointer
to get automatic freeing of memory.

Result was verified by valgrind, which showed
one less loss record.

I think Jonathan is/was working on this transition
but on a larger scale.

[-- Attachment #2: 0001-driver-Fix-memory-leak.patch --]
[-- Type: application/octet-stream, Size: 1576 bytes --]

From cf444a8c5146629ac883da46f5f5300cd9f3d75e Mon Sep 17 00:00:00 2001
From: Costas Argyris <costas.argyris@gmail.com>
Date: Sat, 2 Dec 2023 20:52:07 +0000
Subject: [PATCH] driver: Fix memory leak.

Use std::vector instead of malloc'd pointer
to get automatic freeing of memory.

Result was verified by valgrind, which showed
one less loss record.

Signed-off-by: Costas Argyris <costas.argyris@gmail.com>
---
 gcc/gcc.cc | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/gcc/gcc.cc b/gcc/gcc.cc
index 9f21ad9453e..63be4247f1d 100644
--- a/gcc/gcc.cc
+++ b/gcc/gcc.cc
@@ -47,6 +47,7 @@ compilation is specified by a string called a "spec".  */
 #include "opts-jobserver.h"
 #include "common/common-target.h"
 #include "gcc-urlifier.h"
+#include <vector>
 
 #ifndef MATH_LIBRARY
 #define MATH_LIBRARY "m"
@@ -9534,7 +9535,7 @@ struct mdswitchstr
   int len;
 };
 
-static struct mdswitchstr *mdswitches;
+static std::vector<struct mdswitchstr> mdswitches;
 static int n_mdswitches;
 
 /* Check whether a particular argument was used.  The first time we
@@ -9753,7 +9754,7 @@ set_multilib_dir (void)
     {
       int i = 0;
 
-      mdswitches = XNEWVEC (struct mdswitchstr, n_mdswitches);
+      mdswitches.reserve (n_mdswitches);
       for (start = multilib_defaults; *start != '\0'; start = end + 1)
 	{
 	  while (*start == ' ' || *start == '\t')
@@ -11366,7 +11367,7 @@ driver::finalize ()
   input_from_pipe = 0;
   suffix_subst = NULL;
 
-  mdswitches = NULL;
+  mdswitches.clear ();
   n_mdswitches = 0;
 
   used_arg.finalize ();
-- 
2.30.2

Re: [PATCH] driver: Fix memory leak.

[Jonathan Wakely]

On Sat, 2 Dec 2023 at 21:24, Costas Argyris wrote:
>
> Use std::vector instead of malloc'd pointer
> to get automatic freeing of memory.

You can't include <vector> there. Instead you need to define
INCLUDE_VECTOR before "system.h"

Shouldn't you be using resize, not reserve? Otherwise mdswitches[i] is
undefined.

Re: [PATCH] driver: Fix memory leak.

[Costas Argyris]

[-- Attachment #1.1: Type: text/plain, Size: 453 bytes --]

Attached a new patch with these changes.

On Mon, 4 Dec 2023 at 12:15, Jonathan Wakely <jwakely@redhat.com> wrote:

> On Sat, 2 Dec 2023 at 21:24, Costas Argyris wrote:
> >
> > Use std::vector instead of malloc'd pointer
> > to get automatic freeing of memory.
>
> You can't include <vector> there. Instead you need to define
> INCLUDE_VECTOR before "system.h"
>
> Shouldn't you be using resize, not reserve? Otherwise mdswitches[i] is
> undefined.
>
>

[-- Attachment #2: 0001-driver-Fix-memory-leak.patch --]
[-- Type: application/octet-stream, Size: 1572 bytes --]

From 69ec37b97234fc47e77b633e0969c2646d4f2e4a Mon Sep 17 00:00:00 2001
From: Costas Argyris <costas.argyris@gmail.com>
Date: Sat, 2 Dec 2023 20:52:07 +0000
Subject: [PATCH] driver: Fix memory leak.

Use std::vector instead of malloc'd pointer
to get automatic freeing of memory.

Result was verified by valgrind, which showed
one less loss record.

Signed-off-by: Costas Argyris <costas.argyris@gmail.com>
---
 gcc/gcc.cc | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/gcc/gcc.cc b/gcc/gcc.cc
index 9f21ad9453e..2623ab3a855 100644
--- a/gcc/gcc.cc
+++ b/gcc/gcc.cc
@@ -29,6 +29,7 @@ compilation is specified by a string called a "spec".  */
 
 #define INCLUDE_STRING
 #include "config.h"
+#define INCLUDE_VECTOR
 #include "system.h"
 #include "coretypes.h"
 #include "multilib.h" /* before tm.h */
@@ -9534,7 +9535,7 @@ struct mdswitchstr
   int len;
 };
 
-static struct mdswitchstr *mdswitches;
+static std::vector<struct mdswitchstr> mdswitches;
 static int n_mdswitches;
 
 /* Check whether a particular argument was used.  The first time we
@@ -9753,7 +9754,7 @@ set_multilib_dir (void)
     {
       int i = 0;
 
-      mdswitches = XNEWVEC (struct mdswitchstr, n_mdswitches);
+      mdswitches.resize (n_mdswitches);
       for (start = multilib_defaults; *start != '\0'; start = end + 1)
 	{
 	  while (*start == ' ' || *start == '\t')
@@ -11366,7 +11367,7 @@ driver::finalize ()
   input_from_pipe = 0;
   suffix_subst = NULL;
 
-  mdswitches = NULL;
+  mdswitches.clear ();
   n_mdswitches = 0;
 
   used_arg.finalize ();
-- 
2.30.2

Re: [PATCH] driver: Fix memory leak.

[Jakub Jelinek]

On Wed, Dec 06, 2023 at 02:29:25PM +0000, Costas Argyris wrote:
> Attached a new patch with these changes.
> 
> On Mon, 4 Dec 2023 at 12:15, Jonathan Wakely <jwakely@redhat.com> wrote:
> 
> > On Sat, 2 Dec 2023 at 21:24, Costas Argyris wrote:
> > >
> > > Use std::vector instead of malloc'd pointer
> > > to get automatic freeing of memory.
> >
> > You can't include <vector> there. Instead you need to define
> > INCLUDE_VECTOR before "system.h"
> >
> > Shouldn't you be using resize, not reserve? Otherwise mdswitches[i] is
> > undefined.

Any reason not to use vec.h instead?
I especially don't like the fact that with a global
std::vector<whatever> var;
it means runtime __cxa_atexit for the var the destruction, which it really
doesn't need on exit.

We really don't need to free the memory at exit time, that is just wasted
cycles, all we need is that it is freed before the pointer or vector is
cleared.

	Jakub

Re: [PATCH] driver: Fix memory leak.

[Costas Argyris]

[-- Attachment #1.1: Type: text/plain, Size: 1379 bytes --]

Would that be something like this?

Although it didn't fix the leak, which was the entire point of this
exercise.

Maybe because driver::finalize () is not getting called so the call to
mdswitches.release () doesn't really happen, which was the reason
I went with std::vector in the first place because it takes care of itself.


On Wed, 6 Dec 2023 at 14:39, Jakub Jelinek <jakub@redhat.com> wrote:

> On Wed, Dec 06, 2023 at 02:29:25PM +0000, Costas Argyris wrote:
> > Attached a new patch with these changes.
> >
> > On Mon, 4 Dec 2023 at 12:15, Jonathan Wakely <jwakely@redhat.com> wrote:
> >
> > > On Sat, 2 Dec 2023 at 21:24, Costas Argyris wrote:
> > > >
> > > > Use std::vector instead of malloc'd pointer
> > > > to get automatic freeing of memory.
> > >
> > > You can't include <vector> there. Instead you need to define
> > > INCLUDE_VECTOR before "system.h"
> > >
> > > Shouldn't you be using resize, not reserve? Otherwise mdswitches[i] is
> > > undefined.
>
> Any reason not to use vec.h instead?
> I especially don't like the fact that with a global
> std::vector<whatever> var;
> it means runtime __cxa_atexit for the var the destruction, which it really
> doesn't need on exit.
>
> We really don't need to free the memory at exit time, that is just wasted
> cycles, all we need is that it is freed before the pointer or vector is
> cleared.
>
>         Jakub
>
>

[-- Attachment #2: 0001-driver-Fix-memory-leak.patch --]
[-- Type: application/octet-stream, Size: 1796 bytes --]

From a76942fcacd12ad95667cc782c8d0aff23ceafab Mon Sep 17 00:00:00 2001
From: Costas Argyris <costas.argyris@gmail.com>
Date: Sat, 2 Dec 2023 20:52:07 +0000
Subject: [PATCH] driver: Fix memory leak.

Use vec instead of malloc'd pointer
to get automatic freeing of memory.

Result was verified by valgrind, which showed
one less loss record.

Signed-off-by: Costas Argyris <costas.argyris@gmail.com>
---
 gcc/gcc.cc | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/gcc/gcc.cc b/gcc/gcc.cc
index 9f21ad9453e..6534e09860c 100644
--- a/gcc/gcc.cc
+++ b/gcc/gcc.cc
@@ -9534,7 +9534,7 @@ struct mdswitchstr
   int len;
 };
 
-static struct mdswitchstr *mdswitches;
+static vec<struct mdswitchstr> mdswitches;
 static int n_mdswitches;
 
 /* Check whether a particular argument was used.  The first time we
@@ -9751,9 +9751,7 @@ set_multilib_dir (void)
 
   if (n_mdswitches)
     {
-      int i = 0;
-
-      mdswitches = XNEWVEC (struct mdswitchstr, n_mdswitches);
+      mdswitches.create (n_mdswitches);
       for (start = multilib_defaults; *start != '\0'; start = end + 1)
 	{
 	  while (*start == ' ' || *start == '\t')
@@ -9768,8 +9766,10 @@ set_multilib_dir (void)
 
 	  obstack_grow (&multilib_obstack, start, end - start);
 	  obstack_1grow (&multilib_obstack, 0);
-	  mdswitches[i].str = XOBFINISH (&multilib_obstack, const char *);
-	  mdswitches[i++].len = end - start;
+	  struct mdswitchstr tmp;
+	  tmp.str = XOBFINISH (&multilib_obstack, const char *);
+	  tmp.len = end - start;
+	  mdswitches.safe_push (tmp);
 
 	  if (*end == '\0')
 	    break;
@@ -11366,7 +11366,7 @@ driver::finalize ()
   input_from_pipe = 0;
   suffix_subst = NULL;
 
-  mdswitches = NULL;
+  mdswitches.release ();
   n_mdswitches = 0;
 
   used_arg.finalize ();
-- 
2.30.2

Re: [PATCH] driver: Fix memory leak.

[Jakub Jelinek]

On Thu, Dec 07, 2023 at 02:28:18PM +0000, Costas Argyris wrote:
> Would that be something like this?

Yes.  Or perhaps even easier just change

--- gcc/gcc.cc.jj	2023-12-07 08:31:59.970849379 +0100
+++ gcc/gcc.cc	2023-12-07 15:33:46.616886894 +0100
@@ -11368,6 +11368,7 @@ driver::finalize ()
   input_from_pipe = 0;
   suffix_subst = NULL;
 
+  XDELETE (mdswitches);
   mdswitches = NULL;
   n_mdswitches = 0;
 
> Although it didn't fix the leak, which was the entire point of this
> exercise.
> 
> Maybe because driver::finalize () is not getting called so the call to
> mdswitches.release () doesn't really happen, which was the reason
> I went with std::vector in the first place because it takes care of itself.

In that case you are fixing a non-issue.
exit frees all allocated memory, no need to do it explicitly and waste
compile time cycles on it.

Leak is when some memory is allocated and pointer to it lost, that is not
the case here.

Still reachable memory at exit e.g. from valgrind is not a bug.

E.g. glibc in order to make fewer still reachable memory reports exports
a __libc_freeres function which valgrind and other memory allocation
debuggers can call on exit to free extra memory, something that isn't really
needed to waste time on normally.  But I'm not sure if there is some way
for an application to provide such functions as well.

	Jakub

Re: [PATCH] driver: Fix memory leak.

[Costas Argyris]

[-- Attachment #1: Type: text/plain, Size: 2221 bytes --]

>  Still reachable memory at exit e.g. from valgrind is not a bug.

Indeed, this is coming from a valgrind report here:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93019

where it was noted that the driver memory leaks could be
problematic for JIT.

So, since using std::vector did reduce the valgrind records
by one (I only targeted a single variable to begin with) I took
that as a good sign.

Regarding adding a call to XDELETE (mdswitches), yes,
that would help in the case where driver::finalize () is actually
called, which I think is for JIT.    I was trying to take care of the
case where it doesn't get called as well, but from what you say
I take it that this case is not of interest.

On Thu, 7 Dec 2023 at 14:42, Jakub Jelinek <jakub@redhat.com> wrote:

> On Thu, Dec 07, 2023 at 02:28:18PM +0000, Costas Argyris wrote:
> > Would that be something like this?
>
> Yes.  Or perhaps even easier just change
>
> --- gcc/gcc.cc.jj       2023-12-07 08:31:59.970849379 +0100
> +++ gcc/gcc.cc  2023-12-07 15:33:46.616886894 +0100
> @@ -11368,6 +11368,7 @@ driver::finalize ()
>    input_from_pipe = 0;
>    suffix_subst = NULL;
>
> +  XDELETE (mdswitches);
>    mdswitches = NULL;
>    n_mdswitches = 0;
>
> > Although it didn't fix the leak, which was the entire point of this
> > exercise.
> >
> > Maybe because driver::finalize () is not getting called so the call to
> > mdswitches.release () doesn't really happen, which was the reason
> > I went with std::vector in the first place because it takes care of
> itself.
>
> In that case you are fixing a non-issue.
> exit frees all allocated memory, no need to do it explicitly and waste
> compile time cycles on it.
>
> Leak is when some memory is allocated and pointer to it lost, that is not
> the case here.
>
> Still reachable memory at exit e.g. from valgrind is not a bug.
>
> E.g. glibc in order to make fewer still reachable memory reports exports
> a __libc_freeres function which valgrind and other memory allocation
> debuggers can call on exit to free extra memory, something that isn't
> really
> needed to waste time on normally.  But I'm not sure if there is some way
> for an application to provide such functions as well.
>
>         Jakub
>
>

Re: [PATCH] driver: Fix memory leak.

[Jakub Jelinek]

On Thu, Dec 07, 2023 at 03:16:29PM +0000, Costas Argyris wrote:
> >  Still reachable memory at exit e.g. from valgrind is not a bug.
> 
> Indeed, this is coming from a valgrind report here:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93019
> 
> where it was noted that the driver memory leaks could be
> problematic for JIT.

In the invoke_embedded_driver JIT case yes, that calls driver::finalize (),
which is why it should be freed before clearing the pointer in there (as
then it is a real leak).

> So, since using std::vector did reduce the valgrind records
> by one (I only targeted a single variable to begin with) I took
> that as a good sign.
> 
> Regarding adding a call to XDELETE (mdswitches), yes,
> that would help in the case where driver::finalize () is actually
> called, which I think is for JIT.    I was trying to take care of the
> case where it doesn't get called as well, but from what you say
> I take it that this case is not of interest.

That is wasted compile time on a non-issue.

If you see a JIT issue with definitely lost records, that is something
that obviously should be fixed (but even in that area I think we've been a
little bit lazy in the option handling).
The most important is that the actual compiler binaries (cc1, cc1plus, ...)
don't leak memory (in the definitely lost kind) like crazy, we have
--enable-checking=valgrind
for that purpose, where the driver runs cc1/cc1plus etc. under valgrind,
but this is very expensive and slow, so usually it is run once during a
cycle (if at all), on a fast machine could take even in non-bootstrap mode
a weekend to go through the whole testsuite, then one can look at the leaks.

	Jakub

Re: [PATCH] driver: Fix memory leak.

[Costas Argyris]

[-- Attachment #1.1: Type: text/plain, Size: 2019 bytes --]

Thanks for all the explanations.

In that case I restrict this patch to just freeing the buffer from
within driver::finalize only (I think it should be XDELETEVEC
instead of XDELETE, no?).

On Thu, 7 Dec 2023 at 15:42, Jakub Jelinek <jakub@redhat.com> wrote:

> On Thu, Dec 07, 2023 at 03:16:29PM +0000, Costas Argyris wrote:
> > >  Still reachable memory at exit e.g. from valgrind is not a bug.
> >
> > Indeed, this is coming from a valgrind report here:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93019
> >
> > where it was noted that the driver memory leaks could be
> > problematic for JIT.
>
> In the invoke_embedded_driver JIT case yes, that calls driver::finalize (),
> which is why it should be freed before clearing the pointer in there (as
> then it is a real leak).
>
> > So, since using std::vector did reduce the valgrind records
> > by one (I only targeted a single variable to begin with) I took
> > that as a good sign.
> >
> > Regarding adding a call to XDELETE (mdswitches), yes,
> > that would help in the case where driver::finalize () is actually
> > called, which I think is for JIT.    I was trying to take care of the
> > case where it doesn't get called as well, but from what you say
> > I take it that this case is not of interest.
>
> That is wasted compile time on a non-issue.
>
> If you see a JIT issue with definitely lost records, that is something
> that obviously should be fixed (but even in that area I think we've been a
> little bit lazy in the option handling).
> The most important is that the actual compiler binaries (cc1, cc1plus, ...)
> don't leak memory (in the definitely lost kind) like crazy, we have
> --enable-checking=valgrind
> for that purpose, where the driver runs cc1/cc1plus etc. under valgrind,
> but this is very expensive and slow, so usually it is run once during a
> cycle (if at all), on a fast machine could take even in non-bootstrap mode
> a weekend to go through the whole testsuite, then one can look at the
> leaks.
>
>         Jakub
>
>

[-- Attachment #2: 0001-driver-Fix-memory-leak-in-driver-finalize.patch --]
[-- Type: application/octet-stream, Size: 670 bytes --]

From 13f6ce5c164f1f0fded25f01b1d38f90b354fe13 Mon Sep 17 00:00:00 2001
From: Costas Argyris <costas.argyris@gmail.com>
Date: Thu, 7 Dec 2023 15:52:51 +0000
Subject: [PATCH] driver: Fix memory leak in driver::finalize ()

Free buffer before setting the pointer to NULL.

Signed-off-by: Costas Argyris <costas.argyris@gmail.com>
---
 gcc/gcc.cc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gcc/gcc.cc b/gcc/gcc.cc
index 9f21ad9453e..bf4a02f76e7 100644
--- a/gcc/gcc.cc
+++ b/gcc/gcc.cc
@@ -11366,6 +11366,7 @@ driver::finalize ()
   input_from_pipe = 0;
   suffix_subst = NULL;
 
+  XDELETEVEC (mdswitches);
   mdswitches = NULL;
   n_mdswitches = 0;
 
-- 
2.30.2

Re: [PATCH] driver: Fix memory leak.

[Jakub Jelinek]

On Thu, Dec 07, 2023 at 04:01:11PM +0000, Costas Argyris wrote:
> Thanks for all the explanations.
> 
> In that case I restrict this patch to just freeing the buffer from
> within driver::finalize only (I think it should be XDELETEVEC
> instead of XDELETE, no?).

Both macros are exactly the same, but XDELETEVEC is probably better
counterpart to XNEWVEC.

	Jakub

Re: [PATCH] driver: Fix memory leak.

[Costas Argyris]

[-- Attachment #1: Type: text/plain, Size: 599 bytes --]

Does the simple XDELETEVEC patch need any more work?

I think it just fixes a leak for the JIT case where driver::finalize is
called.

On Thu, 7 Dec 2023 at 16:04, Jakub Jelinek <jakub@redhat.com> wrote:

> On Thu, Dec 07, 2023 at 04:01:11PM +0000, Costas Argyris wrote:
> > Thanks for all the explanations.
> >
> > In that case I restrict this patch to just freeing the buffer from
> > within driver::finalize only (I think it should be XDELETEVEC
> > instead of XDELETE, no?).
>
> Both macros are exactly the same, but XDELETEVEC is probably better
> counterpart to XNEWVEC.
>
>         Jakub
>
>

Re: [PATCH] driver: Fix memory leak.

[Jakub Jelinek]

On Fri, Dec 08, 2023 at 12:18:50PM +0000, Costas Argyris wrote:
> Does the simple XDELETEVEC patch need any more work?

Well, it needs to be actually tested and posted and committed.
I can take care of it in my next bootstraps.

	Jakub