From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20583 invoked by alias); 5 Dec 2008 01:16:35 -0000 Received: (qmail 20568 invoked by uid 22791); 5 Dec 2008 01:16:33 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 05 Dec 2008 01:15:35 +0000 Received: (qmail 30798 invoked from network); 5 Dec 2008 01:15:32 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 5 Dec 2008 01:15:32 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Get rid of stop_pc (was: [RFA] dummy frame handling cleanup, plus inferior fun call signal handling improvement) Date: Fri, 05 Dec 2008 01:16:00 -0000 User-Agent: KMail/1.9.10 Cc: "Ulrich Weigand" , Doug Evans References: <200812050018.mB50I05V031478@d12av02.megacenter.de.ibm.com> <200812050036.56899.pedro@codesourcery.com> In-Reply-To: <200812050036.56899.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_yCIOJ4KkusaSfyQ" Message-Id: <200812050115.30692.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-12/txt/msg00086.txt.bz2 --Boundary-00=_yCIOJ4KkusaSfyQ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Content-length: 1933 On Friday 05 December 2008 00:36:56, Pedro Alves wrote: > On Friday 05 December 2008 00:18:00, Ulrich Weigand wrote: > > Pedro Alves wrote: > > > On Thursday 04 December 2008 22:32:12, Doug Evans wrote: > > > > In the original code, is there a case when stop_pc != registers.pc? > > > > > > Here, > > > > > > > > > (gdb) set $pc = 0xf00 > > > (gdb) call func() > > > > Huh. But that case is in fact *broken*, because GDB will use stop_pc > > incorrectly: for example, the check whether we are about to continue > > at a breakpoint will look at stop_pc, but then continue at $pc. > > This one I believe was the original intention. The rationale being > that you'd not want to hit a breakpoint again at stop_pc (0x1234), > because there's where you stopped; but, you'd want to hit a a breakpoint > at 0xf00, sort of like jump *$pc hits a breakpoint at $pc. > > Note, I'm not saying I agree with this. I did say that probably nobody > would notice if we got rid of stop_pc. > > > It seems to me just about every current user of stop_pc *really* wants > > to look at regcache_read_pc (get_current_regcache ()) ... Is using read_pc instead OK with you? It's what I had written already. > I've been sneaking the idea of getting rid of stop_pc for a while now: > http://sourceware.org/ml/gdb-patches/2008-06/msg00450.html > > In fact, I have a months old patch here that completelly removes stop_pc. > IIRC, there were no visible changes in the testsuite. Say the word, > and I'll brush it up, regtest, submit it. Here it is, it still applied cleanly. It's smallish. Regtested on x86-64-unknown-linux-gnu. My original motivation was to get rid of the ugly checks in switch_to_thread, and to try to minimize the extra thread switching and register reads in non-stop mode. I had held posting this when I wrote it, since I was not sure we'd not miss stop_pc in some case. -- Pedro Alves --Boundary-00=_yCIOJ4KkusaSfyQ Content-Type: text/x-diff; charset="iso 8859-15"; name="stop_pc.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="stop_pc.diff" Content-length: 11578 2008-12-05 Pedro Alves Remove stop_pc. * infrun.c (proceed): Don't check stop_pc against the current pc. (handle_inferior_event): Add a stop_pc local. (handle_step_into_function): Likewise. (handle_step_into_function_backward): Likewise. (normal_stop): Likewise. (struct inferior_status) : Remove it. (save_inferior_status): Don't store the stop_pc. (restore_inferior_status): Don't restore it. * linux-fork.c (fork_load_infrun_state): Don't set it. * m32c-tdep.c (m32c_skip_trampoline_code): Adjust comment. * remote-mips.c (common_open): Remove reading the stop_pc. * solib-sunos.c (disable_break): Replace stop_pc reference by a read_pc call. (sunos_solib_create_inferior_hook): Add a stop_pc local. * target.h (target_wait): Adjust comment. * thread.c (switch_to_thread): Don't read the stop_pc. * infcmd.c (stop_pc): Delete. (step_1, step_once, signal_command, program_info): Adjust. * inferior.h (stop_pc): Delete declaration. --- gdb/fork-child.c | 2 -- gdb/infcmd.c | 19 +++++++++---------- gdb/inferior.h | 4 ---- gdb/infrun.c | 14 +++++--------- gdb/linux-fork.c | 1 - gdb/m32c-tdep.c | 2 +- gdb/remote-mips.c | 1 - gdb/solib-sunos.c | 3 ++- gdb/target.h | 2 +- gdb/thread.c | 8 -------- 10 files changed, 18 insertions(+), 38 deletions(-) Index: src/gdb/infrun.c =================================================================== --- src.orig/gdb/infrun.c 2008-12-05 00:49:28.000000000 +0000 +++ src/gdb/infrun.c 2008-12-05 01:10:15.000000000 +0000 @@ -1311,7 +1311,7 @@ proceed (CORE_ADDR addr, enum target_sig if (addr == (CORE_ADDR) -1) { - if (pc == stop_pc && breakpoint_here_p (pc) + if (breakpoint_here_p (pc) && execution_direction != EXEC_REVERSE) /* There is a breakpoint at the address we will resume at, step one instruction before inserting breakpoints so that @@ -2100,6 +2100,7 @@ handle_inferior_event (struct execution_ int stepped_after_stopped_by_watchpoint = 0; struct symtab_and_line stop_pc_sal; enum stop_kind stop_soon; + CORE_ADDR stop_pc; if (ecs->ws.kind != TARGET_WAITKIND_EXITED && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED @@ -3705,6 +3706,7 @@ handle_step_into_function (struct execut { struct symtab *s; struct symtab_and_line stop_func_sal, sr_sal; + CORE_ADDR stop_pc = read_pc (); s = find_pc_symtab (stop_pc); if (s && s->language != language_asm) @@ -3781,6 +3783,7 @@ handle_step_into_function_backward (stru { struct symtab *s; struct symtab_and_line stop_func_sal, sr_sal; + CORE_ADDR stop_pc = read_pc (); s = find_pc_symtab (stop_pc); if (s && s->language != language_asm) @@ -4283,7 +4286,7 @@ Further execution is probably impossible if (tp->stop_step && frame_id_eq (tp->step_frame_id, get_frame_id (get_current_frame ())) - && step_start_function == find_pc_function (stop_pc)) + && step_start_function == find_pc_function (read_pc ())) source_flag = SRC_LINE; /* finished step, just print source line */ else source_flag = SRC_AND_LOC; /* print location and source line */ @@ -4350,10 +4353,6 @@ Further execution is probably impossible ends with a setting of the current frame, so we can use that next. */ frame_pop (get_current_frame ()); - /* Set stop_pc to what it was before we called the function. - Can't rely on restore_inferior_status because that only gets - called if we don't stop in the called function. */ - stop_pc = read_pc (); select_frame (get_current_frame ()); } @@ -4753,7 +4752,6 @@ signals_info (char *signum_exp, int from struct inferior_status { enum target_signal stop_signal; - CORE_ADDR stop_pc; bpstat stop_bpstat; int stop_step; int stop_stack_dummy; @@ -4792,7 +4790,6 @@ save_inferior_status (int restore_stack_ struct inferior *inf = current_inferior (); inf_status->stop_signal = tp->stop_signal; - inf_status->stop_pc = stop_pc; inf_status->stop_step = tp->stop_step; inf_status->stop_stack_dummy = stop_stack_dummy; inf_status->stopped_by_random_signal = stopped_by_random_signal; @@ -4847,7 +4844,6 @@ restore_inferior_status (struct inferior struct inferior *inf = current_inferior (); tp->stop_signal = inf_status->stop_signal; - stop_pc = inf_status->stop_pc; tp->stop_step = inf_status->stop_step; stop_stack_dummy = inf_status->stop_stack_dummy; stopped_by_random_signal = inf_status->stopped_by_random_signal; Index: src/gdb/linux-fork.c =================================================================== --- src.orig/gdb/linux-fork.c 2008-12-05 00:47:34.000000000 +0000 +++ src/gdb/linux-fork.c 2008-12-05 00:49:53.000000000 +0000 @@ -250,7 +250,6 @@ fork_load_infrun_state (struct fork_info registers_changed (); reinit_frame_cache (); - stop_pc = read_pc (); nullify_last_target_wait_ptid (); /* Now restore the file positions of open file descriptors. */ Index: src/gdb/m32c-tdep.c =================================================================== --- src.orig/gdb/m32c-tdep.c 2008-10-21 15:45:49.000000000 +0100 +++ src/gdb/m32c-tdep.c 2008-12-05 00:49:53.000000000 +0000 @@ -2306,7 +2306,7 @@ m32c_skip_trampoline_code (struct frame_ struct gdbarch_tdep *tdep = gdbarch_tdep (get_frame_arch (frame)); /* It would be nicer to simply look up the addresses of known - trampolines once, and then compare stop_pc with them. However, + trampolines once, and then compare the stop pc with them. However, we'd need to ensure that that cached address got invalidated when someone loaded a new executable, and I'm not quite sure of the best way to do that. find_pc_partial_function does do some Index: src/gdb/remote-mips.c =================================================================== --- src.orig/gdb/remote-mips.c 2008-12-02 12:19:23.000000000 +0000 +++ src/gdb/remote-mips.c 2008-12-05 00:49:53.000000000 +0000 @@ -1583,7 +1583,6 @@ device is attached to the target board ( reinit_frame_cache (); registers_changed (); - stop_pc = read_pc (); print_stack_frame (get_selected_frame (NULL), 0, SRC_AND_LOC); xfree (serial_port_name); } Index: src/gdb/solib-sunos.c =================================================================== --- src.orig/gdb/solib-sunos.c 2008-10-21 15:45:49.000000000 +0100 +++ src/gdb/solib-sunos.c 2008-12-05 00:49:53.000000000 +0000 @@ -533,7 +533,7 @@ disable_break (void) SunOS version we don't know it until the above code is executed. Grumble if we are stopped anywhere besides the breakpoint address. */ - if (stop_pc != breakpoint_addr) + if (read_pc () != breakpoint_addr) { warning (_("stopped at unknown breakpoint while handling shared libraries")); } @@ -790,6 +790,7 @@ sunos_solib_create_inferior_hook (void) if (gdbarch_decr_pc_after_break (target_gdbarch)) { + CORE_ADDR stop_pc = read_pc (); stop_pc -= gdbarch_decr_pc_after_break (target_gdbarch); write_pc (stop_pc); } Index: src/gdb/target.h =================================================================== --- src.orig/gdb/target.h 2008-12-05 00:47:34.000000000 +0000 +++ src/gdb/target.h 2008-12-05 00:49:53.000000000 +0000 @@ -628,7 +628,7 @@ extern void target_resume (ptid_t ptid, _NOT_ OK to throw_exception() out of target_wait() without popping the debugging target from the stack; GDB isn't prepared to get back to the prompt with a debugging target but without the frame cache, - stop_pc, etc., set up. */ + etc., set up. */ #define target_wait(ptid, status) \ (*current_target.to_wait) (ptid, status) Index: src/gdb/thread.c =================================================================== --- src.orig/gdb/thread.c 2008-12-05 00:47:41.000000000 +0000 +++ src/gdb/thread.c 2008-12-05 00:49:53.000000000 +0000 @@ -766,14 +766,6 @@ switch_to_thread (ptid_t ptid) inferior_ptid = ptid; reinit_frame_cache (); registers_changed (); - - /* We don't check for is_stopped, because we're called at times - while in the TARGET_RUNNING state, e.g., while handling an - internal event. */ - if (!is_exited (ptid) && !is_executing (ptid)) - stop_pc = read_pc (); - else - stop_pc = ~(CORE_ADDR) 0; } static void Index: src/gdb/infcmd.c =================================================================== --- src.orig/gdb/infcmd.c 2008-12-05 00:47:41.000000000 +0000 +++ src/gdb/infcmd.c 2008-12-05 00:49:53.000000000 +0000 @@ -144,10 +144,6 @@ static char *inferior_io_terminal; ptid_t inferior_ptid; -/* Address at which inferior stopped. */ - -CORE_ADDR stop_pc; - /* Flag indicating that a command has proceeded the inferior past the current breakpoint. */ @@ -805,6 +801,7 @@ step_1 (int skip_subroutines, int single for (; count > 0; count--) { struct thread_info *tp = inferior_thread (); + CORE_ADDR stop_pc = read_pc (); clear_proceed_status (); frame = get_current_frame (); @@ -924,14 +921,17 @@ step_once (int skip_subroutines, int sin the longjmp breakpoint was not required. Use the INFERIOR_PTID thread instead, which is the same thread when THREAD is set. */ - struct thread_info *tp = inferior_thread (); + struct thread_info *tp; + CORE_ADDR stop_pc; + + tp = inferior_thread (); clear_proceed_status (); frame = get_current_frame (); - if (!frame) /* Avoid coredump here. Why tho? */ - error (_("No current frame")); tp->step_frame_id = get_frame_id (frame); + stop_pc = read_pc (); + if (!single_inst) { find_pc_line_pc_range (stop_pc, @@ -1149,7 +1149,7 @@ signal_command (char *signum_exp, int fr FIXME: Neither should "signal foo" but when I tried passing (CORE_ADDR)-1 unconditionally I got a testsuite failure which I haven't tried to track down yet. */ - proceed (oursig == TARGET_SIGNAL_0 ? (CORE_ADDR) -1 : stop_pc, oursig, 0); + proceed (oursig == TARGET_SIGNAL_0 ? (CORE_ADDR) -1 : read_pc (), oursig, 0); } /* Proceed until we reach a different source line with pc greater than @@ -1585,8 +1585,7 @@ program_info (char *args, int from_tty) stat = bpstat_num (&bs, &num); target_files_info (); - printf_filtered (_("Program stopped at %s.\n"), - hex_string ((unsigned long) stop_pc)); + printf_filtered (_("Program stopped at %s.\n"), paddr_nz (read_pc ())); if (tp->stop_step) printf_filtered (_("It stopped after being stepped.\n")); else if (stat != 0) Index: src/gdb/inferior.h =================================================================== --- src.orig/gdb/inferior.h 2008-12-05 00:47:40.000000000 +0000 +++ src/gdb/inferior.h 2008-12-05 00:49:53.000000000 +0000 @@ -271,10 +271,6 @@ extern void interrupt_target_1 (int all_ extern void detach_command (char *, int); -/* Address at which inferior stopped. */ - -extern CORE_ADDR stop_pc; - /* Flag indicating that a command has proceeded the inferior past the current breakpoint. */ Index: src/gdb/fork-child.c =================================================================== --- src.orig/gdb/fork-child.c 2008-12-05 00:51:40.000000000 +0000 +++ src/gdb/fork-child.c 2008-12-05 00:52:00.000000000 +0000 @@ -530,8 +530,6 @@ startup_inferior (int ntraps) /* Mark all threads non-executing. */ set_executing (pid_to_ptid (-1), 0); - - stop_pc = read_pc (); } /* Implement the "unset exec-wrapper" command. */ --Boundary-00=_yCIOJ4KkusaSfyQ--