[RFC,2/7] gdb linux-nat: Convert linux_nat_event_pipe to the event_pipe class.

Message ID 20210607170932.3954-3-jhb@FreeBSD.org
State New
Headers show
Series
  • FreeBSD target async mode and related refactoring
Related show

Commit Message

John Baldwin June 7, 2021, 5:09 p.m.
gdb/ChangeLog:

	* linux-nat.c: Include gdbsupport/event-pipe.h.
	(linux_nat_event_pipe): Convert to an instance of event_pipe.
	(linux_is_async_p, async_file_flush, async_file_mark): Implement
	as methods of event_pipe.
	(sigchld_handler, linux_async_pipe)
	(linux_nat_target::async_wait_fd, linux_nat_target::async): Update
	to use event_pipe methods.
---
 gdb/ChangeLog   | 10 ++++++++++
 gdb/linux-nat.c | 48 +++++++++++-------------------------------------
 2 files changed, 21 insertions(+), 37 deletions(-)

-- 
2.31.1

Comments

Pedro Alves June 27, 2021, 4:12 p.m. | #1
On 2021-06-07 6:09 p.m., John Baldwin wrote:

> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c

> index 34a2aee41d..150fdd43f8 100644

> --- a/gdb/linux-nat.c

> +++ b/gdb/linux-nat.c

> @@ -48,6 +48,7 @@

>  #include <fcntl.h>		/* for O_RDONLY */

>  #include "inf-loop.h"

>  #include "gdbsupport/event-loop.h"

> +#include "gdbsupport/event-pipe.h"

>  #include "event-top.h"

>  #include <pwd.h>

>  #include <sys/types.h>

> @@ -219,24 +220,17 @@ static int report_thread_events;

>  

>  /* The read/write ends of the pipe registered as waitable file in the

>     event loop.  */


Should we update the comment above?  (Same in gdbserver.)

> -static int linux_nat_event_pipe[2] = { -1, -1 };

> +static event_pipe linux_nat_event_pipe;



>  /* Put something (anything, doesn't matter what, or how much) in event

> @@ -246,21 +240,7 @@ async_file_flush (void)

>  static void

>  async_file_mark (void)


I guess the intro comment could use a little update to talk about marking
the event pipe instead of talking about writing bytes?
John Baldwin July 13, 2021, 1:51 p.m. | #2
On 6/27/21 9:12 AM, Pedro Alves wrote:
> On 2021-06-07 6:09 p.m., John Baldwin wrote:

> 

>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c

>> index 34a2aee41d..150fdd43f8 100644

>> --- a/gdb/linux-nat.c

>> +++ b/gdb/linux-nat.c

>> @@ -48,6 +48,7 @@

>>   #include <fcntl.h>		/* for O_RDONLY */

>>   #include "inf-loop.h"

>>   #include "gdbsupport/event-loop.h"

>> +#include "gdbsupport/event-pipe.h"

>>   #include "event-top.h"

>>   #include <pwd.h>

>>   #include <sys/types.h>

>> @@ -219,24 +220,17 @@ static int report_thread_events;

>>   

>>   /* The read/write ends of the pipe registered as waitable file in the

>>      event loop.  */

> 

> Should we update the comment above?  (Same in gdbserver.)


Yes, fixed.

>> -static int linux_nat_event_pipe[2] = { -1, -1 };

>> +static event_pipe linux_nat_event_pipe;

> 

> 

>>   /* Put something (anything, doesn't matter what, or how much) in event

>> @@ -246,21 +240,7 @@ async_file_flush (void)

>>   static void

>>   async_file_mark (void)

> 

> I guess the intro comment could use a little update to talk about marking

> the event pipe instead of talking about writing bytes?


Yes, I've fixed that too, thanks.

-- 
John Baldwin

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e646fd53e6..cb301e1e81 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@ 
+2021-06-04  John Baldwin  <jhb@FreeBSD.org>
+
+	* linux-nat.c: Include gdbsupport/event-pipe.h.
+	(linux_nat_event_pipe): Convert to an instance of event_pipe.
+	(linux_is_async_p, async_file_flush, async_file_mark): Implement
+	as methods of event_pipe.
+	(sigchld_handler, linux_async_pipe)
+	(linux_nat_target::async_wait_fd, linux_nat_target::async): Update
+	to use event_pipe methods.
+
 2021-06-03  John Baldwin  <jhb@FreeBSD.org>
 
 	* fbsd-tdep.c (FBSD_SI_USER, FBSD_SI_QUEUE, FBSD_SI_TIMER)
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 34a2aee41d..150fdd43f8 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -48,6 +48,7 @@ 
 #include <fcntl.h>		/* for O_RDONLY */
 #include "inf-loop.h"
 #include "gdbsupport/event-loop.h"
