Re: [PATCH 4/6] infcall: stop_registers -> register_dummy_frame_dtor

[Pedro Alves <palves at redhat dot com>]

On 05/08/2015 09:21 PM, Jan Kratochvil wrote:
> Hi,
> 
> with dummy_frame destructors GDB no longer has to use global stop_registers.
> dummy_frame's registers can be now stored associated with their specific
> dummy_frame.
> 
> stop_registers was questioned by Pedro in:
> 	Re: [PATCH v4 11/11] RFC only: compile: Use also inferior munmap
> 	https://sourceware.org/ml/gdb-patches/2015-05/msg00107.html
> 
> I am not completely sure what is right for bpfinishpy_pre_stop_hook but the
> testsuite passes.  I can think more about it if it gets otherwise approved.

Yeah, if the Python API around that gives access the return value,
then something needs to be done there, otherwise, probably nothing else
is needed.

> +/* Free struct call_function_by_hand_dtor.  */
> +
> +static void
> +call_function_by_hand_dtor_data_free (struct call_function_by_hand_dtor *data)
> +{
> +  regcache_xfree (data->retbuf);
> +  xfree (data);
> +}
> +
> +/* Destructor for associated dummy_frame.  */
> +
> +static dummy_frame_dtor_ftype call_function_by_hand_dtor;

This forward declaration is not really necessary, right?
A wrong prototype mismismatch would be caught here:

 if (!find_dummy_frame_dtor (call_function_by_hand_dtor, data))

for example.

> 
> +static void
> +call_function_by_hand_dtor (void *data_voidp, int registers_valid)
> +{
> +  struct call_function_by_hand_dtor *data = data_voidp;
> +
> +  gdb_assert (data->retbuf == NULL);
> +
> +  if (data->drop_done)
> +    call_function_by_hand_dtor_data_free (data);
> +  else if (registers_valid)
> +    data->retbuf = regcache_dup (get_current_regcache ());
> +}


OK, I know why it took me a while to understand all this.  I think
it was due to the function/symbol naming chosen.  
call_function_by_hand_dtor is too easy to confuse with the
dummy frame destructor function itself, and with the
"call_function_by_hand" function itself, instead of the
dummy frame dtor.  It doesn't really hint to me what this is all
doing, which is saving the context of the dummy frame, before that
frame is destroyed.

How about we rename things, to something like in the patch below?
Once I did that locally, things were much clearer to me.
I can push through with the rename afterwards if it makes it
easier for you.

Otherwise this looks good to me.

Thanks for doing this.

>From 9f11bb8611cd86f07a5f54aacf1594ee4227178c Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 13 May 2015 12:38:11 +0100
Subject: [PATCH] Rename call_function_by_hand_dtor -> dummy_frame_context_saver

---
 gdb/infcall.c  | 83 +++++++++++++++++++++++++++++-----------------------------
 gdb/infcall.h  | 15 +++++------
 gdb/infcmd.c   | 28 ++++++++++----------
 gdb/inferior.h |  4 +--
 4 files changed, 64 insertions(+), 66 deletions(-)

diff --git a/gdb/infcall.c b/gdb/infcall.c
index 849f500..5dd908d 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -469,95 +469,94 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
   return call_function_by_hand_dummy (function, nargs, args, NULL, NULL);
 }
 
-/* Data for call_function_by_hand_dtor.  Structure can be freed only
-   after both call_function_by_hand_dtor and
-   call_function_by_hand_dtor_drop have been called for it.  */
+/* Data for dummy_frame_context_saver.  Structure can be freed only
+   after both dummy_frame_context_saver_dtor and
+   dummy_frame_context_saver_drop have been called for it.  */
 
-struct call_function_by_hand_dtor
+struct dummy_frame_context_saver
 {
   /* Inferior registers fetched before associated dummy_frame got freed
      and before any other destructors of associated dummy_frame got called.
      It is initialized to NULL.  */
   struct regcache *retbuf;
 
-  /* It is 1 if this call_function_by_hand_dtor_drop has been already
+  /* It is 1 if this dummy_frame_context_saver_drop has been already
      called.  */
   int drop_done;
 };
 
-/* Free struct call_function_by_hand_dtor.  */
+/* Free struct dummy_frame_context_saver.  */
 
 static void
