[PATCH 2/2] Fix memory leak in create_ada_exception_catchpoint

[PATCH 2/2] Fix memory leak in create_ada_exception_catchpoint

[Tom Tromey]

Phillipe noticed that create_ada_exception_catchpoint was not freeing
the "addr_string" memory:

==14141== 114 bytes in 4 blocks are definitely lost in loss record 1,054 of 3,424
==14141==    at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
==14141==    by 0x405107: xmalloc (common-utils.c:44)
==14141==    by 0x7563F9: xstrdup (xstrdup.c:34)
==14141==    by 0x381B21: ada_exception_sal (ada-lang.c:13217)
==14141==    by 0x381B21: create_ada_exception_catchpoint(gdbarch*, ada_exception_catchpoint_kind, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int, int, int) (ada-lang.c:13251)
==14141==    by 0x3820A8: catch_ada_exception_command(char const*, int, cmd_list_element*) (ada-lang.c:13285)
==14141==    by 0x3F4828: cmd_func(cmd_list_element*, char const*, int) (cli-decode.c:1892)

This patch fixes the problem by changing ada_exception_sal to return a
std::string via its out parameter.

gdb/ChangeLog
2019-02-13  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
	    Tom Tromey  <tom@tromey.com>

	* ada-lang.c (ada_exception_sal): Change addr_string to a
	std::string.
	(create_ada_exception_catchpoint): Update.
