From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9920 invoked by alias); 19 May 2009 19:53:11 -0000 Received: (qmail 9910 invoked by uid 22791); 19 May 2009 19:53:10 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_SOFTFAIL 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; Tue, 19 May 2009 19:53:04 +0000 Received: from localhost (unknown [127.0.0.1]) by mailgw.tensilica.com (Postfix) with ESMTP id 5156811604E5; Tue, 19 May 2009 19:53:03 +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 31567-07; Tue, 19 May 2009 12:53:03 -0700 (PDT) Received: from mail.tensilica.com (mail.tensilica.com [192.168.15.138]) by mailgw.tensilica.com (Postfix) with ESMTP id 12354116051C; Tue, 19 May 2009 12:53:00 -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, 19 May 2009 12:52:02 -0700 Message-ID: <4A130DE1.9060202@tensilica.com> Date: Tue, 19 May 2009 19:53: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: [Fwd: 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/msg00404.txt.bz2 Thanks Pedro. This is the kind of feedback I was looking for in regard to effects on threads and other archs. I will have to go through your comments and think about those scenarios you mentioned with stepping. Before I do that I do want to clarify a couple of things for you. First, the intention is that a program breakpoint is not reported in the case of hitting a GDB breakpoint. That is because you hit the inserted GDB breakpoint. You will execute the program breakpoint at the same location when you remove the GDB breakpoint to step over it. You cannot hit both at the same time. I think this corner case (GDB breakpoint over program breakpoint) will be handled as for other stop reasons that occur while stepping over a GDB breakpoint, but I will look more closely at this. Test cases seem impossible to write completely generically because the test needs a target-specific break instruction. We have one (not in this patch) I could submit for Xtensa. It places 2 sizes of program breakpoints for Xtensa as follows: __asm__("break 7,15"); __asm__("break.n 7"); Is there a way to write a generic test with a program break? I will go through your feedback thouroughly in the next couple of days. Thanks, Ross 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. > > > >>+ 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. > >+ /* "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. >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 shouldn't >just move the PC in that case. I don't see where that is handled. 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. > >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. > > > >>+ /* 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; >>+ } >>+ >> /* 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 = 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. > >+ 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. > >>>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". > >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? > > >