This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set


Pedro Alves <palves@redhat.com> writes:
> gdb/
> 2014-03-05  Pedro Alves  <palves@redhat.com>
>
> 	PR breakpoints/7143
> 	* breakpoint.c (should_be_inserted): Don't insert breakpoints that
> 	are being stepped over.
> 	(breakpoint_address_match): Make extern.
> 	* breakpoint.h (breakpoint_address_match): New declaration.
> 	* inferior.h (stepping_over_breakpoint_at): New declaration.
> 	* infrun.c (step_over_aspace, step_over_address): New globals.
> 	(stepping_over_breakpoint_at): New function.
> 	(handle_inferior_event): Clear step_over_aspace and
> 	step_over_address when trap_expected is cleared.
> 	(proceed): Adjust step-over handling to set or clear
> 	step_over_aspace and step_over_address instead of removing all
> 	breakpoints.
> 	(handle_signal_stop): Clear step_over_aspace and step_over_address
> 	when trap_expected is cleared.
> 	(process_event_stop_test): Clear step_over_aspace,
> 	step_over_address and trap_expected if handling a GDB_SIGNAL_TRAP.
> 	Clear step_over_aspace and step_over_address when trap_expected is
> 	cleared.
> 	(proceed): Adjust step-over handling to set step_over_aspace and
> 	step_over_address instead of removing all breakpoints.

Hi.
I took a look at this patch and have some comments.

First, some nits to get them out of the way:

1) This can be deleted, remove_status is no longer used:

handle_signal_stop:
	  int remove_status = 0;

2) ChangeLog issues:
   - two ChangeLog entries for proceed, one for keep_going is missing
   - ChangeLog entry for process_event_stop_test doesn't match change
   - ChangeLog entry for handle_signal_stop doesn't match change

3) This looks unusual for long line handling:

> +extern int
> +  breakpoint_address_match (struct address_space *aspace1, CORE_ADDR addr1,
> +			    struct address_space *aspace2, CORE_ADDR addr2);

I know I've submitted:

extern int
foo (mumble);

and that got rejected (and I'm pretty sure it wasn't because I didn't
indent foo).

