[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)
Commit Message
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, ®ister_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:
> 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
@@ -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, ®ister_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)
{