Support for "break *ADDRESS thread THREADNO"

Support for "break *ADDRESS thread THREADNO"

[Maciej W. Rozycki]

Hello,

 The current version of gdb does not accept the "thread THREADNO" clause 
for breakpoints at an address.  I'm not sure if that's a bug or feature as 
the info pages carefully avoid documenting what should happen in this 
case, referring to source lines instead.

 Anyway I've found the inability to set up such breakpoints an obstacle, 
so I've implemented the missing bits for it to work.  Here's the result.  
I've run-time tested the C language bit only (i.e. c-exp.y), but the other 
.y changes are essentially the same, so they should work as well.  
Unfortunately I haven't found a way of testing the Ada part which is 
significantly different; I hope it's OK.

2005-04-12  Maciej W. Rozycki  <macro@mips.com>

	* ada-lex.l: Support the "thread THREADNO" clause with breakpoints
	at an address.
	* c-exp.y (yylex): Likewise.
	* f-exp.y (yylex): Likewise.
	* jv-exp.y (yylex): Likewise.
	* m2-exp.y (yylex): Likewise.
	* objc-exp.y (yylex): Likewise.
	* p-exp.y (yylex): Likewise.

 This has been verified for the HEAD version with the test suite for the 
i386-linux-gnu system natively with no regressions.  Please consider.

  Maciej

gdb-6.2.1-20050412-break-addr-thread-0.patch
Index: src/gdb/ada-lex.l
===================================================================
RCS file: /cvsroot/gcc/src-cvs/src/gdb/ada-lex.l,v
retrieving revision 1.2.1000.2
diff -u -p -r1.2.1000.2 ada-lex.l
--- src/gdb/ada-lex.l	5 Aug 2004 14:41:08 -0000	1.2.1000.2
+++ src/gdb/ada-lex.l	12 Apr 2005 14:25:15 -0000
@@ -191,8 +191,9 @@ static int find_dot_all (const char *);
 		   tempbuf_len += yyleng-4;
 		}
 
