Patchwork [v2,3/3] Replace home-grown linked-lists in FreeBSD's native target with STL lists.

login
register
mail settings
Submitter John Baldwin
Date Aug. 9, 2017, 5:47 p.m.
Message ID <20170809174754.49166-4-jhb@FreeBSD.org>
Download mbox | patch
Permalink /patch/22051/
State New
Headers show

Comments

John Baldwin - Aug. 9, 2017, 5:47 p.m.
FreeBSD's native target uses linked-lists to keep track of pending fork
events and fake vfork done events.  Replace the first list with std::list
and the second with std::forward_list.

gdb/ChangeLog:

	* fbsd-nat.c (struct fbsd_fork_info): Remove.
	(fbsd_pending_children): Use std::list.
	(fbsd_remember_child): Likewise.
	(fbsd_is_child_pending): Likewise.
	(fbsd_pending_vfork_done): Use std::forward_list.
	(fbsd_add_vfork_done): Likewise.
	(fbsd_is_vfork_done_pending): Likewise.
	(fbsd_next_vfork_done): Likewise.
---
 gdb/ChangeLog  | 11 +++++++++
 gdb/fbsd-nat.c | 71 +++++++++++++++++-----------------------------------------
 2 files changed, 32 insertions(+), 50 deletions(-)
Simon Marchi - Aug. 9, 2017, 6:16 p.m.
On 2017-08-09 19:47, John Baldwin wrote:
> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index 721e72b85c..c89343a24f 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -41,6 +41,8 @@
>  #include "elf-bfd.h"
>  #include "fbsd-nat.h"
> 
> +#include <list>
> +
>  /* Return the name of a file that can be opened to get the symbols for
>     the child process identified by PID.  */
> 
> @@ -712,13 +714,7 @@ fbsd_update_thread_list (struct target_ops *ops)
>    sake.  FreeBSD versions newer than 9.1 contain both fixes.
>  */
> 
> -struct fbsd_fork_info
> -{
> -  struct fbsd_fork_info *next;
> -  ptid_t ptid;
> -};
> -
> -static struct fbsd_fork_info *fbsd_pending_children;
> +static std::list<ptid_t> fbsd_pending_children;

Can't this be a forward_list as well?

The series LGTM otherwise.

Simon
John Baldwin - Aug. 9, 2017, 7:15 p.m.
On Wednesday, August 09, 2017 08:16:29 PM Simon Marchi wrote:
> On 2017-08-09 19:47, John Baldwin wrote:
> > diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> > index 721e72b85c..c89343a24f 100644
> > --- a/gdb/fbsd-nat.c
> > +++ b/gdb/fbsd-nat.c
> > @@ -41,6 +41,8 @@
> >  #include "elf-bfd.h"
> >  #include "fbsd-nat.h"
> > 
> > +#include <list>
> > +
> >  /* Return the name of a file that can be opened to get the symbols for
> >     the child process identified by PID.  */
> > 
> > @@ -712,13 +714,7 @@ fbsd_update_thread_list (struct target_ops *ops)
> >    sake.  FreeBSD versions newer than 9.1 contain both fixes.
> >  */
> > 
> > -struct fbsd_fork_info
> > -{
> > -  struct fbsd_fork_info *next;
> > -  ptid_t ptid;
> > -};
> > -
> > -static struct fbsd_fork_info *fbsd_pending_children;
> > +static std::list<ptid_t> fbsd_pending_children;
> 
> Can't this be a forward_list as well?

Not trivially.  fbsd_is_child_pending has to walk the list looking for a
specific PID and remove that specific list entry (not always the first
entry).  Right now this is using the following loop:

   for (auto it = fbsd_pending_children.begin ();
       it != fbsd_pending_children.end (); it++)
     if (it->pid () == pid)
       {
         ptid_t ptid = *it;
         fbsd_pending_children.erase (it);
         return ptid;
       }
   return null_ptid;

