[1/2] Remove a VEC from ppc-linux-nat.c

Message ID 20190427143134.14048-2-tom@tromey.com
State New
Headers show
Series
  • Remove two VECs
Related show

Commit Message

Tom Tromey April 27, 2019, 2:31 p.m.
This replaces a VEC in ppc-linux-nat.c with a std::vector.

gdb/ChangeLog
2019-04-27  Tom Tromey  <tom@tromey.com>

	* ppc-linux-nat.c (thread_points_p): Remove typedef and DEF_VEC.
	(ppc_threads): Now a std::vector.  Now static.
	(hwdebug_find_thread_points_by_tid)
	(ppc_linux_nat_target::low_new_thread, ppc_linux_thread_exit):
	Update.
---
 gdb/ChangeLog       |  8 ++++++++
 gdb/ppc-linux-nat.c | 30 +++++++++++++-----------------
 2 files changed, 21 insertions(+), 17 deletions(-)

-- 
2.17.2

Comments

Simon Marchi April 27, 2019, 3:23 p.m. | #1
Hi Tom,

LGTM, I just wrote some suggestions below.

On 2019-04-27 10:31 a.m., Tom Tromey wrote:
> @@ -1758,14 +1757,11 @@ hwdebug_point_cmp (struct ppc_hw_breakpoint *a, struct ppc_hw_breakpoint *b)

>  static struct thread_points *

>  hwdebug_find_thread_points_by_tid (int tid, int alloc_new)

