From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6957 invoked by alias); 16 Aug 2002 18:21:42 -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 6710 invoked from network); 16 Aug 2002 18:21:40 -0000 Received: from unknown (HELO takamaka.act-europe.fr) (142.179.108.108) by sources.redhat.com with SMTP; 16 Aug 2002 18:21:40 -0000 Received: by takamaka.act-europe.fr (Postfix, from userid 507) id CAF0CD2CBD; Fri, 16 Aug 2002 11:21:41 -0700 (PDT) Date: Fri, 16 Aug 2002 11:21:00 -0000 From: Joel Brobecker To: Andrew Cagney Cc: gdb-patches@sources.redhat.com Subject: Re: [RFA] enable software single step on alpha-osf Message-ID: <20020816182141.GJ906@gnat.com> References: <20020718203205.GB26990@gnat.com> <3D4DBBC8.5000906@ges.redhat.com> <20020805184920.GC892@gnat.com> <3D5D323A.2030801@ges.redhat.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="nFreZHaLTZJo0R7j" Content-Disposition: inline In-Reply-To: <3D5D323A.2030801@ges.redhat.com> User-Agent: Mutt/1.4i X-SW-Source: 2002-08/txt/msg00438.txt.bz2 --nFreZHaLTZJo0R7j Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-length: 3370 > Did the re-ordering of that if statement get committed? > This is wfi code so the smaller each change is the better. [muttering something about too many things to do at the same time] I tested (on Linux) and committed the attached patch. This is almost the entire infrun.c change, except that I put the following change on hold: > >>The second part of the change is more tricky: > >> + stop_pc -= DECR_PC_AFTER_BREAK; Concerning this particular change: > >+ /* Readjust the stop_pc as it is off by DECR_PC_AFTER_BREAK > >+ compared to the value it would have if the system stepping > >+ capability was used. This allows the rest of the code in > >+ this function to use this address without having to worry > >+ whether software single step is in use or not. */ > >+ stop_pc -= DECR_PC_AFTER_BREAK; > >+ ecs->random_signal = 0; > > Ok, I'm convinced that this is going in the right direction (and was > most likely the original intent of the code :-). The more we can > localize DECR_PC_AFTER_BREAK (ie. extract it from bpstat_stop_status()) > the better! :-) > > I this will be ok with some tweaks and a litte bit more experimentation. Cool. > I _think_ the above should be also writing the decremented stop_pc back > to the target. This is what appears to happen else where. It also > ensures that code that calls read_pc() instead of using stop_pc, gets a > consistent value. However, there could be some sort of dragon lurking > here. Can you please check this out? The other possability is that > something is detecting that the PC wasn't written (status register) and > hence, assuming it should be decremented? It might even be written by > resume(). It makes sense. I should be able to verify this change sometime soon. > I think, the above code should also set a flag to indicate a ``software > single step trap'' occured. That indication can then be passed down to > bpstat_stop_status() to so that it knows that this software single-step > trap is not a breakpoint and hence that the decr_pc_after_break doesn't > apply. Off hand, a ``software_singlestep_p'' variable (||ed into > not_a_sw_breakpoint) or (better?) an extra enum trap_type (unknown_trap, > software_singlestep_trap) parameter explictly specifying the type of > trap that was identifed. This needs to be investigated. So far, the not_a_sw_breakpoint seems to be sufficient, but adding this flag would definitely make things clearer, especially on the caller side. May I suggest we treat this as a separate patch that would be applied after all the issues in this RFA are dealt with? > For the below, note that I've changed the variable name to > ``not_a_sw_breakpoint'' so you'll get a clash when merging. Thanks. So, to summarize: 1 - The change "+ stop_pc -= DECR_PC_AFTER_BREAK;" seems to be going in the right direction. 2 - However I should hold this change for now because you think I should write the adjusted PC value back to the target, by adding something like "write_pc_pid (stop_pc, ecs->ptid)" I will verify the impact of such a change, and report. 3 - Assuming we get all issues in this RFA resolved, then I will start looking at the addition of the software_singlestep flag. -- Joel --nFreZHaLTZJo0R7j Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="infrun.c.diff" Content-length: 1400 Index: infrun.c =================================================================== RCS file: /cvs/src/src/gdb/infrun.c,v retrieving revision 1.64 diff -c -r1.64 infrun.c *** infrun.c 24 Jul 2002 14:38:55 -0000 1.64 --- infrun.c 16 Aug 2002 17:55:05 -0000 *************** *** 1826,1835 **** if (stop_signal == TARGET_SIGNAL_TRAP) { ! if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p) ! ecs->random_signal = 0; ! else if (breakpoints_inserted ! && breakpoint_here_p (stop_pc - DECR_PC_AFTER_BREAK)) { ecs->random_signal = 0; if (!breakpoint_thread_match (stop_pc - DECR_PC_AFTER_BREAK, --- 1826,1836 ---- 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. */ ! if (breakpoints_inserted ! && breakpoint_here_p (stop_pc - DECR_PC_AFTER_BREAK)) { ecs->random_signal = 0; if (!breakpoint_thread_match (stop_pc - DECR_PC_AFTER_BREAK, *************** *** 1885,1890 **** --- 1886,1895 ---- } } } + else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p) + { + ecs->random_signal = 0; + } } else ecs->random_signal = 1; --nFreZHaLTZJo0R7j--