Re: [PATCH 3/3] fix skipping permanent breakpoints

[Yao Qi <yao at codesourcery dot com>]

Pedro Alves <palves@redhat.com> writes:

> The patch also adds a new gdb.base/bp-permanent.exp test that
> exercises many different code paths related to stepping permanent
> breakpoints, including the stepping with signals cases.  The test uses
> "hack/trick" to make it work on all (or most) platforms -- it doesn't
> really hard code a breakpoint instruction.

It is great to test permanent breakpoint in an arch-independent way.  It
exposes an existing issue on permanent breakpoint which is found when I
test your patch series on arm.  breakpoint.c:bp_location_has_shadow
should return false for permanent breakpoint because permanent
breakpoint doesn't have shadow, otherwise, breakpoint_xfer_memory
destroys contents read from memory_xfer_partial_1 and the internal error
is triggered as below,

(gdb) PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: stepi signal with handler: queue-signal SIGUSR1
stepi^M
../../../git/gdb/infrun.c:2112: internal-error: resume: Assertion `tp->control.step_resume_breakpoint->loc->permanent' failed.^M
A problem internal to GDB has been detected,

> +      else
> +	{
> +	  /* There's no signal to pass.  Skip the permanent
> +	     breakpoint, and arrange for any breakpoint at the new PC
> +	     to be reported back to handle_inferior_event.  */
> +
> +	  if (debug_infrun)
> +	    fprintf_unfiltered (gdb_stdlog,
> +				"infrun: resume: skipping permanent breakpoint\n");
> +	  gdbarch_skip_permanent_breakpoint (gdbarch, regcache);
> +	  /* Update pc to reflect the new address from which we will
> +	     execute instructions.  */
> +	  pc = regcache_read_pc (get_thread_regcache (inferior_ptid));

Is get_thread_regcache (inferior_ptid) equivalent to regcache?  If it
is, we can use regcache which is shorter.

> +	  tp->prev_pc = pc;
> +
> +	  clear_step_over_info ();
> +	  tp->control.trap_expected = 0;
> +
> +	  /* There may or not be a breakpoint at the new address.  If
> +	     we're stepping then we're done.  Otherwise, make sure
> +	     there's a breakpoint at the current address, and let it
> +	     trigger.  */

Sorry, I can't map the comments to the code.  Is "Otherwise" in the
comments about the else branch which doesn't exist?  Can you
elaborate?

> +	  if (step)
> +	    {
> +	      /* If we already have a step-resume breakpoint set, then
> +		 we can just continue until that one is hit.  */

When I read the comments, the code in my mind is like:

if (tp->control.step_resume_breakpoint != NULL)
  step = 0;

which is different from your code below.

> +	      if (tp->control.step_resume_breakpoint == NULL)
> +		insert_step_resume_breakpoint_at_frame (get_current_frame ());
> +	      step = 0;
> +	    }
> +
> +	  insert_breakpoints ();
> +	}
>      }
>  

> +
> +    with_test_prefix "next trips on permanent bp" {
> +	delete_breakpoints
> +
> +	gdb_breakpoint "test_next"
> +	gdb_continue_to_breakpoint "test_next"
> +
> +	gdb_breakpoint "$line_bp"
> +	gdb_test "condition \$bpnum 0"
> +
> +	gdb_test "next" "after next .*"
> +    }
> +
> +    if ![target_info exists gdb,nosignals] {
> +

We need also check can_single_step_to_signal_handler.

-- 
Yao (éå)