-if		{
-		  while (*lexptr != 'i' && *lexptr != 'I')
+if|thread	{
+		  while (*lexptr != 'i' && *lexptr != 'I'
+			 && *lexptr != 't' && *lexptr != 'T')
 		    lexptr -= 1;
 		  yyrestart(NULL);
 		  return 0;
Index: src/gdb/c-exp.y
===================================================================
RCS file: /cvsroot/gcc/src-cvs/src/gdb/c-exp.y,v
retrieving revision 1.23.1000.5
diff -u -p -r1.23.1000.5 c-exp.y
--- src/gdb/c-exp.y	26 Aug 2004 16:38:01 -0000	1.23.1000.5
+++ src/gdb/c-exp.y	12 Apr 2005 14:25:15 -0000
@@ -1620,12 +1620,11 @@ yylex ()
       c = tokstart[++namelen];
     }
 
-  /* The token "if" terminates the expression and is NOT removed from
-     the input stream.  It doesn't count if it appears in the
-     expansion of a macro.  */
-  if (namelen == 2
-      && tokstart[0] == 'i'
-      && tokstart[1] == 'f'
+  /* The tokens "if" and "thread" terminate the expression and are NOT
+     removed from the input stream.  It doesn't count if it appears in
+     the expansion of a macro.  */
+  if (((namelen == 2 && tokstart[0] == 'i' && tokstart[1] == 'f')
+       || (namelen == 6 && strncmp (tokstart, "thread", 6) == 0))
       && ! scanning_macro_expansion ())
     {
       return 0;
Index: src/gdb/f-exp.y
===================================================================
RCS file: /cvsroot/gcc/src-cvs/src/gdb/f-exp.y,v
retrieving revision 1.13.1000.2
diff -u -p -r1.13.1000.2 f-exp.y
--- src/gdb/f-exp.y	5 Aug 2004 14:41:15 -0000	1.13.1000.2
+++ src/gdb/f-exp.y	12 Apr 2005 14:25:15 -0000
@@ -1103,10 +1103,11 @@ yylex ()
 	|| (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')); 
        c = tokstart[++namelen]);
   
-  /* The token "if" terminates the expression and is NOT 
-     removed from the input stream.  */
-  
-  if (namelen == 2 && tokstart[0] == 'i' && tokstart[1] == 'f')
+  /* The tokens "if" and "thread" terminate the expression and are NOT
+     removed from the input stream.  It doesn't count if it appears in
+     the expansion of a macro.  */
+  if ((namelen == 2 && tokstart[0] == 'i' && tokstart[1] == 'f')
+      || (namelen == 6 && strncmp (tokstart, "thread", 6) == 0))
     return 0;
   
   lexptr += namelen;
Index: src/gdb/jv-exp.y
===================================================================
RCS file: /cvsroot/gcc/src-cvs/src/gdb/jv-exp.y,v
retrieving revision 1.16.1000.1
diff -u -p -r1.16.1000.1 jv-exp.y
--- src/gdb/jv-exp.y	10 Dec 2003 19:29:42 -0000	1.16.1000.1
+++ src/gdb/jv-exp.y	12 Apr 2005 14:25:15 -0000
@@ -1118,9 +1118,11 @@ yylex ()
        c = tokstart[++namelen];
      }
 
-  /* The token "if" terminates the expression and is NOT 
-     removed from the input stream.  */
-  if (namelen == 2 && tokstart[0] == 'i' && tokstart[1] == 'f')
+  /* The tokens "if" and "thread" terminate the expression and are NOT
+     removed from the input stream.  It doesn't count if it appears in
+     the expansion of a macro.  */
+  if ((namelen == 2 && tokstart[0] == 'i' && tokstart[1] == 'f')
+      || (namelen == 6 && strncmp (tokstart, "thread", 6) == 0))
     {
       return 0;
     }
Index: src/gdb/m2-exp.y
===================================================================
RCS file: /cvsroot/gcc/src-cvs/src/gdb/m2-exp.y,v
retrieving revision 1.11.1000.1
diff -u -p -r1.11.1000.1 m2-exp.y
--- src/gdb/m2-exp.y	10 Dec 2003 19:29:42 -0000	1.11.1000.1
+++ src/gdb/m2-exp.y	12 Apr 2005 14:25:15 -0000
@@ -981,9 +981,11 @@ yylex ()
        c = tokstart[++namelen])
     ;
 
-  /* The token "if" terminates the expression and is NOT
-     removed from the input stream.  */
-  if (namelen == 2 && tokstart[0] == 'i' && tokstart[1] == 'f')
+  /* The tokens "if" and "thread" terminate the expression and are NOT
+     removed from the input stream.  It doesn't count if it appears in
+     the expansion of a macro.  */
+  if ((namelen == 2 && tokstart[0] == 'i' && tokstart[1] == 'f')
+      || (namelen == 6 && strncmp (tokstart, "thread", 6) == 0))
     {
       return 0;
     }
Index: src/gdb/objc-exp.y
===================================================================
RCS file: /cvsroot/gcc/src-cvs/src/gdb/objc-exp.y,v
retrieving revision 1.14.1000.2
diff -u -p -r1.14.1000.2 objc-exp.y
--- src/gdb/objc-exp.y	5 Aug 2004 14:41:19 -0000	1.14.1000.2
+++ src/gdb/objc-exp.y	12 Apr 2005 14:25:15 -0000
@@ -1574,9 +1574,11 @@ yylex ()
        c = tokstart[++namelen];
      }
 
-  /* The token "if" terminates the expression and is NOT 
-     removed from the input stream.  */
-  if (namelen == 2 && tokstart[0] == 'i' && tokstart[1] == 'f')
+  /* The tokens "if" and "thread" terminate the expression and are NOT
+     removed from the input stream.  It doesn't count if it appears in
+     the expansion of a macro.  */
+  if ((namelen == 2 && tokstart[0] == 'i' && tokstart[1] == 'f')
+      || (namelen == 6 && strncmp (tokstart, "thread", 6) == 0))
     {
       return 0;
     }
Index: src/gdb/p-exp.y
===================================================================
RCS file: /cvsroot/gcc/src-cvs/src/gdb/p-exp.y,v
retrieving revision 1.25.1000.2
diff -u -p -r1.25.1000.2 p-exp.y
--- src/gdb/p-exp.y	5 Aug 2004 14:41:19 -0000	1.25.1000.2
+++ src/gdb/p-exp.y	12 Apr 2005 14:25:15 -0000
@@ -1354,9 +1354,11 @@ yylex ()
 
   uptokstart = uptok(tokstart,namelen);
 
-  /* The token "if" terminates the expression and is NOT
-     removed from the input stream.  */
-  if (namelen == 2 && uptokstart[0] == 'I' && uptokstart[1] == 'F')
+  /* The tokens "if" and "thread" terminate the expression and are NOT
+     removed from the input stream.  It doesn't count if it appears in
+     the expansion of a macro.  */
+  if ((namelen == 2 && uptokstart[0] == 'I' && uptokstart[1] == 'F')
+      || (namelen == 6 && strncmp (uptokstart, "THREAD", 6) == 0))
     {
       return 0;
     }

