RE: [PATCH 5/6] gdb: add inferior-specific breakpoints and watchpoints

[Andrew Burgess <aburgess@redhat.com>]

"Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com> writes:

> On Monday, November 28, 2022 12:26 PM, Andrew Burgess 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
>
> Nit: IMHO, removing "then" makes the sentence sound better.
>
>> 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.
>
> An important difference of an inferior-specific breakpoint wrt using
> conditions that contain the 'thread' keyword or the '$_inferior' variable
> could be that the breakpoints would not be inserted at all on other inferiors.
> For inferiors that have a large number of threads, this could save a
> substantial amount of overhead of stopping, evaluating the condition, and
> resuming.  IMHO, it is worth considering this for inferior-specific breakpoints.
>
> In a downstream debugger, we had included this feature:
> https://github.com/intel/gdb/commit/7d87ac91308cd7a8984ba7b0e333a6689790972d
> (Please see the modifications in 'create_breakpoint').

Neat.  I wasn't aware Intel had already worked on this feature.

I had also thought about not inserting breakpoints into non-matching
inferiors.  In the end I decided to leave that for a follow up patch,
but it's nice to see that Intel have been doing this for a few years now.

>
> With this perspective, I also think that allowing the use of both 'thread'
> and 'inferior' clauses makes sense, because they would have different advantages.

Except, isn't the thread-id passed to a 'thread' condition a global
thread-id?  i.e. "break foo thread 1" isn't thread 1 in every inferior,
it's GDB's global thread 1, which is one thread in one inferior.

So we could (if we implemented it) already limit into which inferiors a
thread specific breakpoint is inserted by just figuring out which
inferior that thread is in.

I think it makes sense, at least initially, to prevent use of 'thread'
and 'inferior' together.  If we decide to relax this restriction later,
then that's no problem.  It's much harder to add more restrictions
later.

