Re: RFC: Program Breakpoints

[Ross Morley <ross@tensilica.com>]

I'm preparing the simplified version I described earlier for submission.

In stepping over a program break I increment the PC without running the
inferior (in step_1 of infcmd.c - see trimmed post below for context).
Then I update the frame cache and report the stop reason, new PC, etc.
My old pre-6.x implementation used a function that was deprecated in
6.x and recently removed.

What is the proper way to update the frame without the call to
   deprecated_update_frame_pc_hack (get_current_frame (), read_pc ());

I tried calling get_frame_pc (get_current_frame ()) and ignoring the result.
That should call frame_pc_unwind (). But the subsequent
    print_stack_frame (get_selected_frame (NULL), -1, LOC_AND_ADDRESS);
reports the address of the break to MI, not the incremented PC.

I also tried reinit_frame_cache () and it works OK. Seems drastic though.

For convenience, here's the code snippet from step_1 of infcmd.c:

  else if (STOPPED_BY_PROGRAM_BREAKPOINT (&program_break_size) &&
           program_break_size != 0 && read_pc() == stop_pc && count > 0)
    {
      /* "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.  */
      count--;
      write_pc (read_pc () + program_break_size);
      if (count == 0)
        {
          deprecated_update_frame_pc_hack (get_current_frame (), read_pc 
());
          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);
        }
    }


Thanks,
Ross


Ross Morley wrote:


>
> The Problem
> -----------
>
> When the target program hits a breakpoint of any kind, GDB receives
> a SIGTRAP event. As Aleksander pointed out, if the PC is not in the
> breakpoint table GDB stops because it doesn't know what to do with
> this event. In fact it doesn't always stop. When GDB is stepping
> (for example when a software watchpoint is set) it sees the SIGTRAP
> as a normal step event, so it continues stepping. If the break
> instruction didn't increment the PC, it keeps executing the same
> break instruction forever. This is very frustrating for users (who
> reported this as a bug) because when you have a s/w watchpoint and
> are stepping, execution is expected to be slow, so you wait a long
> time before you realize it's hung.
>
<stuff deleted>

> The Solution
> ------------
>
> The first part is to recognize when you hit a program break.
> The second is to step over it when you want to resume.
>
<stuff deleted>

> Having discovered that it stopped for a program breakpoint, GDB
> is able to report the fact to both the CLI and MI. I also defined
> a gdbarch function to report the meaning of "kind" in an arch-
> specific way. This improves the debugging experience when the
> target uses different kinds of breakpoints to denote different
> reasons for stopping. (However, in the interest of simplicity,
> my revised proposal won't report all that - it is anyway obvious
> from a disassembly if not a symbol associated with the PC; GDB
> itself only needs to know the size to skip the breakpoint).
>
> When the user elects to continue after a program breakpoint,
> GDB increments the PC by the size of the break that was reported
> in the "kind" argument. A gdbarch function handles this. Of
> course it might do nothing if the PC was already advanced.
>
> I haven't discussed all the implementation details here.
> Please see the attached 6.8 based patch for the actual code.
> Note this patch does not pretend to be ready for submission!
>
<stuff deleted>

> Proposed Improved, Simplified Solution
> --------------------------------------
>
> The remote protocol extension would be:
>    TAAtrap[:size]
> where ":size" is optional and may only be provided to GDB by the
> target agent if the PC is in fact pointing to the instruction
> that caused the break, and if omitted is taken to be 0.
> GDB will skip the break by PC += size (no effect if size is 0).
> Note it is not necessary to call gdbarch_breakpoint_from_pc().
>
> The gdbarch functions to extract the size and decipher "kind"
> are not needed. The target interface function
>    STOPPED_BY_PROGRAM_BREAKPOINT(k)
> becomes
>    STOPPED_BY_PROGRAM_BREAKPOINT(size)
> where 'size' is an address in which the size is returned.
>
> We might also want to consider calling it "program trap" and
> keep the term "breakpoint" for things that GDB knows about.
>
>
> Comments are welcome. We at Tensilica would like to see this
> refined and incorporated into mainline GDB.
>
> This can certainly coexist with permanent breakpoints,
> however it is (I think) a bit more general. If the people
> who use permanent breakpoints would care to comment,
> perhaps we can somehow reconcile these into one feature.
>
> Thanks,
> Ross Morley
> Tensilca, Inc.
> ross@tensilica.com
>
>
>diff -urN gdb-6.8-orig/gdb/infcmd.c gdb-6.8-new/gdb/infcmd.c
>--- gdb-6.8-orig/gdb/infcmd.c	2009-03-19 17:29:48.000000000 -0700
>+++ gdb-6.8-new/gdb/infcmd.c	2009-03-20 11:33:21.000000000 -0700
>@@ -696,6 +696,9 @@
>   struct frame_info *frame;
>   struct cleanup *cleanups = 0;
>   int async_exec = 0;
>+/* TENSILICA_LOCAL */
>+  int program_break_kind;
>+/* END TENSILICA_LOCAL */
> 
>   ERROR_NO_INFERIOR;
> 
>@@ -725,6 +728,33 @@
>       else
>         make_exec_cleanup (disable_longjmp_breakpoint_cleanup, 0 /*ignore*/);
>     }
>+/* TENSILICA_LOCAL */
>+  else if (STOPPED_BY_PROGRAM_BREAKPOINT (&program_break_kind) && 
>+	   read_pc() == stop_pc && count > 0)
>+    {
>+      /* "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.  */
>+      count--;
>+      gdbarch_skip_program_breakpoint (current_gdbarch, program_break_kind);
>+      if (count == 0)
>+	{
>+	  deprecated_update_frame_pc_hack (get_current_frame (), read_pc ());
>+	  if (ui_out_is_mi_like_p (uiout))
>+	    {
>+	      ui_out_field_string (uiout, "reason", "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);
>+	}
>+    }
>+/* END TENSILICA_LOCAL */
> 
>
>  
>