Re: [RFA] Remove a cleanup from scm-frame.c

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2018-04-21 18:39, Tom Tromey wrote:
> This removes a cleanup from scm-frame.c, replacing it with
> unique_xmalloc_ptr and a new scope.  I believe this also fixes a
> latent bug involving calling do_cleanups twice for a single cleanup.
> 
> Regression tested using the gdb.guile test suite on x86-64 Fedora 26.
> 
> ChangeLog
> 2018-04-21  Tom Tromey  <tom@tromey.com>
> 
> 	* guile/scm-frame.c (gdbscm_frame_read_var): Use
> 	gdb::unique_xmalloc_ptr.
> ---
>  gdb/ChangeLog         |  5 +++++
>  gdb/guile/scm-frame.c | 54 
> ++++++++++++++++++++++++---------------------------
>  2 files changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/gdb/guile/scm-frame.c b/gdb/guile/scm-frame.c
> index 4f4766aceb..7b539677ff 100644
> --- a/gdb/guile/scm-frame.c
> +++ b/gdb/guile/scm-frame.c
> @@ -877,7 +877,6 @@ gdbscm_frame_read_var (SCM self, SCM symbol_scm, 
> SCM rest)
>      }
>    else if (scm_is_string (symbol_scm))
>      {
> -      char *var_name;
>        const struct block *block = NULL;
>        struct cleanup *cleanup;
>        struct gdb_exception except = exception_none;
> @@ -893,38 +892,35 @@ gdbscm_frame_read_var (SCM self, SCM symbol_scm, 
> SCM rest)
>  	    gdbscm_throw (except_scm);
>  	}
> 
> -      var_name = gdbscm_scm_to_c_string (symbol_scm);
> -      cleanup = make_cleanup (xfree, var_name);
> -      /* N.B. Between here and the call to do_cleanups, don't do 
> anything
> -	 to cause a Scheme exception without performing the cleanup.  */
> +      {
> +	gdb::unique_xmalloc_ptr<char> var_name
> +	  (gdbscm_scm_to_c_string (symbol_scm));
> +	/* N.B. Between here and the end of the scope, don't do anything
> +	   to cause a Scheme exception.  */
> +
> +	TRY
> +	  {
> +	    struct block_symbol lookup_sym;
> +
> +	    if (block == NULL)
> +	      block = get_frame_block (frame, NULL);
> +	    lookup_sym = lookup_symbol (var_name.get (), block, VAR_DOMAIN,
> +					NULL);
> +	    var = lookup_sym.symbol;
> +	    block = lookup_sym.block;
> +	  }
> +	CATCH (ex, RETURN_MASK_ALL)
> +	  {
> +	    except = ex;
> +	  }
> +	END_CATCH
> +      }
> 
> -      TRY
> -	{
> -	  struct block_symbol lookup_sym;
> -
> -	  if (block == NULL)
> -	    block = get_frame_block (frame, NULL);
> -	  lookup_sym = lookup_symbol (var_name, block, VAR_DOMAIN, NULL);
> -	  var = lookup_sym.symbol;
> -	  block = lookup_sym.block;
> -	}
> -      CATCH (ex, RETURN_MASK_ALL)
> -	{
> -	  except = ex;
> -	}
> -      END_CATCH
> -
> -      do_cleanups (cleanup);
>        GDBSCM_HANDLE_GDB_EXCEPTION (except);
> 
>        if (var == NULL)
> -	{
> -	  do_cleanups (cleanup);
> -	  gdbscm_out_of_range_error (FUNC_NAME, 0, symbol_scm,
> -				     _("variable not found"));
> -	}
> -
> -      do_cleanups (cleanup);
> +	gdbscm_out_of_range_error (FUNC_NAME, 0, symbol_scm,
> +				   _("variable not found"));
>      }
>    else
>      {

This looks good to me at first glance.  Do you know if scm exceptions 
(scm_throw) play well with C++, the destructors of the objects in the 
exited scopes will correctly be called?

Simon