My more meatier comment is that it's hard to reason about the passing
of parameters from the caller of insert_breakpoints to
should_be_inserted via global variables (which is essentially what
you're doing (*1)) because each of them have multiple callers, and some of
the callers don't appear (AFAICT) immediately related to the task at hand.

(*1) It's not clear from my reading of the patch that there isn't more to
it than that though.
I see you clearing step_over_{aspace,address} in handle_inferior_event
so I'm left wondering if I'm missing something.  E.g., is there a reason
to not clear them immediately after calling insert_breakpoints?
If so that needs to be clearly documented.

AFAICT (and if I'm wrong hopefully this will at least lead to more
comments in the code to help clear things up), ideally (in the abstract
since we don't have lambdas) what we want is a variant of insert_breakpoints
that takes a predicate as an argument that says whether each bkpt should be
inserted.  Since we can't easily have that, how about this?

void
insert_breakpoints_except_at (struct address_space *except_aspace,
                              CORE_ADDR except_address;
{
  step_over_aspace = except_aspace;
  step_over_address = except_address

  insert_breakpoints ();

  step_over_aspace = NULL;
  step_over_address = 0;
}

... and not reset step_over_{aspace,address} anywhere else.

That would need to be augmented, of course, to handle the case of
insert_breakpoints throwing an error.  I'm just keeping it simple
for descriptive sake.

Doing it that way would go a long way to helping me, the reader,
to understand what's going on (but see caveats above).


>
> gdb/testsuite/
> 2014-03-05  Pedro Alves  <palves@redhat.com>
>
> 	PR breakpoints/7143
> 	* gdb.base/watchpoint.exp: Mention bugzilla bug number instead of
> 	old gnats gdb/38.  Remove kfail.  Adjust to use gdb_test instead
> 	of gdb_test_multiple.
>
> --
> v2
> 	- always clear step_over_aspace and step_over_address when
>           starting to handle an event.
> ---
>  gdb/breakpoint.c                      |  19 ++--
>  gdb/breakpoint.h                      |   9 ++
>  gdb/inferior.h                        |   6 ++
>  gdb/infrun.c                          | 170 ++++++++++++++++++++--------------
>  gdb/testsuite/gdb.base/watchpoint.exp |  13 +--
>  gdb/testsuite/gdb.cp/annota2.exp      |   3 -
>  gdb/testsuite/gdb.cp/annota3.exp      |   3 -
>  7 files changed, 127 insertions(+), 96 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 2f2c625..4381bbe 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -165,11 +165,6 @@ static void describe_other_breakpoints (struct gdbarch *,
>  					struct program_space *, CORE_ADDR,
>  					struct obj_section *, int);
>  
> -static int breakpoint_address_match (struct address_space *aspace1,
> -				     CORE_ADDR addr1,
> -				     struct address_space *aspace2,
> -				     CORE_ADDR addr2);
> -
>  static int watchpoint_locations_match (struct bp_location *loc1,
>  				       struct bp_location *loc2);
>  
> @@ -2034,6 +2029,13 @@ should_be_inserted (struct bp_location *bl)
>    if (bl->pspace->breakpoints_not_allowed)
>      return 0;
>  
> +  /* Don't insert a breakpoint we're trying to step over.  */
> +  if ((bl->loc_type == bp_loc_software_breakpoint
> +       || bl->loc_type == bp_loc_hardware_breakpoint)
> +      && stepping_over_breakpoint_at (bl->pspace->aspace,
> +				      bl->address))
> +    return 0;
> +
>    return 1;
>  }
>  
> @@ -6792,12 +6794,9 @@ watchpoint_locations_match (struct bp_location *loc1,
>  	  && loc1->length == loc2->length);
>  }
>  
> -/* Returns true if {ASPACE1,ADDR1} and {ASPACE2,ADDR2} represent the
> -   same breakpoint location.  In most targets, this can only be true
> -   if ASPACE1 matches ASPACE2.  On targets that have global
> -   breakpoints, the address space doesn't really matter.  */
> +/* See breakpoint.h.  */
>  
> -static int
> +int
>  breakpoint_address_match (struct address_space *aspace1, CORE_ADDR addr1,
>  			  struct address_space *aspace2, CORE_ADDR addr2)
>  {
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index 3c1fdfe..c682cdd 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -1135,6 +1135,15 @@ extern int hardware_watchpoint_inserted_in_range (struct address_space *,
>  extern int breakpoint_thread_match (struct address_space *, 
>  				    CORE_ADDR, ptid_t);
>  
> +/* Returns true if {ASPACE1,ADDR1} and {ASPACE2,ADDR2} represent the
> +   same breakpoint location.  In most targets, this can only be true
> +   if ASPACE1 matches ASPACE2.  On targets that have global
> +   breakpoints, the address space doesn't really matter.  */
> +
> +extern int
> +  breakpoint_address_match (struct address_space *aspace1, CORE_ADDR addr1,
> +			    struct address_space *aspace2, CORE_ADDR addr2);
> +
>  extern void until_break_command (char *, int, int);
>  
>  /* Initialize a struct bp_location.  */
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index 9f6375e..b6465de 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -220,6 +220,12 @@ void set_step_info (struct frame_info *frame, struct symtab_and_line sal);
>  
>  extern void clear_exit_convenience_vars (void);
>  
> +/* Returns true if we're trying to step over a breakpoint at ADDRESS
> +   in ASPACE.  */
> +
> +extern int stepping_over_breakpoint_at (struct address_space *aspace,
> +					CORE_ADDR address);
> +
>  /* From infcmd.c */
>  
>  extern void post_create_inferior (struct target_ops *, int);
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index bd55505..67f5fe1 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -977,6 +977,36 @@ static CORE_ADDR singlestep_pc;
>  static ptid_t saved_singlestep_ptid;
>  static int stepping_past_singlestep_breakpoint;
>  
> +/* If stepping over a breakpoint (not displaced stepping), this is the
> +   address (and address space) where that breakpoint is inserted.
> +   When not stepping over a breakpoint, STEP_OVER_ASPACE is NULL.
> +
> +   (Note: presently GDB can only step over a breakpoint at any given
> +   time.  Given threads that can't run code in the same address space
> +   as the breakpoint's can't really miss the breakpoint, GDB could be
> +   taught to step-over at most one breakpoint per address space (so
> +   this info could move to the address space object if/when GDB is
> +   extended).  Even with that, the set of breakpoints being stepped
> +   over will normally be much smaller than the set of all breakpoints,
> +   so a flag in the breakpoint location structure would be wasteful.
> +   A separate list also saves complexity and run-time, as otherwise
> +   we'd have to go through all breakpoint locations clearing their
> +   flag whenever we start a new sequence.  Similar considerations
> +   weigh against storing this info in the thread object.)  */
> +static struct address_space *step_over_aspace;
> +static CORE_ADDR step_over_address;
> +
> +/* See inferior.h.  */
> +
> +int
> +stepping_over_breakpoint_at (struct address_space *aspace,
> +			     CORE_ADDR address)
> +{
> +  return (step_over_aspace != NULL
> +	  && breakpoint_address_match (aspace, address,
> +				       step_over_aspace, step_over_address));
> +}
> +
>  
>  /* Displaced stepping.  */
>  
> @@ -1852,8 +1882,11 @@ a command like `return' or `jump' to continue execution."));
>        remove_single_step_breakpoints ();
>        singlestep_breakpoints_inserted_p = 0;
>  
> -      insert_breakpoints ();
> +      step_over_aspace = NULL;
> +      step_over_address = 0;
>        tp->control.trap_expected = 0;
> +
> +      insert_breakpoints ();
>      }
>  
>    if (should_resume)
> @@ -1894,12 +1927,7 @@ a command like `return' or `jump' to continue execution."));
>  	     hit, by single-stepping the thread with the breakpoint
>  	     removed.  In which case, we need to single-step only this
>  	     thread, and keep others stopped, as they can miss this
> -	     breakpoint if allowed to run.
> -
> -	     The current code actually removes all breakpoints when
> -	     doing this, not just the one being stepped over, so if we
> -	     let other threads run, we can actually miss any
> -	     breakpoint, not just the one at PC.  */
> +	     breakpoint if allowed to run.  */
>  	  resume_ptid = inferior_ptid;
>  	}
>  
> @@ -2217,22 +2245,26 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
>    tp = inferior_thread ();
>  
>    if (force_step)
> +    tp->control.trap_expected = 1;
> +
> +  /* If we need to step over a breakpoint, and we're not using
> +     displaced stepping to do so, insert all breakpoints (watchpoints,
> +     etc.) but the one we're stepping over, step one instruction, and
> +     then re-insert the breakpoint when that step is finished.  */
> +  if (tp->control.trap_expected && !use_displaced_stepping (gdbarch))
>      {
> -      tp->control.trap_expected = 1;
> -      /* If displaced stepping is enabled, we can step over the
> -	 breakpoint without hitting it, so leave all breakpoints
> -	 inserted.  Otherwise we need to disable all breakpoints, step
> -	 one instruction, and then re-add them when that step is
> -	 finished.  */
> -      if (!use_displaced_stepping (gdbarch))
> -	remove_breakpoints ();
> +      struct regcache *regcache = get_current_regcache ();
> +
> +      step_over_aspace = get_regcache_aspace (regcache);
> +      step_over_address = regcache_read_pc (regcache);
> +    }
> +  else
> +    {
> +      step_over_aspace = NULL;
> +      step_over_address = 0;
>      }
>  
> -  /* We can insert breakpoints if we're not trying to step over one,
> -     or if we are stepping over one but we're using displaced stepping
> -     to do so.  */
> -  if (! tp->control.trap_expected || use_displaced_stepping (gdbarch))
> -    insert_breakpoints ();
> +  insert_breakpoints ();
>  
>    if (!non_stop)
>      {
> @@ -3220,6 +3252,11 @@ handle_inferior_event (struct execution_control_state *ecs)
>    /* Always clear state belonging to the previous time we stopped.  */
>    stop_stack_dummy = STOP_NONE;
>  
> +  /* Let the breakpoints module insert all breakpoints.  If the next
> +     step requires skipping a breakpoint, we'll set this again.  */
> +  step_over_aspace = NULL;
> +  step_over_address = 0;
> +
>    if (ecs->ws.kind == TARGET_WAITKIND_NO_RESUMED)
>      {
>        /* No unwaited-for children left.  IOW, all resumed children
> @@ -4013,35 +4050,17 @@ handle_signal_stop (struct execution_control_state *ecs)
>  	      singlestep_breakpoints_inserted_p = 0;
>  	    }
>  
> -	  /* If the arch can displace step, don't remove the
> -	     breakpoints.  */
> -	  thread_regcache = get_thread_regcache (ecs->ptid);
> -	  if (!use_displaced_stepping (get_regcache_arch (thread_regcache)))
> -	    remove_status = remove_breakpoints ();
> -
> -	  /* Did we fail to remove breakpoints?  If so, try
> -	     to set the PC past the bp.  (There's at least
> -	     one situation in which we can fail to remove
> -	     the bp's: On HP-UX's that use ttrace, we can't
> -	     change the address space of a vforking child
> -	     process until the child exits (well, okay, not
> -	     then either :-) or execs.  */
> -	  if (remove_status != 0)
> -	    error (_("Cannot step over breakpoint hit in wrong thread"));
> -	  else
> -	    {			/* Single step */
> -	      if (!non_stop)
> -		{
> -		  /* Only need to require the next event from this
> -		     thread in all-stop mode.  */
> -		  waiton_ptid = ecs->ptid;
> -		  infwait_state = infwait_thread_hop_state;
> -		}
> -
> -	      ecs->event_thread->stepping_over_breakpoint = 1;
> -	      keep_going (ecs);
> -	      return;
> +	  if (!non_stop)
> +	    {
> +	      /* Only need to require the next event from this thread
> +		 in all-stop mode.  */
> +	      waiton_ptid = ecs->ptid;
> +	      infwait_state = infwait_thread_hop_state;
>  	    }
> +
> +	  ecs->event_thread->stepping_over_breakpoint = 1;
> +	  keep_going (ecs);
> +	  return;
>  	}
>      }
>  
> @@ -4406,6 +4425,13 @@ process_event_stop_test (struct execution_control_state *ecs)
>    frame = get_current_frame ();
>    gdbarch = get_frame_arch (frame);
>  
> +  if (ecs->event_thread->control.trap_expected
> +      && ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP)
> +    {
> +      /* We got our trap.  */
> +      ecs->event_thread->control.trap_expected = 0;
> +    }
> +
>    what = bpstat_what (ecs->event_thread->control.stop_bpstat);
>  
>    if (what.call_dummy)
> @@ -5727,6 +5753,9 @@ keep_going (struct execution_control_state *ecs)
>      }
>    else
>      {
> +      volatile struct gdb_exception e;
> +      struct regcache *regcache = get_current_regcache ();
> +
>        /* Either the trap was not expected, but we are continuing
>  	 anyway (if we got a signal, the user asked it be passed to
>  	 the child)
> @@ -5740,33 +5769,32 @@ keep_going (struct execution_control_state *ecs)
>  	 already inserted breakpoints.  Therefore, we don't
>  	 care if breakpoints were already inserted, or not.  */
>  
> -      if (ecs->event_thread->stepping_over_breakpoint)
> +      /* If we need to step over a breakpoint, and we're not using
> +	 displaced stepping to do so, insert all breakpoints
> +	 (watchpoints, etc.) but the one we're stepping over, step one
> +	 instruction, and then re-insert the breakpoint when that step
> +	 is finished.  */
> +      if (ecs->event_thread->stepping_over_breakpoint
> +	  && !use_displaced_stepping (get_regcache_arch (regcache)))
>  	{
> -	  struct regcache *thread_regcache = get_thread_regcache (ecs->ptid);
> +	  /* Can't step over more than one breakpoint simultaneously
> +	     without displaced stepping.  */
> +	  gdb_assert (step_over_aspace == NULL);
>  
> -	  if (!use_displaced_stepping (get_regcache_arch (thread_regcache)))
> -	    {
> -	      /* Since we can't do a displaced step, we have to remove
> -		 the breakpoint while we step it.  To keep things
> -		 simple, we remove them all.  */
> -	      remove_breakpoints ();
> -	    }
> +	  step_over_aspace = get_regcache_aspace (regcache);
> +	  step_over_address = regcache_read_pc (regcache);
>  	}
> -      else
> -	{
> -	  volatile struct gdb_exception e;
>  
> -	  /* Stop stepping if inserting breakpoints fails.  */
> -	  TRY_CATCH (e, RETURN_MASK_ERROR)
> -	    {
> -	      insert_breakpoints ();
> -	    }
> -	  if (e.reason < 0)
> -	    {
> -	      exception_print (gdb_stderr, e);
> -	      stop_stepping (ecs);
> -	      return;
> -	    }
> +      /* Stop stepping if inserting breakpoints fails.  */
> +      TRY_CATCH (e, RETURN_MASK_ERROR)
> +	{
> +	  insert_breakpoints ();
> +	}
> +      if (e.reason < 0)
> +	{
> +	  exception_print (gdb_stderr, e);
> +	  stop_stepping (ecs);
> +	  return;
>  	}
>  
>        ecs->event_thread->control.trap_expected
> diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
> index 80d75cb..1123824 100644
> --- a/gdb/testsuite/gdb.base/watchpoint.exp
> +++ b/gdb/testsuite/gdb.base/watchpoint.exp
> @@ -550,21 +550,16 @@ proc test_complex_watchpoint {} {
>  proc test_watchpoint_and_breakpoint {} {
>      global gdb_prompt
>  
> -    # This is a test for PR gdb/38, which involves setting a
> +    # This is a test for PR breakpoints/7143, which involves setting a
>      # watchpoint right after you've reached a breakpoint.
>  
>      if [runto func3] then {
>  	gdb_breakpoint [gdb_get_line_number "second x assignment"]
>  	gdb_continue_to_breakpoint "second x assignment"
>  	gdb_test "watch x" ".*atchpoint \[0-9\]+: x"
> -	gdb_test_multiple "next" "next after watch x" {
> -	    -re ".*atchpoint \[0-9\]+: x\r\n\r\nOld value = 0\r\nNew value = 1\r\n.*$gdb_prompt $" {
> -		pass "next after watch x"
> -	    }
> -	    -re "\[0-9\]+\[\t \]+y = 1;\r\n$gdb_prompt $" {
> -		kfail "gdb/38" "next after watch x"
> -	    }
> -	}
> +	gdb_test "next" \
> +	    ".*atchpoint \[0-9\]+: x\r\n\r\nOld value = 0\r\nNew value = 1\r\n.*" \
> +	    "next after watch x"
>  
>  	gdb_test_no_output "delete \$bpnum" "delete watch x"
>      }
> diff --git a/gdb/testsuite/gdb.cp/annota2.exp b/gdb/testsuite/gdb.cp/annota2.exp
> index 00a6067..6fbf4b5 100644
> --- a/gdb/testsuite/gdb.cp/annota2.exp
> +++ b/gdb/testsuite/gdb.cp/annota2.exp
> @@ -165,9 +165,6 @@ gdb_test_multiple "next" "watch triggered on a.x" {
>      -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n${frames_invalid}${breakpoints_invalid}\r\n\032\032watchpoint 3\r\n.*atchpoint 3: a.x\r\n\r\nOld value = 0\r\nNew value = 1\r\n\r\n\032\032frame-begin 0 $hex\r\n\r\n\032\032frame-function-name\r\nmain\r\n\032\032frame-args\r\n \\(\\)\r\n\032\032frame-source-begin\r\n at \r\n\032\032frame-source-file\r\n.*$srcfile\r\n\032\032frame-source-file-end\r\n:\r\n\032\032frame-source-line\r\n$decimal\r\n\032\032frame-source-end\r\n\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n.*$gdb_prompt$" {
>  	pass "watch triggered on a.x"
>      }
> -    -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n${frames_invalid}\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n$gdb_prompt$" {
> -	kfail "gdb/38" "watch triggered on a.x"
> -    }
>  }
>  
>  
> diff --git a/gdb/testsuite/gdb.cp/annota3.exp b/gdb/testsuite/gdb.cp/annota3.exp
> index 957d371..bbf9a1e 100644
> --- a/gdb/testsuite/gdb.cp/annota3.exp
> +++ b/gdb/testsuite/gdb.cp/annota3.exp
> @@ -169,9 +169,6 @@ gdb_test_multiple "next" "watch triggered on a.x" {
>      -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n\r\n\032\032watchpoint 3\r\n.*atchpoint 3: a.x\r\n\r\nOld value = 0\r\nNew value = 1\r\n\r\n(\032\032frame-begin 0 0x\[0-9a-z\]+\r\n|)main \\(\\) at .*$srcfile:$decimal\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032stopped\r\n.*$gdb_prompt$" { 
>  	pass "watch triggered on a.x"
>      }
> -    -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032stopped\r\n$gdb_prompt$" {
> -	kfail "gdb/38" "watch triggered on a.x"
> -    }
>  }
>  
>  #


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]