RFC: Fix "break *EXP thread NUM"

RFC: Fix "break *EXP thread NUM"

[Daniel Jacobowitz]

This issue has come up several times over the years.  I've CC'd a
couple of folks from the last discussions I found in the list
archives.  Here's an example:

(gdb) # OK
(gdb) break main thread 999
Unknown thread 999.
(gdb) # NG
(gdb) break *main thread 999
A syntax error in expression, near `thread 999'.

References:

http://sourceware.org/ml/gdb-patches/2005-04/msg00092.html
http://sourceware.org/ml/gdb-patches/2005-04/msg00116.html
http://sourceware.org/ml/gdb-patches/2006-07/msg00412.html

The problem is with the expression parser, which has no way to detect
the "end" of an expression (a poorly defined concept).  Since "if" can
not be a valid C identifier, and we do not parse C statements (just
expressions), we have a special technique in the lexer to detect that
token and stop.  So both of these work:

(gdb) break main if 999
Breakpoint 1 at 0x44cce0
(gdb) break *main if 999
Note: breakpoint 1 also set at pc 0x44cce0.
Breakpoint 2 at 0x44cce0

In this patch, I have taken advantage of the fact that even if you
have a local variable named "thread", there is no valid C expression
which has a variable name, whitespace, and an integer or floating
point constant.  So "thread [0-9]" can also safely be treated
as an expression terminator.

Does anyone see a problem with this approach?

I updated the manual, and don't think a NEWS entry is required.
Tested on x86_64-linux, with no regressions.

-- 
Daniel Jacobowitz
CodeSourcery

2009-11-23  Daniel Jacobowitz  <dan@codesourcery.com>

	* c-exp.y (yylex): Stop before "thread N".

	* gdb.texinfo (Thread-Specific Breakpoints): Thread specifiers
	are allowed after the breakpoint condition.

	* gdb.base/condbreak.exp: Test combinations of "break *EXP",
	"if", and "thread".  Correct matching in the previous test.

Index: c-exp.y
===================================================================
RCS file: /cvs/src/src/gdb/c-exp.y,v
retrieving revision 1.65
diff -u -p -r1.65 c-exp.y
--- c-exp.y	11 Nov 2009 16:45:46 -0000	1.65
+++ c-exp.y	23 Nov 2009 21:26:06 -0000
@@ -2250,6 +2250,22 @@ yylex (void)
       return 0;
     }
 
+  /* For the same reason (breakpoint conditions), "thread N"
+     terminates the expression.  "thread" could be an identifier, but
+     an identifier is never followed by a number without intervening
+     punctuation.  */
+  if (namelen == 6
+      && strncmp (tokstart, "thread", 6) == 0
+      && (tokstart[6] == ' ' || tokstart[6] == '\t')
+      && ! scanning_macro_expansion ())
+    {
+      char *p = tokstart + 7;
+      while (*p == ' ' || *p == '\t')
+	p++;
+      if (*p >= '0' && *p <= '9')
+	return 0;
+    }
+
   lexptr += namelen;
 
   tryname:
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.642
diff -u -p -r1.642 gdb.texinfo
--- doc/gdb.texinfo	23 Nov 2009 18:44:10 -0000	1.642
+++ doc/gdb.texinfo	23 Nov 2009 21:26:13 -0000
@@ -5214,8 +5214,8 @@ breakpoint, the breakpoint applies to @e
 program.
 
 You can use the @code{thread} qualifier on conditional breakpoints as
-well; in this case, place @samp{thread @var{threadno}} before the
-breakpoint condition, like this:
+well; in this case, place @samp{thread @var{threadno}} before or
+after the breakpoint condition, like this:
 
 @smallexample
 (@value{GDBP}) break frik.c:13 thread 28 if bartab > lim
Index: testsuite/gdb.base/condbreak.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/condbreak.exp,v
retrieving revision 1.13
diff -u -p -r1.13 condbreak.exp
--- testsuite/gdb.base/condbreak.exp	3 Jan 2009 05:58:03 -0000	1.13
+++ testsuite/gdb.base/condbreak.exp	23 Nov 2009 21:26:13 -0000
@@ -207,10 +207,10 @@ gdb_expect {
 setup_xfail hppa2.0w-*-* 11512CLLbs
 send_gdb "continue\n"
 gdb_expect {
-    -re "Continuing\\..*Breakpoint \[0-9\]+, marker2 \\(a=43\\) at .*$srcfile1:($bp_location8|$bp_location9).*($bp_location8|$bp_location9)\[\t \]+.*" {
+    -re "Continuing\\..*Breakpoint \[0-9\]+, marker2 \\(a=43\\) at .*$srcfile1:($bp_location8|$bp_location9).*($bp_location8|$bp_location9)\[\t \]+.*$gdb_prompt $" {
 	pass "run until breakpoint at marker2"
     }
-    -re "Continuing\\..*Breakpoint \[0-9\]+, $hex in marker2 \\(a=43\\) at .*$srcfile1:($bp_location8|$bp_location9).*($bp_location8|$bp_location9)\[\t \]+.*" {
+    -re "Continuing\\..*Breakpoint \[0-9\]+, $hex in marker2 \\(a=43\\) at .*$srcfile1:($bp_location8|$bp_location9).*($bp_location8|$bp_location9)\[\t \]+.*$gdb_prompt $" {
 	xfail "run until breakpoint at marker2"
     }
     -re "$gdb_prompt $" {
@@ -220,3 +220,16 @@ gdb_expect {
 	fail "(timeout) run until breakpoint at marker2"
     }
 }
+
+# Test combinations of conditional and thread-specific breakpoints.
+gdb_test "break main if (1==1) thread 999" \
+    "Unknown thread 999\\."
+gdb_test "break main thread 999 if (1==1)" \
+    "Unknown thread 999\\."
+
+# Verify that both if and thread can be distinguished from a breakpoint
+# address expression.
+gdb_test "break *main if (1==1) thread 999" \
+    "Unknown thread 999\\."
+gdb_test "break *main thread 999 if (1==1)" \
+    "Unknown thread 999\\."

Re: RFC: Fix "break *EXP thread NUM"

[Joel Brobecker]

> 2009-11-23  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* c-exp.y (yylex): Stop before "thread N".

I'm not a C expert, but FWIW, this seems very reasonable to me.

-- 
Joel

Re: RFC: Fix "break *EXP thread NUM"

[Andrew Stubbs]

On 23/11/09 21:27, Daniel Jacobowitz wrote:
> In this patch, I have taken advantage of the fact that even if you
> have a local variable named "thread", there is no valid C expression
> which has a variable name, whitespace, and an integer or floating
> point constant.  So "thread [0-9]" can also safely be treated
> as an expression terminator.
>
> Does anyone see a problem with this approach?

Unfortunately, I can. :(

It's also valid to say, for example:

   (gdb) b main t 999
   Unknown thread 999.

or

   (gdb) b main thread -10
   Unknown thread -10.

or indeed

   (gdb) b main thread +10
   Unknown thread 10.

Hex and octal are also allowed, but since they always start with zero, I 
think you have them covered.

Andrew

Re: RFC: Fix "break *EXP thread NUM"

[Daniel Jacobowitz]

On Tue, Nov 24, 2009 at 10:32:50AM +0000, Andrew Stubbs wrote:
> Unfortunately, I can. :(
> 
> It's also valid to say, for example:
> 
>   (gdb) b main t 999
>   Unknown thread 999.

Neither documented nor tested :-(  Do we want/need this functionality?

> or
> 
>   (gdb) b main thread -10
>   Unknown thread -10.
> 
> or indeed
> 
>   (gdb) b main thread +10
>   Unknown thread 10.

Ditto, and these are much more trouble because thread +10 is a valid C
expression.  Fortunately, thread numbers are always positive.  Is this
useful?

I see that "task" is listed too now and should be added.  And the
other parsers, including Ada, probably need similar work - except
there I don't know how to do it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Fix "break *EXP thread NUM"

[Joel Brobecker]

> I see that "task" is listed too now and should be added.  And the
> other parsers, including Ada, probably need similar work - except
> there I don't know how to do it.

I'm a little tied up at the moment, but I can take care of this.
You shouldn't feel obliged to help in this area.

It's strange, though, I thought that this was working already.
Ha! Just gave it a try, and indeed:

    (gdb) b *break_me'address task 3
    Error in expression, near `'.