Re: Support for "break *ADDRESS thread THREADNO"

[Daniel Jacobowitz]

On Tue, Apr 12, 2005 at 04:53:55PM +0100, Maciej W. Rozycki wrote:
> Hello,
> 
>  The current version of gdb does not accept the "thread THREADNO" clause 
> for breakpoints at an address.  I'm not sure if that's a bug or feature as 
> the info pages carefully avoid documenting what should happen in this 
> case, referring to source lines instead.
> 
>  Anyway I've found the inability to set up such breakpoints an obstacle, 
> so I've implemented the missing bits for it to work.  Here's the result.  
> I've run-time tested the C language bit only (i.e. c-exp.y), but the other 
> .y changes are essentially the same, so they should work as well.  
> Unfortunately I haven't found a way of testing the Ada part which is 
> significantly different; I hope it's OK.
> 
> 2005-04-12  Maciej W. Rozycki  <macro@mips.com>
> 
> 	* ada-lex.l: Support the "thread THREADNO" clause with breakpoints
> 	at an address.
> 	* c-exp.y (yylex): Likewise.
> 	* f-exp.y (yylex): Likewise.
> 	* jv-exp.y (yylex): Likewise.
> 	* m2-exp.y (yylex): Likewise.
> 	* objc-exp.y (yylex): Likewise.
> 	* p-exp.y (yylex): Likewise.
> 
>  This has been verified for the HEAD version with the test suite for the 
> i386-linux-gnu system natively with no regressions.  Please consider.

No, sorry.  Consider:

(gdb) p *thread
No symbol table is loaded.  Use the "file" command.

After your patch:

(gdb) b *thread
A parse error in expression, near `thread'.

We can get away with this for 'if', because 'if' is a keyword in just
about every language GDB supports, and an unlikely name for a variable.
That's not going to fly for 'thread', however.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: Support for "break *ADDRESS thread THREADNO"

[Maciej W. Rozycki]

On Tue, 12 Apr 2005, Daniel Jacobowitz wrote:

> No, sorry.  Consider:
> 
> (gdb) p *thread
> No symbol table is loaded.  Use the "file" command.
> 
> After your patch:
> 
> (gdb) b *thread
> A parse error in expression, near `thread'.
> 
> We can get away with this for 'if', because 'if' is a keyword in just
> about every language GDB supports, and an unlikely name for a variable.
> That's not going to fly for 'thread', however.

 OK, then I'll file a bug report instead.  Perhaps I'll be able to have a 
look at it again, but not at the moment.  Thanks for looking into it.

  Maciej

Re: Support for "break *ADDRESS thread THREADNO"

[Eli Zaretskii]