I'm not sure if it's legal to do something like this for a forward_list:

   for (auto it = fbsd_pending_children.before_begin ();
        it + 1 != fbsd_pending_children.end (); it++)
    if ((it + 1)->pid () == pid)
       {
         ptid_t ptid = *(it + 1);
         fbsd_pending_childern.erase_after (it);
         return ptid;
       }
   return null_ptid;

Even if it is legal, I'm not sure it is more readable.  These lists should
generally be quite small, so I think readability is more important than
optimization in this case.
Simon Marchi - Aug. 9, 2017, 7:31 p.m.
On 2017-08-09 21:15, John Baldwin wrote:
> On Wednesday, August 09, 2017 08:16:29 PM Simon Marchi wrote:
>> On 2017-08-09 19:47, John Baldwin wrote:
>> > diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
>> > index 721e72b85c..c89343a24f 100644
>> > --- a/gdb/fbsd-nat.c
>> > +++ b/gdb/fbsd-nat.c
>> > @@ -41,6 +41,8 @@
>> >  #include "elf-bfd.h"
>> >  #include "fbsd-nat.h"
>> >
>> > +#include <list>
>> > +
>> >  /* Return the name of a file that can be opened to get the symbols for
>> >     the child process identified by PID.  */
>> >
>> > @@ -712,13 +714,7 @@ fbsd_update_thread_list (struct target_ops *ops)
>> >    sake.  FreeBSD versions newer than 9.1 contain both fixes.
>> >  */
>> >
>> > -struct fbsd_fork_info
>> > -{
>> > -  struct fbsd_fork_info *next;
>> > -  ptid_t ptid;
>> > -};
>> > -
>> > -static struct fbsd_fork_info *fbsd_pending_children;
>> > +static std::list<ptid_t> fbsd_pending_children;
>> 
>> Can't this be a forward_list as well?
> 
> Not trivially.  fbsd_is_child_pending has to walk the list looking for 
> a
> specific PID and remove that specific list entry (not always the first
> entry).  Right now this is using the following loop:
> 
>    for (auto it = fbsd_pending_children.begin ();
>        it != fbsd_pending_children.end (); it++)
>      if (it->pid () == pid)
>        {
>          ptid_t ptid = *it;
>          fbsd_pending_children.erase (it);
>          return ptid;
>        }
>    return null_ptid;
> 
> I'm not sure if it's legal to do something like this for a 
> forward_list:
> 
>    for (auto it = fbsd_pending_children.before_begin ();
>         it + 1 != fbsd_pending_children.end (); it++)
>     if ((it + 1)->pid () == pid)
>        {
>          ptid_t ptid = *(it + 1);
>          fbsd_pending_childern.erase_after (it);
>          return ptid;
>        }
>    return null_ptid;
> 
> Even if it is legal, I'm not sure it is more readable.  These lists 
> should
> generally be quite small, so I think readability is more important than
> optimization in this case.

Ah, ok.  I thought it could be done trivially, but I can't easily 
build-test on FreeBSD right now.

I agree with you.

Simon

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 52cdc60546..eb2ea389df 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,16 @@ 
 2017-08-07  John Baldwin  <jhb@FreeBSD.org>
 
+	* fbsd-nat.c (struct fbsd_fork_info): Remove.
+	(fbsd_pending_children): Use std::list.
+	(fbsd_remember_child): Likewise.
+	(fbsd_is_child_pending): Likewise.
+	(fbsd_pending_vfork_done): Use std::forward_list.
+	(fbsd_add_vfork_done): Likewise.
+	(fbsd_is_vfork_done_pending): Likewise.
+	(fbsd_next_vfork_done): Likewise.
+
+2017-08-07  John Baldwin  <jhb@FreeBSD.org>
+
 	* fbsd-nat.c [HAVE_KINFO_GETVMMAP] (struct free_deleter): New.
 	(fbsd_find_memory_regions): Use free_deleter with std::unique_ptr.
 	[!HAVE_KINFO_GETVMMAP] (fbsd_find_memory_regions): Use std::string
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 721e72b85c..c89343a24f 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -41,6 +41,8 @@ 
 #include "elf-bfd.h"
 #include "fbsd-nat.h"
 
