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 E23293858004 for ; Mon, 22 Mar 2021 17:31:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E23293858004 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 12MHVStk017988 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 22 Mar 2021 13:31:33 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 12MHVStk017988 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)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 4959B1E54D; Mon, 22 Mar 2021 13:31:28 -0400 (EDT) Subject: Re: [PATCH 2/3] gdb: remove push_target free functions To: "Aktemur, Tankut Baris" Cc: "gdb-patches@sourceware.org" References: <20210322032027.3397705-1-simon.marchi@polymtl.ca> <20210322032027.3397705-2-simon.marchi@polymtl.ca> From: Simon Marchi Message-ID: <8c904425-6606-d227-ab67-876ee74211ee@polymtl.ca> Date: Mon, 22 Mar 2021 13:31:28 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 22 Mar 2021 17:31:28 +0000 X-Spam-Status: No, score=-10.3 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 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: Mon, 22 Mar 2021 17:31:38 -0000 On 2021-03-22 12:30 p.m., Aktemur, Tankut Baris wrote: > On Monday, March 22, 2021 4:20 AM, Simon Marchi wrote: >> Same as the previous patch, but for the push_target functions. >> >> The implementation of the move variant is moved to a new overload of >> inferior::push_target. >> >> gdb/ChangeLog: >> >> * target.h (push_target): Remove, update callers to use >> inferior::push_target. >> * target.c (push_target): Remove. >> * inferior.h (class inferior) : New overload. >> >> Change-Id: I5a95496666278b8f3965e5e8aecb76f54a97c185 >> --- >> gdb/aix-thread.c | 2 +- >> gdb/bsd-kvm.c | 2 +- >> gdb/bsd-uthread.c | 2 +- >> gdb/corelow.c | 2 +- >> gdb/darwin-nat.c | 2 +- >> gdb/exec.c | 4 ++-- >> gdb/gnu-nat.c | 6 +++--- >> gdb/go32-nat.c | 2 +- >> gdb/inf-child.c | 2 +- >> gdb/inf-ptrace.c | 7 ++++--- >> gdb/inferior.c | 2 +- >> gdb/inferior.h | 7 +++++++ >> gdb/infrun.c | 6 +++--- >> gdb/linux-thread-db.c | 2 +- >> gdb/nto-procfs.c | 4 ++-- >> gdb/procfs.c | 4 ++-- >> gdb/ravenscar-thread.c | 2 +- >> gdb/record-btrace.c | 2 +- >> gdb/record-full.c | 4 ++-- >> gdb/regcache.c | 2 +- >> gdb/remote-sim.c | 2 +- >> gdb/remote.c | 4 ++-- >> gdb/scoped-mock-context.h | 2 +- >> gdb/sol-thread.c | 2 +- >> gdb/target.c | 19 +------------------ >> gdb/target.h | 5 ----- >> gdb/tracectf.c | 2 +- >> gdb/tracefile-tfile.c | 2 +- >> gdb/windows-nat.c | 4 ++-- >> 29 files changed, 47 insertions(+), 61 deletions(-) >> >> diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c >> index a479d0150bc2..fc34210cf9e2 100644 >> --- a/gdb/aix-thread.c >> +++ b/gdb/aix-thread.c >> @@ -974,7 +974,7 @@ pd_enable (void) >> return; >> >> /* Prepare for thread debugging. */ >> - push_target (&aix_thread_ops); >> + current_inferior ()->push_target (&aix_thread_ops); >> pd_able = 1; >> >> /* If we're debugging a core file or an attached inferior, the >> diff --git a/gdb/bsd-kvm.c b/gdb/bsd-kvm.c >> index 17db2fe1cd60..89da40afcb78 100644 >> --- a/gdb/bsd-kvm.c >> +++ b/gdb/bsd-kvm.c >> @@ -134,7 +134,7 @@ bsd_kvm_target_open (const char *arg, int from_tty) >> bsd_kvm_corefile = filename; >> current_inferior ()->unpush_target (&bsd_kvm_ops); >> core_kd = temp_kd; >> - push_target (&bsd_kvm_ops); >> + current_inferior ()->push_target (&bsd_kvm_ops); >> >> thread_info *thr = add_thread_silent (&bsd_kvm_ops, bsd_kvm_ptid); >> switch_to_thread (thr); >> diff --git a/gdb/bsd-uthread.c b/gdb/bsd-uthread.c >> index 2ee47bfb5c47..3cb8f64c89a7 100644 >> --- a/gdb/bsd-uthread.c >> +++ b/gdb/bsd-uthread.c >> @@ -231,7 +231,7 @@ bsd_uthread_activate (struct objfile *objfile) >> bsd_uthread_thread_ctx_offset = >> bsd_uthread_lookup_offset ("_thread_ctx_offset", objfile); >> >> - push_target (&bsd_uthread_ops); >> + current_inferior ()->push_target (&bsd_uthread_ops); >> bsd_uthread_active = 1; >> return 1; >> } >> diff --git a/gdb/corelow.c b/gdb/corelow.c >> index a4c1f6354c6e..a1943ab2ea6d 100644 >> --- a/gdb/corelow.c >> +++ b/gdb/corelow.c >> @@ -458,7 +458,7 @@ core_target_open (const char *arg, int from_tty) >> if (!current_program_space->exec_bfd ()) >> set_gdbarch_from_file (core_bfd); >> >> - push_target (std::move (target_holder)); >> + current_inferior ()->push_target (std::move (target_holder)); >> >> switch_to_no_thread (); >> >> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c >> index ca95d7b6d385..f8e4443ff408 100644 >> --- a/gdb/darwin-nat.c >> +++ b/gdb/darwin-nat.c >> @@ -1659,7 +1659,7 @@ darwin_attach_pid (struct inferior *inf) >> >> target_ops *darwin_ops = get_native_target (); >> if (!target_is_pushed (darwin_ops)) >> - push_target (darwin_ops); >> + inf->push_target (darwin_ops); >> } >> >> /* Get the thread_info object corresponding to this darwin_thread_info. */ >> diff --git a/gdb/exec.c b/gdb/exec.c >> index bcc54bd966fe..b737bcf6e0f3 100644 >> --- a/gdb/exec.c >> +++ b/gdb/exec.c >> @@ -616,7 +616,7 @@ program_space::add_target_sections (void *owner, >> continue; >> >> switch_to_inferior_no_thread (inf); >> - push_target (&exec_ops); >> + inf->push_target (&exec_ops); > > Similar to the previous patch, it should be OK to also remove the > switch_to_inferior_no_thread here. Similarly, pushing a target can cause another (at the same stratum) to get unpushed, and some target_ops::close implementations refer to the current inferior. So out of caution I would leave it. I'll also add a note to the commit message here. > >> } >> } >> } >> @@ -682,7 +682,7 @@ void >> exec_on_vfork () >> { >> if (!current_program_space->target_sections ().empty ()) >> - push_target (&exec_ops); >> + current_inferior ()->push_target (&exec_ops); >> } >> >> >> >> >> diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c >> index 409d141909b3..9ea4c4089340 100644 >> --- a/gdb/gnu-nat.c >> +++ b/gdb/gnu-nat.c >> @@ -2114,7 +2114,7 @@ gnu_nat_target::create_inferior (const char *exec_file, >> inf_debug (inf, "creating inferior"); >> >> if (!target_is_pushed (this)) >> - push_target (this); >> + current_inferior ()->push_target (this); >> >> pid = fork_inferior (exec_file, allargs, env, gnu_ptrace_me, >> NULL, NULL, NULL, NULL); >> @@ -2190,9 +2190,9 @@ gnu_nat_target::attach (const char *args, int from_tty) >> >> inf_attach (inf, pid); >> >> - push_target (this); >> - >> inferior = current_inferior (); >> + inferior->push_target (this); >> + >> inferior_appeared (inferior, pid); >> inferior->attach_flag = 1; >> >> diff --git a/gdb/go32-nat.c b/gdb/go32-nat.c >> index b18a62e9490d..79c31fcd9877 100644 >> --- a/gdb/go32-nat.c >> +++ b/gdb/go32-nat.c >> @@ -757,7 +757,7 @@ go32_nat_target::create_inferior (const char *exec_file, >> inferior_appeared (inf, SOME_PID); >> >> if (!target_is_pushed (this)) >> - push_target (this); >> + inf->push_target (this); >> >> thread_info *thr = add_thread_silent (ptid_t (SOME_PID)); >> switch_to_thread (thr); >> diff --git a/gdb/inf-child.c b/gdb/inf-child.c >> index b8bc2e2598e6..0e68a40d7c04 100644 >> --- a/gdb/inf-child.c >> +++ b/gdb/inf-child.c >> @@ -166,7 +166,7 @@ inf_child_open_target (const char *arg, int from_tty) >> gdb_assert (dynamic_cast (target) != NULL); >> >> target_preopen (from_tty); >> - push_target (target); >> + current_inferior ()->push_target (target); >> inf_child_explicitly_opened = 1; >> if (from_tty) >> printf_filtered ("Done. Use the \"run\" command to start a process.\n"); >> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c >> index 7ca02dfd8764..e630ba447f40 100644 >> --- a/gdb/inf-ptrace.c >> +++ b/gdb/inf-ptrace.c >> @@ -82,7 +82,7 @@ inf_ptrace_target::create_inferior (const char *exec_file, >> if (! ops_already_pushed) >> { >> /* Clear possible core file with its process_stratum. */ >> - push_target (this); >> + current_inferior ()->push_target (this); >> unpusher.reset (this); >> } >> >> @@ -139,12 +139,14 @@ inf_ptrace_target::attach (const char *args, int from_tty) >> if (pid == getpid ()) /* Trying to masturbate? */ >> error (_("I refuse to debug myself!")); >> >> + inf = current_inferior (); >> + >> target_unpush_up unpusher; >> if (! ops_already_pushed) >> { >> /* target_pid_to_str already uses the target. Also clear possible core >> file with its process_stratum. */ >> - push_target (this); >> + inf->push_target (this); >> unpusher.reset (this); >> } >> >> @@ -169,7 +171,6 @@ inf_ptrace_target::attach (const char *args, int from_tty) >> error (_("This system does not support attaching to a process")); >> #endif >> >> - inf = current_inferior (); >> inferior_appeared (inf, pid); >> inf->attach_flag = 1; >> >> diff --git a/gdb/inferior.c b/gdb/inferior.c >> index 49f869a4c783..69baee34ce9d 100644 >> --- a/gdb/inferior.c >> +++ b/gdb/inferior.c >> @@ -771,7 +771,7 @@ switch_to_inferior_and_push_target (inferior *new_inf, >> /* Reuse the target for new inferior. */ >> if (!no_connection && proc_target != NULL) >> { >> - push_target (proc_target); >> + new_inf->push_target (proc_target); >> if (proc_target->connection_string () != NULL) >> printf_filtered (_("Added inferior %d on connection %d (%s %s)\n"), >> new_inf->num, >> diff --git a/gdb/inferior.h b/gdb/inferior.h >> index b8d5ff94fc56..66fc180ce530 100644 >> --- a/gdb/inferior.h >> +++ b/gdb/inferior.h >> @@ -352,6 +352,13 @@ class inferior : public refcounted_object >> void push_target (struct target_ops *t) >> { m_target_stack.push (t); } >> >> + /* An overload that deletes the target on failure. */ >> + void push_target (target_ops_up &&t) >> + { >> + m_target_stack.push (t.get ()); >> + t.release (); >> + } >> + >> /* Unpush T from this inferior's target stack. */ >> int unpush_target (struct target_ops *t) >> { return m_target_stack.unpush (t); } >> diff --git a/gdb/infrun.c b/gdb/infrun.c >> index 3b65a6de9fe2..50340e6edad4 100644 >> --- a/gdb/infrun.c >> +++ b/gdb/infrun.c >> @@ -477,7 +477,7 @@ holding the child stopped. Try \"set detach-on-fork\" or \ >> set_current_inferior (child_inf); >> switch_to_no_thread (); >> child_inf->symfile_flags = SYMFILE_NO_READ; >> - push_target (parent_inf->process_target ()); >> + child_inf->push_target (parent_inf->process_target ()); >> thread_info *child_thr >> = add_thread_silent (child_inf->process_target (), child_ptid); >> >> @@ -627,7 +627,7 @@ holding the child stopped. Try \"set detach-on-fork\" or \ >> informing the solib layer about this new process. */ >> >> set_current_inferior (child_inf); >> - push_target (target); >> + child_inf->push_target (target); >> } >> >> thread_info *child_thr = add_thread_silent (target, child_ptid); >> @@ -1183,7 +1183,7 @@ follow_exec (ptid_t ptid, const char *exec_file_target) >> >> inferior *org_inferior = current_inferior (); >> switch_to_inferior_no_thread (inf); > > And here. Same. Although here it's a brand new inferior, so we know there isn't an existing process target that is going to get unpushed. But also here, is it maybe important for the current inferior to be correctly set for the remainder of the follow_exec function? Simon