From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11619 invoked by alias); 19 May 2009 18:58:46 -0000 Received: (qmail 11608 invoked by uid 22791); 19 May 2009 18:58:44 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,SPF_PASS 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; Tue, 19 May 2009 18:58:37 +0000 Received: (qmail 9953 invoked from network); 19 May 2009 18:58:34 -0000 Received: from unknown (HELO orlando) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 19 May 2009 18:58:34 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [Fwd: Re: [PATCH] Program Breakpoints] Date: Tue, 19 May 2009 18:58:00 -0000 User-Agent: KMail/1.9.10 Cc: Ross Morley References: <4A12EC43.3010107@tensilica.com> In-Reply-To: <4A12EC43.3010107@tensilica.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline Message-Id: <200905191958.35348.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: 2009-05/txt/msg00402.txt.bz2 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, > + =A0 whether or not it is a software break planted by GDB. =A0If size != =3D 0, > + =A0 sets *size to that of the instruction or 0 if it need not be skippe= d. =A0*/ > + > +#ifndef STOPPED_BY_TRAP_INSTRUCTION > +#define STOPPED_BY_TRAP_INSTRUCTION(size) \ > + =A0(*current_target.to_stopped_by_trap_instruction) (size) > +#endif > =A0#define target_get_ada_task_ptid(lwp, tid) \ > =A0 =A0 =A0 (*current_target.to_get_ada_task_ptid) (lwp,tid) > =A0 Please rename this to target_stopped_by_trap_instruction, and make it unconditionally defined. We don't support overriding of these target macros anymore. > + if (target_has_execution && !ptid_equal (inferior_ptid, null_ptid)) > + { > + struct thread_info *tp =3D inferior_thread (); > + if (tp->program_breakpoint_hit && tp->program_breakpoint_size !=3D= 0 > + && execution_direction !=3D EXEC_REVERSE && read_pc () =3D=3D s= top_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. + /* "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= =20 + the break, display location information as for normal_stop. */ + if (target_has_execution && !ptid_equal (inferior_ptid, null_ptid)) + { + struct thread_info *tp =3D inferior_thread (); + if (tp->program_breakpoint_hit && tp->program_breakpoint_size !=3D 0 + && execution_direction !=3D EXEC_REVERSE && read_pc () =3D=3D sto= p_pc + && count > 0) + { + count--; + write_pc (read_pc () + tp->program_breakpoint_size); + if (count =3D=3D 0) + { + reinit_frame_cache (); + if (ui_out_is_mi_like_p (uiout)) + { + ui_out_field_string=20 + (uiout, "reason",=20 + 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=20 + print_stack_frame (get_selected_frame (NULL), -1, SRC_LINE); + } + } + } + This is broken for the !single_inst cases (step/next). step N is mishandle= d. It also doesn't take into account that there may be a breakpoint at ORIG_PC + program_breakpoint_size. What if a "program breakpoint" is reported for a GDB breakpoint? You shoul= dn't just move the PC in that case. I don't see where that is handled. See co= mment on next hunk. I suspect this short circuit should be done elsewhere, some= where where it will be possible to pass through handle_inferior_event after moving the PC forward. 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. >=20 > + /* Handle case of hitting a program breakpoint (break instruction > + in target program, not planted by or known to GDB). */ > +=20=20 > + if (ecs->event_thread->program_breakpoint_hit) > + { > + stop_print_frame =3D 1; > +=20=20=20=20=20=20 > + /* 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. */ > +=20=20=20=20=20=20 > + stop_stepping (ecs); > + return; > + } > + > /* Handle cases caused by hitting a breakpoint. */ > { Hmmm, confused: can't the target report a program breakpoint hit for a PC where there's a gdb breakpoint inserted as well? 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; +static int +linux_stopped_by_trap_instruction (int *size) +{ + CORE_ADDR stop_pc =3D get_stop_pc(); + int trap_size =3D 0; + + if (the_low_target.trap_size_at !=3D NULL) + trap_size =3D (*the_low_target.trap_size_at) (stop_pc); + else if (the_low_target.breakpoint_at !=3D NULL + && (*the_low_target.breakpoint_at) (stop_pc)) + trap_size =3D the_low_target.breakpoint_len; + + if (trap_size !=3D 0) + { + *size =3D (the_low_target.get_pc !=3D NULL=20 + && (*the_low_target.get_pc) () =3D=3D stop_pc) + ? trap_size : 0; I think I got this bit, but it would be nice to have a comment here. + 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) 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. =46rom 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". 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, with program breakpoint size 0. If you instead stepi through that same sequence, you'll get a "program breakpoint" hit at 0002, with program breakpoint size 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. 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? --=20 Pedro Alves