[PATCH] gdb: add a file event mask parameter to add_file_handler

[PATCH] gdb: add a file event mask parameter to add_file_handler

[Patrick Monnerat]

Tcl/Tk (used as the Insight GUI) delegates event handling to gdb and
requires file events monitoring to be masked with READ/WRITE/EXCEPTION
flags.

Currently, gdb only uses read events, but an unused and incomplete
provision exists for such flags.

This patch adds an event mask parameter to add_file_handler, allowing
to monitor non-read file events. Its value is always given as ored-in gdb
flags, whether effectively using poll or select. This parameter defaults to
GDB_READABLE | GDB_EXCEPTION, resulting in no change for existing calls.

To unify the semantics between select and poll, POLLRDHUP
(HANGUP-sensitiveness) is also included in read events on platforms
supporting it. This is the only change in this patch that may affect
bare gdb.

This patch benefits to Insight: the latter itself does not use non-read
file events. However these must be supported for Tcl/Tk internals.

It should be noted that gdb does not support write events on the mingw
platform but checks this condition with gdb_assert. This patch does not
change this.

Flags GDB_READABLE, GDB_WRITABLE and GDB_EXCEPTION are now made globally
available through the inclusion of event-loop.h.
---
 gdbsupport/event-loop.cc | 40 ++++++++++++++++++++++------------------
 gdbsupport/event-loop.h  | 11 +++++++++--
 2 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
index 941885529f1..b4797ae04b8 100644
--- a/gdbsupport/event-loop.cc
+++ b/gdbsupport/event-loop.cc
@@ -28,6 +28,11 @@
 #elif defined (HAVE_SYS_POLL_H)
 #include <sys/poll.h>
 #endif
+#if defined(POLLRDHUP)
+#define POLL_HANGUP POLLRDHUP
+#else
+#define POLL_HANGUP 0
+#endif
 #endif
 
 #include <sys/types.h>
@@ -40,13 +45,6 @@
 
 debug_event_loop_kind debug_event_loop;
 
-/* Tell create_file_handler what events we are interested in.
-   This is used by the select version of the event loop.  */
-
-#define GDB_READABLE	(1<<1)
-#define GDB_WRITABLE	(1<<2)
-#define GDB_EXCEPTION	(1<<3)
-
 /* Information about each file descriptor we register with the event
    loop.  */
 
