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


On 03/06/2014 12:02 AM, Doug Evans wrote:
> Pedro Alves <palves@redhat.com> writes:
> Hi.
> I took a look at this patch and have some comments.

Thanks!

> 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;
> 

Thanks.  I missed this, as the whole block is removed in the
next patch.

> 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

Fixed.

> 
> 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);

Fixed.

> 
> 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?

Yes.  Say you're in async + bps always-inserted mode, and the
user sets a breakpoint just while GDB is stepping over
a breakpoint.  We'd end up inserting all breakpoints, even
the one being stepped over.  That's actually a bug at present.
So we should make sure that we don't insert a breakpoint at the
location we're trying to step over, until the step over is
done (or aborted).  That's usually until the next event, but
thinking again, it might not be -- say we get a signal while
stepping over the breakpoint.  So I simplified "too much"
in v2...

The code that handles stepping to complete non-continuable
watchpoints:

  /* If necessary, step over this watchpoint.  We'll be back to display
     it in a moment.  */
  if (stopped_by_watchpoint
      && (target_have_steppable_watchpoint
	  || gdbarch_have_nonsteppable_watchpoint (gdbarch)))
    {
      /* At this point, we are stopped at an instruction which has
         attempted to write to a piece of memory under control of
         a watchpoint.  The instruction hasn't actually executed
         yet.  If we were to evaluate the watchpoint expression
         now, we would get the old value, and therefore no change
         would seem to have occurred.

         In order to make watchpoints work `right', we really need
         to complete the memory write, and then evaluate the
         watchpoint expression.  We do this by single-stepping the
	 target.

	 It may not be necessary to disable the watchpoint to stop over
	 it.  For example, the PA can (with some kernel cooperation)
	 single step over a watchpoint without disabling the watchpoint.

	 It is far more common to need to disable a watchpoint to step
	 the inferior over it.  If we have non-steppable watchpoints,
	 we must disable the current watchpoint; it's simplest to
	 disable all watchpoints and breakpoints.  */
      int hw_step = 1;

      if (!target_have_steppable_watchpoint)
	{
	  remove_breakpoints ();
	  /* See comment in resume why we need to stop bypassing signals
	     while breakpoints have been removed.  */
	  target_pass_signals (0, NULL);
	}
	/* Single step */
      hw_step = maybe_software_singlestep (gdbarch, stop_pc);
      target_resume (ecs->ptid, hw_step, GDB_SIGNAL_0);
      waiton_ptid = ecs->ptid;
      if (target_have_steppable_watchpoint)
	infwait_state = infwait_step_watch_state;
      else
	infwait_state = infwait_nonstep_watch_state;
      prepare_to_wait (ecs);
      return;
    }

could also be adjusted to use this scheme, btw.  But I
didn't try that, at least yet.  We might end up generalizing
a bit further stepping_over_breakpoint_at (stepping_past_instruction_at
in v3) into a "can I insert this breakpoint location" predicate,
but I'd rather not add abstraction when it's not necessary yet.

> If so that needs to be clearly documented.

I've polished the patch further, and extended the comments, trying
to capture the rationale for what we end up with.  I think
the result is better and clearer now -- many thanks for the poke.
Let me know if something's still not clear.  Here's what the
main comment says:

~~~~~~~~~~~~~~~~~~~~
/* The step-over info of the location that is being stepped over.

   Note that with async/breakpoint always-inserted mode, a user might
   set a new breakpoint/watchpoint/etc. exactly while a breakpoint is
   being stepped over.  As setting a new breakpoint inserts all
   breakpoints, we need to make sure the breakpoint being stepped over
   isn't inserted then.  We do that by only clearing the step-over
   info when the step-over is actually finished (or aborted).

   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).
   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.  Plus, not all step overs actually
   have breakpoint locations -- e.g., stepping past a single-step
   breakpoint, or stepping to complete a non-continuable
   watchpoint.  */
static struct step_over_info step_over_info;
~~~~~~~~~~~~~~~~~~~~

> 
> 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.

Yeah, I agree it'd look better, though it doesn't really work
for the reasons mentioned above.  My first attempt actually
went in that direction (it's similar to how gdbserver handles
this), and passed down the "except at" address down to
insert_breakpoints all the way to update_global_location_list.
Now that was getting ugly, to the point I considered switching
away from the current breakpoint location duplication scheme
to instead refcount locations/bp_target_info's.  :-P

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

I hope v3 is clearer now:

 https://sourceware.org/ml/gdb-patches/2014-03/msg00166.html

-- 
Pedro Alves


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