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

  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.

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

> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Fri, 20 Jan 2023 09:46:27 +0000
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
> --- 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.

This part is OK, but I wonder whether we should provide an example of
such incorrect usage which will now be flagged, because I'm not sure
everyone will understand what "multiple uses" means in this context.

Thanks.
  

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Andrew Burgess <aburgess@redhat.com>
>> Date: Fri, 20 Jan 2023 09:46:27 +0000
>> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
>> 
>> --- 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.
>
> This part is OK, but I wonder whether we should provide an example of
> such incorrect usage which will now be flagged, because I'm not sure
> everyone will understand what "multiple uses" means in this context.

I've now written:

  * 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. e.g.:
      break foo thread 1 thread 2
    will now give an error rather than using 'thread 2'.
  
  * 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. e.g.:
      watch my_var task 1 task 2
    will now give an error rather than using 'task 2'.  The 'thread'
    keyword already gave an error when used multiple times with the
    watch command, this remains unchanged.

How's that?

Thanks,
Andrew
  

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Thu, 02 Feb 2023 14:08:08 +0000
> 
>   * 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. e.g.:
>       break foo thread 1 thread 2
>     will now give an error rather than using 'thread 2'.
>   
>   * 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. e.g.:
>       watch my_var task 1 task 2
>     will now give an error rather than using 'task 2'.  The 'thread'
>     keyword already gave an error when used multiple times with the
>     watch command, this remains unchanged.
> 
> How's that?

That's fine, but please capitalize "E.g." and leave 2 spaces before
it.
  

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?

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"
>
  

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"
>>
  

Hi Andrew,

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

Pedro raises a good question!

At the language level, I don't think there is anything that says
that Ada tasks can't be implemented independently of threads.
In fact, on baremetal targets, that's what we have to do, since
we don't have an underlying thread layer.

With that said, for GDB itself, the implementation of the ada tasking
layer is done on top of the GDB's thread layer. In simple terms,
what the ada-task.c module does is simply translating Ada task IDs
into thread ptid-s. So, when we say "switch to task X", or "break on
task X", internally, it essentially translates "task X" into "thread Y".

Based on this implementation, it is always suspicious if someone
uses both a thread ID and a task ID in the same command (or I would
view those as "additive", but that's not the direction taken by
your patch series). I would therefore indeed raise an error if
both are used in the same command.

One side note about the baremetal platforms, since I mentioned them:
While the platform itself doesn't provide threads [1], you might ask
yourself how the Ada tasking layer might be implemented. This is where
the ravenscar-thread layer kicks in. It actually relies on the Ada
runtime to determine the list of tasks that exists, and constructs
a list of threads from that data, thus providing the threads that
the ada-task module needs to function.

[1] On baremetal target, we've seen multicore system report each CPU
    as a thread. That's what QEMU does, for instance.

I hope this helps!
  

Joel Brobecker via Gdb-patches <gdb-patches@sourceware.org> writes:

> Hi Andrew,
>
>> > 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.
>
> Pedro raises a good question!
>
> At the language level, I don't think there is anything that says
> that Ada tasks can't be implemented independently of threads.
> In fact, on baremetal targets, that's what we have to do, since
> we don't have an underlying thread layer.
>
> With that said, for GDB itself, the implementation of the ada tasking
> layer is done on top of the GDB's thread layer. In simple terms,
> what the ada-task.c module does is simply translating Ada task IDs
> into thread ptid-s. So, when we say "switch to task X", or "break on
> task X", internally, it essentially translates "task X" into "thread Y".
>
> Based on this implementation, it is always suspicious if someone
> uses both a thread ID and a task ID in the same command (or I would
> view those as "additive", but that's not the direction taken by
> your patch series). I would therefore indeed raise an error if
> both are used in the same command.
>
> One side note about the baremetal platforms, since I mentioned them:
> While the platform itself doesn't provide threads [1], you might ask
> yourself how the Ada tasking layer might be implemented. This is where
> the ravenscar-thread layer kicks in. It actually relies on the Ada
> runtime to determine the list of tasks that exists, and constructs
> a list of threads from that data, thus providing the threads that
> the ada-task module needs to function.
>
> [1] On baremetal target, we've seen multicore system report each CPU
>     as a thread. That's what QEMU does, for instance.
>
> I hope this helps!

Thanks for the detailed write up.  I'm going to keep the original patch
as is for now, but will follow up with an additional patch that will
give an error if a user tries to use 'task' and 'thread' in the same
command.

Thanks,
Andrew
  

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?

Thanks for the review.  I've made the fix Eli suggested, and pushed this
patch.

Given Joel's feedback I'll take a look at a follow up patch the prevents
thread and task 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"
>>