From patchwork Sat Dec 13 22:20:44 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 4239 Received: (qmail 12364 invoked by alias); 13 Dec 2014 22:20:52 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 12354 invoked by uid 89); 13 Dec 2014 22:20:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-qc0-f180.google.com Received: from mail-qc0-f180.google.com (HELO mail-qc0-f180.google.com) (209.85.216.180) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Sat, 13 Dec 2014 22:20:48 +0000 Received: by mail-qc0-f180.google.com with SMTP id i8so7253976qcq.11 for ; Sat, 13 Dec 2014 14:20:46 -0800 (PST) X-Received: by 10.224.104.2 with SMTP id m2mr15044653qao.38.1418509246414; Sat, 13 Dec 2014 14:20:46 -0800 (PST) Received: from [10.0.0.11] ([96.127.221.218]) by mx.google.com with ESMTPSA id i49sm5606171qge.48.2014.12.13.14.20.45 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 13 Dec 2014 14:20:45 -0800 (PST) Message-ID: <548CBBBC.1090909@polymtl.ca> Date: Sat, 13 Dec 2014 17:20:44 -0500 From: Simon Marchi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Joel Brobecker , simon.marchi@ericsson.com CC: gdb-patches@sourceware.org Subject: Re: [PATCH] Introduce utility function find_inferior_ptid References: <1418412193-6259-1-git-send-email-simon.marchi@ericsson.com> <20141213144058.GI5457@adacore.com> In-Reply-To: <20141213144058.GI5457@adacore.com> On 13/12/14 09:40 AM, Joel Brobecker wrote: >> This patch introduces find_inferior_ptid to replace the common idiom >> >> find_inferior_pid (ptid_get_pid (...)); >> >> It replaces all the instances of that idiom that I found with the new >> function. >> >> The change was mostly mechanical and built-tested. >> >> gdb/ChangeLog: >> >> * inferior.c (find_inferior_ptid): New function. >> * inferior.h (find_inferior_ptid): New declaration. >> * ada-tasks.c (ada_get_task_number): Use find_inferior_ptid. >> * corelow.c (core_pid_to_str): Same. >> * darwin-nat.c (darwin_resume): Same. >> * infrun.c (fetch_inferior_event): Same. >> (get_inferior_stop_soon): Same. >> (handle_inferior_event): Same. >> (handle_signal_stop): Same. >> * linux-nat.c (resume_lwp): Same. >> (stop_wait_callback): Same. >> * mi/mi-interp.c (mi_new_thread): Same. >> (mi_thread_exit): Same. >> * proc-service.c (ps_pglobal_lookup): Same. >> * record-btrace.c (record_btrace_step_thread): Same. >> * remote-sim.c (gdbsim_close_inferior): Same. >> (gdbsim_resume): Same. >> (gdbsim_stop): Same. >> * sol2-tdep.c (sol2_core_pid_to_str): Same. >> * target.c (memory_xfer_partial_1): Same. >> (default_thread_address_space): Same. >> * thread.c (thread_change_ptid): Same. >> (switch_to_thread): Same. >> (do_restore_current_thread_cleanup): Same. > > Overall, this looks reasonable. A couple of comments below. > Also, while at it, I would prefer if you tested the change. > With the number of mechanical changes, it's really easy to > let one bad change slip by, no matter how careful we try > to be; testing does not cost much so let's add that as well. Oops, it's a very good thing you made me test the patch again, because there is an horrible, obvious mistake in: struct inferior * find_inferior_ptid (ptid_t ptid) { return find_inferior_ptid (ptid); I actually forgot to git add/git commit --amend my latest fixes before sending the patch, so some important changes about that were still unstaged in my git tree. There were a few extraneous closing parenthesis left. With v2 (below), the testsuite results look similar before/after (just the usual small variations), although it's just for amd64 Linux, and the change touches other targets. >> >> +struct inferior * >> +find_inferior_ptid (ptid_t ptid) { >> + return find_inferior_ptid (ptid); >> +} > > This function needs an introductory commend ("/* See inferior.h */"). > This is something we do systematically now, even if the function's > documentation is present elsewhere. The one-line comment allows us > to know that there is a command, in incidentally where it is. Ok. > Also, the opening curly brace needs to be on the next line. Therefore: > > /* See inferior.h. */ > > struct inferior * > find_inferior_ptid (ptid_t ptid) > { > return find_inferior_ptid (ptid); > } Ok. >> +/* Search function to lookup an inferior whose pid is equal to 'ptid.pid'. */ >> +extern struct inferior * find_inferior_ptid (ptid_t ptid); > > No space after the '*'. Ok. > Pre-approved with those changes and after testing confirmed. > > Thank you, Ok, here is what I will push on Monday if nothing comes up. From 5756f3cd6c26a16d9e0e57b7fd8044c635d0a53d Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Sat, 13 Dec 2014 12:12:10 -0500 Subject: [PATCH] Introduce utility function find_inferior_ptid This patch introduces find_inferior_ptid to replace the common idiom find_inferior_pid (ptid_get_pid (...)); It replaces all the instances of that idiom that I found with the new function. No significant changes before/after the patch in the regression suite on amd64 linux. gdb/ChangeLog: * inferior.c (find_inferior_ptid): New function. * inferior.h (find_inferior_ptid): New declaration. * ada-tasks.c (ada_get_task_number): Use find_inferior_ptid. * corelow.c (core_pid_to_str): Same. * darwin-nat.c (darwin_resume): Same. * infrun.c (fetch_inferior_event): Same. (get_inferior_stop_soon): Same. (handle_inferior_event): Same. (handle_signal_stop): Same. * linux-nat.c (resume_lwp): Same. (stop_wait_callback): Same. * mi/mi-interp.c (mi_new_thread): Same. (mi_thread_exit): Same. * proc-service.c (ps_pglobal_lookup): Same. * record-btrace.c (record_btrace_step_thread): Same. * remote-sim.c (gdbsim_close_inferior): Same. (gdbsim_resume): Same. (gdbsim_stop): Same. * sol2-tdep.c (sol2_core_pid_to_str): Same. * target.c (memory_xfer_partial_1): Same. (default_thread_address_space): Same. * thread.c (thread_change_ptid): Same. (switch_to_thread): Same. (do_restore_current_thread_cleanup): Same. --- gdb/ada-tasks.c | 3 +-- gdb/corelow.c | 2 +- gdb/darwin-nat.c | 2 +- gdb/inferior.c | 8 ++++++++ gdb/inferior.h | 3 +++ gdb/infrun.c | 10 +++++----- gdb/linux-nat.c | 4 ++-- gdb/mi/mi-interp.c | 4 ++-- gdb/proc-service.c | 2 +- gdb/record-btrace.c | 4 ++-- gdb/remote-sim.c | 6 +++--- gdb/sol2-tdep.c | 2 +- gdb/target.c | 4 ++-- gdb/thread.c | 6 +++--- 14 files changed, 35 insertions(+), 25 deletions(-) diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c index 2d5a19d..17d0338 100644 --- a/gdb/ada-tasks.c +++ b/gdb/ada-tasks.c @@ -292,7 +292,7 @@ int ada_get_task_number (ptid_t ptid) { int i; - struct inferior *inf = find_inferior_pid (ptid_get_pid (ptid)); + struct inferior *inf = find_inferior_ptid (ptid); struct ada_tasks_inferior_data *data; gdb_assert (inf != NULL); @@ -1449,4 +1449,3 @@ _initialize_tasks (void) Without argument, this command simply prints the current task ID"), &cmdlist); } - diff --git a/gdb/corelow.c b/gdb/corelow.c index b91ad22..154b2c4 100644 --- a/gdb/corelow.c +++ b/gdb/corelow.c @@ -954,7 +954,7 @@ core_pid_to_str (struct target_ops *ops, ptid_t ptid) /* Otherwise, this isn't a "threaded" core -- use the PID field, but only if it isn't a fake PID. */ - inf = find_inferior_pid (ptid_get_pid (ptid)); + inf = find_inferior_ptid (ptid); if (inf != NULL && !inf->fake_pid_p) return normal_pid_to_str (ptid); diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c index 511f370..36b6021 100644 --- a/gdb/darwin-nat.c +++ b/gdb/darwin-nat.c @@ -859,7 +859,7 @@ darwin_resume (ptid_t ptid, int step, enum gdb_signal signal) } else { - struct inferior *inf = find_inferior_pid (ptid_get_pid (ptid)); + struct inferior *inf = find_inferior_ptid (ptid); long tid = ptid_get_tid (ptid); /* Stop the inferior (should be useless). */ diff --git a/gdb/inferior.c b/gdb/inferior.c index 44f4560..49c479d 100644 --- a/gdb/inferior.c +++ b/gdb/inferior.c @@ -367,6 +367,14 @@ find_inferior_pid (int pid) return NULL; } +/* See inferior.h */ + +struct inferior * +find_inferior_ptid (ptid_t ptid) +{ + return find_inferior_pid (ptid_get_pid (ptid)); +} + /* See inferior.h. */ struct inferior * diff --git a/gdb/inferior.h b/gdb/inferior.h index 0129549..eebc034 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -465,6 +465,9 @@ extern int valid_gdb_inferior_id (int num); /* Search function to lookup an inferior by target 'pid'. */ extern struct inferior *find_inferior_pid (int pid); +/* Search function to lookup an inferior whose pid is equal to 'ptid.pid'. */ +extern struct inferior *find_inferior_ptid (ptid_t ptid); + /* Search function to lookup an inferior by GDB 'num'. */ extern struct inferior *find_inferior_id (int num); diff --git a/gdb/infrun.c b/gdb/infrun.c index 74f9e12..413b052 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -3262,7 +3262,7 @@ fetch_inferior_event (void *client_data) if (!ecs->wait_some_more) { - struct inferior *inf = find_inferior_pid (ptid_get_pid (ecs->ptid)); + struct inferior *inf = find_inferior_ptid (ecs->ptid); delete_just_stopped_threads_infrun_breakpoints (); @@ -3581,7 +3581,7 @@ fill_in_stop_func (struct gdbarch *gdbarch, static enum stop_kind get_inferior_stop_soon (ptid_t ptid) { - struct inferior *inf = find_inferior_pid (ptid_get_pid (ptid)); + struct inferior *inf = find_inferior_ptid (ptid); gdb_assert (inf != NULL); return inf->control.stop_soon; @@ -3815,7 +3815,7 @@ handle_inferior_event (struct execution_control_state *ecs) } inferior_ptid = ecs->ptid; - set_current_inferior (find_inferior_pid (ptid_get_pid (ecs->ptid))); + set_current_inferior (find_inferior_ptid (ecs->ptid)); set_current_program_space (current_inferior ()->pspace); handle_vfork_child_exec_or_exit (0); target_terminal_ours (); /* Must do this before mourn anyway. */ @@ -3899,7 +3899,7 @@ Cannot fill $_exitsignal with the correct signal number.\n")); if (displaced && ptid_equal (displaced->step_ptid, ecs->ptid)) { struct inferior *parent_inf - = find_inferior_pid (ptid_get_pid (ecs->ptid)); + = find_inferior_ptid (ecs->ptid); struct regcache *child_regcache; CORE_ADDR parent_pc; @@ -4495,7 +4495,7 @@ handle_signal_stop (struct execution_control_state *ecs) if (random_signal) { /* Signal not for debugging purposes. */ - struct inferior *inf = find_inferior_pid (ptid_get_pid (ecs->ptid)); + struct inferior *inf = find_inferior_ptid (ecs->ptid); enum gdb_signal stop_signal = ecs->event_thread->suspend.stop_signal; if (debug_infrun) diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c index 21797c1..29133f9 100644 --- a/gdb/linux-nat.c +++ b/gdb/linux-nat.c @@ -1435,7 +1435,7 @@ resume_lwp (struct lwp_info *lp, int step, enum gdb_signal signo) { if (lp->stopped) { - struct inferior *inf = find_inferior_pid (ptid_get_pid (lp->ptid)); + struct inferior *inf = find_inferior_ptid (lp->ptid); if (inf->vfork_child != NULL) { @@ -2388,7 +2388,7 @@ linux_nat_set_status_is_event (struct target_ops *t, static int stop_wait_callback (struct lwp_info *lp, void *data) { - struct inferior *inf = find_inferior_pid (ptid_get_pid (lp->ptid)); + struct inferior *inf = find_inferior_ptid (lp->ptid); /* If this is a vfork parent, bail out, it is not going to report any SIGSTOP until the vfork is done with. */ diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c index 60f0666..09ff7bf 100644 --- a/gdb/mi/mi-interp.c +++ b/gdb/mi/mi-interp.c @@ -371,7 +371,7 @@ static void mi_new_thread (struct thread_info *t) { struct mi_interp *mi = top_level_interpreter_data (); - struct inferior *inf = find_inferior_pid (ptid_get_pid (t->ptid)); + struct inferior *inf = find_inferior_ptid (t->ptid); gdb_assert (inf); @@ -391,7 +391,7 @@ mi_thread_exit (struct thread_info *t, int silent) if (silent) return; - inf = find_inferior_pid (ptid_get_pid (t->ptid)); + inf = find_inferior_ptid (t->ptid); mi = top_level_interpreter_data (); old_chain = make_cleanup_restore_target_terminal (); diff --git a/gdb/proc-service.c b/gdb/proc-service.c index 5819489..1dc98d8 100644 --- a/gdb/proc-service.c +++ b/gdb/proc-service.c @@ -114,7 +114,7 @@ ps_pglobal_lookup (gdb_ps_prochandle_t ph, const char *obj, { struct bound_minimal_symbol ms; struct cleanup *old_chain = save_current_program_space (); - struct inferior *inf = find_inferior_pid (ptid_get_pid (ph->ptid)); + struct inferior *inf = find_inferior_ptid (ph->ptid); ps_err_e result; set_current_program_space (inf->pspace); diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c index 65d28e3..8b98a91 100644 --- a/gdb/record-btrace.c +++ b/gdb/record-btrace.c @@ -1662,7 +1662,7 @@ record_btrace_step_thread (struct thread_info *tp) if (replay == NULL) return btrace_step_no_history (); - inf = find_inferior_pid (ptid_get_pid (tp->ptid)); + inf = find_inferior_ptid (tp->ptid); aspace = inf->aspace; /* Determine the end of the instruction trace. */ @@ -1699,7 +1699,7 @@ record_btrace_step_thread (struct thread_info *tp) if (replay == NULL) replay = record_btrace_start_replaying (tp); - inf = find_inferior_pid (ptid_get_pid (tp->ptid)); + inf = find_inferior_ptid (tp->ptid); aspace = inf->aspace; for (;;) diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c index 8901548..92c41ad 100644 --- a/gdb/remote-sim.c +++ b/gdb/remote-sim.c @@ -774,7 +774,7 @@ gdbsim_close_inferior (struct inferior *inf, void *arg) Thus we need to verify the existence of an inferior using the pid in question before setting inferior_ptid via switch_to_thread() or mourning the inferior. */ - if (find_inferior_pid (ptid_get_pid (ptid)) != NULL) + if (find_inferior_ptid (ptid) != NULL) { switch_to_thread (ptid); generic_mourn_inferior (); @@ -881,7 +881,7 @@ gdbsim_resume (struct target_ops *ops, either have multiple inferiors to resume or an error condition. */ if (sim_data) - gdbsim_resume_inferior (find_inferior_pid (ptid_get_pid (ptid)), &rd); + gdbsim_resume_inferior (find_inferior_ptid (ptid), &rd); else if (ptid_equal (ptid, minus_one_ptid)) iterate_over_inferiors (gdbsim_resume_inferior, &rd); else @@ -928,7 +928,7 @@ gdbsim_stop (struct target_ops *self, ptid_t ptid) } else { - struct inferior *inf = find_inferior_pid (ptid_get_pid (ptid)); + struct inferior *inf = find_inferior_ptid (ptid); if (inf == NULL) error (_("Can't stop pid %d. No inferior found."), diff --git a/gdb/sol2-tdep.c b/gdb/sol2-tdep.c index bf7d6a1..a508978 100644 --- a/gdb/sol2-tdep.c +++ b/gdb/sol2-tdep.c @@ -60,7 +60,7 @@ sol2_core_pid_to_str (struct gdbarch *gdbarch, ptid_t ptid) /* GDB didn't use to put a NT_PSTATUS note in Solaris cores. If that's missing, then we're dealing with a fake PID corelow.c made up. */ - inf = find_inferior_pid (ptid_get_pid (ptid)); + inf = find_inferior_ptid (ptid); if (inf == NULL || inf->fake_pid_p) { xsnprintf (buf, sizeof buf, ""); diff --git a/gdb/target.c b/gdb/target.c index 7161e62..38c6c16 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -1173,7 +1173,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object, return TARGET_XFER_E_IO; if (!ptid_equal (inferior_ptid, null_ptid)) - inf = find_inferior_pid (ptid_get_pid (inferior_ptid)); + inf = find_inferior_ptid (inferior_ptid); else inf = NULL; @@ -2649,7 +2649,7 @@ default_thread_address_space (struct target_ops *self, ptid_t ptid) struct inferior *inf; /* Fall-back to the "main" address space of the inferior. */ - inf = find_inferior_pid (ptid_get_pid (ptid)); + inf = find_inferior_ptid (ptid); if (inf == NULL || inf->aspace == NULL) internal_error (__FILE__, __LINE__, diff --git a/gdb/thread.c b/gdb/thread.c index a3040a7..782fc70 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -747,7 +747,7 @@ thread_change_ptid (ptid_t old_ptid, ptid_t new_ptid) /* It can happen that what we knew as the target inferior id changes. E.g, target remote may only discover the remote process pid after adding the inferior to GDB's list. */ - inf = find_inferior_pid (ptid_get_pid (old_ptid)); + inf = find_inferior_ptid (old_ptid); inf->pid = ptid_get_pid (new_ptid); tp = find_thread_ptid (old_ptid); @@ -1179,7 +1179,7 @@ switch_to_thread (ptid_t ptid) { struct inferior *inf; - inf = find_inferior_pid (ptid_get_pid (ptid)); + inf = find_inferior_ptid (ptid); gdb_assert (inf != NULL); set_current_program_space (inf->pspace); set_current_inferior (inf); @@ -1290,7 +1290,7 @@ do_restore_current_thread_cleanup (void *arg) then don't revert back to it, but instead simply drop back to no thread selected. */ if (tp - && find_inferior_pid (ptid_get_pid (tp->ptid)) != NULL) + && find_inferior_ptid (tp->ptid) != NULL) restore_current_thread (old->inferior_ptid); else {