From: Pedro Alves <pedro@palves.net>
To: Youling Tang <tangyouling@loongson.cn>, gdb-patches@sourceware.org
Subject: [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths
Date: Mon, 16 May 2022 10:31:55 +0100 [thread overview]
Message-ID: <a083b158-003a-42b3-37d1-8db5b94e8a61@palves.net> (raw)
In-Reply-To: <98da4669-0d8c-21ec-35c3-76fc6e5bb5cc@loongson.cn>
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
next prev parent reply other threads:[~2022-05-16 9:31 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-13 7:49 [PATCH] gdbsupport: Remove some unnecessary ifdef HAVE_POLL judgments Youling Tang
2022-05-13 9:28 ` Pedro Alves
2022-05-13 10:09 ` Youling Tang
2022-05-13 10:21 ` Pedro Alves
2022-05-14 1:14 ` Youling Tang
2022-05-16 9:31 ` Pedro Alves [this message]
2022-05-16 12:13 ` [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths Youling Tang
2022-05-16 15:24 ` Tom Tromey
2022-05-16 19:01 ` [ob/pushed] Reindent gdbsupport/event-loop.cc:handle_file_event (Re: [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths) Pedro Alves
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a083b158-003a-42b3-37d1-8db5b94e8a61@palves.net \
--to=pedro@palves.net \
--cc=gdb-patches@sourceware.org \
--cc=tangyouling@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).