+#include <list>
+
 /* Return the name of a file that can be opened to get the symbols for
    the child process identified by PID.  */
 
@@ -712,13 +714,7 @@  fbsd_update_thread_list (struct target_ops *ops)
   sake.  FreeBSD versions newer than 9.1 contain both fixes.
 */
 
-struct fbsd_fork_info
-{
-  struct fbsd_fork_info *next;
-  ptid_t ptid;
-};
-
-static struct fbsd_fork_info *fbsd_pending_children;
+static std::list<ptid_t> fbsd_pending_children;
 
 /* Record a new child process event that is reported before the
    corresponding fork event in the parent.  */
@@ -726,11 +722,7 @@  static struct fbsd_fork_info *fbsd_pending_children;
 static void
 fbsd_remember_child (ptid_t pid)
 {
-  struct fbsd_fork_info *info = XCNEW (struct fbsd_fork_info);
-
-  info->ptid = pid;
-  info->next = fbsd_pending_children;
-  fbsd_pending_children = info;
+  fbsd_pending_children.push_front (pid);
 }
 
 /* Check for a previously-recorded new child process event for PID.
@@ -739,39 +731,26 @@  fbsd_remember_child (ptid_t pid)
 static ptid_t
 fbsd_is_child_pending (pid_t pid)
 {
-  struct fbsd_fork_info *info, *prev;
-  ptid_t ptid;
-
-  prev = NULL;
-  for (info = fbsd_pending_children; info; prev = info, info = info->next)
-    {
-      if (ptid_get_pid (info->ptid) == pid)
-	{
-	  if (prev == NULL)
-	    fbsd_pending_children = info->next;
-	  else
-	    prev->next = info->next;
-	  ptid = info->ptid;
-	  xfree (info);
-	  return ptid;
-	}
-    }
+  for (auto it = fbsd_pending_children.begin ();
+       it != fbsd_pending_children.end (); it++)
+    if (it->pid () == pid)
+      {
+	ptid_t ptid = *it;
+	fbsd_pending_children.erase (it);
+	return ptid;
+      }
   return null_ptid;
 }
 
 #ifndef PTRACE_VFORK
-static struct fbsd_fork_info *fbsd_pending_vfork_done;
+static std::forward_list<ptid_t> fbsd_pending_vfork_done;
 
 /* Record a pending vfork done event.  */
 
 static void
 fbsd_add_vfork_done (ptid_t pid)
 {
-  struct fbsd_fork_info *info = XCNEW (struct fbsd_fork_info);
-
-  info->ptid = pid;
-  info->next = fbsd_pending_vfork_done;
-  fbsd_pending_vfork_done = info;
+  fbsd_pending_vfork_done.push_front (pid);
 }
 
 /* Check for a pending vfork done event for a specific PID.  */
@@ -779,13 +758,10 @@  fbsd_add_vfork_done (ptid_t pid)
 static int
 fbsd_is_vfork_done_pending (pid_t pid)
 {
-  struct fbsd_fork_info *info;
-
-  for (info = fbsd_pending_vfork_done; info != NULL; info = info->next)
-    {
-      if (ptid_get_pid (info->ptid) == pid)
-	return 1;
-    }
+  for (auto it = fbsd_pending_vfork_done.begin ();
+       it != fbsd_pending_vfork_done.end (); it++)
+    if (it->pid () == pid)
+      return 1;
   return 0;
 }
 
@@ -795,15 +771,10 @@  fbsd_is_vfork_done_pending (pid_t pid)
 static ptid_t
 fbsd_next_vfork_done (void)
 {
-  struct fbsd_fork_info *info;
-  ptid_t ptid;
-
-  if (fbsd_pending_vfork_done != NULL)
+  if (!fbsd_pending_vfork_done.empty ())
     {
-      info = fbsd_pending_vfork_done;
-      fbsd_pending_vfork_done = info->next;
-      ptid = info->ptid;
-      xfree (info);
+      ptid_t ptid = fbsd_pending_vfork_done.front ();
+      fbsd_pending_vfork_done.pop_front ();
       return ptid;
     }
   return null_ptid;