>
> More comments are inlined below.
>
> Thanks
> -Baris
>  
>> @@ -3255,6 +3278,32 @@ Thread-specific breakpoint %d deleted - thread %s no longer in the
>> thread list.\
>>      }
>>  }
>> 
>> +/* Called when inferior INF has exited.  Remove per-inferior breakpoints.  */
>> +
>> +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);
>> +
>> +
>
> It seems one of the blank lines is redundant.
>
>> @@ -8430,6 +8500,11 @@ code_breakpoint::code_breakpoint (struct gdbarch *gdbarch_,
>> 
>>    thread = thread_;
>>    task = task_;
>> +  inferior = inferior_;
>> +
>> +  /* A breakpoint can be thread specific, or inferior specific, but not
>> +     both.  This should be checked when the breakpoint condition is parsed.  */
>> +  gdb_assert (!(thread != -1 && inferior != -1));
>
> The previous assertion expressions are easier to read, I think:
>
>   gdb_assert (thread == -1 || inferior == -1);
>
>> @@ -8821,11 +8914,15 @@ find_condition_and_thread (const char *tok, CORE_ADDR pc,
>>        else if (rest)
>>  	{
>>  	  rest->reset (savestring (tok, strlen (tok)));
>> -	  return;
>> +	  break;
>>  	}
>>        else
>>  	error (_("Junk at end of arguments."));
>>      }
>> +
>> +  if (*thread != -1 && *inferior != -1)
>> +    error (_("Invalid use of both 'thread' and 'inferior' in "
>> +	     "breakpoint condition"));
>
> Nit: This error message sounds to me like "the conditions you used
> both in the 'thread' and the 'inferior' clauses are incorrect."  But 
> in fact the problem is, using both clauses at the same time is not
> allowed.  Maybe this would be clearer:
> "Using the 'thread' and 'inferior' conditions together is not allowed."
>
>> @@ -10068,6 +10170,10 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
>>  	  tok++;
>>  	  toklen = end_tok - tok + 1;
>> 
>> +	  if (thread != -1 && inferior != -1)
>> +	    error (_("Invalid use of both 'thread' and 'inferior' in "
>> +		     "watchpoint condition"));
>
> Same comment here.
>
>> diff --git a/gdb/inferior.h b/gdb/inferior.h
>> index 69525a2e053..3b25403d83c 100644
>> --- a/gdb/inferior.h
>> +++ b/gdb/inferior.h
>> @@ -746,4 +746,14 @@ extern void print_selected_inferior (struct ui_out *uiout);
>>  extern void switch_to_inferior_and_push_target
>>    (inferior *new_inf, bool no_connection, inferior *org_inf);
>> 
>> +/* Return true if ID is a valid global inferior number.  */
>> +
>> +inline bool valid_global_inferior_id (int id)
>
> Function name should be at column 0.
>
>> @@ -280,11 +280,69 @@ bppy_set_thread (PyObject *self, PyObject *newvalue, void *closure)
>>        return -1;
>>      }
>> 
>> +  if (self_bp->bp->inferior != -1 && id != -1)
>> +    {
>> +      PyErr_SetString (PyExc_RuntimeError,
>> +		       _("Cannot have both thread and inferior conditions "
>> +			 "on a breakpoint"));
>
> This error message is clearer than the one I commented above and I think
> it would make sense to align the two.
>
>> diff --git a/gdb/testsuite/gdb.multi/inferior-specific-bp.exp
>> b/gdb/testsuite/gdb.multi/inferior-specific-bp.exp
>> new file mode 100644
>> index 00000000000..5d65f19b88c
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.multi/inferior-specific-bp.exp
>> @@ -0,0 +1,183 @@
>> +# Copyright 2022 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# Test inferior-specific breakpoints.
>> +
>> +standard_testfile -1.c -2.c
>> +
>> +if { [use_gdb_stub] } {
>> +    return
>> +}
>> +
>> +set srcfile1 ${srcfile}
>> +set binfile1 ${binfile}-1
>> +set binfile2 ${binfile}-2
>> +
>> +if { [build_executable ${testfile}.exp ${binfile1} "${srcfile1}"] != 0 } {
>> +    return -1
>> +}
>> +
>> +if { [build_executable ${testfile}.exp ${binfile2} "${srcfile2}"] != 0 } {
>> +    return -1
>> +}
>> +
>> +# Start the first inferior.
>> +clean_restart ${binfile1}
>> +if {![runto_main]} {
>
> In the if-statements above, there are spaces around the condition.
>
>> +    return
>> +}
>> +
>> +# Add a second inferior, and start this one too.
>> +gdb_test "add-inferior" "Added inferior 2.*" "add empty inferior 2"
>> +gdb_test "inferior 2" "Switching to inferior 2.*" "switch to inferior 2"
>> +gdb_load $binfile2
>> +if {![runto_main]} {
>> +    return
>> +}
>> +
>> +# Try to create a breakpoint using both the 'inferior' and 'thread' keywords,
>> +# this should fail.  Try with the keywords in both orders just in case the
>> +# parser has a bug.
>> +gdb_test "break foo thread 1.1 inferior 1" \
>> +    "Invalid use of both 'thread' and 'inferior' in breakpoint condition"
>> +gdb_test "break foo inferior 1 thread 1.1" \
>> +    "Invalid use of both 'thread' and 'inferior' in breakpoint condition"
>> +
>> +# While w're here, check that we can't create a watchpoint with both thread
>
> Typo: w're
>
>> +# and inferior keywords.  As above, test with keywords in both orders.
>> +foreach type {watch rwatch awatch} {
>> +    gdb_test "$type global_var thread 1.1 inferior 1" \
>> +	"Invalid use of both 'thread' and 'inferior' in watchpoint condition"
>> +    gdb_test "$type global_var inferior 1 thread 1.1" \
>> +	"Invalid use of both 'thread' and 'inferior' in watchpoint condition"
>> +}
>> +
>> +# Clear out any other breakpoints.
>> +gdb_test "with confirm off -- delete breakpoints"
>
> Why not use "delete_breakpoints"?
>
>> +# Create an inferior specific breakpoint.
>
> Nit: This should be spelled "inferior-specific", as far as I know.
>
>> +gdb_test "break foo inferior 1" \
>> +    "Breakpoint $decimal at $hex: foo\\. \\(3 locations\\)"
>
> Why not use 'gdb_breakpoint'?
>
>> +set saw_header false
>> +set location_count 0
>> +set saw_inf_cond false
>> +gdb_test_multiple "info breakpoints" "" {
>> +    -re "^info breakpoints\r\n" {
>> +	exp_continue
>> +    }
>> +
>> +    -re "^Num\\s+\[^\r\n\]+\r\n" {
>
> Removing "^" at the beginning could help eliminate the first regexp
> above to obtain shorter code.
>
>> +	exp_continue
>> +    }
>> +
>> +    -re "^$decimal\\s+breakpoint\\s+keep\\s+y\\s+<MULTIPLE>\\s*\r\n" {
>> +	set saw_header true
>> +	exp_continue
>> +    }
>> +
>> +    -re "^\\s+stop only in inferior 1\r\n" {
>> +	set saw_inf_cond true
>> +	exp_continue
>> +    }
>> +
>> +    -re "^$decimal\\.\[123\]\\s+y\\s+ $hex in foo at \[^\r\n\]+ inf \[12\] inferior 1\r\n"
>> {
>> +	incr location_count
>> +	exp_continue
>> +    }
>> +
>> +    -re "^$gdb_prompt $" {
>> +	with_test_prefix $gdb_test_name {
>> +	    gdb_assert { $saw_header }
>> +	    gdb_assert { $location_count == 3 }
>> +	    gdb_assert { $saw_inf_cond }
>> +	}
>> +    }
>> +}
>> +
>> +# Create a multi-inferior breakpoint to stop at.
>> +gdb_test "break stop_breakpt"
>> +
>> +# Now resume inferior 2, this should reach 'stop_breakpt'.
>> +gdb_test "continue" "hit Breakpoint $decimal\.$decimal, stop_breakpt \\(\\) .*" \
>> +    "continue in inferior 2"
>> +
>> +# Switch to inferior 1, and try there.
>> +gdb_test "inferior 1" ".*" \
>> +    "select inferior 1 to check the inferior-specific b/p works"
>> +gdb_test "continue " "hit Breakpoint $decimal\.$decimal, foo \\(\\) .*" \
>> +    "first continue in inferior 1"
>
> Maybe we can check here explicitly, for extra confidence, that the hit
> came actually from Thread 1.*.
>
>> +
>> +# Now back to inferior 2, let the inferior exit, the inferior-specific
>> +# breakpoint should not be deleted.
>> +gdb_test "inferior 2" ".*" \
>> +    "switch back to allow inferior 2 to exit"
>> +gdb_test "continue" "Inferior 2 \[^\r\n\]+ exited normally.*" \
>> +    "allow inferior 2 to exit"
>> +
>> +gdb_test "inferior 1" ".*" \
>> +    "select inferior 1 to check inferior-specific b/p still works"
>> +gdb_test "continue " "hit Breakpoint $decimal\.$decimal, foo \\(\\) .*" \
>> +    "second continue in inferior 1"
>> +gdb_test "continue " "hit Breakpoint $decimal\.$decimal, stop_breakpt \\(\\) .*" \
>> +    "third continue in inferior 1"
>> +
>> +# Now allow inferior 1 to exit, the inferior specific breakpoint should be
>> +# deleted.
>> +gdb_test "continue" \
>> +    [multi_line \
>> +	 "\\\[Inferior 1 \[^\r\n\]+ exited normally\\\]" \
>> +	 "Inferior-specific breakpoint $decimal deleted - inferior 1 has exited\\."]
>> +
>> +set saw_header false
>> +set location_count 0
>> +set saw_inf_cond false
>> +gdb_test_multiple "info breakpoints" "info breakpoint after inferior 1 exited" {
>> +    -re "^info breakpoints\r\n" {
>> +	exp_continue
>> +    }
>> +
>> +    -re "^Num\\s+\[^\r\n\]+\r\n" {
>> +	exp_continue
>> +    }
>> +
>> +    -re "^$decimal\\s+breakpoint\\s+keep\\s+y\\s+<MULTIPLE>\\s*\r\n" {
>> +	set saw_header true
>> +	exp_continue
>> +    }
>> +
>> +    -re "^\\s+stop only in inferior 1\r\n" {
>> +	# This should not happen, this breakpoint should have been deleted.
>> +	set saw_inf_cond true
>> +	exp_continue
>> +    }
>> +
>> +    -re "^\\s+breakpoint already hit 2 times\r\n" {
>> +	exp_continue
>> +    }
>> +
>> +    -re "^$decimal\\.\[12\]\\s+y\\s+ $hex in stop_breakpt at \[^\r\n\]+ inf \[12\]\r\n" {
>> +	incr location_count
>> +	exp_continue
>> +    }
>> +
>> +    -re "^$gdb_prompt $" {
>> +	with_test_prefix $gdb_test_name {
>> +	    gdb_assert { $saw_header }
>> +	    gdb_assert { $location_count == 2 }
>> +	    gdb_assert { !$saw_inf_cond }
>> +	}
>> +    }
>> +}
>> diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-
>> breakpoint.exp
>> index c3215b13d5c..8906dea6655 100644
>> --- a/gdb/testsuite/gdb.python/py-breakpoint.exp
>> +++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
>> @@ -112,6 +112,8 @@ proc_with_prefix test_bkpt_basic { } {
>>  	"Get Breakpoint List" 0
>>      gdb_test "python print (blist\[1\].thread)" \
>>  	"None" "Check breakpoint thread"
>> +    gdb_test "python print (blist\[1\].inferior)" \
>> +	"None" "Check breakpoint inferior"
>>      gdb_test "python print (blist\[1\].type == gdb.BP_BREAKPOINT)" \
>>  	"True" "Check breakpoint type"
>>      gdb_test "python print (blist\[0\].number)" \
>> @@ -214,6 +216,46 @@ proc_with_prefix test_bkpt_cond_and_cmds { } {
>>  	"check number of lines in commands"
>>  }
>> 
>> +# Test breakpoint thread and inferior attributes.
>> +proc_with_prefix test_bkpt_thread_and_inferior { } {
>> +    global srcfile testfile hex decimal
>> +
>> +    # Start with a fresh gdb.
>> +    clean_restart ${testfile}
>> +
>> +    if ![runto_main] then {
>
> I thought you had removed the uses of 'then'. :)
>
>> +	return 0
>
> As I wrote previously, the testsuite does not look consistent
> about what to return here.  But "-1" makes more sense to me, because
> not being able to run to main sounds like a major problem.

The difference in the other patch where you pointed this out is that the
return as at the top level of the script (I fixed that other case as per
your suggestion).

In this case the return is from a function.  We don't actually check the
return value, but I'd rather leave this return consistent with all the
other similar return statements, in all the other functions, in this
test script.

I've made all the other updates you suggested, these will be included in
a V2 series shortly.

Thanks,
Andrew

>
>> +    }
>> +
>> +    with_test_prefix "thread" {
>> +	delete_breakpoints
>> +	gdb_test "break multiply thread 1"
>> +	gdb_test "python bp = gdb.breakpoints ()\[0\]"
>> +	gdb_test "python print(bp.thread)" "1"
>> +	gdb_test "python print(bp.inferior)" "None"
>> +	gdb_test "python bp.inferior = 1" \
>> +	    "RuntimeError: Cannot have both thread and inferior conditions on a breakpoint.*"
>> +	gdb_test_no_output "python bp.thread = None"
>> +	gdb_test_no_output "python bp.inferior = 1" \
>> +	    "set the inferior now the thread has been cleared"
>> +	gdb_test "info breakpoints" "stop only in inferior 1\r\n.*"
>> +    }
>> +
>> +    with_test_prefix "inferior" {
>> +	delete_breakpoints
>> +	gdb_test "break multiply inferior 1"
>> +	gdb_test "python bp = gdb.breakpoints ()\[0\]"
>> +	gdb_test "python print(bp.thread)" "None"
>> +	gdb_test "python print(bp.inferior)" "1"
>> +	gdb_test "python bp.thread = 1" \
>> +	    "RuntimeError: Cannot have both thread and inferior conditions on a breakpoint.*"
>> +	gdb_test_no_output "python bp.inferior = None"
>> +	gdb_test_no_output "python bp.thread = 1" \
>> +	    "set the thread now the inferior has been cleared"
>> +	gdb_test "info breakpoints" "stop only in thread 1\r\n.*"
>> +    }
>> +}
>> +
>>  proc_with_prefix test_bkpt_invisible { } {
>>      global srcfile testfile hex decimal
>> 
>> @@ -849,6 +891,7 @@ proc_with_prefix test_bkpt_auto_disable { } {
>>  test_bkpt_basic
>>  test_bkpt_deletion
>>  test_bkpt_cond_and_cmds
>> +test_bkpt_thread_and_inferior
>>  test_bkpt_invisible
>>  test_hardware_breakpoints
>>  test_catchpoints
>> diff --git a/gdb/testsuite/lib/completion-support.exp b/gdb/testsuite/lib/completion-
>> support.exp
>> index 0c03d0f035e..135649ed4d9 100644
>> --- a/gdb/testsuite/lib/completion-support.exp
>> +++ b/gdb/testsuite/lib/completion-support.exp
>> @@ -27,7 +27,7 @@ namespace eval completion {
>>      # List of all quote chars, including no-quote at all.
>>      variable maybe_quoted_list {"" "'" "\""}
>> 
>> -    variable keyword_list {"-force-condition" "if" "task" "thread"}
>> +    variable keyword_list {"-force-condition" "if" "inferior" "task" "thread"}
>> 
>>      variable explicit_opts_list \
>>  	{"-function" "-label" "-line" "-qualified" "-source"}
>> --
>> 2.25.4
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928