From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 610 invoked by alias); 18 Jan 2004 15:19:12 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 599 invoked from network); 18 Jan 2004 15:19:11 -0000 Received: from unknown (HELO nevyn.them.org) (66.93.172.17) by sources.redhat.com with SMTP; 18 Jan 2004 15:19:11 -0000 Received: from drow by nevyn.them.org with local (Exim 4.30 #1 (Debian)) id 1AiEhi-0005dF-E7; Sun, 18 Jan 2004 10:19:10 -0500 Date: Sun, 18 Jan 2004 15:19:00 -0000 From: Daniel Jacobowitz To: Eli Zaretskii Cc: gdb-patches@sources.redhat.com Subject: Re: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun Message-ID: <20040118151909.GA17039@nevyn.them.org> Mail-Followup-To: Eli Zaretskii , gdb-patches@sources.redhat.com References: <20040117222007.GA23563@nevyn.them.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.1i X-SW-Source: 2004-01/txt/msg00472.txt.bz2 On Sun, Jan 18, 2004 at 09:07:23AM +0200, Eli Zaretskii wrote: > > Date: Sat, 17 Jan 2004 17:20:07 -0500 > > From: Daniel Jacobowitz > > > > One case, HANDLE_NONSTEPPABLE_WATCHPOINTS and DECR_PC_AFTER_BREAK, is simply > > removed. There are no targets using this combination, and if one is added, > > it's non-obvious whether a nonsteppable watchpoint really should be affected > > by DECR_PC_AFTER_BREAK. > > Right, but since we don't really know what that feature was about, I'd > suggest to leave a comment in adjust_pc_after_break that mentions > HANDLE_NONSTEPPABLE_WATCHPOINTS and that its support, if needed, > should be added. Well, we know what HANDLE_NONSTEPPABLE_WATCHPOINTS was about. I'd be curious to see whether any target ever used these two together, or if the decrement was just added for consistency. I'll add a comment. > > * breakpoint.c (software_breakpoint_inserted_here_p): New function. > > (bpstat_stop_status): Don't decrement PC. > > * breakpoint.h (software_breakpoint_inserted_here_p): Add > > prototype. > > * infrun.c (adjust_pc_after_break): New function. > > (handle_inferior_event): Call it, early. Remove later references > > to DECR_PC_AFTER_BREAK. > > (normal_stop): Add commentary. > > What happens if a location has both software and hardware > breakpoints? Does the code still DTRT? Hmm, I am not sure. What _is_ the right thing? Decrement if the software breakpoint was inserted, and do nothing if the hardware breakpoint was inserted, and assume that both will not be inserted? > > + /* If we've hit a breakpoint, we'll be stopped with SIGTRAP. */ > > + if (ecs->ws.kind != TARGET_WAITKIND_STOPPED) > > + return; > > + > > + if (ecs->ws.value.sig != TARGET_SIGNAL_TRAP) > > + return; > > The original code didn't check these conditions, right? So why add > them here? (Also, the comment doesn't seem to describe the two > tests, only the second one.) The comment does describe both tests; if != TARGET_WAITKIND_STOPPED, then we aren't stopped by a signal. The other waitkinds correspond to things like exiting and catchpoints, and with the exception of some complications in the FORKED/EXECD cases, stop_signal will not get set to SIGTRAP. Also, the original code did check these conditions, though somewhat indirectly: if (stop_signal == TARGET_SIGNAL_TRAP) { /* Check if a regular breakpoint has been hit before checking for a potential single step breakpoint. Otherwise, GDB will not see this breakpoint hit when stepping onto breakpoints. */ Oh, hum, that's for the first set (thread hit thread-specific BP for a different thread). The second thread does this: if (stop_signal == TARGET_SIGNAL_TRAP || (breakpoints_inserted && (stop_signal == TARGET_SIGNAL_ILL || stop_signal == TARGET_SIGNAL_EMT)) || stop_soon == STOP_QUIETLY || stop_soon == STOP_QUIETLY_NO_SIGSTOP) The stop_soon's aren't relevant here, since they're handled before DECR_PC_AFTER_BREAK, but the ILL/EMT are relevant. They should be added to adjust_pc_after_break - thanks! -- Daniel Jacobowitz MontaVista Software Debian GNU/Linux Developer