Re: [PATCH/DOC RFA] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"

[Pedro Alves <palves at redhat dot com>]

On 09/17/2014 02:12 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> In hindsight, this "auto" setting was unnecessary, and not the ideal
>> solution.  Non-stop mode does depends on breakpoints always-inserted
>> mode, but only as long as any thread is running.  If no thread is
>> running, no breakpoint can be missed.  The same is true for all-stop
>> too.  E.g., if, in all-stop mode, and the user does:
>>
>>  (gdb) c&
>>  (gdb) b foo
>>
>> That breakpoint at "foo" should be inserted immediately, but it
>> currently isn't -- currently it'll end up inserted only if the target
>> happens to trip on some event, and is re-resumed, e.g., an internal
>> breakpoint triggers that doesn't cause a user-visible stop, and so we
>> end up in keep_going calling insert_breakpoints.
>>
>> IOW, no matter whether in non-stop or all-stop, if the target fully
>> stops, we can remove breakpoints.  And no matter whether in all-stop
>> or non-stop, if any thread is running in the target, then we need
>> breakpoints to be immediately inserted.  And then, if the target has
> 
> Could we add some test cases for this? for example, breakpoint should be
> inserted immediately if any thread is running.

I'll try writing one.  May need to skip all-stop/remote, as we can't
talk to the target if anything is running (blocked waiting for the vCont
reply).

>> +/* update_global_location_list's modes of operation wrt to whether to
>> +   insert locations now.  */
>> +enum ugll_insert_mode
>> +{
>> +  /* Don't insert any breakpoint locations into the inferior, only
>> +     remove already-inserted locations that no longer should be
>> +     inserted.  Functions that delete a breakpoint or breakpoints
>> +     should specify this mode, so that deleting a breakpoint doesn't
>> +     have the side effect of inserting the locations of other
>> +     breakpoints that are marked not-inserted, but should_be_inserted
>> +     returns true on them.
>> +
>> +     This behavior is useful is situations close to tear-down -- e.g.,
>> +     after an exec, while the target still has execution, but
>> +     breakpoint shadows of the previous executable image should *NOT*
>> +     be restored to the new image; or before detaching, where the
>> +     target still has execution and wants to delete breakpoints from
>> +     GDB's lists, and all breakpoints had already been removed from
>> +     the inferior.  */
>> +  UGLL_DONT_INSERT,
>> +
>> +  /* May insert breakpoints iff breakpoints_should_be_inserted_now
>> +     claims breakpoints should be inserted now.  */
>> +  UGLL_MAY_INSERT,
>> +
>> +  /* Insert locations now, even if breakpoints_should_be_inserted_now
>> +     would return false.  For example, no thread is resumed yet, but
>> +     we're just about to resume the target.  */
>> +  UGLL_INSERT,
>> +};
>> +
> 
> What does the last sentence in comments mean?  "no thread is resumed
> yet, but we're just about to resume the target."

E.g., say everything all threads are stopped right now, and the user
does "continue".  We need to insert breakpoints _before_ resuming the
target, but breakpoints_should_be_inserted_now returns false at
that point, as no thread is running.  See where it's used:

@@ -2928,13 +2968,7 @@ insert_breakpoints (void)
 	update_watchpoint (w, 0 /* don't reparse.  */);
       }

-  update_global_location_list (1);
-
-  /* update_global_location_list does not insert breakpoints when
-     always_inserted_mode is not enabled.  Explicitly insert them
-     now.  */
-  if (!breakpoints_always_inserted_mode ())
-    insert_breakpoint_locations ();
+  update_global_location_list (UGLL_INSERT);
 }

I'll try to clarify the comment of UGLL_INSERT, and I'll preserve this
comment in insert_breakpoints somehow instead of removing it completely.

> The patch looks good to me.  However, IWBN to split it to two, patch 1
> is about refactor update_global_location_list and add enum ugll_insert_mode
> with only UGLL_DONT_INSERT and UGLL_MAY_INSERT.  patch 2 does the rest.

I'll do that.

Thanks,
Pedro Alves