@@ -269,8 +267,11 @@ gdb_do_one_event (int mstimeout)
 
 void
 add_file_handler (int fd, handler_func *proc, gdb_client_data client_data,
-		  std::string &&name, bool is_ui)
+		  std::string &&name, bool is_ui, int mask)
 {
+  if (fd < 0 || (mask & (GDB_READABLE | GDB_WRITABLE | GDB_EXCEPTION)) == 0)
+    return;
+
 #ifdef HAVE_POLL
   if (use_poll)
     {
@@ -282,25 +283,28 @@ add_file_handler (int fd, handler_func *proc, gdb_client_data client_data,
 	 On m68k-motorola-sysv, tty's are not stream-based and not
 	 `poll'able.  */
       fds.fd = fd;
-      fds.events = POLLIN;
+      fds.events = 0;
+      if (mask & GDB_READABLE)
+        fds.events |= POLLIN | POLL_HANGUP;
+      if (mask & GDB_WRITABLE)
+        fds.events |= POLLOUT;
+      if (mask & GDB_EXCEPTION)
+        fds.events |= POLLPRI;
       if (poll (&fds, 1, 0) == 1 && (fds.revents & POLLNVAL))
 	use_poll = false;
+      else
+	create_file_handler (fd, fds.events, proc, client_data,
+			     std::move (name), is_ui);
     }
-  if (use_poll)
-    {
-      create_file_handler (fd, POLLIN, proc, client_data, std::move (name),
-			   is_ui);
-    }
-  else
+  if (!use_poll)
 #endif /* HAVE_POLL */
-    create_file_handler (fd, GDB_READABLE | GDB_EXCEPTION,
-			 proc, client_data, std::move (name), is_ui);
+    create_file_handler (fd, mask, proc, client_data, std::move (name), is_ui);
 }
 
 /* Helper for add_file_handler.
 
    For the poll case, MASK is a combination (OR) of POLLIN,
-   POLLRDNORM, POLLRDBAND, POLLPRI, POLLOUT, POLLWRNORM, POLLWRBAND:
+   POLLRDNORM, POLLRDBAND, POLLPRI, POLLOUT, POLLWRNORM, POLLWRBAND, POLLRDHUP:
    these are the events we are interested in.  If any of them occurs,
    proc should be called.
 
diff --git a/gdbsupport/event-loop.h b/gdbsupport/event-loop.h
index c82493e9bdf..8c1f831af27 100644
--- a/gdbsupport/event-loop.h
+++ b/gdbsupport/event-loop.h
@@ -70,11 +70,17 @@
 
    Corollary tasks are the creation and deletion of event sources.  */
 
+/* Tell add_file_handler what events we are interested in. */
+
+#define GDB_READABLE	(1<<1)
+#define GDB_WRITABLE	(1<<2)
+#define GDB_EXCEPTION	(1<<3)
+
 typedef void *gdb_client_data;
 typedef void (handler_func) (int, gdb_client_data);
 typedef void (timer_handler_func) (gdb_client_data);
 
-/* Exported functions from event-loop.c */
+/* Exported functions from event-loop.cc */
 
 extern int gdb_do_one_event (int mstimeout = -1);
 extern void delete_file_handler (int fd);
@@ -90,7 +96,8 @@ extern void delete_file_handler (int fd);
 
 extern void add_file_handler (int fd, handler_func *proc,
 			      gdb_client_data client_data,
-			      std::string &&name, bool is_ui = false);
+			      std::string &&name, bool is_ui = false,
+			      int mask = GDB_READABLE | GDB_EXCEPTION);
 
 extern int create_timer (int milliseconds, 
 			 timer_handler_func *proc, 
-- 
2.37.1

Re: [PATCH] gdb: add a file event mask parameter to add_file_handler

[Tom Tromey]

>>>>> "Patrick" == Patrick Monnerat via Gdb-patches <gdb-patches@sourceware.org> writes:

Thanks for the patch.

Patrick> +#if defined(POLLRDHUP)

Space before the '('.

Patrick>  void
Patrick>  add_file_handler (int fd, handler_func *proc, gdb_client_data client_data,
Patrick> -		  std::string &&name, bool is_ui)
Patrick> +		  std::string &&name, bool is_ui, int mask)
Patrick>  {
Patrick> +  if (fd < 0 || (mask & (GDB_READABLE | GDB_WRITABLE | GDB_EXCEPTION)) == 0)
Patrick> +    return;

Is there ever a situation where this can happen and it is not a bug in
the caller?  I am wondering if this ought to be an assert instead.

Patrick> +/* Tell add_file_handler what events we are interested in. */
Patrick> +
Patrick> +#define GDB_READABLE	(1<<1)
Patrick> +#define GDB_WRITABLE	(1<<2)
Patrick> +#define GDB_EXCEPTION	(1<<3)

I tend to think it would be better to use enum_flags here.

Tom

Re: [PATCH] gdb: add a file event mask parameter to add_file_handler

[Patrick Monnerat]

On 8/23/22 21:09, Tom Tromey wrote:
>>>>>> "Patrick" == Patrick Monnerat via Gdb-patches <gdb-patches@sourceware.org> writes:
> Thanks for the patch.
>
> Patrick> +#if defined(POLLRDHUP)
>
> Space before the '('.
Sure: my bad !
> Patrick>  void
> Patrick>  add_file_handler (int fd, handler_func *proc, gdb_client_data client_data,
> Patrick> -		  std::string &&name, bool is_ui)
> Patrick> +		  std::string &&name, bool is_ui, int mask)
> Patrick>  {
> Patrick> +  if (fd < 0 || (mask & (GDB_READABLE | GDB_WRITABLE | GDB_EXCEPTION)) == 0)
> Patrick> +    return;
>
> Is there ever a situation where this can happen and it is not a bug in
> the caller?  I am wondering if this ought to be an assert instead.
Yes. In Insight, command input from stdin is disabled by setting 
main_ui->input_fd to -1. A later call to register_file_handler from 
async_enable_stdin causes add_file_handler to be called with fd = -1. 
Maybe I should add a comment for this.
>
> Patrick> +/* Tell add_file_handler what events we are interested in. */
> Patrick> +
> Patrick> +#define GDB_READABLE	(1<<1)
> Patrick> +#define GDB_WRITABLE	(1<<2)
> Patrick> +#define GDB_EXCEPTION	(1<<3)
>
> I tend to think it would be better to use enum_flags here.

I thought so too and did try to use enum_flags! Unfortunately the same 
int field (file_handler::mask) stores either these bits in case of 
select use, or a poll mask as defined by the system when poll is used.

We then have a type conflict: it can be resolved but is rather 
cumbersome (a simple cast would be awful). Your opinion?


Thanks for the review.

Patrick

Re: [PATCH] gdb: add a file event mask parameter to add_file_handler

[Tom Tromey]

>> Is there ever a situation where this can happen and it is not a bug in
>> the caller?  I am wondering if this ought to be an assert instead.

Patrick> Yes. In Insight, command input from stdin is disabled by setting
main_ui-> input_fd to -1. A later call to register_file_handler from 
Patrick> async_enable_stdin causes add_file_handler to be called with fd =
Patrick> -1. Maybe I should add a comment for this.

I took this same approach in my DAP implementation, which is why I
changed gdb to always call the 'ui' methods to manipulate the event
handlers.  In my patch I then change these methods to check for the -1
case.

I can submit this bit separately next week.
Or you can do it, it's trivial.

Patrick> I thought so too and did try to use enum_flags! Unfortunately the same
Patrick> int field (file_handler::mask) stores either these bits in case of 
Patrick> select use, or a poll mask as defined by the system when poll is used.

Ok, I see.

Tom

Re: [PATCH] gdb: add a file event mask parameter to add_file_handler

[Tom Tromey]

Patrick> Yes. In Insight, command input from stdin is disabled by setting
Patrick> main_ui->input_fd to -1. A later call to register_file_handler from 
Patrick> async_enable_stdin causes add_file_handler to be called with fd =
Patrick> -1. Maybe I should add a comment for this.

Tom> I took this same approach in my DAP implementation, which is why I
Tom> changed gdb to always call the 'ui' methods to manipulate the event
Tom> handlers.  In my patch I then change these methods to check for the -1
Tom> case.

Tom> I can submit this bit separately next week.
Tom> Or you can do it, it's trivial.

What do you think of this?

Tom

commit 73d104a6fc246f0cd7d42a4ed1c009304fb55327
Author: Tom Tromey <tromey@adacore.com>
Date:   Tue Aug 30 11:30:13 2022 -0600

    Let ui::input_fd be -1
    
    This changes gdb so that, if ui::input_fd is set to -1, then it will
    not be registered with the event loop.  This is useful for the DAP
    support code I wrote, but as it turns out to also be useful to
    Insight, it seems best to check it in separately.

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 02b3786320f..4547d614522 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -533,8 +533,9 @@ stdin_event_handler (int error, gdb_client_data client_data)
 void
 ui::register_file_handler ()
 {
-  add_file_handler (input_fd, stdin_event_handler, this,
-		    string_printf ("ui-%d", num), true);
+  if (input_fd != -1)
+    add_file_handler (input_fd, stdin_event_handler, this,
+		      string_printf ("ui-%d", num), true);
 }
 
 /* See top.h.  */
@@ -542,7 +543,8 @@ ui::register_file_handler ()
 void
 ui::unregister_file_handler ()
 {
-  delete_file_handler (input_fd);
+  if (input_fd != -1)
+    delete_file_handler (input_fd);
 }
 
 /* Re-enable stdin after the end of an execution command in
diff --git a/gdb/top.h b/gdb/top.h
index 5c1db84b2ce..9ea07262c39 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -118,7 +118,8 @@ struct ui
   FILE *errstream;
 
   /* The file descriptor for the input stream, so that we can register
-     it with the event loop.  */
+     it with the event loop.  This can be set to -1 to prevent this
+     registration.  */
   int input_fd;
 
   /* Whether ISATTY returns true on input_fd.  Cached here because

Re: [PATCH] gdb: add a file event mask parameter to add_file_handler

[Patrick Monnerat]

On 8/30/22 19:34, Tom Tromey wrote:
> Patrick> Yes. In Insight, command input from stdin is disabled by setting
> Patrick> main_ui->input_fd to -1. A later call to register_file_handler from
> Patrick> async_enable_stdin causes add_file_handler to be called with fd =
> Patrick> -1. Maybe I should add a comment for this.
>
> Tom> I took this same approach in my DAP implementation, which is why I
> Tom> changed gdb to always call the 'ui' methods to manipulate the event
> Tom> handlers.  In my patch I then change these methods to check for the -1
> Tom> case.
>
> Tom> I can submit this bit separately next week.
> Tom> Or you can do it, it's trivial.
>
> What do you think of this?

This is great! I applied your changes to Insight and dropped the fd<0 
test in add_file_handler with total success.

I'll soon submit version 2 of my patch with the fd<0 test removed, and 
an updated test for mask==0 (this case should call delete_file_handler).

This would have to be pushed after your patch, of course!

Thanks a lot.

Patrick

> Tom
>
> commit 73d104a6fc246f0cd7d42a4ed1c009304fb55327
> Author: Tom Tromey <tromey@adacore.com>
> Date:   Tue Aug 30 11:30:13 2022 -0600
>
>      Let ui::input_fd be -1
>      
>      This changes gdb so that, if ui::input_fd is set to -1, then it will
>      not be registered with the event loop.  This is useful for the DAP
>      support code I wrote, but as it turns out to also be useful to
>      Insight, it seems best to check it in separately.
>
> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index 02b3786320f..4547d614522 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -533,8 +533,9 @@ stdin_event_handler (int error, gdb_client_data client_data)
>   void
>   ui::register_file_handler ()
>   {
> -  add_file_handler (input_fd, stdin_event_handler, this,
> -		    string_printf ("ui-%d", num), true);
> +  if (input_fd != -1)
> +    add_file_handler (input_fd, stdin_event_handler, this,
> +		      string_printf ("ui-%d", num), true);
>   }
>   
>   /* See top.h.  */
> @@ -542,7 +543,8 @@ ui::register_file_handler ()
>   void
>   ui::unregister_file_handler ()
>   {
> -  delete_file_handler (input_fd);
> +  if (input_fd != -1)
> +    delete_file_handler (input_fd);
>   }
>   
>   /* Re-enable stdin after the end of an execution command in
> diff --git a/gdb/top.h b/gdb/top.h
> index 5c1db84b2ce..9ea07262c39 100644
> --- a/gdb/top.h
> +++ b/gdb/top.h
> @@ -118,7 +118,8 @@ struct ui
>     FILE *errstream;
>   
>     /* The file descriptor for the input stream, so that we can register
> -     it with the event loop.  */
> +     it with the event loop.  This can be set to -1 to prevent this
> +     registration.  */
>     int input_fd;
>   
>     /* Whether ISATTY returns true on input_fd.  Cached here because

Re: [PATCH] gdb: add a file event mask parameter to add_file_handler

[Tom Tromey]

Patrick> This is great! I applied your changes to Insight and dropped the fd<0
Patrick> test in add_file_handler with total success.

Patrick> I'll soon submit version 2 of my patch with the fd<0 test removed, and
Patrick> an updated test for mask==0 (this case should call
Patrick> delete_file_handler).

Patrick> This would have to be pushed after your patch, of course!

Patrick> Thanks a lot.

No problem, and thanks for trying it out.
I'm going to check it in now.

Tom