-call_function_by_hand_dtor_data_free (struct call_function_by_hand_dtor *data)
+dummy_frame_context_saver_free (struct dummy_frame_context_saver *saver)
 {
-  regcache_xfree (data->retbuf);
-  xfree (data);
+  regcache_xfree (saver->retbuf);
+  xfree (saver);
 }
 
 /* Destructor for associated dummy_frame.  */
 
-static dummy_frame_dtor_ftype call_function_by_hand_dtor;
 static void
-call_function_by_hand_dtor (void *data_voidp, int registers_valid)
+dummy_frame_context_saver_dtor (void *data_voidp, int registers_valid)
 {
-  struct call_function_by_hand_dtor *data = data_voidp;
+  struct dummy_frame_context_saver *data = data_voidp;
 
   gdb_assert (data->retbuf == NULL);
 
   if (data->drop_done)
-    call_function_by_hand_dtor_data_free (data);
+    dummy_frame_context_saver_free (data);
   else if (registers_valid)
     data->retbuf = regcache_dup (get_current_regcache ());
 }
 
 /* Caller is no longer interested in this
-   struct call_function_by_hand_dtor.  After its associated dummy_frame
-   gets freed struct call_function_by_hand_dtor can be also freed.  */
+   struct dummy_frame_context_saver.  After its associated dummy_frame
+   gets freed struct dummy_frame_context_saver can be also freed.  */
 
 void
-call_function_by_hand_dtor_drop (struct call_function_by_hand_dtor *data)
+dummy_frame_context_saver_drop (struct dummy_frame_context_saver *saver)
 {
-  data->drop_done = 1;
+  saver->drop_done = 1;
 
-  if (!find_dummy_frame_dtor (call_function_by_hand_dtor, data))
-    call_function_by_hand_dtor_data_free (data);
+  if (!find_dummy_frame_dtor (dummy_frame_context_saver_dtor, saver))
+    dummy_frame_context_saver_free (saver);
 }
 
-/* Stub for call_function_by_hand_dtor_drop compatible with make_cleanup.  */
+/* Stub dummy_frame_context_saver_drop compatible with make_cleanup.  */
 
 void
-call_function_by_hand_dtor_cleanup (void *data_voidp)
+dummy_frame_context_saver_cleanup (void *data)
 {
-  struct call_function_by_hand_dtor *data = data_voidp;
+  struct dummy_frame_context_saver *saver = data;
 
-  call_function_by_hand_dtor_drop (data);
+  dummy_frame_context_saver_drop (saver);
 }
 
 /* Fetch RETBUF field of possibly opaque DTOR_DATA.
    RETBUF must not be NULL.  */
 
 struct regcache *
-call_function_by_hand_dtor_get (struct call_function_by_hand_dtor *dtor_data)
+dummy_frame_context_saver_get_regs (struct dummy_frame_context_saver *saver)
 {
-  gdb_assert (dtor_data->retbuf != NULL);
-  return dtor_data->retbuf;
+  gdb_assert (saver->retbuf != NULL);
+  return saver->retbuf;
 }
 
 /* Register provider of inferior registers at the time DUMMY_ID frame of
    PTID gets freed (before inferior registers get restored to those
    before dummy_frame).  */
 