Bummer!
-- 
Joel

Re: RFC: Fix "break *EXP thread NUM"

[Daniel Jacobowitz]

On Tue, Nov 24, 2009 at 06:54:22AM -0800, Joel Brobecker wrote:
> I'm a little tied up at the moment, but I can take care of this.

Thanks!

Meanwhile, here's a patch that handles "t" and "task".  I talked to
Andrew about this, and I still think we can get away without "+N" and
"-N"; they're only accepted by an accident of strtol.

-- 
Daniel Jacobowitz
CodeSourcery

2009-11-23  Daniel Jacobowitz  <dan@codesourcery.com>

	* breakpoint.c (find_condition_and_thread): Correct task error message.
	* c-exp.y (yylex): Stop before "thread N", "task N", or abbreviations
	of those.

	* gdb.texinfo (Thread-Specific Breakpoints): Thread specifiers
	are allowed after the breakpoint condition.

	* gdb.base/condbreak.exp: Test combinations of "break *EXP",
	"if", and "thread".  Correct matching in the previous test.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.435
diff -u -p -r1.435 breakpoint.c
--- breakpoint.c	22 Nov 2009 15:38:58 -0000	1.435
+++ breakpoint.c	24 Nov 2009 15:03:10 -0000
@@ -6635,7 +6635,7 @@ find_condition_and_thread (char *tok, CO
 	  if (tok == tmptok)
 	    error (_("Junk after task keyword."));
 	  if (!valid_task_id (*task))
-	    error (_("Unknown task %d\n"), *task);
+	    error (_("Unknown task %d."), *task);
 	}
       else
 	error (_("Junk at end of arguments."));
