public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
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


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