Re: [PATCHv2 4/6] gdb: error if 'thread' or 'task' keywords are overused

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> On 2023-01-20 9:46 a.m., Andrew Burgess via Gdb-patches wrote:
>> When creating a breakpoint or watchpoint, the 'thread' and 'task'
>> keywords can be used to create a thread or task specific breakpoint or
>> watchpoint.
>> 
>> Currently, a thread or task specific breakpoint can only apply for a
>> single thread or task, if multiple threads or tasks are specified when
>> creating the breakpoint (or watchpoint), then the last specified id
>> will be used.
>> 
>> The exception to the above is that when the 'thread' keyword is used
>> during the creation of a watchpoint, GDB will give an error if
>> 'thread' is given more than once.
>> 
>> In this commit I propose making this behaviour consistent, if the
>> 'thread' or 'task' keywords are used more than once when creating
>> either a breakpoint or watchpoint, then GDB will give an error.
>
> The patch looks fine, but does it make sense to allow both thread and task
> at the same time?

I don't know enough about Ada tasks to comment here.  If someone who
knows can say categorically that threads and tasks can't coexist than
I'd be happy to add a patch to prevent them being used together.

Thanks,
Andrew


>
> For instance, with gdb.ada/tasks.exp :
>
> (gdb) b foo thread 1 task 2
> Breakpoint 1 at 0x555555557bd6: file /home/pedro/gdb/rocm/gdb/src/gdb/testsuite/gdb.ada/tasks/foo.adb, line 16.
> (gdb) info breakpoints 
> Num     Type           Disp Enb Address            What
> 1       breakpoint     keep y   0x0000555555557bd6 in foo at /home/pedro/gdb/rocm/gdb/src/gdb/testsuite/gdb.ada/tasks/foo.adb:16 thread 1
>         stop only in thread 1
>
> Pedro Alves
>
>> 
>> I haven't updated the manual, we don't explicitly say that these
>> keywords can be repeated, and (to me), given the keyword takes a
>> single id, I don't think it makes much sense to repeat the keyword.
>> As such, I see this more as adding a missing error to GDB, rather than
>> making some big change.  However, I have added an entry to the NEWS
>> file as I guess it is possible that some people might hit this new
>> error with an existing (I claim, badly written) GDB script.
>> 
>> I've added some new tests to check for the new error.
>> 
>> Just one test needed updating, gdb.linespec/keywords.exp, this test
>> did use the 'thread' keyword twice, and expected the breakpoint to be
>> created.  Looking at what this test was for though, it was checking
>> the use of '-force-condition', and I don't think that being able to
>> repeat 'thread' was actually a critical part of this test.
>> 
>> As such, I've updated this test to expect the error when 'thread' is
>> repeated.
>> ---
>>  gdb/NEWS                                         |  9 +++++++++
>>  gdb/breakpoint.c                                 |  9 +++++++++
>>  gdb/testsuite/gdb.ada/tasks.exp                  |  4 ++++
>>  gdb/testsuite/gdb.linespec/keywords.exp          | 10 ++++++++--
>>  gdb/testsuite/gdb.threads/thread-specific-bp.exp |  4 ++++
>>  gdb/testsuite/gdb.threads/watchthreads2.exp      |  3 +++
>>  6 files changed, 37 insertions(+), 2 deletions(-)
>> 
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index c0aac212e30..fb49f62f7e6 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -9,6 +9,15 @@
>>    This support requires that GDB be built with Python scripting
>>    enabled.
>>  
>> +* For the break command, multiple uses of the 'thread' or 'task'
>> +  keywords will now give an error instead of just using the thread or
>> +  task id from the last instance of the keyword.
>> +
>> +* For the watch command, multiple uses of the 'task' keyword will now
>> +  give an error instead of just using the task id from the last
>> +  instance of the keyword.  The 'thread' keyword already gave an error
>> +  when used multiple times with the watch command, this remains unchanged.
>> +
>>  * New commands
>>  
>>  maintenance print record-instruction [ N ]
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index b2cd89511fb..1400fd49642 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -8801,6 +8801,9 @@ find_condition_and_thread (const char *tok, CORE_ADDR pc,
>>  	  const char *tmptok;
>>  	  struct thread_info *thr;
>>  
>> +	  if (*thread != -1)
>> +	    error(_("You can specify only one thread."));
>> +
>>  	  tok = end_tok + 1;
>>  	  thr = parse_thread_id (tok, &tmptok);
>>  	  if (tok == tmptok)
>> @@ -8812,6 +8815,9 @@ find_condition_and_thread (const char *tok, CORE_ADDR pc,
>>  	{
>>  	  char *tmptok;
>>  
>> +	  if (*task != 0)
>> +	    error(_("You can specify only one task."));
>> +
>>  	  tok = end_tok + 1;
>>  	  *task = strtol (tok, &tmptok, 0);
>>  	  if (tok == tmptok)
>> @@ -10094,6 +10100,9 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
>>  	    {
>>  	      char *tmp;
>>  
>> +	      if (task != 0)
>> +		error(_("You can specify only one task."));
>> +
>>  	      task = strtol (value_start, &tmp, 0);
>>  	      if (tmp == value_start)
>>  		error (_("Junk after task keyword."));
>> diff --git a/gdb/testsuite/gdb.ada/tasks.exp b/gdb/testsuite/gdb.ada/tasks.exp
>> index 23bf3937a20..4441d92719c 100644
>> --- a/gdb/testsuite/gdb.ada/tasks.exp
>> +++ b/gdb/testsuite/gdb.ada/tasks.exp
>> @@ -39,6 +39,10 @@ gdb_test "info tasks" \
>>                 "\r\n"] \
>>           "info tasks before inserting breakpoint"
>>  
>> +# Check that multiple uses of the 'task' keyword will give an error.
>> +gdb_test "break break_me task 1 task 3" "You can specify only one task\\."
>> +gdb_test "watch j task 1 task 3" "You can specify only one task\\."
>> +
>>  # Insert a breakpoint that should stop only if task 1 stops.  Since
>>  # task 1 never calls break_me, this shouldn't actually ever trigger.
>>  # The fact that this breakpoint is created _before_ the next one
>> diff --git a/gdb/testsuite/gdb.linespec/keywords.exp b/gdb/testsuite/gdb.linespec/keywords.exp
>> index bff64249542..dc66e32237c 100644
>> --- a/gdb/testsuite/gdb.linespec/keywords.exp
>> +++ b/gdb/testsuite/gdb.linespec/keywords.exp
>> @@ -80,8 +80,14 @@ foreach prefix {"" "thread 1 "} {
>>      foreach suffix {"" " " " thread 1"} {
>>  	foreach cond {"" " if 1"} {
>>  	    with_test_prefix "prefix: '$prefix', suffix: '$suffix', cond: '$cond'" {
>> -		gdb_breakpoint "main ${prefix}-force-condition${suffix}${cond}"\
>> -		    "message"
>> +
>> +		if { [regexp thread $prefix] && [regexp thread $suffix] } {
>> +		    gdb_test "break main ${prefix}-force-condition${suffix}${cond}" \
>> +			"You can specify only one thread\\."
>> +		} else {
>> +		    gdb_breakpoint "main ${prefix}-force-condition${suffix}${cond}"\
>> +			"message"
>> +		}
>>  	    }
>>  	}
>>      }
>> diff --git a/gdb/testsuite/gdb.threads/thread-specific-bp.exp b/gdb/testsuite/gdb.threads/thread-specific-bp.exp
>> index d33b4f47258..008ae5ed05e 100644
>> --- a/gdb/testsuite/gdb.threads/thread-specific-bp.exp
>> +++ b/gdb/testsuite/gdb.threads/thread-specific-bp.exp
>> @@ -63,6 +63,10 @@ proc check_thread_specific_breakpoint {non_stop} {
>>  	return -1
>>      }
>>  
>> +    # Check that multiple uses of 'thread' keyword give an error.
>> +    gdb_test "break main thread $start_thre thread $main_thre" \
>> +	"You can specify only one thread\\."
>> +
>>      # Set a thread-specific breakpoint at "main".  This can't ever
>>      # be hit, but that's OK.
>>      gdb_breakpoint "main thread $start_thre"
>> diff --git a/gdb/testsuite/gdb.threads/watchthreads2.exp b/gdb/testsuite/gdb.threads/watchthreads2.exp
>> index 09858aee486..a1398d668a4 100644
>> --- a/gdb/testsuite/gdb.threads/watchthreads2.exp
>> +++ b/gdb/testsuite/gdb.threads/watchthreads2.exp
>> @@ -71,6 +71,9 @@ if { $nr_started == $NR_THREADS } {
>>      return -1
>>  }
>>  
>> +# Check that multiple uses of the 'thread' keyword will give an error.
>> +gdb_test "watch x thread 1 thread 2" "You can specify only one thread\\."
>> +
>>  # Watch X, it will be modified by all threads.
>>  # We want this watchpoint to be set *after* all threads are running.
>>  gdb_test "watch x" "Hardware watchpoint 3: x"
>>