Index: c-exp.y
===================================================================
RCS file: /cvs/src/src/gdb/c-exp.y,v
retrieving revision 1.65
diff -u -p -r1.65 c-exp.y
--- c-exp.y	11 Nov 2009 16:45:46 -0000	1.65
+++ c-exp.y	24 Nov 2009 15:03:10 -0000
@@ -2250,6 +2250,24 @@ yylex (void)
       return 0;
     }
 
+  /* For the same reason (breakpoint conditions), "thread N"
+     terminates the expression.  "thread" could be an identifier, but
+     an identifier is never followed by a number without intervening
+     punctuation.  "task" is similar.  Handle abbreviations of these,
+     similarly to breakpoint.c:find_condition_and_thread.  */
+  if (namelen >= 1
+      && (strncmp (tokstart, "thread", namelen) == 0
+	  || strncmp (tokstart, "task", namelen) == 0)
+      && (tokstart[namelen] == ' ' || tokstart[namelen] == '\t')
+      && ! scanning_macro_expansion ())
+    {
+      char *p = tokstart + namelen + 1;
+      while (*p == ' ' || *p == '\t')
+	p++;
+      if (*p >= '0' && *p <= '9')
+	return 0;
+    }
+
   lexptr += namelen;
 
   tryname:
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.642
diff -u -p -r1.642 gdb.texinfo
--- doc/gdb.texinfo	23 Nov 2009 18:44:10 -0000	1.642
+++ doc/gdb.texinfo	24 Nov 2009 15:03:15 -0000
@@ -5214,8 +5214,8 @@ breakpoint, the breakpoint applies to @e
 program.
 
 You can use the @code{thread} qualifier on conditional breakpoints as
-well; in this case, place @samp{thread @var{threadno}} before the
-breakpoint condition, like this:
+well; in this case, place @samp{thread @var{threadno}} before or
+after the breakpoint condition, like this:
 
 @smallexample
 (@value{GDBP}) break frik.c:13 thread 28 if bartab > lim
