[RFA] Change to parse error reporting

[RFA] Change to parse error reporting

[Michael Snyder]

Folks, 

I've never liked the way in which "parse error" messages in gdb
always point to the token _after_ the error, instead of at the
token _causing_ the error.

This change (implemented for the c/c++ parser only) will make it
point at the error token instead of the following token.  
Extending the change to the other languages will be trivial, 
if you agree with this.

OK to commit?


2002-04-12  Michael Snyder  <msnyder@redhat.com>

	* parser-defs.h (prev_lexptr): New external variable.
	* parse.c (parse_exp_1): Set prev_lexptr to null before
	calling the language-specific parser.
	* c-exp.y (yylex): Set prev_lexptr to start of current token.
	(yyerror): Use prev_lexptr in error reporting.

Index: parse.c
===================================================================
RCS file: /cvs/src/src/gdb/parse.c,v
retrieving revision 1.22
diff -p -r1.22 parse.c
*** parse.c	9 Apr 2002 22:14:39 -0000	1.22
--- parse.c	12 Apr 2002 19:40:35 -0000
*************** int arglist_len;
*** 76,81 ****
--- 76,82 ----
  union type_stack_elt *type_stack;
  int type_stack_depth, type_stack_size;
  char *lexptr;
+ char *prev_lexptr;
  char *namecopy;
  int paren_depth;
  int comma_terminates;
