Patchwork [RFA] Change enable_thread_stack_temporaries to an RAII class

login
register
mail settings
Submitter Tom Tromey
Date March 8, 2018, 5:35 a.m.
Message ID <20180308053543.18516-1-tom@tromey.com>
Download mbox | patch
Permalink /patch/26244/
State New
Headers show

Comments

Tom Tromey - March 8, 2018, 5:35 a.m.
This started as a patch to change enable_thread_stack_temporaries to
be an RAII class, but then I noticed that this code used a VEC, so I
went ahead and did a bit more C++-ification, changing
stack_temporaries_enabled to a bool and changing stack_temporaries to
a std::vector.

Regression tested by the buildbot.

gdb/ChangeLog
2018-03-07  Tom Tromey  <tom@tromey.com>

	* infcall.c (struct call_return_meta_info)
	<stack_temporaries_enabled>: Now a bool.
	(get_call_return_value, call_function_by_hand_dummy): Update.
	* thread.c (disable_thread_stack_temporaries): Remove.
	(enable_thread_stack_temporaries): Remove.
	(thread_stack_temporaries_enabled_p): Return bool.
	(push_thread_stack_temporary, value_in_thread_stack_temporaries)
	(get_last_thread_stack_temporary): Update.
	* eval.c (evaluate_subexp): Update.
	* gdbthread.h (class enable_thread_stack_temporaries): Now a
	class, not a function.
	(value_ptr, value_vec): Remove typedefs.
	(class thread_info) <stack_temporaries_enabled>: Now bool.
	<stack_temporaries>: Now a std::vector.
	(thread_stack_temporaries_enabled_p)
	(value_in_thread_stack_temporaries): Return bool.
---
 gdb/ChangeLog   | 19 ++++++++++++++++
 gdb/eval.c      | 17 +++++----------
 gdb/gdbthread.h | 47 +++++++++++++++++++++++++++++++--------
 gdb/infcall.c   |  6 ++---
 gdb/thread.c    | 68 +++++++++++----------------------------------------------
 5 files changed, 78 insertions(+), 79 deletions(-)
Simon Marchi - March 9, 2018, 3:57 a.m.
On 2018-03-08 12:35 AM, Tom Tromey wrote:
> diff --git a/gdb/infcall.c b/gdb/infcall.c
> index b7f4a176db..a77749f514 100644
> --- a/gdb/infcall.c
> +++ b/gdb/infcall.c
> @@ -411,7 +411,7 @@ struct call_return_meta_info
>    CORE_ADDR struct_addr;
>  
>    /* Whether stack temporaries are enabled.  */
> -  int stack_temporaries_enabled;
> +  bool stack_temporaries_enabled;

I think we can just removed this field, it seems to be unused.

Otherwise LGTM.

Simon
Simon Marchi - March 9, 2018, 3:58 a.m.
Oops, forgot this one:

On 2018-03-08 12:35 AM, Tom Tromey wrote:
> @@ -810,29 +771,26 @@ push_thread_stack_temporary (ptid_t ptid, struct value *v)
>    struct thread_info *tp = find_thread_ptid (ptid);
>  
>    gdb_assert (tp != NULL && tp->stack_temporaries_enabled);
> -  VEC_safe_push (value_ptr, tp->stack_temporaries, v);
> +  tp->stack_temporaries.push_back (v);
>  }
>  
> -/* Return 1 if VAL is among the stack temporaries of the thread
> -   with id PTID.  Return 0 otherwise.  */
> +/* Return true if VAL is among the stack temporaries of the thread
> +   with id PTID.  Return false otherwise.  */
>  
> -int
> +bool
>  value_in_thread_stack_temporaries (struct value *val, ptid_t ptid)
>  {
>    struct thread_info *tp = find_thread_ptid (ptid);
>  
>    gdb_assert (tp != NULL && tp->stack_temporaries_enabled);
> -  if (!VEC_empty (value_ptr, tp->stack_temporaries))
> +  if (!tp->stack_temporaries.empty ())
>      {
> -      struct value *v;
> -      int i;
> -
> -      for (i = 0; VEC_iterate (value_ptr, tp->stack_temporaries, i, v); i++)
> +      for (struct value *v : tp->stack_temporaries)
>  	if (v == val)
> -	  return 1;
> +	  return true;

The "if (!tp->stack_temporaries.empty ())" here is now unnecessary.

Simon
Tom Tromey - March 9, 2018, 4:58 a.m.
>> +  bool stack_temporaries_enabled;

Simon> I think we can just removed this field, it seems to be unused.

Thanks for noticing this, I've removed it.

Simon> The "if (!tp->stack_temporaries.empty ())" here is now unnecessary.

I made this change as well.

Tom

Patch

diff --git a/gdb/eval.c b/gdb/eval.c
index 4899011a58..b2032c35f1 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -69,27 +69,20 @@  struct value *
 evaluate_subexp (struct type *expect_type, struct expression *exp,
 		 int *pos, enum noside noside)
 {
-  struct cleanup *cleanups;
   struct value *retval;
-  int cleanup_temps = 0;
 
+  gdb::optional<enable_thread_stack_temporaries> stack_temporaries;
   if (*pos == 0 && target_has_execution
       && exp->language_defn->la_language == language_cplus
       && !thread_stack_temporaries_enabled_p (inferior_ptid))
-    {
-      cleanups = enable_thread_stack_temporaries (inferior_ptid);
-      cleanup_temps = 1;
-    }
+    stack_temporaries.emplace (inferior_ptid);
 
   retval = (*exp->language_defn->la_exp_desc->evaluate_exp)
     (expect_type, exp, pos, noside);
 
-  if (cleanup_temps)
-    {
-      if (value_in_thread_stack_temporaries (retval, inferior_ptid))
-	retval = value_non_lval (retval);
-      do_cleanups (cleanups);
-    }
+  if (stack_temporaries.has_value ()
+      && value_in_thread_stack_temporaries (retval, inferior_ptid))
+    retval = value_non_lval (retval);
 
   return retval;
 }
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index cffdfb9188..9b468dbda7 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -175,10 +175,6 @@  struct thread_suspend_state
   CORE_ADDR stop_pc;
 };
 
-typedef struct value *value_ptr;
-DEF_VEC_P (value_ptr);
-typedef VEC (value_ptr) value_vec;
-
 /* Base class for target-specific thread data.  */
 struct private_thread_info
 {
@@ -358,11 +354,11 @@  public:
 
   /* Flag which indicates that the stack temporaries should be stored while
      evaluating expressions.  */
-  int stack_temporaries_enabled = 0;
+  bool stack_temporaries_enabled = false;
 
   /* Values that are stored as temporaries on stack while evaluating
      expressions.  */
-  value_vec *stack_temporaries = NULL;
+  std::vector<struct value *> stack_temporaries;
 
   /* Step-over chain.  A thread is in the step-over queue if these are
      non-NULL.  If only a single thread is in the chain, then these
@@ -634,15 +630,48 @@  extern void delete_exited_threads (void);
 
 int pc_in_thread_step_range (CORE_ADDR pc, struct thread_info *thread);
 
-extern struct cleanup *enable_thread_stack_temporaries (ptid_t ptid);
+/* Enable storing stack temporaries for thread with id PTID and
+   disable and clear the stack temporaries on destruction.  */
+
+class enable_thread_stack_temporaries
+{
+public:
+
+  explicit enable_thread_stack_temporaries (ptid_t ptid)
+    : m_ptid (ptid)
+  {
+    struct thread_info *tp = find_thread_ptid (ptid);
+
+    gdb_assert (tp != NULL);
+    tp->stack_temporaries_enabled = true;
+    tp->stack_temporaries.clear ();
+  }
+
+  ~enable_thread_stack_temporaries ()
+  {
+    struct thread_info *tp = find_thread_ptid (m_ptid);
+
+    if (tp != NULL)
+      {
+	tp->stack_temporaries_enabled = false;
+	tp->stack_temporaries.clear ();
+      }
+  }
+
+  DISABLE_COPY_AND_ASSIGN (enable_thread_stack_temporaries);
+
+private:
+
+  ptid_t m_ptid;
+};
 
-extern int thread_stack_temporaries_enabled_p (ptid_t ptid);
+extern bool thread_stack_temporaries_enabled_p (ptid_t ptid);
 
 extern void push_thread_stack_temporary (ptid_t ptid, struct value *v);
 
 extern struct value *get_last_thread_stack_temporary (ptid_t);
 
-extern int value_in_thread_stack_temporaries (struct value *, ptid_t);
+extern bool value_in_thread_stack_temporaries (struct value *, ptid_t);
 
 /* Add TP to the end of its inferior's pending step-over chain.  */
 
diff --git a/gdb/infcall.c b/gdb/infcall.c
index b7f4a176db..a77749f514 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -411,7 +411,7 @@  struct call_return_meta_info
   CORE_ADDR struct_addr;
 
   /* Whether stack temporaries are enabled.  */
-  int stack_temporaries_enabled;
+  bool stack_temporaries_enabled;
 };
 
 /* Extract the called function's return value.  */
@@ -420,7 +420,7 @@  static struct value *
 get_call_return_value (struct call_return_meta_info *ri)
 {
   struct value *retval = NULL;
-  int stack_temporaries = thread_stack_temporaries_enabled_p (inferior_ptid);
+  bool stack_temporaries = thread_stack_temporaries_enabled_p (inferior_ptid);
 
   if (TYPE_CODE (ri->value_type) == TYPE_CODE_VOID)
     retval = allocate_value (ri->value_type);
@@ -739,7 +739,7 @@  call_function_by_hand_dummy (struct value *function,
   ptid_t call_thread_ptid;
   struct gdb_exception e;
   char name_buf[RAW_FUNCTION_ADDRESS_SIZE];
-  int stack_temporaries = thread_stack_temporaries_enabled_p (inferior_ptid);
+  bool stack_temporaries = thread_stack_temporaries_enabled_p (inferior_ptid);
 
   if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
     ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
diff --git a/gdb/thread.c b/gdb/thread.c
index 36cfcdd632..0dbfbfbd2e 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -749,55 +749,16 @@  delete_exited_threads (void)
     }
 }
 
-/* Disable storing stack temporaries for the thread whose id is
-   stored in DATA.  */
-
-static void
-disable_thread_stack_temporaries (void *data)
-{
-  ptid_t *pd = (ptid_t *) data;
-  struct thread_info *tp = find_thread_ptid (*pd);
-
-  if (tp != NULL)
-    {
-      tp->stack_temporaries_enabled = 0;
-      VEC_free (value_ptr, tp->stack_temporaries);
-    }
-
-  xfree (pd);
-}
-
-/* Enable storing stack temporaries for thread with id PTID and return a
-   cleanup which can disable and clear the stack temporaries.  */
-
-struct cleanup *
-enable_thread_stack_temporaries (ptid_t ptid)
-{
-  struct thread_info *tp = find_thread_ptid (ptid);
-  ptid_t  *data;
-  struct cleanup *c;
-
-  gdb_assert (tp != NULL);
-
-  tp->stack_temporaries_enabled = 1;
-  tp->stack_temporaries = NULL;
-  data = XNEW (ptid_t);
-  *data = ptid;
-  c = make_cleanup (disable_thread_stack_temporaries, data);
-
-  return c;
-}
-
-/* Return non-zero value if stack temporaies are enabled for the thread
+/* Return true value if stack temporaies are enabled for the thread
    with id PTID.  */
 
-int
+bool
 thread_stack_temporaries_enabled_p (ptid_t ptid)
 {
   struct thread_info *tp = find_thread_ptid (ptid);
 
   if (tp == NULL)
-    return 0;
+    return false;
   else
     return tp->stack_temporaries_enabled;
 }
@@ -810,29 +771,26 @@  push_thread_stack_temporary (ptid_t ptid, struct value *v)
   struct thread_info *tp = find_thread_ptid (ptid);
 
   gdb_assert (tp != NULL && tp->stack_temporaries_enabled);
-  VEC_safe_push (value_ptr, tp->stack_temporaries, v);
+  tp->stack_temporaries.push_back (v);
 }
 
