This is the mail archive of the
mailing list for the GDB project.
Re: [PATCH/DOC RFA] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <yao at codesourcery dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Wed, 17 Sep 2014 14:49:20 +0100
- Subject: Re: [PATCH/DOC RFA] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"
- Authentication-results: sourceware.org; auth=none
- References: <1410913576-4928-1-git-send-email-palves at redhat dot com> <8738bq1jcz dot fsf at codesourcery dot com>
On 09/17/2014 02:12 PM, Yao Qi wrote:
> Pedro Alves <email@example.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
>> +/* 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.