> Date: Tue, 12 Apr 2005 14:13:34 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sources.redhat.com
> 
> > 2005-04-12  Maciej W. Rozycki  <macro@mips.com>
> > 
> > 	* ada-lex.l: Support the "thread THREADNO" clause with breakpoints
> > 	at an address.
> > 	* c-exp.y (yylex): Likewise.
> > 	* f-exp.y (yylex): Likewise.
> > 	* jv-exp.y (yylex): Likewise.
> > 	* m2-exp.y (yylex): Likewise.
> > 	* objc-exp.y (yylex): Likewise.
> > 	* p-exp.y (yylex): Likewise.
> > 
> >  This has been verified for the HEAD version with the test suite for the 
> > i386-linux-gnu system natively with no regressions.  Please consider.
> 
> No, sorry.  Consider:
> 
> (gdb) p *thread
> No symbol table is loaded.  Use the "file" command.
> 
> After your patch:
> 
> (gdb) b *thread
> A parse error in expression, near `thread'.
> 
> We can get away with this for 'if', because 'if' is a keyword in just
> about every language GDB supports, and an unlikely name for a variable.
> That's not going to fly for 'thread', however.

In addition to the above, I don't like the idea of supporting a
general feature in language-specific files.  That seems wrong.

Why was it done like this? because "*ADDRESS" is interpreted as an
expression in the current language, or is there some other reason?

Re: Support for "break *ADDRESS thread THREADNO"

[Daniel Jacobowitz]

On Tue, Apr 12, 2005 at 09:46:31PM +0300, Eli Zaretskii wrote:
> In addition to the above, I don't like the idea of supporting a
> general feature in language-specific files.  That seems wrong.
> 
> Why was it done like this? because "*ADDRESS" is interpreted as an
> expression in the current language, or is there some other reason?

I assume so.  It is parsed as an expression, not just an address; for
instance "break *thread" would actually work if thread is a pointer to
a function.

There's already common code for this, that's where most of the 'thread
THREADNO' support is.  I think it's in linespec.c somewhere, but it
might be in breakpoint.c.  It seems to need a little extra help from
the language... we need to know that a 'thread NUMBER' suffix is not
part of the expression.

TBH, the mechanism is fragile; I can't think of a more robust way but
there must be one.  Right now if and thread are handled similarly.  But
if separates two expressions, whereas thread can only be followed by a
thread number.  Perhaps that can be used to simplify.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: Support for "break *ADDRESS thread THREADNO"

[Eli Zaretskii]

> Date: Tue, 12 Apr 2005 14:55:49 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: "Maciej W. Rozycki" <macro@mips.com>, gdb-patches@sources.redhat.com
> 
> There's already common code for this, that's where most of the 'thread
> THREADNO' support is.  I think it's in linespec.c somewhere, but it
> might be in breakpoint.c.

Doing this in one of these two place would be okay, since they are
language independent.

> TBH, the mechanism is fragile; I can't think of a more robust way but
> there must be one.

How about a convenience variable $thread that will hold the current
thread ID?  Then we could simply use a condition "if $thread == TID".

Or maybe a command option "break --thread=ID *ADDR".  (Or did we
decide to not use such syntax?)

Re: Support for "break *ADDRESS thread THREADNO"

[Stan Shebs]

Eli Zaretskii wrote:

>
>Or maybe a command option "break --thread=ID *ADDR".  (Or did we
>decide to not use such syntax?)
>
"--" is still valid expression syntax, though unlikely in
that context.

Looking at the keyboard, I don't see much that is not already
part of language or GDB syntax. How about '/', which has no
unary usage, and is already used for command modifiers?

Stan

Re: Support for "break *ADDRESS thread THREADNO"

[Daniel Jacobowitz]

On Tue, Apr 12, 2005 at 12:17:34PM -0700, Stan Shebs wrote:
> Eli Zaretskii wrote:
> 
> >
> >Or maybe a command option "break --thread=ID *ADDR".  (Or did we
> >decide to not use such syntax?)
> >
> "--" is still valid expression syntax, though unlikely in
> that context.
> 
> Looking at the keyboard, I don't see much that is not already
> part of language or GDB syntax. How about '/', which has no
> unary usage, and is already used for command modifiers?

Fortunately, break does not take an expression; in 'break *FOO' FOO can
be an expression, but the * is not part of it.

(The question of using -- options is a different one.  Response was
pretty negative the last time we discussed this, IIRC.  But I like it.)

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: Support for "break *ADDRESS thread THREADNO"

[Daniel Jacobowitz]

On Tue, Apr 12, 2005 at 10:01:19PM +0300, Eli Zaretskii wrote:
> > Date: Tue, 12 Apr 2005 14:55:49 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: "Maciej W. Rozycki" <macro@mips.com>, gdb-patches@sources.redhat.com
> > 
> > There's already common code for this, that's where most of the 'thread
> > THREADNO' support is.  I think it's in linespec.c somewhere, but it
> > might be in breakpoint.c.
> 
> Doing this in one of these two place would be okay, since they are
> language independent.
> 
> > TBH, the mechanism is fragile; I can't think of a more robust way but
> > there must be one.
> 
> How about a convenience variable $thread that will hold the current
> thread ID?  Then we could simply use a condition "if $thread == TID".
> 
> Or maybe a command option "break --thread=ID *ADDR".  (Or did we
> decide to not use such syntax?)

Whatever we do, if we change anything, let's use something separate
from the existing argument to break.  The MI can represent this
explicitly, and ought to.  It's just a CLI problem.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: Support for "break *ADDRESS thread THREADNO"

[Maciej W. Rozycki]

On Tue, 12 Apr 2005, Daniel Jacobowitz wrote:

> > Why was it done like this? because "*ADDRESS" is interpreted as an
> > expression in the current language, or is there some other reason?
> 
> I assume so.  It is parsed as an expression, not just an address; for
> instance "break *thread" would actually work if thread is a pointer to
> a function.

 I think the actual problem is the expression parser cannot be told to 
stop successfully on an unparseable token as long as the expression 
collected so far is valid and let the caller deal with that.  AFAICS the 
parser can only stop on a string terminator or optionally a comma, 
otherwise it issues an error.

> There's already common code for this, that's where most of the 'thread
> THREADNO' support is.  I think it's in linespec.c somewhere, but it
> might be in breakpoint.c.  It seems to need a little extra help from
> the language... we need to know that a 'thread NUMBER' suffix is not
> part of the expression.

 It's in break_command_1() actually, so that's breakpoint.c.

> TBH, the mechanism is fragile; I can't think of a more robust way but
> there must be one.  Right now if and thread are handled similarly.  But
> if separates two expressions, whereas thread can only be followed by a
> thread number.  Perhaps that can be used to simplify.

 The current syntax is good enough to be unambiguous -- it's just it's not 
parsed correctly.  But if you find it inadequate, I won't insist on 
keeping it.

  Maciej

Re: Support for "break *ADDRESS thread THREADNO"

[Daniel Jacobowitz]

On Wed, Apr 13, 2005 at 11:35:29AM +0100, Maciej W. Rozycki wrote:
> On Tue, 12 Apr 2005, Daniel Jacobowitz wrote:
> 
> > > Why was it done like this? because "*ADDRESS" is interpreted as an
> > > expression in the current language, or is there some other reason?
> > 
> > I assume so.  It is parsed as an expression, not just an address; for
> > instance "break *thread" would actually work if thread is a pointer to
> > a function.
> 
>  I think the actual problem is the expression parser cannot be told to 
> stop successfully on an unparseable token as long as the expression 
> collected so far is valid and let the caller deal with that.  AFAICS the 
> parser can only stop on a string terminator or optionally a comma, 
> otherwise it issues an error.

And - can yacc do that?

Are you sure that there's no C expression that's valid with and without
a trailing identifier, by the way?  I certainly find that believable,
but I haven't thought about it too hard.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: Support for "break *ADDRESS thread THREADNO"

[Maciej W. Rozycki]

On Wed, 13 Apr 2005, Daniel Jacobowitz wrote:

> > > I assume so.  It is parsed as an expression, not just an address; for
> > > instance "break *thread" would actually work if thread is a pointer to
> > > a function.
> > 
> >  I think the actual problem is the expression parser cannot be told to 
> > stop successfully on an unparseable token as long as the expression 
> > collected so far is valid and let the caller deal with that.  AFAICS the 
> > parser can only stop on a string terminator or optionally a comma, 
> > otherwise it issues an error.
> 
> And - can yacc do that?

 No idea.  With bison, according to documentation, the special "error" 
token might be a way of implementing that.  But I haven't done any real 
lex/yacc coding for years, so I'd have to refresh my dusty memory.

> Are you sure that there's no C expression that's valid with and without
> a trailing identifier, by the way?  I certainly find that believable,
> but I haven't thought about it too hard.

 Well, with C it seems to be true -- I can't recall any expression where a 
trailing identifier would be valid if not preceded by either a type cast, 
which by itself is not a valid expression, or an operator that accepts a 
right-side operand, which cannot be left dangling either.  But I'd have to 
have a look at the grammar spec to be sure I don't miss any obscure corner 
case.

  Maciej

Re: Support for "break *ADDRESS thread THREADNO"

[Eli Zaretskii]

> Date: Wed, 13 Apr 2005 11:35:29 +0100 (BST)
> From: "Maciej W. Rozycki" <macro@mips.com>
> cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sources.redhat.com, 
>     "Maciej W. Rozycki" <macro@linux-mips.org>
> 
>  I think the actual problem is the expression parser cannot be told to 
> stop successfully on an unparseable token as long as the expression 
> collected so far is valid and let the caller deal with that.

We could detect and remove the "thread NUMBER" part from the string we
pass to the parser, couldn't we?

Re: Support for "break *ADDRESS thread THREADNO"

[Daniel Jacobowitz]

On Wed, Apr 13, 2005 at 08:32:04PM +0300, Eli Zaretskii wrote:
> > Date: Wed, 13 Apr 2005 11:35:29 +0100 (BST)
> > From: "Maciej W. Rozycki" <macro@mips.com>
> > cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sources.redhat.com, 
> >     "Maciej W. Rozycki" <macro@linux-mips.org>
> > 
> >  I think the actual problem is the expression parser cannot be told to 
> > stop successfully on an unparseable token as long as the expression 
> > collected so far is valid and let the caller deal with that.
> 
> We could detect and remove the "thread NUMBER" part from the string we
> pass to the parser, couldn't we?

I have the feeling this is going to be pretty tricky to get right.  For
instance, it can come before or after the "if" clause.  (Well, I
thought it could; the manual only says before.)  So if it comes before
then you have a line like:

  break *EXPRESSION thread 5 if EXPRESSION

Suppose one of those expressions includes:
  strcmp ("thread 5", my_thread_name)

Picking out the thread number would require at least lexing, and
possibly parsing.

-- 
Daniel Jacobowitz
CodeSourcery, LLC