From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 35ACA3858423 for ; Wed, 16 Feb 2022 13:36:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 35ACA3858423 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 21GDZXcm007912 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 16 Feb 2022 08:35:38 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 21GDZXcm007912 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id CD0551F0BB; Wed, 16 Feb 2022 08:35:32 -0500 (EST) Message-ID: <8f2def5d-a727-fe3c-334c-823d334216ee@polymtl.ca> Date: Wed, 16 Feb 2022 08:35:30 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.1 Subject: Re: [PATCH v2 6/9] gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on) Content-Language: en-US To: Lancelot SIX Cc: gdb-patches@sourceware.org, Simon Marchi References: <20220118040937.730282-1-simon.marchi@polymtl.ca> <20220118040937.730282-7-simon.marchi@polymtl.ca> <20220216002822.g3g3um6mwfwtcv4q@Plymouth> From: Simon Marchi In-Reply-To: <20220216002822.g3g3um6mwfwtcv4q@Plymouth> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Wed, 16 Feb 2022 13:35:33 +0000 X-Spam-Status: No, score=-3039.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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, 16 Feb 2022 13:36:51 -0000 >> - In follow_fork, no need to stop all threads of the inferior, the >> target has stopped all threads of all its inferiors before returning >> the event. >> - In follow_fork_inferior, we record the vfork parent thread in >> inferior::thread_waiting_for_vfork_done. >> - Back in handle_inferior_event, we also call keep_going. However, we >> only want to resume the event thread here, not all inferior threads. >> In internal_resume_ptid (called by resume_1), we therefore now check >> whether one of the inferiors we are about to resume has >> thread_waiting_for_vfork_done set. If so, we only resume that >> thread. When resuming multiple inferiors, one vforking and one not >> vforking, we could resume the vforking thread from the vforking >> inferior plus all threads from the vforking inferior. However, the > ^ > e > Did you forget a "non-" here? Yes thanks. The last vforking -> non-vforking. >> @@ -1034,6 +1046,53 @@ handle_vfork_child_exec_or_exit (int exec) >> } >> } >> >> +/* Handle TARGET_WAITKIND_VFORK_DONE. */ >> + >> +static void >> +handle_vfork_done (thread_info *event_thread) >> +{ >> + /* We only care about this event if inferior::thread_waiting_for_vfork_done is >> + set, that is if we are waiting for a vfork child not under our control >> + (because we detached it) to exec or exit. >> + >> + If an inferior has vforked and we are debugging the child, we don't use >> + the vfork-done event to get notified about the end of the shared address >> + space window). We rely instead on the child's exec or exit event, and the > ^ > > This closing parens has no corresponding opening one. Removed. >> @@ -1908,6 +1967,16 @@ start_step_over (void) >> continue; >> } >> >> + if (tp->inf->thread_waiting_for_vfork_done) > > Should be tp->inf->thread_waiting_for_vfork_done != nullptr Fixed. >> @@ -3268,7 +3383,12 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) >> error (_("Command aborted.")); >> } >> } >> - else if (!cur_thr->resumed () && !thread_is_in_step_over_chain (cur_thr)) >> + else if (!cur_thr->resumed () >> + && !thread_is_in_step_over_chain (cur_thr) >> + /* In non-stop, forbid resume a thread if some other thread of I fixed resume -> resuming. >> + that inferior is waiting for a vfork-done event (this means >> + breakpoints are out for this inferior). */ >> + && !(non_stop && cur_thr->inf->thread_waiting_for_vfork_done)) > > Should be cur_thr->inf->thread_waiting_for_vfork_done != nullptr Fixed. >> diff --git a/gdb/testsuite/gdb.threads/vfork-multi-thread.c b/gdb/testsuite/gdb.threads/vfork-multi-thread.c >> new file mode 100644 >> index 000000000000..cd3dfcc1e653 >> --- /dev/null >> +++ b/gdb/testsuite/gdb.threads/vfork-multi-thread.c >> @@ -0,0 +1,74 @@ >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +// Start with: >> +// >> +// ./gdb -nx -q --data-directory=data-directory repro -ex "tb break_here_first" -ex r -ex "b should_break_here" >> +// >> +// Then do "continue". >> +// >> +// The main thread will likely cross should_break_here while we are handling >> +// the vfork and breakpoints are removed, therefore missing the breakpoint. >> + >> +static volatile int release_vfork = 0; >> +static volatile int release_main = 0; >> + >> +static void *vforker(void *arg) > > shouldn’t vforker shoult be at column 0, even in test code? Looks like I completely forgot to clean up this file to make it clean for upstream. It's missing a copyright header and the formatting is off. I'll fix all I can find, including what you pointed out below. >> +# Run the test with the given GDB settings. >> + >> +proc do_test { target-non-stop non-stop follow-fork-mode detach-on-fork schedule-multiple } { >> + save_vars { ::GDBFLAGS } { >> + append ::GDBFLAGS " -ex \"maintenance set target-non-stop ${target-non-stop}\"" >> + append ::GDBFLAGS " -ex \"set non-stop ${non-stop}\"" >> + clean_restart ${::binfile} >> + } >> + >> + gdb_test_no_output "set follow-fork-mode ${follow-fork-mode}" >> + gdb_test_no_output "set detach-on-fork ${detach-on-fork}" >> + gdb_test_no_output "set schedule-multiple ${schedule-multiple}" > > OOC, why do you use GDBFLAGS to set 2 settings and gdb_test_no_output > for 3 settings? Because these settings have an effect during the clean_restart, when using some board files. The native-extended-gdbserver board, in particular. This board defines "starting GDB" as: - starting GDB - starting GDBserver with --multi - connecting GDB to GDBserver with extended-remote After clean_restart, the state is that GDB is connected to GDBserver but not debugging anything, ready to attach or run a program. And the non-stop and maint target-non-stop settings must be in effect before the remote connection is set up (for example GDB sends the QNonStop packet during the initial connection), it would be too late to set them after the clean_restart. Thanks for the comments. I saved the changes locally. Since it's all formatting and not functional changes, I won't send a new version right away, as I'm sure there will be more comments about the functionality eventually. Simon