Index: testsuite/gdb.base/condbreak.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/condbreak.exp,v
retrieving revision 1.13
diff -u -p -r1.13 condbreak.exp
--- testsuite/gdb.base/condbreak.exp	3 Jan 2009 05:58:03 -0000	1.13
+++ testsuite/gdb.base/condbreak.exp	24 Nov 2009 15:03:15 -0000
@@ -207,10 +207,10 @@ gdb_expect {
 setup_xfail hppa2.0w-*-* 11512CLLbs
 send_gdb "continue\n"
 gdb_expect {
-    -re "Continuing\\..*Breakpoint \[0-9\]+, marker2 \\(a=43\\) at .*$srcfile1:($bp_location8|$bp_location9).*($bp_location8|$bp_location9)\[\t \]+.*" {
+    -re "Continuing\\..*Breakpoint \[0-9\]+, marker2 \\(a=43\\) at .*$srcfile1:($bp_location8|$bp_location9).*($bp_location8|$bp_location9)\[\t \]+.*$gdb_prompt $" {
 	pass "run until breakpoint at marker2"
     }
-    -re "Continuing\\..*Breakpoint \[0-9\]+, $hex in marker2 \\(a=43\\) at .*$srcfile1:($bp_location8|$bp_location9).*($bp_location8|$bp_location9)\[\t \]+.*" {
+    -re "Continuing\\..*Breakpoint \[0-9\]+, $hex in marker2 \\(a=43\\) at .*$srcfile1:($bp_location8|$bp_location9).*($bp_location8|$bp_location9)\[\t \]+.*$gdb_prompt $" {
 	xfail "run until breakpoint at marker2"
     }
     -re "$gdb_prompt $" {
@@ -220,3 +220,30 @@ gdb_expect {
 	fail "(timeout) run until breakpoint at marker2"
     }
 }
+
+# Test combinations of conditional and thread-specific breakpoints.
+gdb_test "break main if (1==1) thread 999" \
+    "Unknown thread 999\\."
+gdb_test "break main thread 999 if (1==1)" \
+    "Unknown thread 999\\."
+
+# Verify that both if and thread can be distinguished from a breakpoint
+# address expression.
+gdb_test "break *main if (1==1) thread 999" \
+    "Unknown thread 999\\."
+gdb_test "break *main thread 999 if (1==1)" \
+    "Unknown thread 999\\."
+
+# Similarly for task.
+gdb_test "break *main if (1==1) task 999" \
+    "Unknown task 999\\."
+gdb_test "break *main task 999 if (1==1)" \
+    "Unknown task 999\\."
+
+# GDB accepts abbreviations for "thread" and "task".
+gdb_test "break *main if (1==1) t 999" \
+    "Unknown thread 999\\."
+gdb_test "break *main if (1==1) th 999" \
+    "Unknown thread 999\\."
+gdb_test "break *main if (1==1) ta 999" \
+    "Unknown task 999\\."

Re: RFC: Fix "break *EXP thread NUM"

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

>> (gdb) b main thread -10
>> Unknown thread -10.

Daniel> Ditto, and these are much more trouble because thread +10 is a valid C
Daniel> expression.  Fortunately, thread numbers are always positive.  Is this
Daniel> useful?

I think it is fine for us to accept only whatever it is that gdb prints.
Translating gdb's thread id to octal can't be a common thing to do.

Thanks for working on this; it has come up several time.  BTW, it is PR
8704.

Tom

Re: RFC: Fix "break *EXP thread NUM"

[Daniel Jacobowitz]

On Tue, Nov 24, 2009 at 10:05:33AM -0500, Daniel Jacobowitz wrote:
> Meanwhile, here's a patch that handles "t" and "task".  I talked to
> Andrew about this, and I still think we can get away without "+N" and
> "-N"; they're only accepted by an accident of strtol.

I have checked this in.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Fix "break *EXP thread NUM"

[Maciej W. Rozycki]

On Wed, 25 Nov 2009, Daniel Jacobowitz wrote:

> > Meanwhile, here's a patch that handles "t" and "task".  I talked to
> > Andrew about this, and I still think we can get away without "+N" and
> > "-N"; they're only accepted by an accident of strtol.
> 
> I have checked this in.

 While we are at it -- it may be worth thinking about propagating thread 
information associated with breakpoints and watchpoints down to the 
respective backends.  Some processors (like the MIPS 34K multi-threaded 
core) supports thread qualification for execution and data breakpoints in 
hardware and debug stubs would be eager to make use of that for 
performance gain.  Has it been considered before?

  Maciej

Re: RFC: Fix "break *EXP thread NUM"

[Daniel Jacobowitz]

On Tue, Dec 01, 2009 at 07:53:37PM +0000, Maciej W. Rozycki wrote:
>  While we are at it -- it may be worth thinking about propagating thread 
> information associated with breakpoints and watchpoints down to the 
> respective backends.  Some processors (like the MIPS 34K multi-threaded 
> core) supports thread qualification for execution and data breakpoints in 
> hardware and debug stubs would be eager to make use of that for 
> performance gain.  Has it been considered before?

It's been discussed before, but no one has ever sat down to do the
work.

-- 
Daniel Jacobowitz
CodeSourcery

[commit] Fix "break *EXP thread/task NUM" for Ada (was: "Re: RFC: Fix "break *EXP thread NUM"")

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

I finally took the time to fix this for Ada as well.  Initially,
I just fixed it for "task", until I realized that I should have
fixed it for both "thread" and "task". Hence two patches instead
of just one.

2010-01-01  Joel Brobecker  <brobecker@adacore.com>

        Fix break *FUN'address task NUM.
        * ada-lex.l (task): New rule.
        * ada-lang.c (valid_task_id): Make sure the Ada task list has
        been built before using it.

2010-01-01  Joel Brobecker  <brobecker@adacore.com>

        Fix break *FUN'address thread NUM.
        * ada-lex.l (task): Expand rule to also match the thread keyword.

Tested on x86_64-linux.

-- 
Joel

[-- Attachment #2: task-bp-at-addr.diff --]
[-- Type: text/x-diff, Size: 1020 bytes --]

commit 366b0ad634564fcc3ee6242510d91fab52d994db
Author: brobecke <brobecke@f8352e7e-cb20-0410-8ce7-b5d9e71c585c>
Date:   Fri Jan 1 05:18:51 2010 +0000

    Fix break *FUN'address task NUM.
    
            * ada-lex.l (task): New rule.
            * ada-lang.c (valid_task_id): Make sure the Ada task list has
            been built before using it.

diff --git a/gdb/ada-lex.l b/gdb/ada-lex.l
index 745ed64..9505c2d 100644
--- a/gdb/ada-lex.l
+++ b/gdb/ada-lex.l
@@ -160,6 +160,13 @@ if		{
 		  return 0;
 		}
 
+task		{
+		  while (*lexptr != 't' && *lexptr != 'T')
+		    lexptr--;
+		  yyrestart(NULL);
+		  return 0;
+		}
+
 	/* ADA KEYWORDS */
 
 abs		{ return ABS; }
diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index 806548a..af80877 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -198,6 +198,7 @@ get_task_number_from_id (CORE_ADDR task_id)
 int
 valid_task_id (int task_num)
 {
+  ada_build_task_list (0);
   return (task_num > 0
           && task_num <= VEC_length (ada_task_info_s, task_list));
 }

[-- Attachment #3: thread-bp-at-addr.diff --]
[-- Type: text/x-diff, Size: 691 bytes --]

commit 7d7cd745c3f76445eb0f92a76e347de999d938ff
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Fri Jan 1 10:22:11 2010 +0400

    Fix break *FUN'address thread NUM.
    
            * ada-lex.l (task): Expand rule to also match the thread keyword.
            Related to J101-001.

diff --git a/gdb/ada-lex.l b/gdb/ada-lex.l
index 9505c2d..8c47418 100644
--- a/gdb/ada-lex.l
+++ b/gdb/ada-lex.l
@@ -160,7 +160,9 @@ if		{
 		  return 0;
 		}
 
-task		{
+(task|thread)	{
+                  /* This keyword signals the end of the expression and
+                     will be processed separately.  */
 		  while (*lexptr != 't' && *lexptr != 'T')
 		    lexptr--;
 		  yyrestart(NULL);