>  {

> -  int i;

> -  struct thread_points *t;

> -

> -  for (i = 0; VEC_iterate (thread_points_p, ppc_threads, i, t); i++)

> +  for (thread_points *t : ppc_threads)

>      if (t->tid == tid)

>        return t;


Could you add braces to this for, to match our coding style?

> @@ -2392,22 +2388,22 @@ ppc_linux_thread_exit (struct thread_info *tp, int silent)

>    int i;

>    int tid = tp->ptid.lwp ();

>    struct hw_break_tuple *hw_breaks;

> -  struct thread_points *t = NULL, *p;

> +  struct thread_points *t = NULL;

>  

>    if (!have_ptrace_hwdebug_interface ())

>      return;

>  

> -  for (i = 0; VEC_iterate (thread_points_p, ppc_threads, i, p); i++)

> -    if (p->tid == tid)

> +  for (i = 0; i < ppc_threads.size (); i++)

> +    if (ppc_threads[i].tid == tid)

>        {

> -	t = p;

> +	t = &ppc_threads[i];

>  	break;

>        }


Here too?

I believe this loop could be a range-based one, though it also works like this.

Simon
Simon Marchi April 27, 2019, 3:31 p.m. | #2
On 2019-04-27 11:23 a.m., Simon Marchi wrote:
> Hi Tom,

> 

> LGTM, I just wrote some suggestions below.

> 

> On 2019-04-27 10:31 a.m., Tom Tromey wrote:

>> @@ -1758,14 +1757,11 @@ hwdebug_point_cmp (struct ppc_hw_breakpoint *a, struct ppc_hw_breakpoint *b)

>>  static struct thread_points *

>>  hwdebug_find_thread_points_by_tid (int tid, int alloc_new)

>>  {

>> -  int i;

>> -  struct thread_points *t;

>> -

>> -  for (i = 0; VEC_iterate (thread_points_p, ppc_threads, i, t); i++)

>> +  for (thread_points *t : ppc_threads)

>>      if (t->tid == tid)

>>        return t;

> 

> Could you add braces to this for, to match our coding style?

> 

>> @@ -2392,22 +2388,22 @@ ppc_linux_thread_exit (struct thread_info *tp, int silent)

>>    int i;

>>    int tid = tp->ptid.lwp ();

>>    struct hw_break_tuple *hw_breaks;

>> -  struct thread_points *t = NULL, *p;

>> +  struct thread_points *t = NULL;

>>  

>>    if (!have_ptrace_hwdebug_interface ())

>>      return;

>>  

>> -  for (i = 0; VEC_iterate (thread_points_p, ppc_threads, i, p); i++)

>> -    if (p->tid == tid)

>> +  for (i = 0; i < ppc_threads.size (); i++)

>> +    if (ppc_threads[i].tid == tid)

>>        {

>> -	t = p;

>> +	t = &ppc_threads[i];

>>  	break;

>>        }

> 

> Here too?

> 

> I believe this loop could be a range-based one, though it also works like this.


Nevermind about this last suggestion, as the value of the `i` variable is required
below to do the removal.

But actually when looking at the code a bit closer, I am not sure it works, since
 ppc_threads[i] yields a pointer, so using the '.' operator is not valid.  And indeed:

/home/simark/src/binutils-gdb/gdb/ppc-linux-nat.c: In function 'void ppc_linux_thread_exit(thread_info*, int)':
/home/simark/src/binutils-gdb/gdb/ppc-linux-nat.c:2397:24: error: request for member 'tid' in 'ppc_threads.std::vector<_Tp, _Alloc>::operator[]<thread_points*, std::allocator<thread_points*> >(((std::vector<thread_points*>::size_type)i))', which is of pointer type '__gnu_cxx::__alloc_traits<std::allocator<thread_points*> >::value_type {aka thread_points*}' (maybe you meant to use '->' ?)
     if (ppc_threads[i].tid == tid)
                        ^~~
/home/simark/src/binutils-gdb/gdb/ppc-linux-nat.c:2399:20: error: cannot convert 'thread_points**' to 'thread_points*' in assignment
  t = &ppc_threads[i];
                    ^

Building also points out this error, where you probably meant to use .back ():

/home/simark/src/binutils-gdb/gdb/ppc-linux-nat.c: In member function 'virtual void ppc_linux_nat_target::low_new_thread(lwp_info*)':
/home/simark/src/binutils-gdb/gdb/ppc-linux-nat.c:2362:23: error: 'class std::vector<thread_points*>' has no member named 'last'; did you mean 'at'?
       p = ppc_threads.last ();
                       ^~~~

Applying this fixup to your patch makes it build for me:

From b687773f708d21e04dfe3b7d7094514d4cf8b7b7 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>

Date: Sat, 27 Apr 2019 11:30:58 -0400
Subject: [PATCH] fixup

---
 gdb/ppc-linux-nat.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index 45cf594ffcac..59e7ca5863f1 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -2359,7 +2359,7 @@ ppc_linux_nat_target::low_new_thread (struct lwp_info *lp)
 	return;

       /* Get a list of breakpoints from any thread.  */
-      p = ppc_threads.last ();
+      p = ppc_threads.back ();
       hw_breaks = p->hw_breaks;

       /* Copy that thread's breakpoints and watchpoints to the new thread.  */
@@ -2394,9 +2394,9 @@ ppc_linux_thread_exit (struct thread_info *tp, int silent)
     return;

   for (i = 0; i < ppc_threads.size (); i++)
-    if (ppc_threads[i].tid == tid)
+    if (ppc_threads[i]->tid == tid)
       {
-	t = &ppc_threads[i];
+	t = ppc_threads[i];
 	break;
       }

-- 
2.21.0
Tom Tromey May 4, 2019, 8:26 p.m. | #3
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


Simon> Applying this fixup to your patch makes it build for me:

Thanks.  I've included this & made the other changes you requested.

Tom

Patch

diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index 628e3d5e8f6..45cf594ffca 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1578,7 +1578,7 @@  struct hw_break_tuple
 /* This is an internal VEC created to store information about *points inserted
    for each thread.  This is used when PowerPC HWDEBUG ptrace interface is
    available.  */
-typedef struct thread_points
+struct thread_points
   {
     /* The TID to which this *point relates.  */
     int tid;
@@ -1589,10 +1589,9 @@  typedef struct thread_points
        size of these vector is MAX_SLOTS_NUMBER.  If the hw_break element of
        the tuple is NULL, then the position in the vector is free.  */
     struct hw_break_tuple *hw_breaks;
-  } *thread_points_p;
-DEF_VEC_P (thread_points_p);
+  };
 
-VEC(thread_points_p) *ppc_threads = NULL;
+static std::vector<thread_points *> ppc_threads;
 
 /* The version of the PowerPC HWDEBUG kernel interface that we will use, if
    available.  */
@@ -1758,14 +1757,11 @@  hwdebug_point_cmp (struct ppc_hw_breakpoint *a, struct ppc_hw_breakpoint *b)
 static struct thread_points *
 hwdebug_find_thread_points_by_tid (int tid, int alloc_new)
 {
-  int i;
-  struct thread_points *t;
-
-  for (i = 0; VEC_iterate (thread_points_p, ppc_threads, i, t); i++)
+  for (thread_points *t : ppc_threads)
     if (t->tid == tid)
       return t;
 
-  t = NULL;
+  struct thread_points *t = NULL;
 
   /* Do we need to allocate a new point_item
      if the wanted one does not exist?  */
@@ -1774,7 +1770,7 @@  hwdebug_find_thread_points_by_tid (int tid, int alloc_new)
       t = XNEW (struct thread_points);
       t->hw_breaks = XCNEWVEC (struct hw_break_tuple, max_slots_number);
       t->tid = tid;
-      VEC_safe_push (thread_points_p, ppc_threads, t);
+      ppc_threads.push_back (t);
     }
 
   return t;
@@ -2359,11 +2355,11 @@  ppc_linux_nat_target::low_new_thread (struct lwp_info *lp)
       struct thread_points *p;
       struct hw_break_tuple *hw_breaks;
 
-      if (VEC_empty (thread_points_p, ppc_threads))
+      if (ppc_threads.empty ())
 	return;
 
       /* Get a list of breakpoints from any thread.  */
-      p = VEC_last (thread_points_p, ppc_threads);
+      p = ppc_threads.last ();
       hw_breaks = p->hw_breaks;
 
       /* Copy that thread's breakpoints and watchpoints to the new thread.  */
@@ -2392,22 +2388,22 @@  ppc_linux_thread_exit (struct thread_info *tp, int silent)
   int i;
   int tid = tp->ptid.lwp ();
   struct hw_break_tuple *hw_breaks;
-  struct thread_points *t = NULL, *p;
+  struct thread_points *t = NULL;
 
   if (!have_ptrace_hwdebug_interface ())
     return;
 
-  for (i = 0; VEC_iterate (thread_points_p, ppc_threads, i, p); i++)
-    if (p->tid == tid)
+  for (i = 0; i < ppc_threads.size (); i++)
+    if (ppc_threads[i].tid == tid)
       {
-	t = p;
+	t = &ppc_threads[i];
 	break;
       }
 
   if (t == NULL)
     return;
 
-  VEC_unordered_remove (thread_points_p, ppc_threads, i);
+  unordered_remove (ppc_threads, i);
 
   hw_breaks = t->hw_breaks;