Re: [RFC] gdbscm_wrap, eliminate GDBSCM_HANDLE_GDB_EXCEPTION_WITH_CLEANUPS (Re: [RFA 2/4] Change gdbscm_exception_message_to_string to return a unique_xmalloc_ptr)

[Pedro Alves <palves@redhat.com>]

On 07/18/2018 08:33 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> I gave this a try today.  How about this?
> 
> Pedro> +/* Use this to wrap a callable to throw the appropriate Scheme
> Pedro> +   exception if the callable throws a GDB error.  ARGS are forwarded
> Pedro> +   to FUNC.  Returns the result of FUNC, unless FUNC returns a Scheme
> Pedro> +   exception, in which case that exception is thrown.  Note that while
> Pedro> +   the callable is free is use objects of types with destructors,
> 
> Typo - "free to use objects".

Thanks, fixed.

> 
> Pedro> +++ b/gdb/guile/scm-frame.c
> Pedro> @@ -783,13 +783,11 @@ gdbscm_frame_read_register (SCM self, SCM register_scm)
> Pedro>    char *register_str;
> Pedro>    struct value *value = NULL;
> Pedro>    struct frame_info *frame = NULL;
> Pedro> -  struct cleanup *cleanup;
> Pedro>    frame_smob *f_smob;
>  
> Pedro>    f_smob = frscm_get_frame_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME);
> Pedro>    gdbscm_parse_function_args (FUNC_NAME, SCM_ARG2, NULL, "s",
> Pedro>  			      register_scm, &register_str);
> Pedro> -  cleanup = make_cleanup (xfree, register_str);
>  
> Pedro>    TRY
> Pedro>      {
> Pedro> @@ -811,7 +809,7 @@ gdbscm_frame_read_register (SCM self, SCM register_scm)
> Pedro>      }
> Pedro>    END_CATCH
>  
> Pedro> -  do_cleanups (cleanup);
> Pedro> +  xfree (register_str);
> 
> I think this will leak register_str if anything throws.  Maybe the xfree
> should be duplicated in the catch block, or maybe the whole try/catch
> should be replaced with a call to gdbscm_wrap (where the callback would
> take ownership of register_str).
> 
> This seems like a latent bug here as well - I think the code should be using
> GDBSCM_HANDLE_GDB_EXCEPTION_WITH_CLEANUPS here.
> 

Indeed.  I fixed this using the same pattern used elsewhere, of saving
the exception object in the CATCH block:

--- c/gdb/guile/scm-frame.c
+++ w/gdb/guile/scm-frame.c
@@ -783,13 +783,13 @@ gdbscm_frame_read_register (SCM self, SCM register_scm)
   char *register_str;
   struct value *value = NULL;
   struct frame_info *frame = NULL;
-  struct cleanup *cleanup;
   frame_smob *f_smob;
 
   f_smob = frscm_get_frame_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME);
   gdbscm_parse_function_args (FUNC_NAME, SCM_ARG2, NULL, "s",
                              register_scm, &register_str);
-  cleanup = make_cleanup (xfree, register_str);
+
+  struct gdb_exception except = exception_none;
 
   TRY
     {
@@ -805,13 +805,14 @@ gdbscm_frame_read_register (SCM self, SCM register_scm)
            value = value_of_register (regnum, frame);
        }
     }
-  CATCH (except, RETURN_MASK_ALL)
+  CATCH (ex, RETURN_MASK_ALL)
     {
-      GDBSCM_HANDLE_GDB_EXCEPTION (except);
+      except = ex;
     }
   END_CATCH
 
-  do_cleanups (cleanup);
+  xfree (register_str);
+  GDBSCM_HANDLE_GDB_EXCEPTION (except);
 
   if (frame == NULL)
     {

> Otherwise this all looks good to me.  Thanks for doing this, guile was
> one of the larger cleanup holdouts

Great.  I wrote a ChangeLog entry and pushed it in:
 https://sourceware.org/ml/gdb-patches/2018-07/msg00567.html

The only other change is that I copied the first paragraph of
the commit log to guile/guile-internal.h.

Thanks,
Pedro Alves