* [PATCH] gdbsupport: Remove some unnecessary ifdef HAVE_POLL judgments @ 2022-05-13 7:49 Youling Tang 2022-05-13 9:28 ` Pedro Alves 0 siblings, 1 reply; 9+ messages in thread From: Youling Tang @ 2022-05-13 7:49 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdbsupport: Remove some unnecessary ifdef HAVE_POLL judgments 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 0 siblings, 1 reply; 9+ messages in thread From: Pedro Alves @ 2022-05-13 9:28 UTC (permalink / raw) To: Youling Tang, gdb-patches 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdbsupport: Remove some unnecessary ifdef HAVE_POLL judgments 2022-05-13 9:28 ` Pedro Alves @ 2022-05-13 10:09 ` Youling Tang 2022-05-13 10:21 ` Pedro Alves 0 siblings, 1 reply; 9+ messages in thread From: Youling Tang @ 2022-05-13 10:09 UTC (permalink / raw) To: Pedro Alves, gdb-patches 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdbsupport: Remove some unnecessary ifdef HAVE_POLL judgments 2022-05-13 10:09 ` Youling Tang @ 2022-05-13 10:21 ` Pedro Alves 2022-05-14 1:14 ` Youling Tang 0 siblings, 1 reply; 9+ messages in thread From: Pedro Alves @ 2022-05-13 10:21 UTC (permalink / raw) To: Youling Tang, gdb-patches 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdbsupport: Remove some unnecessary ifdef HAVE_POLL judgments 2022-05-13 10:21 ` Pedro Alves @ 2022-05-14 1:14 ` Youling Tang 2022-05-16 9:31 ` [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths Pedro Alves 0 siblings, 1 reply; 9+ messages in thread From: Youling Tang @ 2022-05-14 1:14 UTC (permalink / raw) To: Pedro Alves, gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths 2022-05-14 1:14 ` Youling Tang @ 2022-05-16 9:31 ` Pedro Alves 2022-05-16 12:13 ` Youling Tang 2022-05-16 15:24 ` Tom Tromey 0 siblings, 2 replies; 9+ messages in thread From: Pedro Alves @ 2022-05-16 9:31 UTC (permalink / raw) To: Youling Tang, gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths 2022-05-16 9:31 ` [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths Pedro Alves @ 2022-05-16 12:13 ` Youling Tang 2022-05-16 15:24 ` Tom Tromey 1 sibling, 0 replies; 9+ messages in thread From: Youling Tang @ 2022-05-16 12:13 UTC (permalink / raw) To: Pedro Alves, gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths 2022-05-16 9:31 ` [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths Pedro Alves 2022-05-16 12:13 ` 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 1 sibling, 1 reply; 9+ messages in thread From: Tom Tromey @ 2022-05-16 15:24 UTC (permalink / raw) To: Pedro Alves; +Cc: Youling Tang, gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [ob/pushed] Reindent gdbsupport/event-loop.cc:handle_file_event (Re: [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths) 2022-05-16 15:24 ` Tom Tromey @ 2022-05-16 19:01 ` Pedro Alves 0 siblings, 0 replies; 9+ messages in thread From: Pedro Alves @ 2022-05-16 19:01 UTC (permalink / raw) To: Tom Tromey; +Cc: Youling Tang, gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-05-16 19:01 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 ` [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths Pedro Alves 2022-05-16 12:13 ` 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
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).