From patchwork Fri Sep 2 08:04:57 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yao Qi X-Patchwork-Id: 15176 Received: (qmail 45582 invoked by alias); 2 Sep 2016 08:05:21 -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 45534 invoked by uid 89); 2 Sep 2016 08:05:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy=H*RU:209.85.220.68, Hx-spam-relays-external:209.85.220.68, sk:status_ X-HELO: mail-pa0-f68.google.com Received: from mail-pa0-f68.google.com (HELO mail-pa0-f68.google.com) (209.85.220.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 02 Sep 2016 08:05:08 +0000 Received: by mail-pa0-f68.google.com with SMTP id ez1so5259503pab.3 for ; Fri, 02 Sep 2016 01:05:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-transfer-encoding; bh=Ao9AFhsUN2IuLPCyG8R3itfWnKAMkPD4r9x6kRsgSbw=; b=dLlJREMbBqrY0Ld+vtBcscmQ8UHseVDNwawNPhzyRjhkvq/aXRAfjorV2NfKRf1xoq 9OXxMMW0I59NQE/C0za1m3Ww1h/gj4ZCX8oRdrZaTkZ4Z69IqEt7eXsdqY2WcMR+JLmO bBzkXzbvX9cy97/iHE1fPIEusHSTWen2gsSB9q5rXsfBUKRT7pIpC+zu7QTuk19vt2Vo w4gZElK5sJwfq5fCaC9avMZhaFc6Az4SpLYW3ku7Evy+Z8m+Z4m6xPilSJZNwY/61Bcv jSAJUK/7f2bjG8gFBawVgDFRfogg/wqaty16xYGlF/22ZzlwTzBdK9ItTjpFgQelFIPc qiCw== X-Gm-Message-State: AE9vXwPeb7u4O/36Azx5EOSMtvi66Zd8g2pxpFLKXHoCXJvhJ8d/MrtR3rmsLPYTFETi9g== X-Received: by 10.66.173.14 with SMTP id bg14mr34602578pac.42.1472803506760; Fri, 02 Sep 2016 01:05:06 -0700 (PDT) Received: from E107787-LIN (gcc115.osuosl.org. [140.211.9.73]) by smtp.gmail.com with ESMTPSA id d9sm12678802pan.7.2016.09.02.01.05.01 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Fri, 02 Sep 2016 01:05:06 -0700 (PDT) From: Yao Qi To: Pedro Alves Cc: Yao Qi , gdb-patches@sourceware.org Subject: Re: [PATCH] [GDBserver] Replace "reinsert_breakpoint" with "singlestep_breakpoint" References: <1472720480-4651-1-git-send-email-yao.qi@linaro.org> Date: Fri, 02 Sep 2016 09:04:57 +0100 In-Reply-To: (Pedro Alves's message of "Thu, 1 Sep 2016 17:18:32 +0100") Message-ID: <86r392buc6.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes Pedro Alves writes: > I hate to be this picky, but I have to say that it itches me that both > gdb and gdbserver currently largely use "single-step" instead of "singlestep" > in comments and largely uses "single_step" instead of "singlestep" in > function signatures: > > $ cd gdb > $ grep -rn "single-step" --exclude='ChangeLog*' --exclude="gdbarch.c" > --exclude="gdbarch.h" | wc -l > 371 > $ grep -rn "single_step" --exclude='ChangeLog*' --exclude="gdbarch.c" > --exclude="gdbarch.h" | wc -l > 276 > $ grep -rn "singlestep" --exclude='ChangeLog*' --exclude="gdbarch.c" > --exclude="gdbarch.h" | wc -l > 37 > > Did you consider following this scheme? Sure. Let us use "single_step" in code and "single-step" in comments. How about the patch below? diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c index 45061ac..21bc06e 100644 --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -550,11 +550,11 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat) && can_software_single_step () && event == PTRACE_EVENT_VFORK) { - /* If we leave reinsert breakpoints there, child will - hit it, so uninsert reinsert breakpoints from parent + /* If we leave single-step breakpoints there, child will + hit it, so uninsert single-step breakpoints from parent (and child). Once vfork child is done, reinsert them back to parent. */ - uninsert_reinsert_breakpoints (event_thr); + uninsert_single_step_breakpoints (event_thr); } clone_all_breakpoints (child_thr, event_thr); @@ -581,8 +581,8 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat) event_lwp->status_pending_p = 1; event_lwp->status_pending = wstat; - /* If the parent thread is doing step-over with reinsert - breakpoints, the list of reinsert breakpoints are cloned + /* If the parent thread is doing step-over with single-step + breakpoints, the list of single-step breakpoints are cloned from the parent's. Remove them from the child process. In case of vfork, we'll reinsert them back once vforked child is done. */ @@ -592,10 +592,10 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat) /* The child process is forked and stopped, so it is safe to access its memory without stopping all other threads from other processes. */ - delete_reinsert_breakpoints (child_thr); + delete_single_step_breakpoints (child_thr); - gdb_assert (has_reinsert_breakpoints (event_thr)); - gdb_assert (!has_reinsert_breakpoints (child_thr)); + gdb_assert (has_single_step_breakpoints (event_thr)); + gdb_assert (!has_single_step_breakpoints (child_thr)); } /* Report the event. */ @@ -649,9 +649,9 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat) if (event_lwp->bp_reinsert != 0 && can_software_single_step ()) { - reinsert_reinsert_breakpoints (event_thr); + reinsert_single_step_breakpoints (event_thr); - gdb_assert (has_reinsert_breakpoints (event_thr)); + gdb_assert (has_single_step_breakpoints (event_thr)); } /* Report the event. */ @@ -2622,9 +2622,9 @@ maybe_hw_step (struct thread_info *thread) return 1; else { - /* GDBserver must insert reinsert breakpoint for software + /* GDBserver must insert single-step breakpoint for software single step. */ - gdb_assert (has_reinsert_breakpoints (thread)); + gdb_assert (has_single_step_breakpoints (thread)); return 0; } } @@ -3330,13 +3330,13 @@ linux_wait_1 (ptid_t ptid, the breakpoint address. So in the case of the hardware single step advance the PC manually past the breakpoint and in the case of software single step advance only - if it's not the reinsert_breakpoint we are hitting. + if it's not the single_step_breakpoint we are hitting. This avoids that a program would keep trapping a permanent breakpoint forever. */ if (!ptid_equal (step_over_bkpt, null_ptid) && event_child->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT && (event_child->stepping - || !reinsert_breakpoint_inserted_here (event_child->stop_pc))) + || !single_step_breakpoint_inserted_here (event_child->stop_pc))) { int increment_pc = 0; int breakpoint_kind = 0; @@ -3390,8 +3390,8 @@ linux_wait_1 (ptid_t ptid, /* We have a SIGTRAP, possibly a step-over dance has just finished. If so, tweak the state machine accordingly, - reinsert breakpoints and delete any reinsert (software - single-step) breakpoints. */ + reinsert breakpoints and delete any single-step + breakpoints. */ step_over_finished = finish_step_over (event_child); /* Now invoke the callbacks of any internal breakpoints there. */ @@ -3705,48 +3705,48 @@ linux_wait_1 (ptid_t ptid, /* Alright, we're going to report a stop. */ - /* Remove reinsert breakpoints. */ + /* Remove single-step breakpoints. */ if (can_software_single_step ()) { - /* Remove reinsert breakpoints or not. It it is true, stop all + /* Remove single-step breakpoints or not. It it is true, stop all lwps, so that other threads won't hit the breakpoint in the staled memory. */ - int remove_reinsert_breakpoints_p = 0; + int remove_single_step_breakpoints_p = 0; if (non_stop) { - remove_reinsert_breakpoints_p - = has_reinsert_breakpoints (current_thread); + remove_single_step_breakpoints_p + = has_single_step_breakpoints (current_thread); } else { /* In all-stop, a stop reply cancels all previous resume - requests. Delete all reinsert breakpoints. */ + requests. Delete all single-step breakpoints. */ struct inferior_list_entry *inf, *tmp; ALL_INFERIORS (&all_threads, inf, tmp) { struct thread_info *thread = (struct thread_info *) inf; - if (has_reinsert_breakpoints (thread)) + if (has_single_step_breakpoints (thread)) { - remove_reinsert_breakpoints_p = 1; + remove_single_step_breakpoints_p = 1; break; } } } - if (remove_reinsert_breakpoints_p) + if (remove_single_step_breakpoints_p) { - /* If we remove reinsert breakpoints from memory, stop all lwps, + /* If we remove single-step breakpoints from memory, stop all lwps, so that other threads won't hit the breakpoint in the staled memory. */ stop_all_lwps (0, event_child); if (non_stop) { - gdb_assert (has_reinsert_breakpoints (current_thread)); - delete_reinsert_breakpoints (current_thread); + gdb_assert (has_single_step_breakpoints (current_thread)); + delete_single_step_breakpoints (current_thread); } else { @@ -3756,8 +3756,8 @@ linux_wait_1 (ptid_t ptid, { struct thread_info *thread = (struct thread_info *) inf; - if (has_reinsert_breakpoints (thread)) - delete_reinsert_breakpoints (thread); + if (has_single_step_breakpoints (thread)) + delete_single_step_breakpoints (thread); } } @@ -4279,7 +4279,7 @@ install_software_single_step_breakpoints (struct lwp_info *lwp) next_pcs = (*the_low_target.get_next_pcs) (regcache); for (i = 0; VEC_iterate (CORE_ADDR, next_pcs, i, pc); ++i) - set_reinsert_breakpoint (pc, current_ptid); + set_single_step_breakpoint (pc, current_ptid); do_cleanups (old_chain); } @@ -4880,7 +4880,7 @@ start_step_over (struct lwp_info *lwp) } /* Finish a step-over. Reinsert the breakpoint we had uninserted in - start_step_over, if still there, and delete any reinsert + start_step_over, if still there, and delete any single-step breakpoints we've set, on non hardware single-step targets. */ static int @@ -4902,15 +4902,15 @@ finish_step_over (struct lwp_info *lwp) lwp->bp_reinsert = 0; - /* Delete any software-single-step reinsert breakpoints. No - longer needed. We don't have to worry about other threads - hitting this trap, and later not being able to explain it, - because we were stepping over a breakpoint, and we hold all - threads but LWP stopped while doing that. */ + /* Delete any single-step breakpoints. No longer needed. We + don't have to worry about other threads hitting this trap, + and later not being able to explain it, because we were + stepping over a breakpoint, and we hold all threads but + LWP stopped while doing that. */ if (!can_hardware_single_step ()) { - gdb_assert (has_reinsert_breakpoints (current_thread)); - delete_reinsert_breakpoints (current_thread); + gdb_assert (has_single_step_breakpoints (current_thread)); + delete_single_step_breakpoints (current_thread); } step_over_bkpt = null_ptid; @@ -5226,10 +5226,11 @@ proceed_one_lwp (struct inferior_list_entry *entry, void *except) debug_printf (" stepping LWP %ld, client wants it stepping\n", lwpid_of (thread)); - /* If resume_step is requested by GDB, install reinsert + /* If resume_step is requested by GDB, install single-step breakpoints when the thread is about to be actually resumed if - the reinsert breakpoints weren't removed. */ - if (can_software_single_step () && !has_reinsert_breakpoints (thread)) + the single-step breakpoints weren't removed. */ + if (can_software_single_step () + && !has_single_step_breakpoints (thread)) install_software_single_step_breakpoints (lwp); step = maybe_hw_step (thread); diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c index 7cd037f..a8e12d8 100644 --- a/gdb/gdbserver/mem-break.c +++ b/gdb/gdbserver/mem-break.c @@ -134,7 +134,7 @@ enum bkpt_type gdb_breakpoint_Z4, /* A basic-software-single-step breakpoint. */ - reinsert_breakpoint, + single_step_breakpoint, /* Any other breakpoint type that doesn't require specific treatment goes here. E.g., an event breakpoint. */ @@ -206,9 +206,9 @@ struct other_breakpoint int (*handler) (CORE_ADDR); }; -/* Reinsert breakpoint. */ +/* Breakpoint for single step. */ -struct reinsert_breakpoint +struct single_step_breakpoint { struct breakpoint base; @@ -828,12 +828,12 @@ set_breakpoint (enum bkpt_type type, enum raw_bkpt_type raw_type, other_bp->handler = handler; bp = (struct breakpoint *) other_bp; } - else if (type == reinsert_breakpoint) + else if (type == single_step_breakpoint) { - struct reinsert_breakpoint *reinsert_bp - = XCNEW (struct reinsert_breakpoint); + struct single_step_breakpoint *ss_bp + = XCNEW (struct single_step_breakpoint); - bp = (struct breakpoint *) reinsert_bp; + bp = (struct breakpoint *) ss_bp; } else gdb_assert_not_reached ("unhandled breakpoint type"); @@ -1479,19 +1479,19 @@ gdb_breakpoint_here (CORE_ADDR where) } void -set_reinsert_breakpoint (CORE_ADDR stop_at, ptid_t ptid) +set_single_step_breakpoint (CORE_ADDR stop_at, ptid_t ptid) { - struct reinsert_breakpoint *bp; + struct single_step_breakpoint *bp; gdb_assert (ptid_get_pid (current_ptid) == ptid_get_pid (ptid)); - bp = (struct reinsert_breakpoint *) set_breakpoint_type_at (reinsert_breakpoint, - stop_at, NULL); + bp = (struct single_step_breakpoint *) set_breakpoint_type_at (single_step_breakpoint, + stop_at, NULL); bp->ptid = ptid; } void -delete_reinsert_breakpoints (struct thread_info *thread) +delete_single_step_breakpoints (struct thread_info *thread) { struct process_info *proc = get_thread_process (thread); struct breakpoint *bp, **bp_link; @@ -1501,8 +1501,8 @@ delete_reinsert_breakpoints (struct thread_info *thread) while (bp) { - if (bp->type == reinsert_breakpoint - && ptid_equal (((struct reinsert_breakpoint *) bp)->ptid, + if (bp->type == single_step_breakpoint + && ptid_equal (((struct single_step_breakpoint *) bp)->ptid, ptid_of (thread))) { struct thread_info *saved_thread = current_thread; @@ -1591,15 +1591,15 @@ uninsert_all_breakpoints (void) } void -uninsert_reinsert_breakpoints (struct thread_info *thread) +uninsert_single_step_breakpoints (struct thread_info *thread) { struct process_info *proc = get_thread_process (thread); struct breakpoint *bp; for (bp = proc->breakpoints; bp != NULL; bp = bp->next) { - if (bp->type == reinsert_breakpoint - && ptid_equal (((struct reinsert_breakpoint *) bp)->ptid, + if (bp->type == single_step_breakpoint + && ptid_equal (((struct single_step_breakpoint *) bp)->ptid, ptid_of (thread))) { gdb_assert (bp->raw->inserted > 0); @@ -1663,7 +1663,7 @@ reinsert_breakpoints_at (CORE_ADDR pc) } int -has_reinsert_breakpoints (struct thread_info *thread) +has_single_step_breakpoints (struct thread_info *thread) { struct process_info *proc = get_thread_process (thread); struct breakpoint *bp, **bp_link; @@ -1673,8 +1673,8 @@ has_reinsert_breakpoints (struct thread_info *thread) while (bp) { - if (bp->type == reinsert_breakpoint - && ptid_equal (((struct reinsert_breakpoint *) bp)->ptid, + if (bp->type == single_step_breakpoint + && ptid_equal (((struct single_step_breakpoint *) bp)->ptid, ptid_of (thread))) return 1; else @@ -1701,15 +1701,15 @@ reinsert_all_breakpoints (void) } void -reinsert_reinsert_breakpoints (struct thread_info *thread) +reinsert_single_step_breakpoints (struct thread_info *thread) { struct process_info *proc = get_thread_process (thread); struct breakpoint *bp; for (bp = proc->breakpoints; bp != NULL; bp = bp->next) { - if (bp->type == reinsert_breakpoint - && ptid_equal (((struct reinsert_breakpoint *) bp)->ptid, + if (bp->type == single_step_breakpoint + && ptid_equal (((struct single_step_breakpoint *) bp)->ptid, ptid_of (thread))) { gdb_assert (bp->raw->inserted > 0); @@ -1839,13 +1839,13 @@ hardware_breakpoint_inserted_here (CORE_ADDR addr) /* See mem-break.h. */ int -reinsert_breakpoint_inserted_here (CORE_ADDR addr) +single_step_breakpoint_inserted_here (CORE_ADDR addr) { struct process_info *proc = current_process (); struct breakpoint *bp; for (bp = proc->breakpoints; bp != NULL; bp = bp->next) - if (bp->type == reinsert_breakpoint + if (bp->type == single_step_breakpoint && bp->raw->pc == addr && bp->raw->inserted) return 1; @@ -1885,9 +1885,9 @@ delete_disabled_breakpoints (void) next = bp->next; if (bp->raw->inserted < 0) { - /* If reinsert_breakpoints become disabled, that means the + /* If single_step_breakpoints become disabled, that means the manipulations (insertion and removal) of them are wrong. */ - gdb_assert (bp->type != reinsert_breakpoint); + gdb_assert (bp->type != single_step_breakpoint); delete_breakpoint_1 (proc, bp); } } @@ -2200,15 +2200,15 @@ clone_one_breakpoint (const struct breakpoint *src, ptid_t ptid) other_dest->handler = ((struct other_breakpoint *) src)->handler; dest = (struct breakpoint *) other_dest; } - else if (src->type == reinsert_breakpoint) + else if (src->type == single_step_breakpoint) { - struct reinsert_breakpoint *reinsert_dest - = XCNEW (struct reinsert_breakpoint); + struct single_step_breakpoint *ss_dest + = XCNEW (struct single_step_breakpoint); - dest = (struct breakpoint *) reinsert_dest; - /* Since reinsert breakpoint is thread specific, don't copy + dest = (struct breakpoint *) ss_dest; + /* Since single-step breakpoint is thread specific, don't copy thread id from SRC, use ID instead. */ - reinsert_dest->ptid = ptid; + ss_dest->ptid = ptid; } else gdb_assert_not_reached ("unhandled breakpoint type"); diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h index 3322ec5..9e7ee93 100644 --- a/gdb/gdbserver/mem-break.h +++ b/gdb/gdbserver/mem-break.h @@ -101,9 +101,9 @@ int software_breakpoint_inserted_here (CORE_ADDR addr); int hardware_breakpoint_inserted_here (CORE_ADDR addr); -/* Returns TRUE if there's any reinsert breakpoint at ADDR. */ +/* Returns TRUE if there's any single-step breakpoint at ADDR. */ -int reinsert_breakpoint_inserted_here (CORE_ADDR addr); +int single_step_breakpoint_inserted_here (CORE_ADDR addr); /* Clear all breakpoint conditions and commands associated with a breakpoint. */ @@ -152,32 +152,32 @@ struct breakpoint *set_breakpoint_at (CORE_ADDR where, int delete_breakpoint (struct breakpoint *bkpt); -/* Set a reinsert breakpoint at STOP_AT for thread represented by +/* Set a single-step breakpoint at STOP_AT for thread represented by PTID. */ -void set_reinsert_breakpoint (CORE_ADDR stop_at, ptid_t ptid); +void set_single_step_breakpoint (CORE_ADDR stop_at, ptid_t ptid); -/* Delete all reinsert breakpoints of THREAD. */ +/* Delete all single-step breakpoints of THREAD. */ -void delete_reinsert_breakpoints (struct thread_info *thread); +void delete_single_step_breakpoints (struct thread_info *thread); -/* Reinsert all reinsert breakpoints of THREAD. */ +/* Reinsert all single-step breakpoints of THREAD. */ -void reinsert_reinsert_breakpoints (struct thread_info *thread); +void reinsert_single_step_breakpoints (struct thread_info *thread); -/* Uninsert all reinsert breakpoints of THREAD. This still leaves - the reinsert breakpoints in the table. */ +/* Uninsert all single-step breakpoints of THREAD. This still leaves + the single-step breakpoints in the table. */ -void uninsert_reinsert_breakpoints (struct thread_info *thread); +void uninsert_single_step_breakpoints (struct thread_info *thread); /* Reinsert breakpoints at WHERE (and change their status to inserted). */ void reinsert_breakpoints_at (CORE_ADDR where); -/* The THREAD has reinsert breakpoints or not. */ +/* The THREAD has single-step breakpoints or not. */ -int has_reinsert_breakpoints (struct thread_info *thread); +int has_single_step_breakpoints (struct thread_info *thread); /* Uninsert breakpoints at WHERE (and change their status to uninserted). This still leaves the breakpoints in the table. */