[PATCH] gdbsupport: Remove some unnecessary ifdef HAVE_POLL judgments

[PATCH] gdbsupport: Remove some unnecessary ifdef HAVE_POLL judgments

[Youling Tang]

By searching event-loop.cc we found that there are only the following 2
places to assign values to use_poll,
$ grep -nr use_poll gdbsupport/event-loop.cc
  88:static unsigned char use_poll = USE_POLL;
  263:	use_poll = 0;

This USE_POLL is defined as follows,
 #ifdef HAVE_POLL
 #define USE_POLL 1
 #else
 #define USE_POLL 0
 #endif

So use_poll may be 1 only if HAVE_POLL is defined. Removed "ifdef
HAVE_POLL" judgment in use_poll control block.

Signed-off-by: Youling Tang <tangyouling@loongson.cn>
---
 gdbsupport/event-loop.cc | 61 +++++-----------------------------------
 1 file changed, 7 insertions(+), 54 deletions(-)

diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
index 385b45b2de1..0bdaa4e7d35 100644
--- a/gdbsupport/event-loop.cc
+++ b/gdbsupport/event-loop.cc
@@ -248,13 +248,10 @@ void
 add_file_handler (int fd, handler_func *proc, gdb_client_data client_data,
 		  std::string &&name, bool is_ui)
 {
-#ifdef HAVE_POLL
-  struct pollfd fds;
-#endif
-
   if (use_poll)
     {
-#ifdef HAVE_POLL
+      struct pollfd fds;
+
       /* Check to see if poll () is usable.  If not, we'll switch to
 	 use select.  This can happen on systems like
 	 m68k-motorola-sys, `poll' cannot be used to wait for `stdin'.
@@ -264,21 +261,10 @@ add_file_handler (int fd, handler_func *proc, gdb_client_data client_data,
       fds.events = POLLIN;
       if (poll (&fds, 1, 0) == 1 && (fds.revents & POLLNVAL))
 	use_poll = 0;
-#else
-      internal_error (__FILE__, __LINE__,
-		      _("use_poll without HAVE_POLL"));
-#endif /* HAVE_POLL */
     }
   if (use_poll)
-    {
-#ifdef HAVE_POLL
-      create_file_handler (fd, POLLIN, proc, client_data, std::move (name),
+    create_file_handler (fd, POLLIN, proc, client_data, std::move (name),
 			   is_ui);
-#else
-      internal_error (__FILE__, __LINE__,
-		      _("use_poll without HAVE_POLL"));
-#endif
-    }
   else
     create_file_handler (fd, GDB_READABLE | GDB_EXCEPTION,
 			 proc, client_data, std::move (name), is_ui);
@@ -323,7 +309,6 @@ create_file_handler (int fd, int mask, handler_func * proc,
 
       if (use_poll)
 	{
-#ifdef HAVE_POLL
 	  gdb_notifier.num_fds++;
 	  if (gdb_notifier.poll_fds)
 	    gdb_notifier.poll_fds =
@@ -336,10 +321,6 @@ create_file_handler (int fd, int mask, handler_func * proc,
 	  (gdb_notifier.poll_fds + gdb_notifier.num_fds - 1)->fd = fd;
 	  (gdb_notifier.poll_fds + gdb_notifier.num_fds - 1)->events = mask;
 	  (gdb_notifier.poll_fds + gdb_notifier.num_fds - 1)->revents = 0;
-#else
-	  internal_error (__FILE__, __LINE__,
-			  _("use_poll without HAVE_POLL"));
-#endif /* HAVE_POLL */
 	}
       else
 	{
@@ -402,10 +383,6 @@ delete_file_handler (int fd)
 {
   file_handler *file_ptr, *prev_ptr = NULL;
   int i;
-#ifdef HAVE_POLL
-  int j;
-  struct pollfd *new_poll_fds;
-#endif
 
   /* Find the entry for the given file.  */
 
@@ -421,7 +398,9 @@ delete_file_handler (int fd)
 
   if (use_poll)
     {
-#ifdef HAVE_POLL
+      int j;
+      struct pollfd *new_poll_fds;
+
       /* Create a new poll_fds array by copying every fd's information
 	 but the one we want to get rid of.  */
 
@@ -442,10 +421,6 @@ delete_file_handler (int fd)
       xfree (gdb_notifier.poll_fds);
       gdb_notifier.poll_fds = new_poll_fds;
       gdb_notifier.num_fds--;
-#else
-      internal_error (__FILE__, __LINE__,
-		      _("use_poll without HAVE_POLL"));
-#endif /* HAVE_POLL */
     }
   else
     {
@@ -510,9 +485,6 @@ static void
 handle_file_event (file_handler *file_ptr, int ready_mask)
 {
   int mask;
-#ifdef HAVE_POLL
-  int error_mask;
-#endif
 
     {
 	{
@@ -528,7 +500,7 @@ handle_file_event (file_handler *file_ptr, int ready_mask)
 
 	  if (use_poll)
 	    {
-#ifdef HAVE_POLL
+	      int error_mask;
 	      /* POLLHUP means EOF, but can be combined with POLLIN to
 		 signal more data to read.  */
 	      error_mask = POLLHUP | POLLERR | POLLNVAL;
@@ -547,10 +519,6 @@ handle_file_event (file_handler *file_ptr, int ready_mask)
 		}
 	      else
 		file_ptr->error = 0;
-#else
-	      internal_error (__FILE__, __LINE__,
-			      _("use_poll without HAVE_POLL"));
-#endif /* HAVE_POLL */
 	    }
 	  else
 	    {
@@ -601,7 +569,6 @@ gdb_wait_for_event (int block)
 
   if (use_poll)
     {
-#ifdef HAVE_POLL
       int timeout;
 
       if (block)
@@ -616,10 +583,6 @@ gdb_wait_for_event (int block)
 	 signal.  */
       if (num_found == -1 && errno != EINTR)
 	perror_with_name (("poll"));
-#else
-      internal_error (__FILE__, __LINE__,
-		      _("use_poll without HAVE_POLL"));
-#endif /* HAVE_POLL */
     }
   else
     {
@@ -672,7 +635,6 @@ gdb_wait_for_event (int block)
      may change between invocations, but this is good enough.  */
   if (use_poll)
     {
-#ifdef HAVE_POLL
       int i;
       int mask;
 
@@ -699,10 +661,6 @@ gdb_wait_for_event (int block)
       mask = (gdb_notifier.poll_fds + i)->revents;
       handle_file_event (file_ptr, mask);
       return 1;
-#else
-      internal_error (__FILE__, __LINE__,
-		      _("use_poll without HAVE_POLL"));
-#endif /* HAVE_POLL */
     }
   else
     {
@@ -858,12 +816,7 @@ update_wait_timeout (void)
       /* Update the timeout for select/ poll.  */
       if (use_poll)
 	{
-#ifdef HAVE_POLL
 	  gdb_notifier.poll_timeout = timeout.tv_sec * 1000;
-#else
-	  internal_error (__FILE__, __LINE__,
-			  _("use_poll without HAVE_POLL"));
-#endif /* HAVE_POLL */
 	}
       else
 	{
-- 
2.20.1

Re: [PATCH] gdbsupport: Remove some unnecessary ifdef HAVE_POLL judgments

[Pedro Alves]

On 2022-05-13 08:49, Youling Tang wrote:
> By searching event-loop.cc we found that there are only the following 2
> places to assign values to use_poll,
> $ grep -nr use_poll gdbsupport/event-loop.cc
>   88:static unsigned char use_poll = USE_POLL;
>   263:	use_poll = 0;
> 
> This USE_POLL is defined as follows,
>  #ifdef HAVE_POLL
>  #define USE_POLL 1
>  #else
>  #define USE_POLL 0
>  #endif
> 
> So use_poll may be 1 only if HAVE_POLL is defined. Removed "ifdef
> HAVE_POLL" judgment in use_poll control block.
> 
> Signed-off-by: Youling Tang <tangyouling@loongson.cn>

Maybe I'm missing something, but ISTM this can't possible work on hosts that don'
 have poll at all.  Like e.g., mingw:

 $ grep POLL *
 config.h:/* #undef HAVE_POLL */
 config.h:/* #undef HAVE_POLL_H */
 config.h:/* #undef HAVE_SYS_POLL_H */

Surely they'll fail to compile after the patch?

Re: [PATCH] gdbsupport: Remove some unnecessary ifdef HAVE_POLL judgments

[Youling Tang]

Hi, Pedro

On 05/13/2022 05:28 PM, Pedro Alves wrote:
> On 2022-05-13 08:49, Youling Tang wrote:
>> By searching event-loop.cc we found that there are only the following 2
>> places to assign values to use_poll,
>> $ grep -nr use_poll gdbsupport/event-loop.cc
>>    88:static unsigned char use_poll = USE_POLL;
>>    263:	use_poll = 0;
>>
>> This USE_POLL is defined as follows,
>>   #ifdef HAVE_POLL
>>   #define USE_POLL 1
>>   #else
>>   #define USE_POLL 0
>>   #endif
>>
>> So use_poll may be 1 only if HAVE_POLL is defined. Removed "ifdef
>> HAVE_POLL" judgment in use_poll control block.
>>
>> Signed-off-by: Youling Tang <tangyouling@loongson.cn>
> Maybe I'm missing something, but ISTM this can't possible work on hosts that don'
>   have poll at all.  Like e.g., mingw:
>
>   $ grep POLL *
>   config.h:/* #undef HAVE_POLL */
>   config.h:/* #undef HAVE_POLL_H */
>   config.h:/* #undef HAVE_SYS_POLL_H */
>
> Surely they'll fail to compile after the patch?
You are right my oversight.

But we can remove these internal_error handling, similar to the 
following modification:

  add_file_handler (int fd, handler_func *proc, gdb_client_data client_data,
                   std::string &&name, bool is_ui)
  {
-#ifdef HAVE_POLL
-  struct pollfd fds;
-#endif
-
    if (use_poll)
      {
  #ifdef HAVE_POLL
+      struct pollfd fds;
+
        /* Check to see if poll () is usable.  If not, we'll switch to
          use select.  This can happen on systems like
          m68k-motorola-sys, `poll' cannot be used to wait for `stdin'.
@@ -264,19 +263,13 @@ add_file_handler (int fd, handler_func *proc, 
gdb_client_data client_data,
        fds.events = POLLIN;
        if (poll (&fds, 1, 0) == 1 && (fds.revents & POLLNVAL))
         use_poll = 0;
-#else
-      internal_error (__FILE__, __LINE__,
-                     _("use_poll without HAVE_POLL"));
-#endif /* HAVE_POLL */
+#endif

Thanks,
Youling.

Re: [PATCH] gdbsupport: Remove some unnecessary ifdef HAVE_POLL judgments

[Pedro Alves]

On 2022-05-13 11:09, Youling Tang wrote:
> Hi, Pedro
> 
> On 05/13/2022 05:28 PM, Pedro Alves wrote:
>> On 2022-05-13 08:49, Youling Tang wrote:
>>> By searching event-loop.cc we found that there are only the following 2
>>> places to assign values to use_poll,
>>> $ grep -nr use_poll gdbsupport/event-loop.cc
>>>    88:static unsigned char use_poll = USE_POLL;
>>>    263:    use_poll = 0;
>>>
>>> This USE_POLL is defined as follows,
>>>   #ifdef HAVE_POLL
>>>   #define USE_POLL 1
>>>   #else
>>>   #define USE_POLL 0
>>>   #endif
>>>
>>> So use_poll may be 1 only if HAVE_POLL is defined. Removed "ifdef
>>> HAVE_POLL" judgment in use_poll control block.
>>>
>>> Signed-off-by: Youling Tang <tangyouling@loongson.cn>
>> Maybe I'm missing something, but ISTM this can't possible work on hosts that don'
>>   have poll at all.  Like e.g., mingw:
>>
>>   $ grep POLL *
>>   config.h:/* #undef HAVE_POLL */
>>   config.h:/* #undef HAVE_POLL_H */
>>   config.h:/* #undef HAVE_SYS_POLL_H */
>>
>> Surely they'll fail to compile after the patch?
> You are right my oversight.
> 
> But we can remove these internal_error handling, similar to the following modification:

We can remove _every_ internal_error call in GDB, since by design, they are not supposed
to ever execute, unless we have something wrong.  This is the scenario here, guarding against
someone mistakenly doing that change that would result in use_poll as 1 on a host that doesn't
support poll.  Why change it?

Re: [PATCH] gdbsupport: Remove some unnecessary ifdef HAVE_POLL judgments

[Youling Tang]

Hi, Pedro
On 05/13/2022 06:21 PM, Pedro Alves wrote:
> On 2022-05-13 11:09, Youling Tang wrote:
>> Hi, Pedro
>>
>> On 05/13/2022 05:28 PM, Pedro Alves wrote:
>>> On 2022-05-13 08:49, Youling Tang wrote:
>>>> By searching event-loop.cc we found that there are only the following 2
>>>> places to assign values to use_poll,
>>>> $ grep -nr use_poll gdbsupport/event-loop.cc
>>>>     88:static unsigned char use_poll = USE_POLL;
>>>>     263:    use_poll = 0;
>>>>
>>>> This USE_POLL is defined as follows,
>>>>    #ifdef HAVE_POLL
>>>>    #define USE_POLL 1
>>>>    #else
>>>>    #define USE_POLL 0
>>>>    #endif
>>>>
>>>> So use_poll may be 1 only if HAVE_POLL is defined. Removed "ifdef
>>>> HAVE_POLL" judgment in use_poll control block.
>>>>
>>>> Signed-off-by: Youling Tang <tangyouling@loongson.cn>
>>> Maybe I'm missing something, but ISTM this can't possible work on hosts that don'
>>>    have poll at all.  Like e.g., mingw:
>>>
>>>    $ grep POLL *
>>>    config.h:/* #undef HAVE_POLL */
>>>    config.h:/* #undef HAVE_POLL_H */
>>>    config.h:/* #undef HAVE_SYS_POLL_H */
>>>
>>> Surely they'll fail to compile after the patch?
>> You are right my oversight.
>>
>> But we can remove these internal_error handling, similar to the following modification:
> We can remove _every_ internal_error call in GDB, since by design, they are not supposed
> to ever execute, unless we have something wrong.  This is the scenario here, guarding against
> someone mistakenly doing that change that would result in use_poll as 1 on a host that doesn't
> support poll.  Why change it?
Thank you for letting me know the design background, this change is not 
needed.

Thanks,
Youling

[PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths

[Pedro Alves]

On 2022-05-14 02:14, Youling Tang wrote:
> Hi, Pedro
> On 05/13/2022 06:21 PM, Pedro Alves wrote:
>> On 2022-05-13 11:09, Youling Tang wrote:
>>> Hi, Pedro
>>>
>>> On 05/13/2022 05:28 PM, Pedro Alves wrote:
>>>> On 2022-05-13 08:49, Youling Tang wrote:
>>>>> By searching event-loop.cc we found that there are only the following 2
>>>>> places to assign values to use_poll,
>>>>> $ grep -nr use_poll gdbsupport/event-loop.cc
>>>>>     88:static unsigned char use_poll = USE_POLL;
>>>>>     263:    use_poll = 0;
>>>>>
>>>>> This USE_POLL is defined as follows,
>>>>>    #ifdef HAVE_POLL
>>>>>    #define USE_POLL 1
>>>>>    #else
>>>>>    #define USE_POLL 0
>>>>>    #endif
>>>>>
>>>>> So use_poll may be 1 only if HAVE_POLL is defined. Removed "ifdef
>>>>> HAVE_POLL" judgment in use_poll control block.
>>>>>
>>>>> Signed-off-by: Youling Tang <tangyouling@loongson.cn>
>>>> Maybe I'm missing something, but ISTM this can't possible work on hosts that don'
>>>>    have poll at all.  Like e.g., mingw:
>>>>
>>>>    $ grep POLL *
>>>>    config.h:/* #undef HAVE_POLL */
>>>>    config.h:/* #undef HAVE_POLL_H */
>>>>    config.h:/* #undef HAVE_SYS_POLL_H */
>>>>
>>>> Surely they'll fail to compile after the patch?
>>> You are right my oversight.
>>>
>>> But we can remove these internal_error handling, similar to the following modification:
>> We can remove _every_ internal_error call in GDB, since by design, they are not supposed
>> to ever execute, unless we have something wrong.  This is the scenario here, guarding against
>> someone mistakenly doing that change that would result in use_poll as 1 on a host that doesn't
>> support poll.  Why change it?
> Thank you for letting me know the design background, this change is not needed.

Hmm, so the issue was the possibility of ending up with 'use_poll' true, and HAVE_POLL
not defined.  How about if we ALSO guard the definition of 'use_poll' with HAVE_POLL?
Then it's way way more unlikely that such a bad scenario happens by accident, making it
OK to remove the internal_error calls throughout.

Like in this following patch.  WDYT?

From 2931c1c66fa0ebcb3fce7dbc7032b9c4243fa4f2 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Mon, 16 May 2022 10:11:15 +0100
Subject: [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths

gdbsupport/even-loop.cc throughout handles the case of use_poll being
true on a system where HAVE_POLL is not defined, by calling
internal_error if that situation ever happens.

Simplify this by moving the "use_poll" global itself under HAVE_POLL,
so that it's way more unlikely to ever end up in such a situation.
Then, move the code that checks the value of use_poll under HAVE_POLL
too, and remove the internal_error calls.  Like, from:

    if (use_poll)
      {
  #ifdef HAVE_POLL
        // poll code
  #else
      internal_error (....);
  #endif /* HAVE_POLL */
      }
    else
      {
	// select code
      }

to

  #ifdef HAVE_POLL
    if (use_poll)
      {
        // poll code
      }
    else
  #endif /* HAVE_POLL */
      {
	// select code
      }

While at it, make use_poll be a bool.  The current code is using
unsigned char most probably to save space, but I don't think it really
matters here.

Co-Authored-By: Youling Tang <tangyouling@loongson.cn>
Change-Id: I0dd74fdd4d393ccd057906df4cd75e8e83c1cdb4
---
 gdbsupport/event-loop.cc | 88 ++++++++++++----------------------------
 1 file changed, 27 insertions(+), 61 deletions(-)

diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
index 385b45b2de1..18f57ef8d61 100644
--- a/gdbsupport/event-loop.cc
+++ b/gdbsupport/event-loop.cc
@@ -78,14 +78,12 @@ struct file_handler
   struct file_handler *next_file;
 };
 
-/* Do we use poll or select ? */
 #ifdef HAVE_POLL
-#define USE_POLL 1
-#else
-#define USE_POLL 0
-#endif /* HAVE_POLL */
-
-static unsigned char use_poll = USE_POLL;
+/* Do we use poll or select?  Some systems have poll, but then it's
+   not useable with all kinds of files.  We probe that whenever a new
+   file handler is added.  */
+static bool use_poll = true;
+#endif
 
 #ifdef USE_WIN32API
 #include <windows.h>
@@ -249,12 +247,10 @@ add_file_handler (int fd, handler_func *proc, gdb_client_data client_data,
 		  std::string &&name, bool is_ui)
 {
 #ifdef HAVE_POLL
-  struct pollfd fds;
-#endif
-
   if (use_poll)
     {
-#ifdef HAVE_POLL
+      struct pollfd fds;
+
       /* Check to see if poll () is usable.  If not, we'll switch to
 	 use select.  This can happen on systems like
 	 m68k-motorola-sys, `poll' cannot be used to wait for `stdin'.
@@ -263,23 +259,15 @@ add_file_handler (int fd, handler_func *proc, gdb_client_data client_data,
       fds.fd = fd;
       fds.events = POLLIN;
       if (poll (&fds, 1, 0) == 1 && (fds.revents & POLLNVAL))
-	use_poll = 0;
-#else
-      internal_error (__FILE__, __LINE__,
-		      _("use_poll without HAVE_POLL"));
-#endif /* HAVE_POLL */
+	use_poll = false;
     }
   if (use_poll)
     {
-#ifdef HAVE_POLL
       create_file_handler (fd, POLLIN, proc, client_data, std::move (name),
 			   is_ui);
-#else
-      internal_error (__FILE__, __LINE__,
-		      _("use_poll without HAVE_POLL"));
-#endif
     }
   else
+#endif /* HAVE_POLL */
     create_file_handler (fd, GDB_READABLE | GDB_EXCEPTION,
 			 proc, client_data, std::move (name), is_ui);
 }
@@ -321,9 +309,9 @@ create_file_handler (int fd, int mask, handler_func * proc,
       file_ptr->next_file = gdb_notifier.first_file_handler;
       gdb_notifier.first_file_handler = file_ptr;
 
+#ifdef HAVE_POLL
       if (use_poll)
 	{
-#ifdef HAVE_POLL
 	  gdb_notifier.num_fds++;
 	  if (gdb_notifier.poll_fds)
 	    gdb_notifier.poll_fds =
@@ -336,12 +324,9 @@ create_file_handler (int fd, int mask, handler_func * proc,
 	  (gdb_notifier.poll_fds + gdb_notifier.num_fds - 1)->fd = fd;
 	  (gdb_notifier.poll_fds + gdb_notifier.num_fds - 1)->events = mask;
 	  (gdb_notifier.poll_fds + gdb_notifier.num_fds - 1)->revents = 0;
-#else
-	  internal_error (__FILE__, __LINE__,
-			  _("use_poll without HAVE_POLL"));
-#endif /* HAVE_POLL */
 	}
       else
+#endif /* HAVE_POLL */
 	{
 	  if (mask & GDB_READABLE)
 	    FD_SET (fd, &gdb_notifier.check_masks[0]);
@@ -402,10 +387,6 @@ delete_file_handler (int fd)
 {
   file_handler *file_ptr, *prev_ptr = NULL;
   int i;
-#ifdef HAVE_POLL
-  int j;
-  struct pollfd *new_poll_fds;
-#endif
 
   /* Find the entry for the given file.  */
 
@@ -419,9 +400,12 @@ delete_file_handler (int fd)
   if (file_ptr == NULL)
     return;
 
+#ifdef HAVE_POLL
   if (use_poll)
     {
-#ifdef HAVE_POLL
+      int j;
+      struct pollfd *new_poll_fds;
+
       /* Create a new poll_fds array by copying every fd's information
 	 but the one we want to get rid of.  */
 
@@ -442,12 +426,9 @@ delete_file_handler (int fd)
       xfree (gdb_notifier.poll_fds);
       gdb_notifier.poll_fds = new_poll_fds;
       gdb_notifier.num_fds--;
-#else
-      internal_error (__FILE__, __LINE__,
-		      _("use_poll without HAVE_POLL"));
-#endif /* HAVE_POLL */
     }
   else
+#endif /* HAVE_POLL */
     {
       if (file_ptr->mask & GDB_READABLE)
 	FD_CLR (fd, &gdb_notifier.check_masks[0]);
@@ -510,9 +491,6 @@ static void
 handle_file_event (file_handler *file_ptr, int ready_mask)
 {
   int mask;
-#ifdef HAVE_POLL
-  int error_mask;
-#endif
 
     {
 	{
@@ -526,9 +504,11 @@ handle_file_event (file_handler *file_ptr, int ready_mask)
 	  /* See if the desired events (mask) match the received
 	     events (ready_mask).  */
 
+#ifdef HAVE_POLL
 	  if (use_poll)
 	    {
-#ifdef HAVE_POLL
+	      int error_mask;
+
 	      /* POLLHUP means EOF, but can be combined with POLLIN to
 		 signal more data to read.  */
 	      error_mask = POLLHUP | POLLERR | POLLNVAL;
@@ -547,12 +527,9 @@ handle_file_event (file_handler *file_ptr, int ready_mask)
 		}
 	      else
 		file_ptr->error = 0;
-#else
-	      internal_error (__FILE__, __LINE__,
-			      _("use_poll without HAVE_POLL"));
-#endif /* HAVE_POLL */
 	    }
 	  else
+#endif /* HAVE_POLL */
 	    {
 	      if (ready_mask & GDB_EXCEPTION)
 		{
@@ -599,9 +576,9 @@ gdb_wait_for_event (int block)
   if (block)
     update_wait_timeout ();
 
+#ifdef HAVE_POLL
   if (use_poll)
     {
-#ifdef HAVE_POLL
       int timeout;
 
       if (block)
@@ -616,12 +593,9 @@ gdb_wait_for_event (int block)
 	 signal.  */
       if (num_found == -1 && errno != EINTR)
 	perror_with_name (("poll"));
-#else
-      internal_error (__FILE__, __LINE__,
-		      _("use_poll without HAVE_POLL"));
-#endif /* HAVE_POLL */
     }
   else
+#endif /* HAVE_POLL */
     {
       struct timeval select_timeout;
       struct timeval *timeout_p;
@@ -670,9 +644,9 @@ gdb_wait_for_event (int block)
   /* To level the fairness across event descriptors, we handle them in
      a round-robin-like fashion.  The number and order of descriptors
      may change between invocations, but this is good enough.  */
+#ifdef HAVE_POLL
   if (use_poll)
     {
-#ifdef HAVE_POLL
       int i;
       int mask;
 
@@ -699,12 +673,9 @@ gdb_wait_for_event (int block)
       mask = (gdb_notifier.poll_fds + i)->revents;
       handle_file_event (file_ptr, mask);
       return 1;
-#else
-      internal_error (__FILE__, __LINE__,
-		      _("use_poll without HAVE_POLL"));
-#endif /* HAVE_POLL */
     }
   else
+#endif /* HAVE_POLL */
     {
       /* See comment about even source fairness above.  */
       int mask = 0;
@@ -856,16 +827,11 @@ update_wait_timeout (void)
 	}
 
       /* Update the timeout for select/ poll.  */
-      if (use_poll)
-	{
 #ifdef HAVE_POLL
-	  gdb_notifier.poll_timeout = timeout.tv_sec * 1000;
-#else
-	  internal_error (__FILE__, __LINE__,
-			  _("use_poll without HAVE_POLL"));
-#endif /* HAVE_POLL */
-	}
+      if (use_poll)
+	gdb_notifier.poll_timeout = timeout.tv_sec * 1000;
       else
+#endif /* HAVE_POLL */
 	{
 	  gdb_notifier.select_timeout.tv_sec = timeout.tv_sec;
 	  gdb_notifier.select_timeout.tv_usec = timeout.tv_usec;

base-commit: b7ff32f191ed7e708412e9faa31cf691f08ca695
-- 
2.36.0

Re: [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths

[Youling Tang]

Hi, Pedro
On 05/16/2022 05:31 PM, Pedro Alves wrote:
> On 2022-05-14 02:14, Youling Tang wrote:
>> Hi, Pedro
>> On 05/13/2022 06:21 PM, Pedro Alves wrote:
>>> On 2022-05-13 11:09, Youling Tang wrote:
>>>> Hi, Pedro
>>>>
>>>> On 05/13/2022 05:28 PM, Pedro Alves wrote:
>>>>> On 2022-05-13 08:49, Youling Tang wrote:
>>>>>> By searching event-loop.cc we found that there are only the following 2
>>>>>> places to assign values to use_poll,
>>>>>> $ grep -nr use_poll gdbsupport/event-loop.cc
>>>>>>      88:static unsigned char use_poll = USE_POLL;
>>>>>>      263:    use_poll = 0;
>>>>>>
>>>>>> This USE_POLL is defined as follows,
>>>>>>     #ifdef HAVE_POLL
>>>>>>     #define USE_POLL 1
>>>>>>     #else
>>>>>>     #define USE_POLL 0
>>>>>>     #endif
>>>>>>
>>>>>> So use_poll may be 1 only if HAVE_POLL is defined. Removed "ifdef
>>>>>> HAVE_POLL" judgment in use_poll control block.
>>>>>>
>>>>>> Signed-off-by: Youling Tang <tangyouling@loongson.cn>
>>>>> Maybe I'm missing something, but ISTM this can't possible work on hosts that don'
>>>>>     have poll at all.  Like e.g., mingw:
>>>>>
>>>>>     $ grep POLL *
>>>>>     config.h:/* #undef HAVE_POLL */
>>>>>     config.h:/* #undef HAVE_POLL_H */
>>>>>     config.h:/* #undef HAVE_SYS_POLL_H */
>>>>>
>>>>> Surely they'll fail to compile after the patch?
>>>> You are right my oversight.
>>>>
>>>> But we can remove these internal_error handling, similar to the following modification:
>>> We can remove _every_ internal_error call in GDB, since by design, they are not supposed
>>> to ever execute, unless we have something wrong.  This is the scenario here, guarding against
>>> someone mistakenly doing that change that would result in use_poll as 1 on a host that doesn't
>>> support poll.  Why change it?
>> Thank you for letting me know the design background, this change is not needed.
> Hmm, so the issue was the possibility of ending up with 'use_poll' true, and HAVE_POLL
> not defined.  How about if we ALSO guard the definition of 'use_poll' with HAVE_POLL?
> Then it's way way more unlikely that such a bad scenario happens by accident, making it
> OK to remove the internal_error calls throughout.
Looks reasonable to me.

Thanks,
Youling.
> Like in this following patch.  WDYT?
>
>  From 2931c1c66fa0ebcb3fce7dbc7032b9c4243fa4f2 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Mon, 16 May 2022 10:11:15 +0100
> Subject: [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths
>
> gdbsupport/even-loop.cc throughout handles the case of use_poll being
> true on a system where HAVE_POLL is not defined, by calling
> internal_error if that situation ever happens.
>
> Simplify this by moving the "use_poll" global itself under HAVE_POLL,
> so that it's way more unlikely to ever end up in such a situation.
> Then, move the code that checks the value of use_poll under HAVE_POLL
> too, and remove the internal_error calls.  Like, from:
>
>      if (use_poll)
>        {
>    #ifdef HAVE_POLL
>          // poll code
>    #else
>        internal_error (....);
>    #endif /* HAVE_POLL */
>        }
>      else
>        {
> 	// select code
>        }
>
> to
>
>    #ifdef HAVE_POLL
>      if (use_poll)
>        {
>          // poll code
>        }
>      else
>    #endif /* HAVE_POLL */
>        {
> 	// select code
>        }
>
> While at it, make use_poll be a bool.  The current code is using
> unsigned char most probably to save space, but I don't think it really
> matters here.
>
> Co-Authored-By: Youling Tang <tangyouling@loongson.cn>
> Change-Id: I0dd74fdd4d393ccd057906df4cd75e8e83c1cdb4
> ---
>   gdbsupport/event-loop.cc | 88 ++++++++++++----------------------------
>   1 file changed, 27 insertions(+), 61 deletions(-)
>
> diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
> index 385b45b2de1..18f57ef8d61 100644
> --- a/gdbsupport/event-loop.cc
> +++ b/gdbsupport/event-loop.cc
> @@ -78,14 +78,12 @@ struct file_handler
>     struct file_handler *next_file;
>   };
>   
> -/* Do we use poll or select ? */
>   #ifdef HAVE_POLL
> -#define USE_POLL 1
> -#else
> -#define USE_POLL 0
> -#endif /* HAVE_POLL */
> -
> -static unsigned char use_poll = USE_POLL;
> +/* Do we use poll or select?  Some systems have poll, but then it's
> +   not useable with all kinds of files.  We probe that whenever a new
> +   file handler is added.  */
> +static bool use_poll = true;
> +#endif
>   
>   #ifdef USE_WIN32API
>   #include <windows.h>
> @@ -249,12 +247,10 @@ add_file_handler (int fd, handler_func *proc, gdb_client_data client_data,
>   		  std::string &&name, bool is_ui)
>   {
>   #ifdef HAVE_POLL
> -  struct pollfd fds;
> -#endif
> -
>     if (use_poll)
>       {
> -#ifdef HAVE_POLL
> +      struct pollfd fds;
> +
>         /* Check to see if poll () is usable.  If not, we'll switch to
>   	 use select.  This can happen on systems like
>   	 m68k-motorola-sys, `poll' cannot be used to wait for `stdin'.
> @@ -263,23 +259,15 @@ add_file_handler (int fd, handler_func *proc, gdb_client_data client_data,
>         fds.fd = fd;
>         fds.events = POLLIN;
>         if (poll (&fds, 1, 0) == 1 && (fds.revents & POLLNVAL))
> -	use_poll = 0;
> -#else
> -      internal_error (__FILE__, __LINE__,
> -		      _("use_poll without HAVE_POLL"));
> -#endif /* HAVE_POLL */
> +	use_poll = false;
>       }
>     if (use_poll)
>       {
> -#ifdef HAVE_POLL
>         create_file_handler (fd, POLLIN, proc, client_data, std::move (name),
>   			   is_ui);
> -#else
> -      internal_error (__FILE__, __LINE__,
> -		      _("use_poll without HAVE_POLL"));
> -#endif
>       }
>     else
> +#endif /* HAVE_POLL */
>       create_file_handler (fd, GDB_READABLE | GDB_EXCEPTION,
>   			 proc, client_data, std::move (name), is_ui);
>   }
> @@ -321,9 +309,9 @@ create_file_handler (int fd, int mask, handler_func * proc,
>         file_ptr->next_file = gdb_notifier.first_file_handler;
>         gdb_notifier.first_file_handler = file_ptr;
>   
> +#ifdef HAVE_POLL
>         if (use_poll)
>   	{
> -#ifdef HAVE_POLL
>   	  gdb_notifier.num_fds++;
>   	  if (gdb_notifier.poll_fds)
>   	    gdb_notifier.poll_fds =
> @@ -336,12 +324,9 @@ create_file_handler (int fd, int mask, handler_func * proc,
>   	  (gdb_notifier.poll_fds + gdb_notifier.num_fds - 1)->fd = fd;
>   	  (gdb_notifier.poll_fds + gdb_notifier.num_fds - 1)->events = mask;
>   	  (gdb_notifier.poll_fds + gdb_notifier.num_fds - 1)->revents = 0;
> -#else
> -	  internal_error (__FILE__, __LINE__,
> -			  _("use_poll without HAVE_POLL"));
> -#endif /* HAVE_POLL */
>   	}
>         else
> +#endif /* HAVE_POLL */
>   	{
>   	  if (mask & GDB_READABLE)
>   	    FD_SET (fd, &gdb_notifier.check_masks[0]);
> @@ -402,10 +387,6 @@ delete_file_handler (int fd)
>   {
>     file_handler *file_ptr, *prev_ptr = NULL;
>     int i;
> -#ifdef HAVE_POLL
> -  int j;
> -  struct pollfd *new_poll_fds;
> -#endif
>   
>     /* Find the entry for the given file.  */
>   
> @@ -419,9 +400,12 @@ delete_file_handler (int fd)
>     if (file_ptr == NULL)
>       return;
>   
> +#ifdef HAVE_POLL
>     if (use_poll)
>       {
> -#ifdef HAVE_POLL
> +      int j;
> +      struct pollfd *new_poll_fds;
> +
>         /* Create a new poll_fds array by copying every fd's information
>   	 but the one we want to get rid of.  */
>   
> @@ -442,12 +426,9 @@ delete_file_handler (int fd)
>         xfree (gdb_notifier.poll_fds);
>         gdb_notifier.poll_fds = new_poll_fds;
>         gdb_notifier.num_fds--;
> -#else
> -      internal_error (__FILE__, __LINE__,
> -		      _("use_poll without HAVE_POLL"));
> -#endif /* HAVE_POLL */
>       }
>     else
> +#endif /* HAVE_POLL */
>       {
>         if (file_ptr->mask & GDB_READABLE)
>   	FD_CLR (fd, &gdb_notifier.check_masks[0]);
> @@ -510,9 +491,6 @@ static void
>   handle_file_event (file_handler *file_ptr, int ready_mask)
>   {
>     int mask;
> -#ifdef HAVE_POLL
> -  int error_mask;
> -#endif
>   
>       {
>   	{
> @@ -526,9 +504,11 @@ handle_file_event (file_handler *file_ptr, int ready_mask)
>   	  /* See if the desired events (mask) match the received
>   	     events (ready_mask).  */
>   
> +#ifdef HAVE_POLL
>   	  if (use_poll)
>   	    {
> -#ifdef HAVE_POLL
> +	      int error_mask;
> +
>   	      /* POLLHUP means EOF, but can be combined with POLLIN to
>   		 signal more data to read.  */
>   	      error_mask = POLLHUP | POLLERR | POLLNVAL;
> @@ -547,12 +527,9 @@ handle_file_event (file_handler *file_ptr, int ready_mask)
>   		}
>   	      else
>   		file_ptr->error = 0;
> -#else
> -	      internal_error (__FILE__, __LINE__,
> -			      _("use_poll without HAVE_POLL"));
> -#endif /* HAVE_POLL */
>   	    }
>   	  else
> +#endif /* HAVE_POLL */
>   	    {
>   	      if (ready_mask & GDB_EXCEPTION)
>   		{
> @@ -599,9 +576,9 @@ gdb_wait_for_event (int block)
>     if (block)
>       update_wait_timeout ();
>   
> +#ifdef HAVE_POLL
>     if (use_poll)
>       {
> -#ifdef HAVE_POLL
>         int timeout;
>   
>         if (block)
> @@ -616,12 +593,9 @@ gdb_wait_for_event (int block)
>   	 signal.  */
>         if (num_found == -1 && errno != EINTR)
>   	perror_with_name (("poll"));
> -#else
> -      internal_error (__FILE__, __LINE__,
> -		      _("use_poll without HAVE_POLL"));
> -#endif /* HAVE_POLL */
>       }
>     else
> +#endif /* HAVE_POLL */
>       {
>         struct timeval select_timeout;
>         struct timeval *timeout_p;
> @@ -670,9 +644,9 @@ gdb_wait_for_event (int block)
>     /* To level the fairness across event descriptors, we handle them in
>        a round-robin-like fashion.  The number and order of descriptors
>        may change between invocations, but this is good enough.  */
> +#ifdef HAVE_POLL
>     if (use_poll)
>       {
> -#ifdef HAVE_POLL
>         int i;
>         int mask;
>   
> @@ -699,12 +673,9 @@ gdb_wait_for_event (int block)
>         mask = (gdb_notifier.poll_fds + i)->revents;
>         handle_file_event (file_ptr, mask);
>         return 1;
> -#else
> -      internal_error (__FILE__, __LINE__,
> -		      _("use_poll without HAVE_POLL"));
> -#endif /* HAVE_POLL */
>       }
>     else
> +#endif /* HAVE_POLL */
>       {
>         /* See comment about even source fairness above.  */
>         int mask = 0;
> @@ -856,16 +827,11 @@ update_wait_timeout (void)
>   	}
>   
>         /* Update the timeout for select/ poll.  */
> -      if (use_poll)
> -	{
>   #ifdef HAVE_POLL
> -	  gdb_notifier.poll_timeout = timeout.tv_sec * 1000;
> -#else
> -	  internal_error (__FILE__, __LINE__,
> -			  _("use_poll without HAVE_POLL"));
> -#endif /* HAVE_POLL */
> -	}
> +      if (use_poll)
> +	gdb_notifier.poll_timeout = timeout.tv_sec * 1000;
>         else
> +#endif /* HAVE_POLL */
>   	{
>   	  gdb_notifier.select_timeout.tv_sec = timeout.tv_sec;
>   	  gdb_notifier.select_timeout.tv_usec = timeout.tv_usec;
>
> base-commit: b7ff32f191ed7e708412e9faa31cf691f08ca695

Re: [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> Like in this following patch.  WDYT?

It seems like a good idea to me.

Pedro> From 2931c1c66fa0ebcb3fce7dbc7032b9c4243fa4f2 Mon Sep 17 00:00:00 2001
Pedro> From: Pedro Alves <pedro@palves.net>
Pedro> Date: Mon, 16 May 2022 10:11:15 +0100
Pedro> Subject: [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths

Missing a 't' in the file name in the subject here.

Tom

[ob/pushed] Reindent gdbsupport/event-loop.cc:handle_file_event (Re: [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths)

[Pedro Alves]

On 2022-05-16 16:24, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> Like in this following patch.  WDYT?
> 
> It seems like a good idea to me.
> 
> Pedro> From 2931c1c66fa0ebcb3fce7dbc7032b9c4243fa4f2 Mon Sep 17 00:00:00 2001
> Pedro> From: Pedro Alves <pedro@palves.net>
> Pedro> Date: Mon, 16 May 2022 10:11:15 +0100
> Pedro> Subject: [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths
> 
> Missing a 't' in the file name in the subject here.

Thanks, I fixed that (and the other instance), and applied the obvious follow up
patch at the bottom.  git diff -w looks like this:

~~~
diff --git c/gdbsupport/event-loop.cc w/gdbsupport/event-loop.cc
index 18f57ef8d61..f078c1278a8 100644
--- c/gdbsupport/event-loop.cc
+++ w/gdbsupport/event-loop.cc
@@ -492,23 +492,21 @@ handle_file_event (file_handler *file_ptr, int ready_mask)
 {
   int mask;
 
-    {
-	{
-	  /* With poll, the ready_mask could have any of three events
-	     set to 1: POLLHUP, POLLERR, POLLNVAL.  These events
-	     cannot be used in the requested event mask (events), but
-	     they can be returned in the return mask (revents).  We
-	     need to check for those event too, and add them to the
-	     mask which will be passed to the handler.  */
-
-	  /* See if the desired events (mask) match the received
-	     events (ready_mask).  */
+  /* See if the desired events (mask) match the received events
+     (ready_mask).  */
 
 #ifdef HAVE_POLL
   if (use_poll)
     {
       int error_mask;
 
+      /* With poll, the ready_mask could have any of three events set
+	 to 1: POLLHUP, POLLERR, POLLNVAL.  These events cannot be
+	 used in the requested event mask (events), but they can be
+	 returned in the return mask (revents).  We need to check for
+	 those event too, and add them to the mask which will be
+	 passed to the handler.  */
+
       /* POLLHUP means EOF, but can be combined with POLLIN to
 	 signal more data to read.  */
       error_mask = POLLHUP | POLLERR | POLLNVAL;
@@ -551,8 +549,6 @@ handle_file_event (file_handler *file_ptr, int ready_mask)
       file_ptr->proc (file_ptr->error, file_ptr->client_data);
     }
 }
-    }
-}
 
 /* Wait for new events on the monitored file descriptors.  Run the
    event handler if the first descriptor that is detected by the poll.
~~~

Here's what I pushed.

From 187075ebbc0cba7b58685c7214028e791b5af844 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Mon, 16 May 2022 12:20:08 +0100
Subject: [PATCH] Reindent gdbsupport/event-loop.cc:handle_file_event

The handle_file_event function has a few unnecessary {} lexical
blocks, presumably because they were originally if blocks, and the
conditions were removed, or something along those lines.

Remove the unnecessary blocks, and reindent.

Change-Id: Iaecbe5c9f4940a80b81dbbc42e51ce506f6aafb2
---
 gdbsupport/event-loop.cc | 104 +++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 54 deletions(-)

diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
index 18f57ef8d61..f078c1278a8 100644
--- a/gdbsupport/event-loop.cc
+++ b/gdbsupport/event-loop.cc
@@ -492,65 +492,61 @@ handle_file_event (file_handler *file_ptr, int ready_mask)
 {
   int mask;
 
+  /* See if the desired events (mask) match the received events
+     (ready_mask).  */
+
+#ifdef HAVE_POLL
+  if (use_poll)
     {
-	{
-	  /* With poll, the ready_mask could have any of three events
-	     set to 1: POLLHUP, POLLERR, POLLNVAL.  These events
-	     cannot be used in the requested event mask (events), but
-	     they can be returned in the return mask (revents).  We
-	     need to check for those event too, and add them to the
-	     mask which will be passed to the handler.  */
+      int error_mask;
 
-	  /* See if the desired events (mask) match the received
-	     events (ready_mask).  */
+      /* With poll, the ready_mask could have any of three events set
+	 to 1: POLLHUP, POLLERR, POLLNVAL.  These events cannot be
+	 used in the requested event mask (events), but they can be
+	 returned in the return mask (revents).  We need to check for
+	 those event too, and add them to the mask which will be
+	 passed to the handler.  */
 
-#ifdef HAVE_POLL
-	  if (use_poll)
-	    {
-	      int error_mask;
-
-	      /* POLLHUP means EOF, but can be combined with POLLIN to
-		 signal more data to read.  */
-	      error_mask = POLLHUP | POLLERR | POLLNVAL;
-	      mask = ready_mask & (file_ptr->mask | error_mask);
-
-	      if ((mask & (POLLERR | POLLNVAL)) != 0)
-		{
-		  /* Work in progress.  We may need to tell somebody
-		     what kind of error we had.  */
-		  if (mask & POLLERR)
-		    warning (_("Error detected on fd %d"), file_ptr->fd);
-		  if (mask & POLLNVAL)
-		    warning (_("Invalid or non-`poll'able fd %d"),
-			     file_ptr->fd);
-		  file_ptr->error = 1;
-		}
-	      else
-		file_ptr->error = 0;
-	    }
-	  else
-#endif /* HAVE_POLL */
-	    {
-	      if (ready_mask & GDB_EXCEPTION)
-		{
-		  warning (_("Exception condition detected on fd %d"),
-			   file_ptr->fd);
-		  file_ptr->error = 1;
-		}
-	      else
-		file_ptr->error = 0;
-	      mask = ready_mask & file_ptr->mask;
-	    }
+      /* POLLHUP means EOF, but can be combined with POLLIN to
+	 signal more data to read.  */
+      error_mask = POLLHUP | POLLERR | POLLNVAL;
+      mask = ready_mask & (file_ptr->mask | error_mask);
 
-	  /* If there was a match, then call the handler.  */
-	  if (mask != 0)
-	    {
-	      event_loop_ui_debug_printf (file_ptr->is_ui,
-					  "invoking fd file handler `%s`",
-					  file_ptr->name.c_str ());
-	      file_ptr->proc (file_ptr->error, file_ptr->client_data);
-	    }
+      if ((mask & (POLLERR | POLLNVAL)) != 0)
+	{
+	  /* Work in progress.  We may need to tell somebody
+	     what kind of error we had.  */
+	  if (mask & POLLERR)
+	    warning (_("Error detected on fd %d"), file_ptr->fd);
+	  if (mask & POLLNVAL)
+	    warning (_("Invalid or non-`poll'able fd %d"),
+		     file_ptr->fd);
+	  file_ptr->error = 1;
+	}
+      else
+	file_ptr->error = 0;
+    }
+  else
+#endif /* HAVE_POLL */
+    {
+      if (ready_mask & GDB_EXCEPTION)
+	{
+	  warning (_("Exception condition detected on fd %d"),
+		   file_ptr->fd);
+	  file_ptr->error = 1;
 	}
+      else
+	file_ptr->error = 0;
+      mask = ready_mask & file_ptr->mask;
+    }
+
+  /* If there was a match, then call the handler.  */
+  if (mask != 0)
+    {
+      event_loop_ui_debug_printf (file_ptr->is_ui,
+				  "invoking fd file handler `%s`",
+				  file_ptr->name.c_str ());
+      file_ptr->proc (file_ptr->error, file_ptr->client_data);
     }
 }
 

base-commit: 36a5b3705352f228cdd14a7f9d5e85177fad034c
-- 
2.36.0