From: Andrew Burgess <aburgess@redhat.com>
To: Pedro Alves <pedro@palves.net>, gdb-patches@sourceware.org
Subject: Re: [PATCHv2 5/6] gdb: add inferior-specific breakpoints and watchpoints
Date: Fri, 03 Feb 2023 16:55:03 +0000 [thread overview]
Message-ID: <87k00yr8ns.fsf@redhat.com> (raw)
In-Reply-To: <b4b50812-f65d-cdb2-6777-a5ede2471e7a@palves.net>
Pedro Alves <pedro@palves.net> writes:
> On 2023-01-20 9:46 a.m., Andrew Burgess via Gdb-patches wrote:
>> This commit extends the breakpoint mechanism to allow for inferior
>> specific breakpoints (and watchpoints).
>>
>> As GDB gains better support for multiple connections, and so for
>> running multiple (possibly unrelated) inferiors, then it is not hard
>> to imagine that a user might wish to create breakpoints that apply to
>> any thread in a single inferior. To achieve this currently, the user
>> would need to create a condition possibly making use of the $_inferior
>> convenience variable, which, though functional, isn't the most user
>> friendly.
>
> So IIUC, if you do:
>
> (gdb) break foo inferior 1
>
> and foo exists in other inferiors, the resulting breakpoint will still
> have locations for the other inferiors. Is that so?
>
> I think that instead, we should not get any location for other inferiors.
> I.e., we should do location pre-filtering, simply by not looking for
> locations in pspaces other than the inferior's. I think that can be
> done easily by simply passing the inferior's pspace as search_pspace argument
> to decode_line_full.
Thanks for all your great feedback. I'll work through the rest of your
comments, but I want to follow up on this one.
Baris already raised the point of filtering per-inferior breakpoints so
a to avoid adding extra locations.
I 100% agree that this is something that we should do, and this is on my
plan if/when I land this series.
But I don't see any particular difference between per-inferior and
per-thread breakpoints in this regard. Please correct me if I'm wrong,
but a per-thread breakpoint will insert into every inferior despite only
ever triggering for a single thread in a single inferior (per thread
breakpoints are on the global thread-id).
So, because this oddity already exists in GDB, my plan was to land this
feature, then follow up with a patch to limit both per-thread and
per-inferior breakpoints to only insert into the correct pspace.
Thanks,
Andrew
>
> And then it might be interesting to have a testcase that makes sure that after
> following a vfork, the breakpoint still only triggers in the inferior the user
> specified, even though the breakpoint is physicaly planted in both
> parent/child (because they share the address space).
>
>> @@ -1462,11 +1462,28 @@ breakpoint_set_thread (struct breakpoint *b, int thread)
>> {
>> int old_thread = b->thread;
>>
>> + gdb_assert (thread == -1 || b->inferior == -1);
>> +
>> b->thread = thread;
>> if (old_thread != thread)
>> gdb::observers::breakpoint_modified.notify (b);
>> }
>>
>> +/* Set the inferior for breakpoint B to INFERIOR. If INFERIOR is -1, make
>> + the breakpoint work for any inferior. */
>> +
>> +void
>> +breakpoint_set_inferior (struct breakpoint *b, int inferior)
>> +{
>> + int old_inferior = b->inferior;
>> +
>> + gdb_assert (inferior == -1 || b->thread == -1);
>> +
>> + b->inferior = inferior;
>> + if (old_inferior != inferior)
>> + gdb::observers::breakpoint_modified.notify (b);
>> +}
>> +
>> /* Set the task for this breakpoint. If TASK is 0, make the
>> breakpoint work for any task. */
>>
>> @@ -3152,6 +3169,12 @@ insert_breakpoint_locations (void)
>> && !valid_global_thread_id (bl->owner->thread))
>> continue;
>>
>> + /* Or inferior specific breakpoints if the inferior no longer
>> + exists. */
>> + if (bl->owner->inferior != -1
>> + && !valid_global_inferior_id (bl->owner->inferior))
>> + continue;
>> +
>> switch_to_program_space_and_thread (bl->pspace);
>>
>> /* For targets that support global breakpoints, there's no need
>> @@ -3256,6 +3279,31 @@ Thread-specific breakpoint %d deleted - thread %s no longer in the thread list.\
>> }
>> }
>>
>> +/* Called when inferior INF has exited. Remove per-inferior breakpoints. */
>
> We remove thread-specific breapoints when the thread exits, because when the
> thread exits, we delete it (the thread). But when the inferior exits, the inferior
> is not deleted, it still exists, ready for a re-run. So why would we remove the breakpoints
> when the inferior has exited, if the inferior still exists? I'd have thought we'd
> remove the breakpoints when the inferior is _removed_, instead.
>
> If removing then on exit, then it feels like it could be done from
> breakpoint_init_inferior, as that is called when mourning the inferior, and it
> is responsible for deleting breakpoints that no longer make sense to carry
> for the inferior.
>
> But I'm not convinced we should delete the breakpoint on exit, anyhow.
>
>> +
>> +static void
>> +remove_inferior_breakpoints (struct inferior *inf)
>> +{
>> + for (breakpoint *b : all_breakpoints_safe ())
>> + {
>> + if (b->inferior == inf->num && user_breakpoint_p (b))
>> + {
>> + /* Tell the user the breakpoint has been deleted. But only for
>> + breakpoints that would not normally have been deleted at the
>> + next stop anyway. */
>> + if (b->disposition != disp_del
>> + && b->disposition != disp_del_at_next_stop)
>> + gdb_printf (_("\
>> +Inferior-specific breakpoint %d deleted - inferior %d has exited.\n"),
>> + b->number, inf->num);
>> +
>> + /* Hide it from the user and mark it for deletion. */
>> + b->number = 0;
>
> Doesn't this mess up MI breakpoint deleted notifications?
>
>> + b->disposition = disp_del_at_next_stop;
>
> If the inferior is gone, can't we delete the breakpoint straight away? Maybe
> we'd need to call mark_breakpoints_out ?
>
>
>> + }
>> + }
>> +}
>> +
>> /* See breakpoint.h. */
>>
>> void
>
>
>> @item break
>> When called without any arguments, @code{break} sets a breakpoint at
>> @@ -4983,7 +5036,7 @@
>>
>> @table @code
>> @kindex watch
>> -@item watch @r{[}-l@r{|}-location@r{]} @var{expr} @r{[}thread @var{thread-id}@r{]} @r{[}mask @var{maskvalue}@r{]} @r{[}task @var{task-id}@r{]}
>> +@item watch @r{[}-l@r{|}-location@r{]} @var{expr} @r{[}thread @var{thread-id}@r{]} @r{[}inferior @var{inferior-id}@r{]} @r{[}mask @var{maskvalue}@r{]} @r{[}task @var{task-id}@r{]}
>> Set a watchpoint for an expression. @value{GDBN} will break when the
>> expression @var{expr} is written into by the program and its value
>> changes. The simplest (and the most popular) use of this command is
>> @@ -5000,8 +5053,10 @@
>> that watchpoints restricted to a single thread in this way only work
>> with Hardware Watchpoints.
>>
>> -Similarly, if the @code{task} argument is given, then the watchpoint
>> -will be specific to the indicated Ada task (@pxref{Ada Tasks}).
>> +Similarly, if the @code{inferior} argument is given, then the
>> +watchpoint will trigger only for the specific inferior, or if the
>> +@code{task} argument is given, then the watchpoint will be specific to
>> +the indicated Ada task (@pxref{Ada Tasks}).
>
> A watchpoint is already per-pspace, so inherently per-inferior in most scenarios -- we only
> evaluate the watched expression in the pspace where the watchpoint was created, and only
> track the resulting value for changes in one pspace. Note what happens currently if you
> set a watchpoint and you have multiple inferiors:
>
> (gdb) info inferiors
> Num Description Connection Executable
> 1 process 3974843 1 (native) /home/pedro/watch
> * 2 process 3975163 1 (native) /home/pedro/watch
> (gdb) watch global
> Hardware watchpoint 3: global
> (gdb) info breakpoints
> Num Type Disp Enb Address What
> 3 hw watchpoint keep y global inf 2
> ^^^^^
> (gdb)
>
> The only case where the new feature is useful for watchpoints, is if
> you're debugging multiple inferiors that share the address space, like right
> after following a vfork. In that case, here's what you get today:
>
> $ gdb ./testsuite/outputs/gdb.base/foll-vfork/foll-vfork
> GNU gdb (GDB) 14.0.50.20230127-git
> ...
> (gdb) start
> ...
> Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdbe8) at /home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/foll-vfork.c:31
> 31 pid = 1 + argc;
> (gdb) set detach-on-fork off
> (gdb) set follow-fork-mode child
> (gdb) info inferiors
> Num Description Connection Executable
> * 1 process 3979825 1 (native) /home/pedro/rocm/gdb/build/gdb/testsuite/outputs/gdb.base/foll-vfork/foll-vfork
> (gdb) n
> [Attaching after Thread 0x7ffff7d6f740 (LWP 3979825) vfork to child process 3980120]
> [New inferior 2 (process 3980120)]
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> 44 in ../sysdeps/unix/sysv/linux/x86_64/vfork.S
> (gdb) info inferiors
> Num Description Connection Executable
> 1 process 3979825 1 (native) /home/pedro/rocm/gdb/build/gdb/testsuite/outputs/gdb.base/foll-vfork/foll-vfork
> is vfork parent of inferior 2
> * 2 process 3980120 1 (native) /home/pedro/rocm/gdb/build/gdb/testsuite/outputs/gdb.base/foll-vfork/foll-vfork
> is vfork child of inferior 1
> (gdb) up
> #1 0x000055555555523f in main (argc=1, argv=0x7fffffffdbe8) at /home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/foll-vfork.c:32
> 32 pid = vfork (); /* VFORK */
> (gdb) watch pid
> Hardware watchpoint 2: pid
> (gdb) info breakpoints
> Num Type Disp Enb Address What
> 2 hw watchpoint keep y pid inf 1, 2
> ^^^^^^^^
>
> so gdb is saying that the watchpoint will trigger in either inferior, because they are
> both sharing the same address space.
>
> I think the manual need to explain this right, otherwise the user might be confused
> about whether watchpoints triggers on all inferiors if you don't use
> "watch ... inferior". Also, this deserves a testcase. I'd think it is best even to
> split watchpoints support to a separate patch, as it's sufficiently different from
> breakpoints.
>
> I noticed that the NEWS entry does not mention watchpoints.
>
> Pedro Alves
next prev parent reply other threads:[~2023-02-03 16:55 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-28 11:25 [PATCH 0/6] Inferior specific breakpoints Andrew Burgess
2022-11-28 11:25 ` [PATCH 1/6] gdb/remote: announce thread exit events for remote targets Andrew Burgess
2022-11-28 11:25 ` [PATCH 2/6] gdb/testsuite: don't try to set non-stop mode on a running target Andrew Burgess
2022-11-28 11:25 ` [PATCH 3/6] gdb: fix display of thread condition for multi-location breakpoints Andrew Burgess
2022-12-23 8:37 ` Aktemur, Tankut Baris
2022-11-28 11:25 ` [PATCH 4/6] gdb: error if 'thread' or 'task' keywords are overused Andrew Burgess
2022-11-28 13:10 ` Eli Zaretskii
2022-11-28 11:25 ` [PATCH 5/6] gdb: add inferior-specific breakpoints and watchpoints Andrew Burgess
2022-11-28 13:18 ` Eli Zaretskii
2022-12-23 10:05 ` Aktemur, Tankut Baris
2023-01-19 19:13 ` Andrew Burgess
2023-01-20 13:12 ` Aktemur, Tankut Baris
2022-11-28 11:25 ` [PATCH 6/6] gdb: convert the 'start' breakpoint to use inferior keyword Andrew Burgess
2022-12-23 10:17 ` Aktemur, Tankut Baris
2022-12-23 10:55 ` [PATCH 0/6] Inferior specific breakpoints Aktemur, Tankut Baris
2023-01-20 9:46 ` [PATCHv2 " Andrew Burgess
2023-01-20 9:46 ` [PATCHv2 1/6] gdb/remote: announce thread exit events for remote targets Andrew Burgess
2023-02-02 17:50 ` Pedro Alves
2023-02-04 15:46 ` Andrew Burgess
2023-01-20 9:46 ` [PATCHv2 2/6] gdb/testsuite: don't try to set non-stop mode on a running target Andrew Burgess
2023-02-04 16:22 ` Andrew Burgess
2023-01-20 9:46 ` [PATCHv2 3/6] gdb: fix display of thread condition for multi-location breakpoints Andrew Burgess
2023-02-02 18:13 ` Pedro Alves
2023-02-06 14:48 ` Andrew Burgess
2023-02-06 17:01 ` Pedro Alves
2023-02-07 14:42 ` Andrew Burgess
2023-01-20 9:46 ` [PATCHv2 4/6] gdb: error if 'thread' or 'task' keywords are overused Andrew Burgess
2023-01-20 13:22 ` Eli Zaretskii
2023-02-02 14:08 ` Andrew Burgess
2023-02-02 14:31 ` Eli Zaretskii
2023-02-02 18:21 ` Pedro Alves
2023-02-03 16:41 ` Andrew Burgess
2023-02-04 5:52 ` Joel Brobecker
2023-02-04 15:40 ` Andrew Burgess
2023-02-06 11:06 ` Andrew Burgess
2023-01-20 9:46 ` [PATCHv2 5/6] gdb: add inferior-specific breakpoints and watchpoints Andrew Burgess
2023-01-20 13:25 ` Eli Zaretskii
2023-02-02 19:17 ` Pedro Alves
2023-02-03 16:55 ` Andrew Burgess [this message]
2023-02-06 17:24 ` Pedro Alves
2023-02-16 12:56 ` Aktemur, Tankut Baris
2023-01-20 9:46 ` [PATCHv2 6/6] gdb: convert the 'start' breakpoint to use inferior keyword Andrew Burgess
2023-02-16 12:59 ` Aktemur, Tankut Baris
2023-03-16 17:03 ` [PATCHv3 0/2] Inferior specific breakpoints Andrew Burgess
2023-03-16 17:03 ` [PATCHv3 1/2] gdb: cleanup around some set_momentary_breakpoint_at_pc calls Andrew Burgess
2023-04-03 14:12 ` Andrew Burgess
2023-03-16 17:03 ` [PATCHv3 2/2] gdb: add inferior-specific breakpoints Andrew Burgess
2023-04-03 14:14 ` [PATCHv4] " Andrew Burgess
2023-05-15 19:15 ` [PATCHv5] " Andrew Burgess
2023-05-30 20:41 ` [PATCHv6] " Andrew Burgess
2023-07-07 10:23 ` [PATCHv7] " Andrew Burgess
2023-08-17 15:53 ` [PUSHEDv8] " Andrew Burgess
2023-08-23 8:06 ` [PUSHED] gdb: add missing notify_breakpoint_modified call Andrew Burgess
2023-08-23 8:19 ` [PUSHED] gdb/testsuite: improve MI support for inferior specific breakpoints Andrew Burgess
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87k00yr8ns.fsf@redhat.com \
--to=aburgess@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=pedro@palves.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).