From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30017 invoked by alias); 27 May 2009 01:49:36 -0000 Received: (qmail 29726 invoked by uid 22791); 27 May 2009 01:49:33 -0000 X-SWARE-Spam-Status: No, hits=0.2 required=5.0 tests=AWL,BAYES_50,SPF_HELO_PASS,SPF_PASS,TRACKER_ID X-Spam-Check-By: sourceware.org Received: from mailgw.tensilica.com (HELO mailgw.tensilica.com) (65.205.227.134) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 27 May 2009 01:49:27 +0000 Received: from localhost (unknown [127.0.0.1]) by mailgw.tensilica.com (Postfix) with ESMTP id 756521160546; Wed, 27 May 2009 01:49:25 +0000 (UTC) Received: from mailgw.tensilica.com ([127.0.0.1]) by localhost (mailgw.tensilica.com [127.0.0.1]) (amavisd-maia, port 10024) with ESMTP id 27109-10; Tue, 26 May 2009 18:49:25 -0700 (PDT) Received: from mail.tensilica.com (mail.tensilica.com [192.168.15.138]) by mailgw.tensilica.com (Postfix) with ESMTP id 3B40D1160516; Tue, 26 May 2009 18:49:25 -0700 (PDT) Received: from [172.16.150.11] (172.16.150.11) by exch.hq.tensilica.com (192.168.15.138) with Microsoft SMTP Server (TLS) id 8.1.358.0; Tue, 26 May 2009 18:49:24 -0700 Message-ID: <4A1C9C23.9010102@tensilica.com> Date: Wed, 27 May 2009 01:49:00 -0000 From: Ross Morley User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.2) Gecko/20040805 Netscape/7.2 MIME-Version: 1.0 To: Pedro Alves CC: "gdb-patches@sourceware.org" Subject: Re: [PATCH] Program Breakpoints References: <4A12EC43.3010107@tensilica.com> <200905191958.35348.pedro@codesourcery.com> In-Reply-To: <200905191958.35348.pedro@codesourcery.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit 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: 2009-05/txt/msg00581.txt.bz2 target_stopped_by_trap_instruction Thanks for your feedback. Detailed reply in-line below. Pedro Alves wrote: >Okay, first try. Sorry for the delay... > >On Tuesday 19 May 2009 18:28:35, Ross Morley wrote: > > >>+/* Returns non-zero if we were stopped by a trap or break instruction, >>+ whether or not it is a software break planted by GDB. If size != 0, >>+ sets *size to that of the instruction or 0 if it need not be skipped. */ >>+ >>+#ifndef STOPPED_BY_TRAP_INSTRUCTION >>+#define STOPPED_BY_TRAP_INSTRUCTION(size) \ >>+ (*current_target.to_stopped_by_trap_instruction) (size) >>+#endif >> #define target_get_ada_task_ptid(lwp, tid) \ >> (*current_target.to_get_ada_task_ptid) (lwp,tid) >> >> >> > >Please rename this to target_stopped_by_trap_instruction, and >make it unconditionally defined. We don't support overriding >of these target macros anymore. > > OK. > > >>+ if (target_has_execution && !ptid_equal (inferior_ptid, null_ptid)) >>+ { >>+ struct thread_info *tp = inferior_thread (); >>+ if (tp->program_breakpoint_hit && tp->program_breakpoint_size != 0 >>+ && execution_direction != EXEC_REVERSE && read_pc () == stop_pc >>+ && count > 0) >> >> > >Sorry, `read_pc' is now gone from the sources. Replace that by > >regcache_read_pc (get_current_regcache ()), or get_frame_pc if there's a >frame handy. > > OK. >+ /* "stepi" off program breakpoint: the first step is to just increment >+ the PC past the break, then there are count-1 steps to go. >+ Note proceed() won't be called the first time, and on subsequent >+ steps the PC will already be off the break, so the entire handling >+ of "stepi" off a program breakpoint is done here. If stopping after >+ the break, display location information as for normal_stop. */ >+ if (target_has_execution && !ptid_equal (inferior_ptid, null_ptid)) >+ { >+ struct thread_info *tp = inferior_thread (); >+ if (tp->program_breakpoint_hit && tp->program_breakpoint_size != 0 >+ && execution_direction != EXEC_REVERSE && read_pc () == stop_pc >+ && count > 0) >+ { >+ count--; >+ write_pc (read_pc () + tp->program_breakpoint_size); >+ if (count == 0) >+ { >+ reinit_frame_cache (); >+ if (ui_out_is_mi_like_p (uiout)) >+ { >+ ui_out_field_string >+ (uiout, "reason", >+ async_reason_lookup (EXEC_ASYNC_END_STEPPING_RANGE)); >+ ui_out_field_int (uiout, "thread-id", >+ pid_to_thread_id (inferior_ptid)); >+ print_stack_frame (get_selected_frame (NULL), -1, LOC_AND_ADDRESS); >+ } >+ else >+ print_stack_frame (get_selected_frame (NULL), -1, SRC_LINE); >+ } >+ } >+ } >+ > >This is broken for the !single_inst cases (step/next). step N is mishandled. > > You are right, it can mess up with step/next where count is lines and not insns. In case of step/next, I need to decrement count only if the PC increment would change the line number. Stepping over a program breakpoint is primarily handled in resume. This fixup just handles a special case in which a stepi/nexti would step one instruction farther than expected. It re-"executes" the trap as a no-op when in fact the inferior has not run at all. Without this code a stepi would execute the instruction following: 1: program breakpoint 2: insn2 3: insn3 Starting at 1, stepi/nexti (with no arg or 1) should result in PC at 2, but without this fixup it ends up at 3. Of course, this does not apply for decr_pc_after_break targets. If you think ending up at 3 is OK, we can just omit this whole chunk. >It also doesn't take into account that there may be a >breakpoint at ORIG_PC + program_breakpoint_size. > > I hadn't noticed this before, that GDB reports a breakpoint hit when it stops with PC at a breakpoint location, even it hasn't yet tried to execute at that location. So a breakpoint immediately after the program breakpoint would need to be reported when the program breakpoint is stepped over, to be consistent (also when a program breakpoint is first hit on a decr_pc_after_break target). There may be other cases. When I first did program breakpoints I studied bpstat, and it seemed much harder and less appropriate. bpstat is very tied to the breakpoint table, but program breakpoints are encountered (not known in advance) and are not in the table. A bpstat has a bp_location which has an "owner" breakpoint in the table. bpstat_stop_status scans the breakpoint table to find possible reasons for the stop (it won't find a program breakpoint). So I'm not sure it would be appropriate to use bpstat, and it would likely amount to a much bigger change. I also noticed that x86 native is not entirely consistent in this case. It does not report a breakpoint if it hit while stepping over another. Take the following example transcript: (gdb) b main Breakpoint 1 at 0x8048384: file breakop.c, line 14. (gdb) r Starting program: /opt/local/ross/test/breakop/breakop Breakpoint 1, main (argc=0x1, argv=0xbff6a674) at breakop.c:14 14 printf("About to execute 'break'...\n"); (gdb) disass Dump of assembler code for function main: 0x08048368 : push %ebp 0x08048369 : mov %esp,%ebp 0x0804836b : sub $0x8,%esp 0x0804836e : and $0xfffffff0,%esp 0x08048371 : mov $0x0,%eax 0x08048376 : add $0xf,%eax 0x08048379 : add $0xf,%eax 0x0804837c : shr $0x4,%eax 0x0804837f : shl $0x4,%eax 0x08048382 : sub %eax,%esp 0x08048384 : sub $0xc,%esp 0x08048387 : push $0x80484a0 0x0804838c : call 0x80482b0 <_init+56> 0x08048391 : add $0x10,%esp 0x08048394 : mov 0x80495f0,%eax 0x08048399 : incl 0x80495f0 0x0804839f : sub $0x4,%esp 0x080483a2 : push %eax 0x080483a3 : pushl 0x80495f0 0x080483a9 : push $0x80484c0 0x080483ae : call 0x80482b0 <_init+56> 0x080483b3 : add $0x10,%esp 0x080483b6 : mov $0x0,%eax 0x080483bb : leave 0x080483bc : ret End of assembler dump. (gdb) p/x $pc $1 = 0x8048384 (gdb) b * 0x08048387 Breakpoint 2 at 0x8048387: file breakop.c, line 14. (gdb) b * 0x0804838c Breakpoint 3 at 0x804838c: file breakop.c, line 14. (gdb) si 0x08048387 14 printf("About to execute 'break'...\n"); -------> Note the breakpoint was not reported. (gdb) si 0x0804838c 14 printf("About to execute 'break'...\n"); -------> Nor was this one. (gdb) si 0x080482b0 in ?? () (gdb) r The program being debugged has been started already. Start it from the beginning? (y or n) y Starting program: /opt/local/ross/test/breakop/breakop Breakpoint 1, main (argc=0x1, argv=0xbffd1164) at breakop.c:14 14 printf("About to execute 'break'...\n"); (gdb) dis 2 (gdb) si 0x08048387 14 printf("About to execute 'break'...\n"); (gdb) si -------> With the preceding BP diabled, this one is reported. Breakpoint 3, 0x0804838c in main (argc=0x1, argv=0xbffd1164) at breakop.c:14 14 printf("About to execute 'break'...\n"); (gdb) So I don't have an ideal way to handle these corner cases. I lean toward a philosophy that a breakpoint is not hit until there is an attempt to execute at that location, so I'm not sure it really makes sense for GDB to report a breakpoint when it stops (for some other reason) just before it. Perhaps in this case it doesn't matter because on resume or continued stepping you will certainly hit the subsequent breakpoint and GDB should report it then. Keep in mind the goal here is to make sure GDB stops for a program breakpoint and (secondarily) can resume afterward. If a GDB breakpoint immediately following is not reported until resuming, that's not so bad. >What if a "program breakpoint" is reported for a GDB breakpoint? You shouldn't >just move the PC in that case. I don't see where that is handled. > I gather you are talking about the coincident case (GDB breakpoint at the same location as a program breakpoint). They cannot both hit at the same time. One could think of a GDB breakpoint conceptually lying just before an instruction, to be hit before it is executed. A program breakpoint is reported only when a trap is executed and there is no GDB s/w breakpoint inserted at that location. A h/w breakpoint would be hit before the trap is actually executed. Here is my understanding of what happens for a s/w breakpoint coincident with a program breakpoint. GDB first hits the s/w breakpoint by executing the inserted trap. It stops and reports a GDB breakpoint (and if decr_pc_after_break, backs up the PC). On resume it replaces the program breakpoint trap and hits it while stepping over the GDB breakpoint, and reports a program breakpoint. On resume from that it increments the PC over the program breakpoint (if size != 0), reinserts the GDB breakpoint and continues as usual. Here is my understanding of what happens for a h/w breakpoint coincident with a program breakpoint. GDB first hits the h/w breakpoint by triggering on the fetch PC. It stops and reports a GDB breakpoint. On resume it disables the h/w breakpoint to step over it, executes the program breakpoint, and reports it. On resume from there it increments the PC over the program breakpoint (if size != 0), enables the h/w breakpoint and continues as normal. Is that a reasonable assessment of the coincident case? > See comment >on next hunk. I suspect this short circuit should be done elsewhere, somewhere >where it will be possible to pass through handle_inferior_event after >moving the PC forward. > > It seems it might be possible to arrange for step_1 or step_once to call normal_stop directly. I'm not deeply familiar with the infrun control flow however, and I don't have the time to study it that much. I wonder if this is something that could be improved later by an expert, but in the meantime my less than perfect implementation would improve current behavior and not lock us into implementation details. The only things we'd be stuck with would be the interface function to_stopped_by_trap_instruction(size) and the remote protocol stop-reply extension "TNNtrap:size". Improving the way infrun handles things would be possible without changing how hitting a trap is reported to infrun. The critical thing is to make sure no regression tests are broken by this, espeically for targets that don't yet support reporting the trap (and I did run the testsuite with no regressions on x86/linux native). Please read on for comments on your other suggestions. >Shouldn't you do something in prepare_to_proceed as well? > Use case is: > > > (gdb) thread 2 > (gdb) continue > > GDB is expected to make progress here, instead of reporting > the same program breakpoint again. Thread support was added since I originally did program breakpoints and I am unfamiliar with it (it's not supported on our target). In merging with cvs head, I tried to guess what would be needed, and I moved the program_breakpoint_{hit,size} variables into the thread structure (previously they were global like some other things that moved to the thread structure). Now I wonder if I should have left it as it was. The stop state seems more associated with the inferior than with the GDB thread (which is only which thread GDB is looking at). Would it be correct then to just put it back how it was, with those flags global? Both? Otherwise, I will have to add a test in prepare_to_proceed to switch back to the original thread like for a GDB breakpoint, ie. if (breakpoint_here_p (regcache_read_pc (regcache)) || (tp->program_breakpoint_hit && tp->program_breakpoint_size != 0 && regcache_read_pc (regcache) == stop_pc)) { : } I'd have to compute tp and stop_pc because they're not passed into prepare_to_proceed. Once we get to resume, stepping over a program breakpoint is done at the same point skipping a permanent breakpoint is done. It's the same problem except skip_permanent_breakpoint has implicit knowledge of the trap size. I've previously suggested making a single function that could be used for both cases, with a size parameter. But for now I did not want to interfere with permanent breakpoints, so kept it simple thinking could be a future enhancement after program breakpoints are accepted, if anyone cares. >>+ /* Handle case of hitting a program breakpoint (break instruction >>+ in target program, not planted by or known to GDB). */ >>+ >>+ if (ecs->event_thread->program_breakpoint_hit) >>+ { >>+ stop_print_frame = 1; >>+ >>+ /* We are about to nuke the step_resume_breakpoint and >>+ through_sigtramp_breakpoint via the cleanup chain, so >>+ no need to worry about it here. */ >>+ >>+ stop_stepping (ecs); >>+ return; >>+ } >>+ >> > Above was copied from other cases in infrun where we need to stop. I don't mind if that comment gets nuked. > >Hmmm, confused: can't the target report a program breakpoint hit >for a PC where there's a gdb breakpoint inserted as well? Not if it's inserted - the program breakpoint trap won't be seen. GDB will see the trap instruction it inserted (which may or may not be the same instruction). If it's a h/w breakpoint, GDB will stop before it executes the trap. > Wouldn't >it better to have program breakpoints checked from inside >stop_bpstat/bpstop_status/bpstat_explains_signal/bpstat_print? >Then you wouldn't need to add >those if (program_breakpoint) do_this; else handle bpstat; I thought about this, but it seemed bpstat is not well suited to this. See earlier discussion. >+static int >+linux_stopped_by_trap_instruction (int *size) >+{ >+ CORE_ADDR stop_pc = get_stop_pc(); >+ int trap_size = 0; >+ >+ if (the_low_target.trap_size_at != NULL) >+ trap_size = (*the_low_target.trap_size_at) (stop_pc); >+ else if (the_low_target.breakpoint_at != NULL >+ && (*the_low_target.breakpoint_at) (stop_pc)) >+ trap_size = the_low_target.breakpoint_len; >+ >+ if (trap_size != 0) >+ { >+ *size = (the_low_target.get_pc != NULL >+ && (*the_low_target.get_pc) () == stop_pc) >+ ? trap_size : 0; > >I think I got this bit, but it would be nice to have a >comment here. OK. Something like: /* if we hit a trap insn and the PC still points to it, report the size of the trap so GDB can step over it. */ ? >+ return 1; >+ } >+ else >+ return 0; >+} > > >Before we stick with this forever, it would be very >important to have tests covering corner cases like: > > single-stepping through consecutive > "program breakpoint"->gdb breakpoint > > single-stepping through consecutive > "program breakpoint"->"program breakpoint" > >single- stepping through consecutive > gdb breakpoint->"program breakpoint" > >(step, stepi, stepi n) I agree we need test cases. I'll develop some before submitting a revised patch (which I hope to do only once after we hash out the basic issues). > insn1 > "program breakpoint" >foo: gdb breakpoint > insn2 > insn3 > jmp foo > >"step" over this: > >function () > >line 1: > insn1 > "program breakpoint" > insn2 > insn3 >line 2: > ... > >... and have "program breakpoint" be reported, instead >of end-stepping-range at line 2. >>>From the looks of it, you don't have to do much, if anything >else at all, to have these cases covered on x86; I'd really >like to make sure this is sane on decr_pc_after_break archs, as >this is the way to fix the "step over range with trap >doesn't stop at trap" "bug". I think that's the same bug I'm trying to fix, but it manifests differently for a non decr_pc_after_break arch. It keeps executing the trap, never advancing. I guess on x86 it just keeps going. I think decr_pc_after_break archs will just fall out except perhaps for a few corner cases. But not being familiar with the x86 I'd have to learn now to implement target_ops.to_stopped_by_trap_instruction for x86 native. It would be better if someone else could implement that, and I could test it. Certainly I will run the test suite on native linux/x86 to make sure nothing breaks without that function. Each target/arch can implement it if/when someone cares. GDB behavior should not change until that is implemented for any given target. Meanwhile the program breakpoint test cases would be expected failures. >If I read the code correctly, on x86, if you continue >this: > >0001 insn1 >0002 "program breakpoint" > >... you'll report a "program breakpoint" hit at 0003, I'll report it hit, and the PC will be 0003. It was actually hit at 0002, but on this arch the PC is incremented. >with >program breakpoint size 0. Yes. > If you instead stepi through that same >sequence, you'll get a "program breakpoint" hit at 0002, with >program breakpoint size 1. That's up to target_ops.to_stopped_by_trap_instruction. I gather on x86, when stepping the PC is not incremented after a trap, and the trap size is 1? > That sounds good, but, if there's a >GDB breakpoint installed at "0002", gdb shouldn't be moving the PC >forward magically, without executing the instruction that >was under the breakpoint originally. The instruction under the breakpoint is the program breakpoint trap insn. The program breakpoint won't be executed while the GDB breakpoint is inserted. So GDB will first hit the GDB breakpoint. If it's a s/w breakpoint it will be removed and the program breakpoint replaced (the latter has not yet been executed/hit). If it's a h/w breakpoint, I'm guessing the target stopped before the program breakpoint was executed (else the target would have undo the effects of any insn at a h/w breakpoint). On continuing from there, GDB will remove the GDB breakpoint and single-step. Target will execute the program breakpoint, stop at 0002 (since it single-stepped), and report being stopped by a trap with size 1. Since the GDB breakpoint was not inserted, GDB will interpret it as a program breakpoint. I'm assuming now that GDB forgets it was trying to step over a GDB breakpoint, and reinserts it as usual on continue. Now it knows it hit a program breakpoint and the PC has not changed. so it will increment the PC by the size (1) to 0003 and continue. Suppose a decr_pc_after_break arch also increments the pc after single-stepping a trap insn. In that case the program breakpoint will be reported with size 0 when pc is at 0003, and no step-over will be done. >So, if I get this correctly, adjust_pc_after_break should >never try to roll back if program size > 0, even on >decr_pc_after_break archs, but we don't need to check this, >because this will only happen if single-stepping, and it >already does nothing in that case. Right? > adjust_pc_after_break does exactly the right thing already. It should only roll back the PC if decr_pc_after_break and software_breakpoint_inserted_here_p (breakpoint_pc) . The only reason to roll back the PC is to execute the instruction under a GDB s/w breakpoint. In summary, if you agree, I need to make the following changes: - Rename STOPPED_BY_TRAP_INSTRUCTION to target_stopped_by_trap_instruction (unconditionally defined). - Replace read_pc () with regcache_read_pc (get_current_regcache ()). - Move program_breakpoint_{hit,size} out of the thread structure back to global variables? - In step_1, !single_inst cases, decrement count only if the line number would change. - Write generic test cases, passing in the trap insn at compile. - Test on Xtensa. - Test on x86/native (program breakpoint tests would XFAIL but all others that passed before would still pass). - Add the cross-ref to the doc that Eli requested. - Re-base and resubmit patch. Thanks, Ross