+#include "gdbsupport/event-pipe.h"
 #include "event-top.h"
 #include <pwd.h>
 #include <sys/types.h>
@@ -219,24 +220,17 @@  static int report_thread_events;
 
 /* The read/write ends of the pipe registered as waitable file in the
    event loop.  */
-static int linux_nat_event_pipe[2] = { -1, -1 };
+static event_pipe linux_nat_event_pipe;
 
 /* True if we're currently in async mode.  */
-#define linux_is_async_p() (linux_nat_event_pipe[0] != -1)
+#define linux_is_async_p() (linux_nat_event_pipe.active ())
 
 /* Flush the event pipe.  */
 
 static void
 async_file_flush (void)
 {
-  int ret;
-  char buf;
-
-  do
-    {
-      ret = read (linux_nat_event_pipe[0], &buf, 1);
-    }
-  while (ret >= 0 || (ret == -1 && errno == EINTR));
+  linux_nat_event_pipe.flush ();
 }
 
 /* Put something (anything, doesn't matter what, or how much) in event
@@ -246,21 +240,7 @@  async_file_flush (void)
 static void
 async_file_mark (void)
 {
-  int ret;
-
-  /* It doesn't really matter what the pipe contains, as long we end
-     up with something in it.  Might as well flush the previous
-     left-overs.  */
-  async_file_flush ();
-
-  do
-    {
-      ret = write (linux_nat_event_pipe[1], "+", 1);
-    }
-  while (ret == -1 && errno == EINTR);
-
-  /* Ignore EAGAIN.  If the pipe is full, the event loop will already
-     be awakened anyway.  */
+  linux_nat_event_pipe.mark ();
 }
 
 static int kill_lwp (int lwpid, int signo);
@@ -4041,7 +4021,7 @@  sigchld_handler (int signo)
     gdb_stdlog->write_async_safe ("sigchld\n", sizeof ("sigchld\n") - 1);
 
   if (signo == SIGCHLD
-      && linux_nat_event_pipe[0] != -1)
+      && linux_nat_event_pipe.active ())
     async_file_mark (); /* Let the event loop know that there are
 			   events to handle.  */
 
@@ -4073,19 +4053,13 @@  linux_async_pipe (int enable)
 
       if (enable)
 	{
-	  if (gdb_pipe_cloexec (linux_nat_event_pipe) == -1)
+	  if (!linux_nat_event_pipe.open ())
 	    internal_error (__FILE__, __LINE__,
 			    "creating event pipe failed.");
-
-	  fcntl (linux_nat_event_pipe[0], F_SETFL, O_NONBLOCK);
-	  fcntl (linux_nat_event_pipe[1], F_SETFL, O_NONBLOCK);
 	}
       else
 	{
-	  close (linux_nat_event_pipe[0]);
-	  close (linux_nat_event_pipe[1]);
-	  linux_nat_event_pipe[0] = -1;
-	  linux_nat_event_pipe[1] = -1;
+	  linux_nat_event_pipe.close ();
 	}
 
       restore_child_signals_mask (&prev_mask);
@@ -4097,7 +4071,7 @@  linux_async_pipe (int enable)
 int
 linux_nat_target::async_wait_fd ()
 {
-  return linux_nat_event_pipe[0];
+  return linux_nat_event_pipe.event_fd ();
 }
 
 /* target_async implementation.  */
@@ -4109,7 +4083,7 @@  linux_nat_target::async (int enable)
     {
       if (!linux_async_pipe (1))
 	{
-	  add_file_handler (linux_nat_event_pipe[0],
+	  add_file_handler (linux_nat_event_pipe.event_fd (),
 			    handle_target_event, NULL,
 			    "linux-nat");
 	  /* There may be pending events to handle.  Tell the event loop
@@ -4119,7 +4093,7 @@  linux_nat_target::async (int enable)
     }
   else
     {
-      delete_file_handler (linux_nat_event_pipe[0]);
+      delete_file_handler (linux_nat_event_pipe.event_fd ());
       linux_async_pipe (0);
     }
   return;