From patchwork Sat Dec 3 21:13:37 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 61457 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 662AD385B517 for ; Sat, 3 Dec 2022 21:14:40 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-wm1-f49.google.com (mail-wm1-f49.google.com [209.85.128.49]) by sourceware.org (Postfix) with ESMTPS id 2C1283858002 for ; Sat, 3 Dec 2022 21:13:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2C1283858002 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-f49.google.com with SMTP id ay27-20020a05600c1e1b00b003d070f4060bso7057362wmb.2 for ; Sat, 03 Dec 2022 13:13:53 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=4sZ4MvFeaH5AY0KGn/CfnqawYXxlBTBETfY74xmjyAk=; b=udW663nYvNWdXmbrTgmRc0fy7BgQx3WMW9jv6ZeGNf8K7RZlIFUtuqti9kym+zqDu8 4FsgORmh5RgCLaT7DpCF2mnylgJlIpRJZPZMdrW47ButnrXB6+n/5L+owttGl2qG0uKX RU8gwH1fSDHLwi4E41Kh8EFik2de9+Xb+FS5ZAI2BWVPP53CdG8YSwjffMEwgEfe7/sf 1H2Yl1rRdsftAdIOE3t0SHSMZvCqVaa2PbXDI0CnbpnO6Qvcfj5+2JyLeeiagIWjMcuc kCw+aPx4iFpZ2Q0nNEzb0i5BLhaTvzjilLXNjNuRuMVqkNLF6MO8jTfEZjjxphm5EWT7 CCvA== X-Gm-Message-State: ANoB5pnphspAR2UmupMKurBsSpYvQye4hSrB5VFjzb8NjQL1RknSr3ks IVyQViVVXwwmHV58ua7uIXQsr91HZ1j/hQ== X-Google-Smtp-Source: AA0mqf7QoyA5LfSp6RZpHC/AGg26v8gMr2l8E6F8OxKq1FsAHAQftcqfhShs6McGBNG0VXQtLXIPWA== X-Received: by 2002:a05:600c:1e89:b0:3cf:774b:ce6f with SMTP id be9-20020a05600c1e8900b003cf774bce6fmr12054759wmb.133.1670102032131; Sat, 03 Dec 2022 13:13:52 -0800 (PST) Received: from localhost ([2001:8a0:f912:6700:afd9:8b6d:223f:6170]) by smtp.gmail.com with ESMTPSA id co16-20020a0560000a1000b00241b6d27ef1sm10659199wrb.104.2022.12.03.13.13.51 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 03 Dec 2022 13:13:51 -0800 (PST) From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH 5/6] all-stop "follow-fork parent" and selecting another thread Date: Sat, 3 Dec 2022 21:13:37 +0000 Message-Id: <20221203211338.2264994-6-pedro@palves.net> X-Mailer: git-send-email 2.36.0 In-Reply-To: <20221203211338.2264994-1-pedro@palves.net> References: <20221203211338.2264994-1-pedro@palves.net> MIME-Version: 1.0 X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP 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: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" With: - catch a fork in thread 1 - select thread 2 - set follow-fork child - next ... follow_fork notices that thread 1 had last stopped for a fork which hasn't been followed yet, and because thread 1 is not the current thread, GDB aborts the execution command, presenting the stop in thread 1. That makes sense, as only the forking thread (thread 1) survives in the child, so better stop and let the user decide how to proceed. However, with: - catch a fork in thread 1 - select thread 2 - set follow-fork parent << note difference here - next ... GDB does the same: follow_fork notices that thread 1 had last stopped for a fork which hasn't been followed yet, and because thread 1 is not the current thread, GDB aborts the execution command, presenting the stop in thread 1. Aborting/stopping in this case doesn't make sense to me. As we're following the parent, thread 2 will still continue to exist in the parent. What the child does after we've followed the parent shouldn't matter -- it can go on running free, be detached, etc., depending on "set schedule-multiple", "set detach-on-fork", etc. That does not influence the execution command that the user issued for the parent thread. So this patch changes GDB in that direction -- in follow_fork, if following the parent, and we've switched threads meanwhile, switch back to the unfollowed thread, follow it (stay with the parent), and don't abort/stop. If we're following a fork (as opposed to vfork), then switch back again to the thread that the user was trying to resume. If following a vfork, however, stay with the vforking-thread selected, as we will need to see a vfork_done event first, before we can resume any other thread. As I was working on this, I managed to end up calling target_resume for a solo-thread resume (to collect the vfork_done event), with scope_ptid pointing at the vfork parent thread, and inferior_ptid pointing to the vfork child. For a solo-thread resume, the scope_ptid argument to target_resume must the same as inferior_ptid. The mistake was caught by the assertion in target_resume, like so: ... [infrun] resume_1: step=0, signal=GDB_SIGNAL_0, trap_expected=0, current thread [1722839.1722839.0] at 0x5555555553c3 [infrun] do_target_resume: resume_ptid=1722839.1722939.0, step=0, sig=GDB_SIGNAL_0 ../../src/gdb/target.c:2661: internal-error: target_resume: Assertion `inferior_ptid.matches (scope_ptid)' failed. ... but I think it doesn't hurt to catch such a mistake earlier, hence the change in internal_resume_ptid. Change-Id: I896705506a16d2488b1bfb4736315dd966f4e412 --- gdb/infrun.c | 88 ++++++++- .../gdb.threads/foll-fork-other-thread.c | 84 +++++++++ .../gdb.threads/foll-fork-other-thread.exp | 172 ++++++++++++++++++ 3 files changed, 335 insertions(+), 9 deletions(-) create mode 100644 gdb/testsuite/gdb.threads/foll-fork-other-thread.c create mode 100644 gdb/testsuite/gdb.threads/foll-fork-other-thread.exp diff --git a/gdb/infrun.c b/gdb/infrun.c index 617b6309e99..f1afc4ba8e2 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -756,13 +756,56 @@ follow_fork () if (tp == cur_thr) continue; - if (tp->pending_follow.kind () != TARGET_WAITKIND_SPURIOUS) + /* follow_fork_inferior clears tp->pending_follow, and below + we'll need the value after the follow_fork_inferior + call. */ + target_waitkind kind = tp->pending_follow.kind (); + + if (kind != TARGET_WAITKIND_SPURIOUS) { infrun_debug_printf ("need to follow-fork [%s] first", tp->ptid.to_string ().c_str ()); switch_to_thread (tp); - should_resume = false; + + /* Set up inferior(s) as specified by the caller, and + tell the target to do whatever is necessary to follow + either parent or child. */ + if (follow_child) + { + /* The thread that started the execution command + won't exist in the child. Abort the command and + immediately stop in this thread, in the child, + inside fork. */ + should_resume = false; + } + else + { + /* Following the parent, so let the thread fork its + child freely, it won't influence the current + execution command. */ + if (follow_fork_inferior (follow_child, detach_fork)) + { + /* Target refused to follow, or there's some + other reason we shouldn't resume. */ + switch_to_thread (cur_thr); + set_last_target_status_stopped (cur_thr); + return false; + } + + /* If we're following a vfork, when we need to leave + the just-forked thread as selected, as we need to + solo-resume it to collect the VFORK_DONE event. + If we're following a fork, however, switch back + to the original thread that we continue stepping + it, etc. */ + if (kind != TARGET_WAITKIND_VFORKED) + { + gdb_assert (kind == TARGET_WAITKIND_FORKED); + switch_to_thread (cur_thr); + } + } + break; } } @@ -2200,6 +2243,29 @@ user_visible_resume_target (ptid_t resume_ptid) : current_inferior ()->process_target ()); } +/* Find a thread from the inferiors that we'll resume that is waiting + for a vfork-done event. */ + +static thread_info * +find_thread_waiting_for_vfork_done () +{ + gdb_assert (!target_is_non_stop_p ()); + + if (sched_multi) + { + for (inferior *inf : all_non_exited_inferiors ()) + if (inf->thread_waiting_for_vfork_done != nullptr) + return inf->thread_waiting_for_vfork_done; + } + else + { + inferior *cur_inf = current_inferior (); + if (cur_inf->thread_waiting_for_vfork_done != nullptr) + return cur_inf->thread_waiting_for_vfork_done; + } + return nullptr; +} + /* Return a ptid representing the set of threads that we will resume, in the perspective of the target, assuming run control handling does not require leaving some threads stopped (e.g., stepping past @@ -2240,14 +2306,18 @@ internal_resume_ptid (int user_step) Since we don't have that flexibility (we can only pass one ptid), just resume the first thread waiting for a vfork-done event we find (e.g. thread 2.1). */ - if (sched_multi) - { - for (inferior *inf : all_non_exited_inferiors ()) - if (inf->thread_waiting_for_vfork_done != nullptr) - return inf->thread_waiting_for_vfork_done->ptid; + thread_info *thr = find_thread_waiting_for_vfork_done (); + if (thr != nullptr) + { + /* If we have a thread that is waiting for a vfork-done event, + then we should have switched to it earlier. Calling + target_resume with thread scope is only possible when the + current thread matches the thread scope. */ + gdb_assert (thr->ptid == inferior_ptid); + gdb_assert (thr->inf->process_target () + == inferior_thread ()->inf->process_target ()); + return thr->ptid; } - else if (current_inferior ()->thread_waiting_for_vfork_done != nullptr) - return current_inferior ()->thread_waiting_for_vfork_done->ptid; return user_visible_resume_ptid (user_step); } diff --git a/gdb/testsuite/gdb.threads/foll-fork-other-thread.c b/gdb/testsuite/gdb.threads/foll-fork-other-thread.c new file mode 100644 index 00000000000..47e8b1c7389 --- /dev/null +++ b/gdb/testsuite/gdb.threads/foll-fork-other-thread.c @@ -0,0 +1,84 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2022 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +#include +#include +#include +#include +#include +#include +#include + +/* Set by GDB. */ +volatile int stop_looping = 0; + +static void * +gdb_forker_thread (void *arg) +{ + int ret; + int stat; + pid_t pid = FORK_FUNC (); + + if (pid == 0) + _exit (0); + + assert (pid > 0); + + /* Wait for child to exit. */ + do + { + ret = waitpid (pid, &stat, 0); + } + while (ret == -1 && errno == EINTR); + + assert (ret == pid); + assert (WIFEXITED (stat)); + assert (WEXITSTATUS (stat) == 0); + + stop_looping = 1; + + return NULL; +} + +static void +sleep_a_bit (void) +{ + usleep (1000 * 50); +} + +int +main (void) +{ + int i; + int ret; + pthread_t thread; + + alarm (60); + + ret = pthread_create (&thread, NULL, gdb_forker_thread, NULL); + assert (ret == 0); + + while (!stop_looping) /* while loop */ + { + sleep_a_bit (); /* break here */ + sleep_a_bit (); /* other line */ + } + + pthread_join (thread, NULL); + + return 0; /* exiting here */ +} diff --git a/gdb/testsuite/gdb.threads/foll-fork-other-thread.exp b/gdb/testsuite/gdb.threads/foll-fork-other-thread.exp new file mode 100644 index 00000000000..ed62bfc54d9 --- /dev/null +++ b/gdb/testsuite/gdb.threads/foll-fork-other-thread.exp @@ -0,0 +1,172 @@ +# Copyright 2022 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +# Test catching a vfork/fork in one thread, and then doing a "next" in +# another thread, in different combinations of "set follow-fork +# parent/child", and other execution modes. + +standard_testfile + +# Line where to stop the main thread. +set break_here_line [gdb_get_line_number "break here"] + +# Build executables, one for each fork flavor. +foreach_with_prefix fork_func {fork vfork} { + set opts [list debug pthreads additional_flags=-DFORK_FUNC=${fork_func}] + if { [build_executable "failed to prepare" \ + ${testfile}-${fork_func} ${srcfile} $opts] } { + return + } +} + +# Run the test with the given parameters: +# +# - FORK_FUNC: fork flavor, "fork" or "vfork". +# - FOLLOW: "set follow-fork" value, either "parent" or "child". +# - TARGET-NON-STOP: "maintenance set target-non-stop" value, "auto", "on" or +# "off". +# - NON-STOP: "set non-stop" value, "on" or "off". +# - DISPLACED-STEPPING: "set displaced-stepping" value, "auto", "on" or "off". + +proc do_test { fork_func follow target-non-stop non-stop displaced-stepping } { + 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}-${fork_func} + } + + gdb_test_no_output "set displaced-stepping ${displaced-stepping}" + + if { ![runto_main] } { + return + } + + delete_breakpoints + + gdb_test "catch $fork_func" "Catchpoint .*" + + # Verify that the catchpoint is mentioned in an "info breakpoints", + # and further that the catchpoint mentions no process id. + gdb_test "info breakpoints" \ + ".*catchpoint.*keep y.*fork\[\r\n\]+" \ + "info breakpoints before fork" + + gdb_test "continue" \ + "Catchpoint \[0-9\]* \\(.?forked process \[0-9\]*\\),.*" \ + "explicit child follow, catch fork" + + # Verify that the catchpoint is mentioned in an "info breakpoints", + # and further that the catchpoint managed to capture a process id. + gdb_test "info breakpoints" \ + ".*catchpoint.*keep y.*fork, process.*" \ + "info breakpoints after fork" + + gdb_test "thread 1" "Switching to .*" + + gdb_test_no_output "set scheduler-locking on" + + # Advance the next-ing thread to the point where we'll execute the + # next. + gdb_test "break $::srcfile:$::break_here_line" "Breakpoint $::decimal at $::hex.*" + gdb_test "continue" "hit Breakpoint $::decimal, main.*" + + # Disable schedlock and step. The pending fork should no longer + # be pending afterwards. + + gdb_test "set scheduler-locking off" + + # Make sure GDB doesn't try to step over the breakpoint at PC + # first, we want to make sure that GDB doesn't lose focus of the + # step/next in this thread. A breakpoint would make GDB switch + # focus anyhow, thus hide a potential bug. + delete_breakpoints + + gdb_test_no_output "set follow-fork $follow" + + set any "\[^\r\n\]*" + + if {$follow == "child"} { + + # For fork, GDB detaches from the parent at follow-fork time. + # For vfork, GDB detaches from the parent at child exit/exec + # time. + if {$fork_func == "fork"} { + set detach_parent \ + [multi_line \ + "\\\[Detaching after $fork_func from parent process $any\\\]" \ + "\\\[Inferior 1 $any detached\\\]"] + } else { + set detach_parent "" + } + + gdb_test "next" \ + [multi_line \ + "\\\[Attaching after $any $fork_func to child $any\\\]" \ + "\\\[New inferior 2 $any\\\]" \ + "$detach_parent.*warning: Not resuming: switched threads before following fork child\\." \ + "\\\[Switching to $any\\\]" \ + ".*"] \ + "next aborts resumption" + + # The child should be stopped inside the fork implementation + # in the runtime. Exactly at which instruction/function is + # system dependent, but we can check that our + # "gdb_forker_thread" function appears in the backtrace. + gdb_test "bt" " in gdb_forker_thread ${any} at ${any}${::srcfile}:.*" + + # The child is now thread 1. + gdb_test "print \$_thread" " = 1" + + if {$fork_func == "fork"} { + gdb_test "continue" \ + [multi_line \ + "Continuing." \ + "\\\[Inferior 2 \\\(process $any\\\) exited normally\\\]"] \ + "continue to exit" + } else { + gdb_test "continue" \ + [multi_line \ + "Continuing." \ + "\\\[Detaching vfork parent process $any after child exit\\\]" \ + "\\\[Inferior 1 \\\(process $any\\\) detached\\\]" \ + "\\\[Inferior 2 \\\(process $any\\\) exited normally\\\]"] \ + "continue to exit" + } + } else { + gdb_test "next" \ + "\\\[Detaching after $fork_func from child process ${any}\\\].* other line .*" \ + "next to other line" + + gdb_test "print \$_thread" " = 1" + + gdb_test "continue" \ + [multi_line \ + "Continuing." \ + "\\\[Inferior 1 \\\(process $any\\\) exited normally\\\]"] \ + "continue to exit" + } +} + +foreach_with_prefix fork_func {fork vfork} { + foreach_with_prefix follow {child} { + foreach_with_prefix target-non-stop {auto on off} { + foreach_with_prefix non-stop {off} { + foreach_with_prefix displaced-stepping {auto on off} { + do_test ${fork_func} ${follow} ${target-non-stop} ${non-stop} ${displaced-stepping} + } + } + } + } +}