---
 gdb/ChangeLog  | 7 +++++++
 gdb/ada-lang.c | 8 ++++----
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 602facb35e7..c775b09b89e 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -13176,7 +13176,7 @@ ada_exception_catchpoint_cond_string (const char *excep_string,
 
 static struct symtab_and_line
 ada_exception_sal (enum ada_exception_catchpoint_kind ex,
-		   const char **addr_string, const struct breakpoint_ops **ops)
+		   std::string *addr_string, const struct breakpoint_ops **ops)
 {
   const char *sym_name;
   struct symbol *sym;
@@ -13196,7 +13196,7 @@ ada_exception_sal (enum ada_exception_catchpoint_kind ex,
     error (_("Unable to insert catchpoint. %s is not a function."), sym_name);
 
   /* Set ADDR_STRING.  */
-  *addr_string = xstrdup (sym_name);
+  *addr_string = sym_name;
 
   /* Set OPS.  */
   *ops = ada_exception_breakpoint_ops (ex);
@@ -13228,12 +13228,12 @@ create_ada_exception_catchpoint (struct gdbarch *gdbarch,
 				 int disabled,
 				 int from_tty)
 {
-  const char *addr_string = NULL;
+  std::string addr_string;
   const struct breakpoint_ops *ops = NULL;
   struct symtab_and_line sal = ada_exception_sal (ex_kind, &addr_string, &ops);
 
   std::unique_ptr<ada_catchpoint> c (new ada_catchpoint ());
-  init_ada_exception_breakpoint (c.get (), gdbarch, sal, addr_string,
+  init_ada_exception_breakpoint (c.get (), gdbarch, sal, addr_string.c_str (),
 				 ops, tempflag, disabled, from_tty);
   c->excep_string = excep_string;
   create_excep_cond_exprs (c.get (), ex_kind);
-- 
2.20.1

[PATCH 0/2] Fix memory leaks in create_ada_exception_catchpoint

[Tom Tromey]

These patches fix a couple of memory leaks in
create_ada_exception_catchpoint.  This is based on Philippe's patch
and analysis.

Tested on x86-64 Fedora 29.  I also tried "catch exception" under
valgrind to verify the fix.

Tom

[PATCH 1/2] C++-ify bp_location

[Tom Tromey]

Philippe noticed a memory leak coming from ada_catchpoint_location --
it was not freeing the "function_name" member from its base class:

==14141== 114 bytes in 4 blocks are definitely lost in loss record 1,055 of 3,424
==14141==    at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
==14141==    by 0x405107: xmalloc (common-utils.c:44)
==14141==    by 0x7563F9: xstrdup (xstrdup.c:34)
==14141==    by 0x3B82B3: set_breakpoint_location_function(bp_location*, int) (breakpoint.c:7156)
==14141==    by 0x3C112B: add_location_to_breakpoint(breakpoint*, symtab_and_line const*) (breakpoint.c:8609)
==14141==    by 0x3C127A: init_raw_breakpoint(breakpoint*, gdbarch*, symtab_and_line, bptype, breakpoint_ops const*) (breakpoint.c:7187)
==14141==    by 0x3C1B52: init_ada_exception_breakpoint(breakpoint*, gdbarch*, symtab_and_line, char const*, breakpoint_ops const*, int, int, int) (breakpoint.c:11262)
==14141==    by 0x381C2E: create_ada_exception_catchpoint(gdbarch*, ada_exception_catchpoint_kind, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int, int, int) (ada-lang.c:13255)

This patch fixes the problem by further C++-ifying bp_location.  In
particular, bp_location_ops is now removed, and the "dtor" function
pointer is replaced with an ordinary destructor.

gdb/ChangeLog
2019-02-13  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
	    Tom Tromey  <tom@tromey.com>

	* breakpoint.c (~bp_location): Rename from bp_location_dtor.
	(bp_location_ops): Remove.
	(base_breakpoint_allocate_location): Update.
	(free_bp_location): Update.
	* ada-lang.c (class ada_catchpoint_location)
	<ada_catchpoint_location>: Remove ops parameter.
	(ada_catchpoint_location_dtor): Remove.
	(ada_catchpoint_location_ops): Remove.
	(allocate_location_exception): Update.
	* breakpoint.h (struct bp_location_ops): Remove.
	(class bp_location) <bp_location>: Remove bp_location_ops
	parameter.
	<~bp_location>: Add destructor.
	<ops>: Remove.
---
 gdb/ChangeLog    | 18 ++++++++++++++++++
 gdb/ada-lang.c   | 24 +++---------------------
 gdb/breakpoint.c | 20 ++++----------------
 gdb/breakpoint.h | 18 +++---------------
 4 files changed, 28 insertions(+), 52 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index a878d4d1af2..602facb35e7 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -12396,8 +12396,8 @@ static std::string ada_exception_catchpoint_cond_string
 class ada_catchpoint_location : public bp_location
 {
 public:
-  ada_catchpoint_location (const bp_location_ops *ops, breakpoint *owner)
-    : bp_location (ops, owner)
+  ada_catchpoint_location (breakpoint *owner)
+    : bp_location (owner)
   {}
 
   /* The condition that checks whether the exception that was raised
@@ -12406,24 +12406,6 @@ public:
   expression_up excep_cond_expr;
 };
 
-/* Implement the DTOR method in the bp_location_ops structure for all
-   Ada exception catchpoint kinds.  */
-
-static void
-ada_catchpoint_location_dtor (struct bp_location *bl)
-{
-  struct ada_catchpoint_location *al = (struct ada_catchpoint_location *) bl;
-
-  al->excep_cond_expr.reset ();
-}
-
-/* The vtable to be used in Ada catchpoint locations.  */
-
-static const struct bp_location_ops ada_catchpoint_location_ops =
-{
-  ada_catchpoint_location_dtor
-};
-
 /* An instance of this type is used to represent an Ada catchpoint.  */
 
 struct ada_catchpoint : public breakpoint
@@ -12493,7 +12475,7 @@ static struct bp_location *
 allocate_location_exception (enum ada_exception_catchpoint_kind ex,
 			     struct breakpoint *self)
 {
-  return new ada_catchpoint_location (&ada_catchpoint_location_ops, self);
+  return new ada_catchpoint_location (self);
 }
 
 /* Implement the RE_SET method in the breakpoint_ops structure for all
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 67d83e6f465..9be99ff4efa 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6958,13 +6958,10 @@ adjust_breakpoint_address (struct gdbarch *gdbarch,
     }
 }
 
-bp_location::bp_location (const bp_location_ops *ops, breakpoint *owner)
+bp_location::bp_location (breakpoint *owner)
 {
   bp_location *loc = this;
 
-  gdb_assert (ops != NULL);
-
-  loc->ops = ops;
   loc->owner = owner;
   loc->cond_bytecode = NULL;
   loc->shlib_disabled = 0;
@@ -7033,7 +7030,6 @@ allocate_bp_location (struct breakpoint *bpt)
 static void
 free_bp_location (struct bp_location *loc)
 {
-  loc->ops->dtor (loc);
   delete loc;
 }
 
@@ -12166,19 +12162,11 @@ say_where (struct breakpoint *b)
     }
 }
 
-/* Default bp_location_ops methods.  */
-
-static void
-bp_location_dtor (struct bp_location *self)
+bp_location::~bp_location ()
 {
-  xfree (self->function_name);
+  xfree (function_name);
 }
 
-static const struct bp_location_ops bp_location_ops =
-{
-  bp_location_dtor
-};
-
 /* Destructor for the breakpoint base class.  */
 
 breakpoint::~breakpoint ()
@@ -12191,7 +12179,7 @@ breakpoint::~breakpoint ()
 static struct bp_location *
 base_breakpoint_allocate_location (struct breakpoint *self)
 {
-  return new bp_location (&bp_location_ops, self);
+  return new bp_location (self);
 }
 
 static void
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 8c8c66a8158..a91e3e334cf 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -301,31 +301,19 @@ enum bp_loc_type
   bp_loc_other			/* Miscellaneous...  */
 };
 
-/* This structure is a collection of function pointers that, if
-   available, will be called instead of performing the default action
-   for this bp_loc_type.  */
-
-struct bp_location_ops
-{
-  /* Destructor.  Releases everything from SELF (but not SELF
-     itself).  */
-  void (*dtor) (struct bp_location *self);
-};
-
 class bp_location
 {
 public:
   bp_location () = default;
 
-  bp_location (const bp_location_ops *ops, breakpoint *owner);
+  bp_location (breakpoint *owner);
+
+  virtual ~bp_location ();
 
   /* Chain pointer to the next breakpoint location for
      the same parent breakpoint.  */
   bp_location *next = NULL;
 
-  /* Methods associated with this location.  */
-  const bp_location_ops *ops = NULL;
-
   /* The reference count.  */
   int refc = 0;
 
-- 
2.20.1

Re: [PATCH 0/2] Fix memory leaks in create_ada_exception_catchpoint

[Philippe Waroquiers]

I have read the patches and they look ok to me.
IMO, citing me as having done an analysis is largely
good enough (as you have written the patches).

Thanks for doing this,

Philippe



On Wed, 2019-02-13 at 06:45 -0700, Tom Tromey wrote:
> These patches fix a couple of memory leaks in
> create_ada_exception_catchpoint.  This is based on Philippe's patch
> and analysis.
> 
> Tested on x86-64 Fedora 29.  I also tried "catch exception" under
> valgrind to verify the fix.
> 
> Tom
> 
> 

Re: [PATCH 0/2] Fix memory leaks in create_ada_exception_catchpoint

[Tom Tromey]

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> I have read the patches and they look ok to me.
Philippe> IMO, citing me as having done an analysis is largely
Philippe> good enough (as you have written the patches).

Philippe> Thanks for doing this,

Thanks.  I'm going to check these in shortly.

Tom