*************** parse_exp_1 (char **stringptr, struct bl
*** 1126,1131 ****
--- 1127,1133 ----
    struct cleanup *old_chain;
  
    lexptr = *stringptr;
+   prev_lexptr = NULL;
  
    paren_depth = 0;
    type_stack_depth = 0;
Index: parser-defs.h
===================================================================
RCS file: /cvs/src/src/gdb/parser-defs.h,v
retrieving revision 1.7
diff -p -r1.7 parser-defs.h
*** parser-defs.h	9 Apr 2002 22:14:39 -0000	1.7
--- parser-defs.h	12 Apr 2002 19:40:35 -0000
*************** extern struct type *follow_types (struct
*** 150,155 ****
--- 150,159 ----
  
  extern char *lexptr;
  
+ /* After a token has been recognized, this variable points to it.  
+    Currently used only for error reporting.  */
+ extern char *prev_lexptr;
+ 
  /* Tokens that refer to names do so with explicit pointer and length,
     so they can share the storage that lexptr is parsing.
  
Index: c-exp.y
===================================================================
RCS file: /cvs/src/src/gdb/c-exp.y,v
retrieving revision 1.7
diff -p -r1.7 c-exp.y
*** c-exp.y	15 Nov 2001 01:55:59 -0000	1.7
--- c-exp.y	12 Apr 2002 19:40:35 -0000
*************** yylex ()
*** 1218,1223 ****
--- 1218,1224 ----
     
   retry:
  
+   prev_lexptr = lexptr;
    unquoted_expr = 1;
  
    tokstart = lexptr;
*************** void
*** 1738,1742 ****
--- 1739,1746 ----
  yyerror (msg)
       char *msg;
  {
+   if (prev_lexptr)
+     lexptr = prev_lexptr;
+ 
    error ("A %s in expression, near `%s'.", (msg ? msg : "error"), lexptr);
  }

[RFA] lin-lwp.c change to avoid obscure hanging

[Paul N. Hilfinger]

Under certain obscure conditions, GDB will hang when continuing a program
that uses GNU/Linux LWPs.  For example, one set of circumstances that
were uncovered in an Ada program were:

1. A breakpoint is tripped over in thread A.

2. When all threads are stopped, it turns out that thread B != A has received
   a signal (specifically SIG32, which is apparently used by the thread
   package).  This signal, furthermore, is "not of interest" to GDB
   (as if according to 'set handle SIG32 pass noprint nostop').
   A and B here stand for integer literals, of course.

3. The user executes the commands
     delete
     thread B
     continue     

I am uncertain as to the proper fix is for this, but I have provided
one possibility below (that works for us).  The long ugly comment
will, I hope, prompt the more experienced among you to resolve the
outstanding questions.

Paul Hilfinger
ACT, Inc.



2002-04-10  Paul N. Hilfinger  <hilfingr@otisco.mckusick.com>

	* lin-lwp.c (lin_lwp_resume): Pass unprocessed signals to the 
	selected lwp, if GDB is not interested in them, and no specific
	signal is requested.  Fixes regression (hanging) displayed by 
	9820-005__task_switch.


Index: gdb/lin-lwp.c
===================================================================
RCS file: /cvs/src/src/gdb/lin-lwp.c,v
retrieving revision 1.34
diff -u -p -r1.34 lin-lwp.c
--- gdb/lin-lwp.c	31 Mar 2002 15:10:38 -0000	1.34
+++ gdb/lin-lwp.c	13 Apr 2002 08:41:20 -0000
@@ -605,6 +605,32 @@ lin_lwp_resume (ptid_t ptid, int step, e
       /* Mark this LWP as resumed.  */
       lp->resumed = 1;
 
+      if (WIFSTOPPED (lp->status))
+	{
+	  enum target_signal stopsig 
+	    = target_signal_from_host (WSTOPSIG (lp->status));
+
+	  /* If we have a signal GDB is not interested in, then
+	     arrange to pass it to the process without comment.  This
+	     should be able to happen only when one changes threads 
+	     before continuing.  To be conservative, we do this only
+	     if signo is TARGET_SIGNAL_0.  
+	     FIXME: hilfinger/2002-04-10: But this means we can't
+	     switch to this thread and clear its signal.  We could try
+	     simply not resuming when lp->status is non-zero, and
+	     leave it to lin_lwp_wait to catch the unhandled signal,
+	     but it will resume ONLY this thread (see Kettenis's FIXME
+	     in lin_lwp_wait), and this is not always right. */
+
+	  if (signo == TARGET_SIGNAL_0 
+	      && signal_stop_state (stopsig) == 0
+	      && signal_print_state (stopsig) == 0
+	      && signal_pass_state (stopsig) == 1)
+	    {
+	      signo = stopsig;
+	      lp->status = 0;
+	    }
+	}
       /* If we have a pending wait status for this thread, there is no
          point in resuming the process.  */
       if (lp->status)

Re: [RFA] lin-lwp.c change to avoid obscure hanging

[Michael Snyder]

"Paul N. Hilfinger" wrote:
> 
> Under certain obscure conditions, GDB will hang when continuing a program
> that uses GNU/Linux LWPs.  For example, one set of circumstances that
> were uncovered in an Ada program were:
> 
> 1. A breakpoint is tripped over in thread A.
> 
> 2. When all threads are stopped, it turns out that thread B != A has received
>    a signal (specifically SIG32, which is apparently used by the thread
>    package).  This signal, furthermore, is "not of interest" to GDB
>    (as if according to 'set handle SIG32 pass noprint nostop').
>    A and B here stand for integer literals, of course.
> 
> 3. The user executes the commands
>      delete
>      thread B
>      continue

This isn't really a sensable thing to do anyway.
Probably the user is imagining that this will cause
thread B to be treated in some way specially (eg. 
resumed before the others), but it will not
(or at least it should not).

> I am uncertain as to the proper fix is for this, but I have provided
> one possibility below (that works for us).  The long ugly comment
> will, I hope, prompt the more experienced among you to resolve the
> outstanding questions.

Puzzling: shouldn't "prepare_to_proceed" be handling this?
In theory, before gdb actually resumes the child, it should
restore the "current thread" to be thread A.





> 
> 2002-04-10  Paul N. Hilfinger  <hilfingr@otisco.mckusick.com>
> 
>         * lin-lwp.c (lin_lwp_resume): Pass unprocessed signals to the
>         selected lwp, if GDB is not interested in them, and no specific
>         signal is requested.  Fixes regression (hanging) displayed by
>         9820-005__task_switch.
> 
> Index: gdb/lin-lwp.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/lin-lwp.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 lin-lwp.c
> --- gdb/lin-lwp.c       31 Mar 2002 15:10:38 -0000      1.34
> +++ gdb/lin-lwp.c       13 Apr 2002 08:41:20 -0000
> @@ -605,6 +605,32 @@ lin_lwp_resume (ptid_t ptid, int step, e
>        /* Mark this LWP as resumed.  */
>        lp->resumed = 1;
> 
> +      if (WIFSTOPPED (lp->status))
> +       {
> +         enum target_signal stopsig
> +           = target_signal_from_host (WSTOPSIG (lp->status));
> +
> +         /* If we have a signal GDB is not interested in, then
> +            arrange to pass it to the process without comment.  This
> +            should be able to happen only when one changes threads
> +            before continuing.  To be conservative, we do this only
> +            if signo is TARGET_SIGNAL_0.
> +            FIXME: hilfinger/2002-04-10: But this means we can't
> +            switch to this thread and clear its signal.  We could try
> +            simply not resuming when lp->status is non-zero, and
> +            leave it to lin_lwp_wait to catch the unhandled signal,
> +            but it will resume ONLY this thread (see Kettenis's FIXME
> +            in lin_lwp_wait), and this is not always right. */
> +
> +         if (signo == TARGET_SIGNAL_0
> +             && signal_stop_state (stopsig) == 0
> +             && signal_print_state (stopsig) == 0
> +             && signal_pass_state (stopsig) == 1)
> +           {
> +             signo = stopsig;
> +             lp->status = 0;
> +           }
> +       }
>        /* If we have a pending wait status for this thread, there is no
>           point in resuming the process.  */
>        if (lp->status)

Re: [RFA] lin-lwp.c change to avoid obscure hanging

[Paul N. Hilfinger]

> > 3. The user executes the commands
> >      delete
> >      thread B
> >      continue

> This isn't really a sensable thing to do anyway.
> Probably the user is imagining that this will cause
> thread B to be treated in some way specially (eg. 
> resumed before the others), but it will not
> (or at least it should not).

Oh, I'm SO glad you brought this up, because it is a point that could stand 
clarification.  

Q1. Consider the following sequence (user input preceded by prompts)

	  (gdb) run
	  ...
	  Breakpoint 1, philosopher ...
	  (gdb) info thread
          * 7 Thread 5126 (runnable) ...
	  ...
	  (gdb) thread 6   # I.e., Some thread other than the current one
	  (gdb) signal 1

     What is supposed to happen?  What DOES happen (on Linux) is that thread 7 
     receives SIGHUP.

Q2. Now consider what happens when one thread is sent an asynchronous SIGHUP
    (on Linux, there are kernel threads, and you can address a signal to
    a specific thread from the command line with the kill command).

          (gdb) run
	  ...
	  Program received signal SIGHUP, Hangup.
	  ...
	  (gdb) info thread
          * 7 Thread 5126 (runnable) ...
	  ...
	  (gdb) thread 6
	  (gdb) cont

     Here what happens is that thread *6* receives SIGHUP.

Q3.  Finally, we have

          (gdb) run
	  ...
	  Program received signal SIGHUP, Hangup.
	  ...
	  (gdb) info thread
          * 7 Thread 5126 (runnable) ...
	  ...
	  (gdb) thread 6
	  (gdb) signal 1

     Again, thread 6 receives SIGHUP.

Question: How much of this, if any, is intentional?  What should happen in
these cases?

Paul Hilfinger

Re: [RFA] lin-lwp.c change to avoid obscure hanging

[Michael Snyder]

"Paul N. Hilfinger" wrote:
> 
> > > 3. The user executes the commands
> > >      delete
> > >      thread B
> > >      continue
> 
> > This isn't really a sensable thing to do anyway.
> > Probably the user is imagining that this will cause
> > thread B to be treated in some way specially (eg.
> > resumed before the others), but it will not
> > (or at least it should not).
> 
> Oh, I'm SO glad you brought this up, because it is a point that could stand
> clarification.
> 
> Q1. Consider the following sequence (user input preceded by prompts)
> 
>           (gdb) run
>           ...
>           Breakpoint 1, philosopher ...
>           (gdb) info thread
>           * 7 Thread 5126 (runnable) ...
>           ...
>           (gdb) thread 6   # I.e., Some thread other than the current one
>           (gdb) signal 1
> 
>      What is supposed to happen?  What DOES happen (on Linux) is that thread 7
>      receives SIGHUP.

There are actually three predicates here:
  - what is supposed to happen
  - what does happen
  - what is expected to happen.

What you describe above (thread 7 receives the signal)
is what is expected to happen.

GDB's thread model is lacking in several areas, and one of them
is the ability to resume a thread other than the one that stopped.
Since we have not designed an interface for doing this in general, 
we don't do it at all (even on platforms such as Linux where it
might be possible).  So the thread that resumes is always the
thread that stopped, no matter what thread you switch to in the
mean time.


> 
> Q2. Now consider what happens when one thread is sent an asynchronous SIGHUP
>     (on Linux, there are kernel threads, and you can address a signal to
>     a specific thread from the command line with the kill command).
> 
>           (gdb) run
>           ...
>           Program received signal SIGHUP, Hangup.
>           ...
>           (gdb) info thread
>           * 7 Thread 5126 (runnable) ...
>           ...
>           (gdb) thread 6
>           (gdb) cont
> 
>      Here what happens is that thread *6* receives SIGHUP.

Ugh!  Well, that's a bug.  
That's not what is "supposed" or "expected" to happen.

> 
> Q3.  Finally, we have
> 
>           (gdb) run
>           ...
>           Program received signal SIGHUP, Hangup.
>           ...
>           (gdb) info thread
>           * 7 Thread 5126 (runnable) ...
>           ...
>           (gdb) thread 6
>           (gdb) signal 1
> 
>      Again, thread 6 receives SIGHUP.

No, that's a bug too.


> Question: How much of this, if any, is intentional?  What should happen in
> these cases?

Q1 is intentional, Q2 and Q3 are not.

Michael

Re: [RFA] Change to parse error reporting

[Michael Snyder]

Michael Snyder wrote:
> 
> Folks,
> 
> I've never liked the way in which "parse error" messages in gdb
> always point to the token _after_ the error, instead of at the
> token _causing_ the error.
> 
> This change (implemented for the c/c++ parser only) will make it
> point at the error token instead of the following token.
> Extending the change to the other languages will be trivial,
> if you agree with this.
> 
> OK to commit?

Does anybody have any opinion about this?
If not I'll check it in next week.

> 
> 2002-04-12  Michael Snyder  <msnyder@redhat.com>
> 
>         * parser-defs.h (prev_lexptr): New external variable.
>         * parse.c (parse_exp_1): Set prev_lexptr to null before
>         calling the language-specific parser.
>         * c-exp.y (yylex): Set prev_lexptr to start of current token.
>         (yyerror): Use prev_lexptr in error reporting.
> 
> Index: parse.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/parse.c,v
> retrieving revision 1.22
> diff -p -r1.22 parse.c
> *** parse.c     9 Apr 2002 22:14:39 -0000       1.22
> --- parse.c     12 Apr 2002 19:40:35 -0000
> *************** int arglist_len;
> *** 76,81 ****
> --- 76,82 ----
>   union type_stack_elt *type_stack;
>   int type_stack_depth, type_stack_size;
>   char *lexptr;
> + char *prev_lexptr;
>   char *namecopy;
>   int paren_depth;
>   int comma_terminates;
> *************** parse_exp_1 (char **stringptr, struct bl
> *** 1126,1131 ****
> --- 1127,1133 ----
>     struct cleanup *old_chain;
> 
>     lexptr = *stringptr;
> +   prev_lexptr = NULL;
> 
>     paren_depth = 0;
>     type_stack_depth = 0;
> Index: parser-defs.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/parser-defs.h,v
> retrieving revision 1.7
> diff -p -r1.7 parser-defs.h
> *** parser-defs.h       9 Apr 2002 22:14:39 -0000       1.7
> --- parser-defs.h       12 Apr 2002 19:40:35 -0000
> *************** extern struct type *follow_types (struct
> *** 150,155 ****
> --- 150,159 ----
> 
>   extern char *lexptr;
> 
> + /* After a token has been recognized, this variable points to it.
> +    Currently used only for error reporting.  */
> + extern char *prev_lexptr;
> +
>   /* Tokens that refer to names do so with explicit pointer and length,
>      so they can share the storage that lexptr is parsing.
> 
> Index: c-exp.y
> ===================================================================
> RCS file: /cvs/src/src/gdb/c-exp.y,v
> retrieving revision 1.7
> diff -p -r1.7 c-exp.y
> *** c-exp.y     15 Nov 2001 01:55:59 -0000      1.7
> --- c-exp.y     12 Apr 2002 19:40:35 -0000
> *************** yylex ()
> *** 1218,1223 ****
> --- 1218,1224 ----
> 
>    retry:
> 
> +   prev_lexptr = lexptr;
>     unquoted_expr = 1;
> 
>     tokstart = lexptr;
> *************** void
> *** 1738,1742 ****
> --- 1739,1746 ----
>   yyerror (msg)
>        char *msg;
>   {
> +   if (prev_lexptr)
> +     lexptr = prev_lexptr;
> +
>     error ("A %s in expression, near `%s'.", (msg ? msg : "error"), lexptr);
>   }

Re: [RFA] Change to parse error reporting

[Michael Snyder]

Michael Snyder wrote:
> 
> Michael Snyder wrote:
> >
> > Folks,
> >
> > I've never liked the way in which "parse error" messages in gdb
> > always point to the token _after_ the error, instead of at the
> > token _causing_ the error.
> >
> > This change (implemented for the c/c++ parser only) will make it
> > point at the error token instead of the following token.
> > Extending the change to the other languages will be trivial,
> > if you agree with this.
> >
> > OK to commit?
> 
> Does anybody have any opinion about this?
> If not I'll check it in next week.

OK, committed.


> > 2002-04-12  Michael Snyder  <msnyder@redhat.com>
> >
> >         * parser-defs.h (prev_lexptr): New external variable.
> >         * parse.c (parse_exp_1): Set prev_lexptr to null before
> >         calling the language-specific parser.
> >         * c-exp.y (yylex): Set prev_lexptr to start of current token.
> >         (yyerror): Use prev_lexptr in error reporting.
> >
> > Index: parse.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/parse.c,v
> > retrieving revision 1.22
> > diff -p -r1.22 parse.c
> > *** parse.c     9 Apr 2002 22:14:39 -0000       1.22
> > --- parse.c     12 Apr 2002 19:40:35 -0000
> > *************** int arglist_len;
> > *** 76,81 ****
> > --- 76,82 ----
> >   union type_stack_elt *type_stack;
> >   int type_stack_depth, type_stack_size;
> >   char *lexptr;
> > + char *prev_lexptr;
> >   char *namecopy;
> >   int paren_depth;
> >   int comma_terminates;
> > *************** parse_exp_1 (char **stringptr, struct bl
> > *** 1126,1131 ****
> > --- 1127,1133 ----
> >     struct cleanup *old_chain;
> >
> >     lexptr = *stringptr;
> > +   prev_lexptr = NULL;
> >
> >     paren_depth = 0;
> >     type_stack_depth = 0;
> > Index: parser-defs.h
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/parser-defs.h,v
> > retrieving revision 1.7
> > diff -p -r1.7 parser-defs.h
> > *** parser-defs.h       9 Apr 2002 22:14:39 -0000       1.7
> > --- parser-defs.h       12 Apr 2002 19:40:35 -0000
> > *************** extern struct type *follow_types (struct
> > *** 150,155 ****
> > --- 150,159 ----
> >
> >   extern char *lexptr;
> >
> > + /* After a token has been recognized, this variable points to it.
> > +    Currently used only for error reporting.  */
> > + extern char *prev_lexptr;
> > +
> >   /* Tokens that refer to names do so with explicit pointer and length,
> >      so they can share the storage that lexptr is parsing.
> >
> > Index: c-exp.y
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/c-exp.y,v
> > retrieving revision 1.7
> > diff -p -r1.7 c-exp.y
> > *** c-exp.y     15 Nov 2001 01:55:59 -0000      1.7
> > --- c-exp.y     12 Apr 2002 19:40:35 -0000
> > *************** yylex ()
> > *** 1218,1223 ****
> > --- 1218,1224 ----
> >
> >    retry:
> >
> > +   prev_lexptr = lexptr;
> >     unquoted_expr = 1;
> >
> >     tokstart = lexptr;
> > *************** void
> > *** 1738,1742 ****
> > --- 1739,1746 ----
> >   yyerror (msg)
> >        char *msg;
> >   {
> > +   if (prev_lexptr)
> > +     lexptr = prev_lexptr;
> > +
> >     error ("A %s in expression, near `%s'.", (msg ? msg : "error"), lexptr);
> >   }