-struct call_function_by_hand_dtor *
-call_function_by_hand_dtor_setup (struct frame_id dummy_id, ptid_t ptid)
+struct dummy_frame_context_saver *
+dummy_frame_context_saver_setup (struct frame_id dummy_id, ptid_t ptid)
 {
-  struct call_function_by_hand_dtor *our_dtor_data;
+  struct dummy_frame_context_saver *saver;
 
-  our_dtor_data = xmalloc (sizeof (*our_dtor_data));
-  our_dtor_data->retbuf = NULL;
-  our_dtor_data->drop_done = 0;
+  saver = xmalloc (sizeof (*saver));
+  saver->retbuf = NULL;
+  saver->drop_done = 0;
   register_dummy_frame_dtor (dummy_id, inferior_ptid,
-			     call_function_by_hand_dtor, our_dtor_data);
-  return our_dtor_data;
+			     dummy_frame_context_saver_dtor, saver);
+  return saver;
 }
 
 /* All this stuff with a dummy frame may seem unnecessarily complicated
@@ -604,8 +603,8 @@ call_function_by_hand_dummy (struct value *function,
   struct gdb_exception e;
   char name_buf[RAW_FUNCTION_ADDRESS_SIZE];
   int stack_temporaries = thread_stack_temporaries_enabled_p (inferior_ptid);
-  struct call_function_by_hand_dtor *our_dtor_data;
-  struct cleanup *our_dtor_cleanup;
+  struct dummy_frame_context_saver *context_saver;
+  struct cleanup *context_saver_cleanup;
 
   if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
     ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
@@ -992,12 +991,12 @@ call_function_by_hand_dummy (struct value *function,
     register_dummy_frame_dtor (dummy_id, inferior_ptid,
 			       dummy_dtor, dummy_dtor_data);
 
-  /* call_function_by_hand_dtor_setup should be called last so that its
+  /* dummy_frame_context_saver_setup must be called last so that its
      saving of inferior registers gets called first (before possible
      DUMMY_DTOR destructor).  */
-  our_dtor_data = call_function_by_hand_dtor_setup (dummy_id, inferior_ptid);
-  our_dtor_cleanup = make_cleanup (call_function_by_hand_dtor_cleanup,
-				   our_dtor_data);
+  context_saver = dummy_frame_context_saver_setup (dummy_id, inferior_ptid);
+  context_saver_cleanup = make_cleanup (dummy_frame_context_saver_cleanup,
+					context_saver);
 
   /* Register a clean-up for unwind_on_terminating_exception_breakpoint.  */
   terminate_bp_cleanup = make_cleanup (cleanup_delete_std_terminate_breakpoint,
@@ -1240,7 +1239,7 @@ When the function is done executing, GDB will silently stop."),
       {
 	retval = allocate_value (values_type);
 	gdbarch_return_value (gdbarch, function, values_type,
-			      call_function_by_hand_dtor_get (our_dtor_data),
+			      dummy_frame_context_saver_get_regs (context_saver),
 			      value_contents_raw (retval), NULL);
 	if (stack_temporaries && class_or_union_p (values_type))
 	  {
@@ -1256,7 +1255,7 @@ When the function is done executing, GDB will silently stop."),
 	  }
       }
 
-    do_cleanups (our_dtor_cleanup);
+    do_cleanups (context_saver_cleanup);
 
     gdb_assert (retval);
     return retval;
diff --git a/gdb/infcall.h b/gdb/infcall.h
index 9ec8617..43b5f66 100644
--- a/gdb/infcall.h
+++ b/gdb/infcall.h
@@ -50,14 +50,13 @@ extern struct value *
 			       dummy_frame_dtor_ftype *dummy_dtor,
 			       void *dummy_dtor_data);
 