-/* Return 1 if VAL is among the stack temporaries of the thread
-   with id PTID.  Return 0 otherwise.  */
+/* Return true if VAL is among the stack temporaries of the thread
+   with id PTID.  Return false otherwise.  */
 
-int
+bool
 value_in_thread_stack_temporaries (struct value *val, ptid_t ptid)
 {
   struct thread_info *tp = find_thread_ptid (ptid);
 
   gdb_assert (tp != NULL && tp->stack_temporaries_enabled);
-  if (!VEC_empty (value_ptr, tp->stack_temporaries))
+  if (!tp->stack_temporaries.empty ())
     {
-      struct value *v;
-      int i;
-
-      for (i = 0; VEC_iterate (value_ptr, tp->stack_temporaries, i, v); i++)
+      for (struct value *v : tp->stack_temporaries)
 	if (v == val)
-	  return 1;
+	  return true;
     }
 
-  return 0;
+  return false;
 }
 
 /* Return the last of the stack temporaries for thread with id PTID.
@@ -845,8 +803,8 @@  get_last_thread_stack_temporary (ptid_t ptid)
   struct thread_info *tp = find_thread_ptid (ptid);
 
   gdb_assert (tp != NULL);
-  if (!VEC_empty (value_ptr, tp->stack_temporaries))
-    lastval = VEC_last (value_ptr, tp->stack_temporaries);
+  if (!tp->stack_temporaries.empty ())
+    lastval = tp->stack_temporaries.back ();
 
   return lastval;
 }