[RFA] Fix leaks in ada catch command

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

Philippe> Fix the first leak by xfree-ing the addr_string allocated by ada_exception_sal.
Philippe> Fix the second leak by calling the base bp_location dtor.

Philippe> Tested on debian/amd64, natively and under valgrind.

Thanks.

Philippe> -  const char *addr_string = NULL;
Philippe> +  char *addr_string = NULL;
Philippe>    const struct breakpoint_ops *ops = NULL;
Philippe> -  struct symtab_and_line sal = ada_exception_sal (ex_kind, &addr_string, &ops);
Philippe> +  struct symtab_and_line sal = ada_exception_sal (ex_kind,
Philippe> +						  (const char **) &addr_string,
Philippe> +						  &ops);

This cast seems questionable to me -- and since ada_exception_sal is
only called from this one spot, it seems like it would be just as
simple, but also more clear and more obviously correct to simply change
the type of "addr_string" to "std::string *".  This would also avoid any
potential leak if find_function_start_sal can throw (I didn't check).

Philippe>  static void
Philippe> -bp_location_dtor (struct bp_location *self)
Philippe> +base_bp_location_dtor (struct bp_location *self)
Philippe>  {
Philippe>    xfree (self->function_name);
Philippe>  }

For this part, I think it's easy in this case to just further C++-ify
bp_location.  I've done this in the appended -- let me know what you
think.

Tom

commit 65a60067205f97696a91dd7f134013ffa112dc21
Author: Tom Tromey <tromey@adacore.com>
Date:   Tue Feb 12 14:28:07 2019 -0700

    C++-ify bp_location
    
    Philippe noticed a memory leak coming from ada_catchpoint_location --
    it was not freeing the "function_name" member from its base class.
    
    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-12  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.
  

On Tue, 2019-02-12 at 14:35 -0700, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> 
> Philippe> Fix the first leak by xfree-ing the addr_string allocated by ada_exception_sal.
> Philippe> Fix the second leak by calling the base bp_location dtor.
> 
> Philippe> Tested on debian/amd64, natively and under valgrind.
> 
> Thanks.
> 
> Philippe> -  const char *addr_string = NULL;
> Philippe> +  char *addr_string = NULL;
> Philippe>    const struct breakpoint_ops *ops = NULL;
> Philippe> -  struct symtab_and_line sal = ada_exception_sal (ex_kind, &addr_string, &ops);
> Philippe> +  struct symtab_and_line sal = ada_exception_sal (ex_kind,
> Philippe> +						  (const char **) &addr_string,
> Philippe> +						  &ops);
> 
> This cast seems questionable to me -- and since ada_exception_sal is
> only called from this one spot, it seems like it would be just as
> simple, but also more clear and more obviously correct to simply change
> the type of "addr_string" to "std::string *".  This would also avoid any
> potential leak if find_function_start_sal can throw (I didn't check).
> 
> Philippe>  static void
> Philippe> -bp_location_dtor (struct bp_location *self)
> Philippe> +base_bp_location_dtor (struct bp_location *self)
> Philippe>  {
> Philippe>    xfree (self->function_name);
> Philippe>  }
> 
> For this part, I think it's easy in this case to just further C++-ify
> bp_location.  I've done this in the appended -- let me know what you
> think.
> 
> Tom

Yes, your suggestions to use std::string * and the below patch seem
better to me.  So, I assume you will push fixes for both

Thanks
Philippe

 



> 
> commit 65a60067205f97696a91dd7f134013ffa112dc21
> Author: Tom Tromey <tromey@adacore.com>
> Date:   Tue Feb 12 14:28:07 2019 -0700
> 
>     C++-ify bp_location
>     
>     Philippe noticed a memory leak coming from ada_catchpoint_location --
>     it was not freeing the "function_name" member from its base class.
>     
>     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-12  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.
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 194ae465769..a773943b8e8 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,20 @@
> +2019-02-12  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.
> +
>  2019-02-12  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
>  
>  	* symtab.h (struct minimal_symbol data_p): New const method.
> 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;
>
  

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

Philippe> Yes, your suggestions to use std::string * and the below patch seem
Philippe> better to me.  So, I assume you will push fixes for both

I've sent the patches now, please take a look.

Tom