Re: [PATCHv2 5/6] gdb: add inferior-specific breakpoints and watchpoints

[Pedro Alves <pedro@palves.net>]

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.

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