From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) by sourceware.org (Postfix) with ESMTP id 19540395C06B for ; Wed, 13 May 2020 21:15:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 19540395C06B Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-331-5Vt95BJeM-Gau_vqYrsYUA-1; Wed, 13 May 2020 17:15:06 -0400 X-MC-Unique: 5Vt95BJeM-Gau_vqYrsYUA-1 Received: by mail-wm1-f70.google.com with SMTP id h6so12286602wmi.7 for ; Wed, 13 May 2020 14:15:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=0fg+rXHmGOp8d1DGVvWrdME2ch8YzLopBTT4EJO0kKk=; b=bKy6BMk8CKp36cfn7EAgxC7VPVRA3qzIfXWEvFvX9Kbwqh9mBqemqTfkQBw13TiH1e iJ+OHMwN8Ebm3K+4ma+sm02VQbgH4d2jq20RfUIAuaLwtt2EOjIzfU7UslpKteooFRCW Awk8CFlYSEf4A0lN1WnIAIgLZC3pSnpw/nAJV688t4lu1DdfaFcRpQ5CcuaJQK8g3G9Z It8xUkZCdknNszFC/qRu67u5VN75ZO1eOhljz2XuB48swEaM8mYdNVVB0hdF+iD6k9PK F5hHSLmt6wrwGWLmSO422RjU3UKrViOqJ2PPxdr/tgnPu9zKWrqMM3W4BNO5vFUo5XuF NtDQ== X-Gm-Message-State: AOAM531p2tUO2eGR8HUQS7dqCNPgTLpLbg0OzI44LLofZC5Xp8dIY/ik 0M2iQqLW0cn2McQguGWNdVzKbGOqvMhJRUrzPyMCROKk+dsu+x4R2pH0xCgHOfGEZxFUPrQ7XIc ovW6agQdckMiDNOa8BhxBmA== X-Received: by 2002:adf:c508:: with SMTP id q8mr1383658wrf.4.1589404504526; Wed, 13 May 2020 14:15:04 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyIaqPcfGMZGt6tQKTpXocviE1QtRXx+wfSUNBkShDCq6jB5XiAPemWelcYm6AWTVid+W5Q8g== X-Received: by 2002:adf:c508:: with SMTP id q8mr1383633wrf.4.1589404504126; Wed, 13 May 2020 14:15:04 -0700 (PDT) Received: from ?IPv6:2001:8a0:f909:7b00:56ee:75ff:fe8d:232b? ([2001:8a0:f909:7b00:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id m7sm14337564wmc.40.2020.05.13.14.15.03 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 13 May 2020 14:15:03 -0700 (PDT) Subject: Re: [PATCH v7 5/5] gdb/infrun: handle already-exited threads when attempting to stop To: Tankut Baris Aktemur , gdb-patches@sourceware.org References: From: Pedro Alves Message-ID: Date: Wed, 13 May 2020 22:15:02 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Wed, 13 May 2020 21:15:13 -0000 Hi Tankut, I've sent my updated version of this patch, along with the series, here: https://sourceware.org/pipermail/gdb-patches/2020-May/168478.html See some comments below. On 4/22/20 4:00 PM, Tankut Baris Aktemur via Gdb-patches wrote: > In stop_all_threads, GDB sends signals to other threads in an attempt > to stop them. While in a typical scenario the expected wait status is > TARGET_WAITKIND_STOPPED, it is possible that the thread GDB attempted > to stop has already terminated. If so, a waitstatus other than > TARGET_WAITKIND_STOPPED would be received. Handle this case > appropriately. > > If a wait status that denotes thread termination is ignored, GDB goes > into an infinite loop in stop_all_threads. > E.g.: > > $ gdb ./a.out > (gdb) start > ... > (gdb) add-inferior -exec ./a.out > ... > (gdb) inferior 2 > ... > (gdb) start > ... > (gdb) set schedule-multiple on > (gdb) set debug infrun 2 > (gdb) continue > Continuing. > infrun: clear_proceed_status_thread (process 10449) > infrun: clear_proceed_status_thread (process 10453) > infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT) > infrun: proceed: resuming process 10449 > infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 10449] at 0x55555555514e > infrun: infrun_async(1) > infrun: prepare_to_wait > infrun: proceed: resuming process 10453 > infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 10453] at 0x55555555514e > infrun: prepare_to_wait > infrun: Found 2 inferiors, starting at #0 > infrun: target_wait (-1.0.0, status) = > infrun: 10449.10449.0 [process 10449], > infrun: status->kind = exited, status = 0 > infrun: handle_inferior_event status->kind = exited, status = 0 > [Inferior 1 (process 10449) exited normally] > infrun: stop_waiting > infrun: stop_all_threads > infrun: stop_all_threads, pass=0, iterations=0 > infrun: process 10453 executing, need stop > infrun: target_wait (-1.0.0, status) = > infrun: 10453.10453.0 [process 10453], > infrun: status->kind = exited, status = 0 > infrun: stop_all_threads status->kind = exited, status = 0 process 10453 > infrun: process 10453 executing, already stopping > infrun: target_wait (-1.0.0, status) = > infrun: -1.0.0 [process -1], > infrun: status->kind = no-resumed > infrun: infrun_async(0) > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > infrun: stop_all_threads status->kind = no-resumed process -1 > infrun: process 10453 executing, already stopping > ... > > And this polling goes on forever. This patch prevents the infinite > looping behavior. For the same scenario above, we obtain the > following behavior: > > ... > (gdb) continue > Continuing. > infrun: clear_proceed_status_thread (process 31229) > infrun: clear_proceed_status_thread (process 31233) > infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT) > infrun: proceed: resuming process 31229 > infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 31229] at 0x55555555514e > infrun: infrun_async(1) > infrun: prepare_to_wait > infrun: proceed: resuming process 31233 > infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 31233] at 0x55555555514e > infrun: prepare_to_wait > infrun: Found 2 inferiors, starting at #0 > infrun: target_wait (-1.0.0, status) = > infrun: 31229.31229.0 [process 31229], > infrun: status->kind = exited, status = 0 > infrun: handle_inferior_event status->kind = exited, status = 0 > [Inferior 1 (process 31229) exited normally] > infrun: stop_waiting > infrun: stop_all_threads > infrun: stop_all_threads, pass=0, iterations=0 > infrun: process 31233 executing, need stop > infrun: target_wait (-1.0.0, status) = > infrun: 31233.31233.0 [process 31233], > infrun: status->kind = exited, status = 0 > infrun: stop_all_threads status->kind = exited, status = 0 process 31233 > infrun: saving status status->kind = exited, status = 0 for 31233.31233.0 > infrun: process 31233 not executing > infrun: stop_all_threads, pass=1, iterations=1 > infrun: process 31233 not executing > infrun: stop_all_threads done > (gdb) > > The exit event from Inferior 1 is received and shown to the user. > The exit event from Inferior 2 is not displayed, but kept pending. > > (gdb) info inferiors > Num Description Connection Executable > * 1 a.out > 2 process 31233 1 (native) a.out > (gdb) inferior 2 > [Switching to inferior 2 [process 31233] (a.out)] > [Switching to thread 2.1 (process 31233)] > Couldn't get registers: No such process. > (gdb) continue > Continuing. > infrun: clear_proceed_status_thread (process 31233) > infrun: clear_proceed_status_thread: thread process 31233 has pending wait status status->kind = exited, status = 0 (currently_stepping=0). > infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT) > infrun: proceed: resuming process 31233 > infrun: resume: thread process 31233 has pending wait status status->kind = exited, status = 0 (currently_stepping=0). > infrun: prepare_to_wait > infrun: Using pending wait status status->kind = exited, status = 0 for process 31233. > infrun: target_wait (-1.0.0, status) = > infrun: 31233.31233.0 [process 31233], > infrun: status->kind = exited, status = 0 > infrun: handle_inferior_event status->kind = exited, status = 0 > [Inferior 2 (process 31233) exited normally] > infrun: stop_waiting > (gdb) info inferiors > Num Description Connection Executable > 1 a.out > * 2 a.out > (gdb) > > Regression-tested on X86_64 Linux. > > gdb/ChangeLog: > 2020-02-05 Tankut Baris Aktemur > Tom de Vries > > PR threads/25478 > * infrun.c (stop_all_threads): Do NOT ignore > TARGET_WAITKIND_NO_RESUMED, TARGET_WAITKIND_THREAD_EXITED, > TARGET_WAITKIND_EXITED, TARGET_WAITKIND_SIGNALLED wait statuses > received from threads we attempt to stop. > > gdb/testsuite/ChangeLog: > 2019-11-04 Tankut Baris Aktemur > > * gdb.multi/multi-exit.c: New file. > * gdb.multi/multi-exit.exp: New file. > * gdb.multi/multi-kill.c: New file. > * gdb.multi/multi-kill.exp: New file. > > Change-Id: I7cec98f40283773b79255d998511da434e9cd408 > --- > gdb/infrun.c | 101 ++++++++++++++++++++-- > gdb/testsuite/gdb.multi/multi-exit.c | 22 +++++ > gdb/testsuite/gdb.multi/multi-exit.exp | 111 +++++++++++++++++++++++++ > gdb/testsuite/gdb.multi/multi-kill.c | 42 ++++++++++ > gdb/testsuite/gdb.multi/multi-kill.exp | 110 ++++++++++++++++++++++++ > 5 files changed, 379 insertions(+), 7 deletions(-) > create mode 100644 gdb/testsuite/gdb.multi/multi-exit.c > create mode 100644 gdb/testsuite/gdb.multi/multi-exit.exp > create mode 100644 gdb/testsuite/gdb.multi/multi-kill.c > create mode 100644 gdb/testsuite/gdb.multi/multi-kill.exp > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 167d50ff3ab..93169269553 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -4781,7 +4781,47 @@ stop_all_threads (void) > { > int need_wait = 0; > > - update_thread_list (); > + for (inferior *inf : all_non_exited_inferiors ()) > + { > + update_thread_list (); > + > + /* After updating the thread list, it's possible to end > + up with pid != 0 but no threads, if the inf's process > + has exited but we have not processed that event yet. > + The exit event must be waiting somewhere in the queue > + to be processed. Silently add a thread so that we do > + a wait_one() below to pick the pending event. */ > + > + bool has_threads = false; > + for (thread_info *tp ATTRIBUTE_UNUSED > + : inf->non_exited_threads ()) > + { > + has_threads = true; > + break; > + } > + > + if (has_threads) > + continue; > + > + ptid_t ptid (inf->pid, inf->pid, 0); > + This here is what make me go think through all this. I don't really like it to have common code make up the ptid to use for the new thread. That should be the responsibility of the target backend. It may be that the target backend uses the tid field to store important information, for example. I also think that having to re-add a thread is not ideal. As we move GDB towards more multi-target support, and potentially other kinds of execution objects, I think that it's likely that we will always make it the responsibility of the target backend to create a thread, since it's going to the the target that knows the actual (C++) type of the thread Imagine target-specific classes that inherit from a common thread_info class, with virtual methods. After giving it some thought and experimentation, I think we should go back to your idea of not deleting the last thread of the process. But let's keep it simple without current_inferior() checks. When that solution was brought up before, I pointed at the code handle_no_resumed that handles the case of inferiors with no threads. I managed to reproduce that scenario, and confirm that we still handle it OK. I ended up squashing the remote and infrun changes in a single patch, as they're part of the same logical change. Finally, while playing with the testcases, I thought I'd make them spawn more inferiors, so that we're more sure we hit the race window. The way I've adjusted the testcases, it's simple to make them spawn any number of inferiors -- you just have to change one global. Please take a look and let me know what you think. Thanks, Pedro Alves