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

Message ID 20180421223936.31584-1-tom@tromey.com
State New
Headers show
Series
  • [RFA] Remove a cleanup from scm-frame.c
Related show

Commit Message

Tom Tromey April 21, 2018, 10:39 p.m.
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(-)

-- 
2.13.6

Comments

Simon Marchi April 22, 2018, 2:21 p.m. | #1
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
Tom Tromey April 22, 2018, 11:47 p.m. | #2
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:


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

I don't believe so, which is why this patch introduces a new scope.

Tom

Patch

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
     {