-struct call_function_by_hand_dtor;
-extern void call_function_by_hand_dtor_drop
-  (struct call_function_by_hand_dtor *data);
-extern void call_function_by_hand_dtor_cleanup
-  (void *data_voidp);
-extern struct regcache *call_function_by_hand_dtor_get
-  (struct call_function_by_hand_dtor *dtor_data);
-extern struct call_function_by_hand_dtor *call_function_by_hand_dtor_setup
+struct dummy_frame_context_saver;
+extern void dummy_frame_context_saver_drop
+  (struct dummy_frame_context_saver *data);
+extern void dummy_frame_context_saver_cleanup (void *data_voidp);
+extern struct regcache *dummy_frame_context_saver_get_regs
+  (struct dummy_frame_context_saver *saver);
+extern struct dummy_frame_context_saver *dummy_frame_context_saver_setup
   (struct frame_id dummy_id, ptid_t ptid);
 
 #endif
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 5df8e0f..3bcab77 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1512,7 +1512,7 @@ advance_command (char *arg, int from_tty)
 
 struct value *
 get_return_value (struct value *function, struct type *value_type,
-		  struct call_function_by_hand_dtor *dtor_data)
+		  struct dummy_frame_context_saver *ctx_saver)
 {
   struct regcache *stop_regs = NULL;
   struct gdbarch *gdbarch;
@@ -1520,8 +1520,8 @@ get_return_value (struct value *function, struct type *value_type,
   struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
 
   /* If were not saved, use the current registers.  */
-  if (dtor_data != NULL)
-    stop_regs = call_function_by_hand_dtor_get (dtor_data);
+  if (ctx_saver != NULL)
+    stop_regs = dummy_frame_context_saver_get_regs (ctx_saver);
   else
     {
       stop_regs = regcache_dup (get_current_regcache ());
@@ -1568,9 +1568,9 @@ get_return_value (struct value *function, struct type *value_type,
 
 static void
 print_return_value (struct value *function, struct type *value_type,
-		    struct call_function_by_hand_dtor *dtor_data)
+		    struct dummy_frame_context_saver *ctx_saver)
 {
-  struct value *value = get_return_value (function, value_type, dtor_data);
+  struct value *value = get_return_value (function, value_type, ctx_saver);
   struct ui_out *uiout = current_uiout;
 
   if (value)
@@ -1625,7 +1625,7 @@ struct finish_command_continuation_args
   /* Inferior registers stored right before dummy_frame has been freed
      after an inferior call.  It can be NULL if no inferior call was
      involved, GDB will then use current inferior registers.  */
-  struct call_function_by_hand_dtor *dtor_data;
+  struct dummy_frame_context_saver *ctx_saver;
 };
 
 static void
@@ -1666,7 +1666,7 @@ finish_command_continuation (void *arg, int err)
 		  /* print_return_value can throw an exception in some
 		     circumstances.  We need to catch this so that we still
 		     delete the breakpoint.  */
-		  print_return_value (func, value_type, a->dtor_data);
+		  print_return_value (func, value_type, a->ctx_saver);
 		}
 	      CATCH (ex, RETURN_MASK_ALL)
 		{
@@ -1692,8 +1692,8 @@ finish_command_continuation_free_arg (void *arg)
 {
   struct finish_command_continuation_args *cargs = arg;
 
-  if (cargs->dtor_data != NULL)
-    call_function_by_hand_dtor_drop (cargs->dtor_data);
+  if (cargs->ctx_saver != NULL)
+    dummy_frame_context_saver_drop (cargs->ctx_saver);
   xfree (cargs);
 }
 
@@ -1762,16 +1762,16 @@ finish_forward (struct symbol *function, struct frame_info *frame)
   struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
   struct finish_command_continuation_args *cargs;
   int thread = tp->num;
-  struct call_function_by_hand_dtor *dtor_data = NULL;
+  struct dummy_frame_context_saver *saver = NULL;
 
   sal = find_pc_line (get_frame_pc (frame), 0);
   sal.pc = get_frame_pc (frame);
 
   if (get_frame_type (frame) == DUMMY_FRAME)
     {
-      dtor_data = call_function_by_hand_dtor_setup (get_stack_frame_id (frame),
-						    inferior_ptid);
-      make_cleanup (call_function_by_hand_dtor_cleanup, dtor_data);
+      saver = dummy_frame_context_saver_setup (get_stack_frame_id (frame),
+					       inferior_ptid);
+      make_cleanup (dummy_frame_context_saver_cleanup, saver);
     }
 
   breakpoint = set_momentary_breakpoint (gdbarch, sal,
@@ -1793,7 +1793,7 @@ finish_forward (struct symbol *function, struct frame_info *frame)
   cargs->thread = thread;
   cargs->breakpoint = breakpoint;
   cargs->function = function;
-  cargs->dtor_data = dtor_data;
+  cargs->ctx_saver = saver;
   add_continuation (tp, finish_command_continuation, cargs,
                     finish_command_continuation_free_arg);
   proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
diff --git a/gdb/inferior.h b/gdb/inferior.h
index f305e79..f0c5b52 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -165,10 +165,10 @@ extern void detach_command (char *, int);
 
 extern void notice_new_inferior (ptid_t, int, int);
 
-struct call_function_by_hand_dtor;
+struct dummy_frame_context_saver;
 extern struct value *get_return_value
   (struct value *function, struct type *value_type,
-   struct call_function_by_hand_dtor *dtor_data);
+   struct dummy_frame_context_saver *dtor_data);
 
 /* Prepare for execution command.  TARGET is the target that will run
    the command.  BACKGROUND determines whether this is a foreground
-- 
1.9.3