From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f54.google.com (mail-wm1-f54.google.com [209.85.128.54]) by sourceware.org (Postfix) with ESMTPS id 7055D3858010 for ; Mon, 16 May 2022 09:31:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7055D3858010 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-f54.google.com with SMTP id bg25so8316698wmb.4 for ; Mon, 16 May 2022 02:31:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:references:from:in-reply-to :content-transfer-encoding; bh=A5dabfLcZWkEVIwjsyf+A9A3MVz5zxQNsJTSXqWz2AU=; b=jLJ3Ncg9JoHdsjTBNG2jYML5B3hhDm4ZkebtmGlo84HJsg3uoSob/iIxtFYrKYbi2P 87vOUjnxwZ2La/t4GeHHR6vgnHkicSzO1f/iM5Dy+ohxRYsaMGw8flUFwktu2tBxTjGc dhnRL7raZpXwfaOuVegnv0eQLaHN1Y/1eGHYtos9I9+g4kEmXmS2hyWEmZuryeNkN3y9 Zs/lYv1ejuR5DMXP6GTayaoKISejsTNkj1B+/bf1158ZYCNJAB4lH3cKzxrxNVv5zgTy HpE1yFypnEKgViDKkVrbcRibA0XnBbz8utfUjnHW++X44TppCIzkgPOaLJ4mtGQk44fK FV1w== X-Gm-Message-State: AOAM530aVqeX7sJ6qwIjFhedeiiHY6pEDbObmfBOlMxol2xergw7cWkj uti+9vg5AugxpD1uzh5HiZwSXNgq0Yo= X-Google-Smtp-Source: ABdhPJyT19beGVg3CJhccCqDLeZ+sFi38KYfLtW2LBUwkygx6pAUHngpF+V4jAn1XXZ1m6CMwHyBng== X-Received: by 2002:a05:600c:2844:b0:393:f6fb:5897 with SMTP id r4-20020a05600c284400b00393f6fb5897mr16292547wmb.66.1652693517114; Mon, 16 May 2022 02:31:57 -0700 (PDT) Received: from ?IPV6:2001:8a0:f924:2600:209d:85e2:409e:8726? ([2001:8a0:f924:2600:209d:85e2:409e:8726]) by smtp.gmail.com with ESMTPSA id m7-20020adfa3c7000000b0020d02262664sm5157311wrb.25.2022.05.16.02.31.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 16 May 2022 02:31:56 -0700 (PDT) Message-ID: Date: Mon, 16 May 2022 10:31:55 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: [PATCH] gdbsupport/even-loop.cc: simplify !HAVE_POLL paths Content-Language: en-US To: Youling Tang , gdb-patches@sourceware.org References: <20220513074905.21554-1-tangyouling@loongson.cn> <634dfd3e-dca9-3c5a-cf63-2a86ca23e40e@palves.net> <182b2afa-d1ac-510b-b454-f81a98a51598@loongson.cn> <497ca477-d498-9c26-f00c-ca8105ee19ed@palves.net> <98da4669-0d8c-21ec-35c3-76fc6e5bb5cc@loongson.cn> From: Pedro Alves In-Reply-To: <98da4669-0d8c-21ec-35c3-76fc6e5bb5cc@loongson.cn> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.8 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 May 2022 09:32:00 -0000 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 >>>> 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 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 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 @@ -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